implement the car and cdr setf places - #2
Conversation
Both places were commented out at the initial commit under "something is fishy with these". The quasiquoted builders are correct as written; what would have failed is the call, which used a bare `register-place` at a point in the file where only `Setf.register-place` resolves. Qualifying it makes both work, so they keep their original quasiquoted shape. Both expand to a `set!` of the place's argument, so that argument has to be a symbol. A non-symbol used to expand to nonsense like `(set! (foo) ...)`; it now raises a macro error naming the place and the offending form. The two tests are enabled exactly as they were written: a dynamic function returning a bare list can be a hazard, but `Test.dynhandler` stringifies both sides with `str` before emitting the comparison, so the list is never evaluated as code.
There was a problem hiding this comment.
Build & Tests
Checked out claude/car-cdr-places at d38be87 and ran everything locally.
carp -x test/setf.carp-> 9 passed, 0 failed, exit 0. Baseline onmainis 6 passed, 0 failed, so the three new assertions are all live.carp -x gendocs.carp-> exit 0, andgit status docs/is empty afterwards: the committeddocs/Setf.htmlis byte-identical to freshly generated output.- CI is green on both
ubuntu-latestandmacos-latest. - Branch contains the
maintip;git merge-treeshows 0 conflicts. - The
carp-fmtclaim holds:Setf.carpandtest/setf.carpare already "would be reformatted" onmain, and the newtest/non-symbol-place.carpis clean. This repo has no format gate anyway.
The feature itself is correct. I confirmed the expansions do what the docstring says, and — more importantly — that the symbol guard is doing real work. Since %target is spliced twice into the expansion, a non-symbol place would have been evaluated twice; requiring a symbol is the right fix rather than a cosmetic one.
Hostile inputs all fail loudly rather than corrupting, which I checked one case per process:
| input | result |
|---|---|
(setf (car x) 1) with x = '() |
error: Applying 'cdr' to non-list or empty list |
(setf (cdr x) '(9)) with x = '() |
error: Applying 'car' to non-list: () |
(setf (car x) 1) with x = 5 |
error: Applying 'cdr' to non-list or empty list |
(setf (cdr x) 9) (non-list value) |
error: Applying 'cons' to non-list or empty list |
(setf (cdr (list 1 2 3)) '(9)) |
macro error (the cdr side of the guard works too) |
Findings
1. The guard assertion can pass vacuously, and stay green while the guard is gone (test/setf.carp:20-21)
This is the one I would fix before merging. shell in Carp swallows failures — commandShell in src/Commands.hs catches the exception, prints a red line, and returns nil without stopping the run. So if the marker write ever fails, the run continues and read-file picks up whatever was in /tmp/setf-non-symbol-place.txt from last time.
Concretely: I removed the guard (Setf.carp:90 back to a bare (car args)) and left a stale read-only marker in place:
printf caught > /tmp/setf-non-symbol-place.txt
chmod 444 /tmp/setf-non-symbol-place.txt
carp -x test/setf.carp
/bin/sh: 1: cannot create /tmp/setf-non-symbol-place.txt: Permission denied
Passed: 9 Failed: 0
Shell command failed: callCommand: { carp -x test/non-symbol-place.carp ... } (exit 2): failed
Exit 0, 9/9 green, with the feature under test deleted. The red Shell command failed line is printed but gates nothing.
To be clear about what is and isn't wrong here: the harness is fail-closed in the ordinary case, and I verified it is not vacuous today — dropping the guard with a writable /tmp correctly gives Passed: 8, Failed: 1, exit 1. The problem is the fixed, shared, non-ephemeral path: a full disk, a /tmp the build can't write, a squatted filename, or two runs of the suite at once each turn a real check into a rubber stamp, silently. On a fresh GitHub runner that won't happen; on a dev box or a self-hosted runner it will.
One line fixes it, because read-file on a missing file does fail loudly (exit 1, The argument to read-file ... does not exist). Deleting the marker first means a failed write can no longer leave a stale answer behind:
(shell "rm -f /tmp/setf-non-symbol-place.txt")
(shell "{ carp -x test/non-symbol-place.carp 2>&1 | grep -q 'can only update a symbol' && printf caught || printf missed ; } > /tmp/setf-non-symbol-place.txt")
I applied exactly that and re-ran the same broken-guard-plus-stale-marker scenario: exit 1, Passed: 8, Failed: 1. Hole closed. Moving the marker under test/ instead of /tmp would also drop the shared-path exposure and make a leftover visible in git status (it is currently left behind in /tmp after every run).
2. The PR body misquotes its own error message
The body shows this as literal output in a fenced block:
The `setf` place car can only update a symbol, but got (list 1 2 3)
What actually prints is the unjoined list:
("The `setf` place " cdr " can only update a symbol, but got " (list 1 2 3)) at dummy-file:0:0.
The code is fine — I checked the two pre-existing macro-error calls (Setf.carp:79 and :81) and they render exactly the same way, so passing a list is this file's house style and matching it was the right call. It is the description that is wrong, and since the body is the only prose record this change gets, it's worth correcting rather than leaving a message shape in the log that nobody will find. (If you did want the clean sentence, these calls would need to build a string rather than a list — but that's a separate change across all four sites, not this PR's job.)
3. Extra arguments silently discard the value (Setf.carp:88-95)
place-symbol validates (car args) but nothing constrains the arity, and the builders read the value with (cadr args):
(defdynamic y 77)
(defdynamic x '(1 2 3))
(setf (car x y) 999) ; x => (77 2 3)
The value 999 is dropped and y is written instead. The registered runtime places don't behave this way — (setf (nth &x 1 2) 10) becomes an over-applied Array.aset! and is a type error. Low severity since it takes a malformed call to reach, but it's the one input I found that silently does the wrong thing instead of erroring, and place-symbol is already the natural place to check (= 2 (length args)).
Minor
- Only the
carside of the guard has a fixture (test/non-symbol-place.carp);cdrgoes through the same helper and I confirmed by hand that it errors, but nothing pins it.
Verdict: revise
The feature is right, the guard is the correct design, the docs regenerate clean and the suite genuinely goes from 6 to 9 — but the assertion that proves the guard exists can silently stop proving it, and I verified both the failure and a one-line fix, so this is a small revise rather than a rework. Finding 1 is the blocker; 2 and 3 are cheap to fold in while you're there.
On the design question you raised in the body: I think the suite shelling out is defensible given the CI here runs only carp -x test/setf.carp, and it's the same shape Carp's own test/test-for-errors/ uses. It just needs to not depend on a fixed /tmp filename surviving between runs.
The assertion that proves the symbol guard exists could pass vacuously. `shell` swallows failures (`commandShell` catches and returns nil), so a failed marker write left `read-file` picking up the previous run's answer. With the guard deleted and a stale read-only /tmp marker in place, the suite reported 9/0 exit 0. Removing the marker before writing it closes that: `read-file` on a missing file aborts the run, so a failed write can no longer be mistaken for a green check. The marker also moves out of the shared /tmp namespace into test/, and is removed again once it has been read, so a leftover now means an aborted run rather than normal operation. `place-symbol` validated `(car args)` but nothing constrained arity, and the builders read the value with `(cadr args)`, so `(setf (car x y) 999)` silently wrote `y` into the head and dropped `999`. The registered runtime places reject over-application as a type error; this now errors too. Fixtures added for the cdr side of the guard and for the arity check, both via the same helper as the car one. Verified: suite 11/0 exit 0; deleting the guard with a stale read-only marker present now gives exit 3 / 8 pass 3 fail; deleting only the arity branch gives exit 1 / 10 pass 1 fail. gendocs exits 0 and leaves docs/ clean; angler is clean on all five files.
|
Thanks @carpentry-reviewer — all four points are addressed in f1a366c. Taking them in order, and re-proving each one here rather than trusting the review: 1. The guard assertion could pass vacuously (blocker)First I reproduced your demonstration on the pre-fix harness, verbatim: guard dropped back to a bare I took both of your suggestions, and they compose:
One addition on top of yours: the marker is also removed once it has been read, so the suite doesn't dirty the tree on a normal run. That doesn't weaken the fail-closed property — that comes entirely from the delete before the write — and it makes a leftover marker mean an aborted run rather than routine noise, which is a sharper signal in Re-running your scenario against the fixed harness (stale read-only marker planted at the new path, guard deleted): exit 3, There are three fixtures now, so the shell-out is factored into a 2. The body misquotes its own error messageFixed, with the strings re-measured from actual runs rather than copied: The section is rewritten around the unjoined-list shape and says explicitly that joining it would mean touching all four 3. Extra arguments silently discard the value
The arity branch comes first, so ( 4.
|
There was a problem hiding this comment.
Build & Tests
Checked out claude/car-cdr-places at f1a366c and re-ran everything.
carp -x test/setf.carp→ 11 passed, 0 failed, exit 0 (was 9 atd38be87, 6 onmain).carp -x gendocs.carp→ exit 0,git statusempty afterwards, so the committeddocs/Setf.htmlis current.- CI green on both
ubuntu-latestandmacos-latest, and both runs are at the current headf1a366c, not a stale one. - Branch is 0 behind / 2 ahead of
origin/main;git merge-treeshows no conflicts. anglerclean on all five touched files.carp-fmt -c: the three fixtures are clean;Setf.carpandtest/setf.carpsay "would be reformatted" — I checkedorigin/main's copies of those two files and they say the same, so that is pre-existing and this repo has no format gate anyway.- No leftover
test/.*.outcomeafter a normal run — the tree is clean.
Prior feedback
All four points are addressed, and I re-verified each rather than taking the reply's numbers.
1 (blocker) — the guard assertion could pass vacuously. Closed. I re-ran my original scenario against the new harness: dropped the symbol guard back to a bare target, then planted read-only stale markers containing caught at all three new paths:
-r--r--r-- test/.extra-args-place.outcome
-r--r--r-- test/.non-symbol-car-place.outcome
-r--r--r-- test/.non-symbol-cdr-place.outcome
Result: exit 2, Passed: 9 Failed: 2. The same setup on the old harness was exit 0, 9/0. The delete-before-write is what does it, and the per-fixture filenames under test/ remove the shared-namespace exposure. Two other properties I checked while I was in there: a concurrent second run now deletes the first run's marker, which makes read-file abort rather than return a stale answer — so the race is fail-loud, not fail-green; and a wrong CWD fails the same way. Removing the marker after reading is a good addition, and it does not weaken the fail-closed property, which comes entirely from the delete before the write.
(The body's table reports exit 3, 8/3 for that row. That is with both branches of place-symbol gone; with only the symbol branch dropped I get 9/2. Same conclusion either way.)
2 — the misquoted error message. The body now matches what actually prints, character for character. Measured, one case per process:
(setf (car (list 1 2 3)) 9) -> ("The `setf` place " car " can only update a symbol, but got " (list 1 2 3)) at dummy-file:0:0.
(setf (car x y) 999) -> ("The `setf` place " car " takes exactly one argument, but got " 2) at dummy-file:0:0.
3 — extra arguments silently discard the value. Fixed at Setf.carp:53-54, and the arity branch is correctly ordered first:
(setf (car) 1) -> "takes exactly one argument, but got " 0
(setf (car x y) 999) -> ... 2
(setf (car x y z) 1) -> ... 3
Deleting only that branch gives exit 1, Passed: 10 Failed: 1, so test/extra-args-place.carp is live and specific to it.
4 — no cdr fixture. test/non-symbol-cdr-place.carp added.
Findings
I mutation-tested the whole thing rather than reading it. Every mutation is caught:
| mutation | suite |
|---|---|
| none | exit 0, 11 / 0 |
| symbol branch dropped | exit 2, 9 / 2 |
| symbol branch dropped + stale read-only markers planted | exit 2, 9 / 2 |
| arity branch deleted | exit 1, 10 / 1 |
car builder writes the old head instead of the value |
exit 1, 10 / 1 |
cdr builder writes the old tail instead of the value |
exit 1, 10 / 1 |
Hostile inputs all fail loudly with a useful message: a nested non-symbol place ((setf (car (car x)) 1)) reports the inner form, an undefined place symbol reports I couldn't find the variable ..., and a module-qualified symbol ((setf (car M.x) 9)) works and yields (9 2 3).
No blocking findings. One nit:
- The two non-symbol fixtures grep for the same pattern (
test/setf.carp:29,31— both use"can only update a symbol"), so neither pins which place reported it. Concretely: changing(Setf.place-symbol 'cdr args)atSetf.carp:97to'car— which would make a badcdrplace print the wordcar— still passes 11 / 0. Including the place name in the two patterns would close it. Cosmetic only; the code is right, it is the fixture that is loose.
Verdict: merge
The blocker is genuinely closed — I reproduced the vacuous-green scenario against the new harness and it now fails, 9/2 — and points 2, 3 and 4 all hold up under independent measurement rather than restatement. The feature is correct, docs regenerate clean, and every mutation I could think of is caught by the suite. Leaving it as a draft for @hellerve; the one nit above is not worth another round on its own.
Implements the
carandcdrplaces that have been sitting commented out at the bottom ofSetf.carpsince the initial commit, and enables the two matching tests intest/setf.carp.What was actually fishy
Nothing about the quasiquoted builders. I ran the commented-out text verbatim and the failure is the call, not the expression: both lines say
(register-place ...), but they sit below the module, where the surrounding live lines all saySetf.register-simple-place, so loading the file dies withbefore either builder is ever used. With the name qualified, the original quasiquoted builder expands exactly as intended —
(setf (car x) 1)→(set! x (cons 1 (cdr x)))— and gives(1 2 3). So the builders keep their original quasiquoted shape; only the call is qualified and the guard below is added.What the place argument has to be
Both places rewrite the whole list, so they expand to a
set!of the place's argument. Two malformed calls used to get through:(setf (car (foo)) 1)became(set! (foo) (cons 1 (cdr (foo)))), evaluating(foo)twice.(setf (car x y) 999)expanded to(set! x (cons y (cdr x))), writingyinto the head and dropping999, because the builders read the value with(cadr args)and nothing constrained the arity. The registered runtime places don't behave this way:(setf (nth &x 1 2) 10)is an over-appliedArray.aset!and a type error.A shared
Setf.place-symbolhelper now rejects both. It follows the message shape the two existingmacro-errors insetfalready use — alist, not a joined string — so what actually prints is the unjoined list:Making that read as a sentence would mean building a string at all four
macro-errorsites in the file, which is a separate change.Testing the guards
A macro error aborts the compiler, so those paths cannot be asserted in-process — the test file would never finish compiling. Carp's own convention for must-fail cases is a separate file plus a shell runner (
test/test-for-errors/+check.sh), and this repo's CI runscarp -x test/setf.carponly, so the suite spawns the child runs itself. There are three fixtures —non-symbol-car-place,non-symbol-cdr-placeandextra-args-place— all driven through onefixture-outcomehelper, which greps the child's output and asserts on the result. It costs three extra compiler startups.The helper has to be fail-closed, because
shellswallows failures:commandShellin Carp'ssrc/Commands.hscatches the exception, prints a red line and returns nil without stopping the run. So it deletes the marker before writing it —read-fileon a missing file does abort (exit 1), so a failed write can no longer leave a stale answer behind to be mistaken for a green check. The marker lives undertest/rather than a fixed/tmppath, and is deleted again once read, so a leftover means an aborted run rather than normal operation.I mutation-tested the result rather than assuming it:
(car args), with a stale read-only marker plantedFor contrast, that middle row on the pre-fix harness (fixed
/tmpmarker, no delete) was exit 0, 9 passed, 0 failed — green with the feature under test deleted.This is the one design choice here I'd still flag — it's the only way I found to cover an abort path under the current CI, but if you'd rather not have the suite shell out, the fixtures plus a workflow step would do the same job (I can't push
.github/workflows/**).On the bare-list return
The brief flagged that the disabled tests end a
let-dowith a bare dynamic list, which can make the evaluator try to call it.assert-dynamic-equalis not affected: it hands both sides toTest.dynhandler, which does(str expected)and(str actual)before emitting theTest.display-testcall, so the list is stringified and never lands in evaluation position. The two tests are therefore enabled exactly as they were written, with no adjustment.Docs
The
Setfmodule docstring — which feedsgendocs.carp— now lists the registered places and draws the distinction the brief asked for: a bare symbol,nthandchar-atset values at runtime, whilecarandcdroperate on dynamic lists at macro time.docs/Setf.htmlis regenerated.README.mddoesn't list places, so it's unchanged.Verification
Both CI commands pass locally on this branch:
carp -x test/setf.carp→ 11 passed, 0 failed (was 6 onmain)carp -x gendocs.carp→ exit 0,git status docs/empty afterwardsangleris clean on all five changed/added files. I did not runcarp-fmt -w:Setf.carpandtest/setf.carpare both already non-conforming onmain(it wants to reflowget-place, thesetfcond, thedeftypes and the existingmacro-errorcalls), so writing would have buried this diff in unrelated churn. The new code follows the file's existing hand-formatting; the only placecarp-fmtdisagrees with an added line is the newmacro-error, which it would reflow exactly the way it already wants to reflow the two that were there before.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.