Repository navigation
fix(hash): keep the folded salt-key constant an ArrayVector - #205
Merged
Merged
Conversation
`with-folded-salt`'s fully-literal branch encodes the whole salt-key at
expansion time and splices the byte buffer into the emitted form as a
compile-time constant. `encode-salt` builds that buffer with `conj`, and
let-go's `conj` promotes `ArrayVector` to `PersistentVector` above 32
elements.
`compileForm`'s type switch has a case for `ArrayVectorType` but none for
`PersistentVectorType`, and no `default:`, so a `PersistentVector` form
emits no bytecode at all. The macro expansion compiled clean and then
panicked `index out of range [-1]` on a short stack when called.
So every fully-literal salt-key encoding to 33 bytes or more was a
runtime crash. That is not an exotic size — the macro's own docstring
example, [:combat :hit attacker target], reaches it the moment its
variable terms are literals:
(defn f [seed] (with-folded-salt seed [:combat :hit 12 34]))
(f 42) ;=> index out of range [-1]
`vec` rebuilds the buffer as an `ArrayVector`, which the compiler does
handle. `into` would not work here: it goes through `conj` and promotes
straight back. The macro's other three splice sites already go through
`vec`, which is why only this branch broke.
Filed upstream as nooga/let-go#943, which also covers the missing
`default:` — the sorted map and set types are absent from the same switch
and fail identically, so this is a class of silent miscompile rather than
one missing case. This workaround comes out when that ships.
The regression test straddles the boundary with one character of keyword
name: [:combat :hi 1 2] is 32 bytes and always worked, [:combat :hit 1 2]
is 33 and panicked. It also covers a 48-byte key mixing strings with
keywords and ints, and multi-byte UTF-8 in a folded string term. Note it
cannot fail softly — drop the `vec` and the panic takes the whole suite
down rather than reporting an assertion failure, which for a silent
miscompile is the point. Verified: `lg xsofy/test/run.lg` exits 1 without
the fix and 0 with it.
The existing coverage could not have caught this.
with-folded-salt-equals-combine-fully-literal uses [:combat :hit] at 15
bytes, and the other three with-folded-salt tests exercise paths that
build their vectors at runtime, where a PersistentVector is harmless.
331 tests, 2870 assertions, 0 failures. Layer-1 smoke passes.
Refs #204
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mparrett
approved these changes
Sep 21, 2026
mparrett
left a comment
Collaborator
There was a problem hiding this comment.
No findings.
Verified locally:
- Reproduced the pre-fix
index out of range [-1]panic at the 33-byte boundary on the base commit. - Confirmed the patched expansion forces the folded constant back to
ArrayVectorand returns the same hash ascombine. lg xsofy/test/run.lg: 331 tests, 2,870 assertions, 0 failures, 0 errors.make smoke-lg: passes.
The workaround is narrowly scoped, and the regression test covers both sides of the 32/33-byte boundary plus larger and multibyte inputs.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
with-folded-salt's fully-literal branch crashes at runtime for any salt-key encoding to 33 bytes or more. Fixes #204.What was happening
The branch encodes the whole salt-key at expansion time and splices the byte buffer into the emitted form as a compile-time constant.
encode-saltbuilds that buffer withconj, and let-go'sconjpromotesArrayVectortoPersistentVectorabove 32 elements.let-go's
compileFormtype switch has a case forArrayVectorTypebut none forPersistentVectorType, and nodefault:— so the form emits no bytecode at all. The expansion compiled clean and then panicked on a short stack when called:33 bytes is not an exotic size. The macro's own docstring example,
[:combat :hit attacker target], reaches it the moment its variable terms are literals.The fix
One
vec, which rebuilds the buffer as anArrayVector.intowould not do: it goes throughconjand promotes straight back. The macro's other three splice sites already go throughvec, which is why this was the only branch affected — and why the partial-literal path, which people actually hit in the inner loop, kept working and hid this.Filed upstream as nooga/let-go#943 with a core-only 8-line repro. That issue also covers the missing
default:, sinceSortedMapTypeandSortedSetTypeare absent from the same switch and fail identically — it is a class of silent miscompile, not one missing case. This workaround comes out when the upstream fix ships; the comment says so and names the test that will catch it going early.The test
Straddles the boundary with one character of keyword name:
[:combat :hi 1 2][:combat :hit 1 2]Also covers a 48-byte key mixing strings with keywords and ints, and multi-byte UTF-8 in a folded string term.
Worth knowing: this guard cannot fail softly. Drop the
vecand the panic takes the whole suite down rather than reporting an assertion failure. For a silent miscompile that is the point. Verified both directions —lg xsofy/test/run.lgexits 1 without the fix, 0 with it, so CI does catch it.Why nothing caught it before
with-folded-salt-equals-combine-fully-literaluses[:combat :hit]— 15 bytes, under the threshold. The other threewith-folded-salttests exercise the partial-literal, no-literal-prefix and non-vector paths, which build their vectors at runtime where aPersistentVectoris harmless. So the fully-literal path had coverage that could not fail.Verification
make smoke-lgpassesvecproduces an equal vector, and the existing golden-value tests pin thatBranched off
main; touches onlywith-folded-salt, so it is independent of #202.🤖 Generated with Claude Code