Skip to content

test: pin apply*'s no-mutation property end to end - #707

Open
mparrett wants to merge 2 commits into
mainfrom
wt/apply-rest-ratchet
Open

test: pin apply*'s no-mutation property end to end#707
mparrett wants to merge 2 commits into
mainfrom
wt/apply-rest-ratchet

Conversation

@mparrett

@mparrett mparrett commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Point 5 of my 2026-07-22 comment on #620 reported that applying a variadic bytecode fn to a vector handed the vector back mutated, because rest packing appended into the caller's storage:

(let [v [1 2 3]] [(apply* (fn [x & xs] xs) v) v])
;; 2026-07-22: [(2 3) [1 (2 3) 3]]

#645 fixed that in resolveBytecodeCall and pinned the invariant where the fix lives, in TestResolveBytecodeCallVariadicPackingDoesNotMutateBorrowedArgs. I said on #620 that no test pinned it, which was wrong — I searched for apply* in the test files and the guard does not mention apply* anywhere. Correcting that separately on the issue.

What is genuinely uncovered is the route into the resolver. The existing test calls resolveBytecodeCall directly, so nothing asserts that apply* reaches it, and #620 Phase 3 rewrites exactly that route while proposing a borrowed-argument rule. This adds the end-to-end spelling the original report used.

The guard is verified in both directions

Against the pre-fix append(prepared[:t.arity-1], restlist) it fails with v = [1 (2 3) 3]; on main it passes. A test that passes either way would not be a ratchet.

Public apply was never exposed to this

apply* aliases the caller's storage only when it is handed an ArrayVector — the one branch in its pkg/rt/lang.go body that passes vs[1] to ec.Invoke unchanged — and no apply arity hands it one. The 2-arg arity passes (seq args), a seq view over the vector (apply in pkg/rt/core/core.lg); the 3+-arg arities pass a list* / spread cons chain. Both reach apply*'s seq branch, which copies the arguments into a fresh slice via appendSeqValues. Only the raw apply* hands the vector straight through.

That means the apply assertions pass against the pre-fix code as well. Rather than delete them or let them read as coverage they do not provide, the file groups them under a heading that says they are characterization, with the reason. If a later change makes apply alias its argument, they start failing, which is the argument for writing them down instead of assuming.

Thanks to @nnunley for catching this. The earlier wording attributed all of it to list* / spread, which does not cover the 2-arg arity the first characterization assertion exercises. The test file's comment block carried the same claim and is fixed too.

Placement

test/apply_rest_aliasing_test.lg, picked up by TestRunner's .lg walk. If you would rather this rode the Phase 3 branch than land ahead of it, say so and I will close this — the offer on #620 was to keep it off that branch, not to insist.

#620 point 5 reported that applying a variadic bytecode fn to a vector came
back with the vector mutated -- rest packing appended into the caller's
storage. #645 fixed it and pinned the invariant at the resolver, in
TestResolveBytecodeCallVariadicPackingDoesNotMutateBorrowedArgs. Nothing
covered the route into the resolver, and #620 Phase 3 rewrites that route.

The guard is verified in both directions: against the pre-fix
`append(prepared[:arity-1], restlist)` it fails with v = [1 (2 3) 3], and it
passes on main.

Public `apply` turns out never to have been exposed to this. For 2+ args it
reaches apply* through list*/spread, which allocates a fresh seq, so the
caller's storage is never the backing array -- only the raw apply* hands it
straight through. Those assertions pass against the pre-fix code too, so the
file marks them as characterization rather than letting them read as guards.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@nnunley

nnunley commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Ran the assertions against ./lg on current main before saying anything: (apply* take-rest [1 2 3])(2 3) with v still [1 2 3], and both apply spellings likewise. The guard is real — pkg/rt/lang.go:2506-2507 hands an ArrayVector's backing []Value straight to ec.Invoke, so pre-#645 this would have written through into the caller's vector. It fails on the old code for the right reason, which is the property that makes a ratchet worth having.

Placing it ahead of #620 Phase 3 is the right order too — it will fail loudly if the resolveBytecodeCall rewrite reintroduces borrowed-arg mutation.

One factual correction, in the body and in the file's comment block. The claim that "for 2+ args apply reaches apply* through list*/spread, which allocates a fresh seq" holds only for the 3+-arg arities. The 2-arg arity is:

(defn apply
  ([f args]
   (let [s (seq args)]
     (if s (apply* f s) (f))))

core.lg:1113-1116 — no list*, no spread. It is safe for a different reason: seq yields a non-ArrayVector Seq, which apply* materializes through appendSeqValues into a fresh slice (lang.go:2509-2517). The conclusion is right and the test is right; the stated mechanism just does not describe the arity your first characterization assertion actually exercises. Worth a line edit so the comment does not teach the wrong thing later.

Nothing else blocking. Coverage is honestly scoped — only ArrayVector aliases, and a plain variadic Func callee rather than a Closure/MultiArityFn wrapper — which reads as deliberate for a ratchet.

The 2-arg arity of apply reaches apply* through (seq args), not list*/spread;
it is safe because apply* only aliases an ArrayVector, and a seq view is not
one. Cites both call sites so the claim is checkable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mparrett

mparrett commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed, and fixed in both places. The mechanism I stated does not describe the arity the first characterization assertion exercises.

(seq [1 2 3]) comes back as a seq view with vector? false, so it misses apply*'s ArrayVector branch and copies through appendSeqValues.

Rather than patch the 2-arg sentence, I replaced the explanation with the one that covers every arity: apply* aliases only when it is handed an ArrayVector, and no apply arity hands it one. The 2-arg arity passes (seq args), the 3+-arg arities pass a list* / spread cons chain, and both reach the seq branch.

One deliberate change on the way through: the body names that branch by what it does instead of by line number. Your citation was accurate when you wrote it — #710 merged two and a half hours later and moved the ec.Invoke(f, av) hand-off six lines. The numbers I had drafted were correct when I wrote them too, and would go the same way on the next change through that file. Line numbers in prose have no way to go stale loudly.

Body updated; the comment block is fixed in c05fd703. The offer on #620 stands: if you would rather this rode the Phase 3 branch than land ahead of it, say so and I will close it.

@mparrett
mparrett requested a review from nnunley August 11, 2026 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants