fix(reader): set literals read as sets, not as (hash-set …) - #736
fix(reader): set literals read as sets, not as (hash-set …)#736nnunley wants to merge 4 commits into
Conversation
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
The representation change exposes three additional traversal gaps:
- Deftype/defrecord field references inside sets no longer compile.
-dt-rewriteinpkg/rt/core/core.lgtreats non-sequential forms as atomic. Sets previously arrived as(hash-set …)lists, so their elements were recursively rewritten. On this branch:
(defprotocol SetFieldP (field-set [this]))
(deftype SetFieldBox [x] SetFieldP (field-set [this] #{x}))
(field-set (->SetFieldBox 7))fails with Can't resolve x in this context; the base branch returns #{7}. Please add set traversal there and in the native dt-rewrite-field equivalent.
-
Native IR lowering does not macroexpand set elements.
expand-allinpkg/rt/core/ir/passes/pipeline.lgdescends into vectors and maps but not sets. A gogen fixture with(defn run [] #{(when true 1)})returns#{1}under bytecode but#{(if true (do 1) nil)}under native dispatch. The gogen trampoline passes onmainand fails on this PR.expand-allneeds a set branch beforebuild-setreceives the elements. -
Syntax-quoted sets bypass unquote and symbol processing.
syntaxQuoteinpkg/compiler/reader.gohandles vectors, maps, and lists, but the newSetTypefalls into the default quote branch.(let [x 1] `#{~x})now yields#{(unquote x)}instead of#{1}; gensym and namespace qualification inside syntax-quoted sets are likewise skipped.
The regular go test -count=1 ./test/ ./pkg/... suite passes and make check-generated is clean, so these need focused regression coverage.
Performance: branch vs mainThe ratchet's baseline (
Branch measured against
Allocation counts are identical between control and branch — Interleaved A/B
Every family median lands within ±0.8% (largest: Limits of the above
Separately, |
9120285 to
96647a9
Compare
|
Addressed in Your three items — all three reproduced
Two of them predate this PRProbing each site with map/set/vector/list literals turned up more than set gaps:
So item 1's map half and item 2 are pre-existing defects this PR surfaces, not causes. Item 3 is genuinely new. A fourth site
Why not just add
|
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
All three items from the first round reproduce as fixed. I built 96647a94 and its stack base ca44b6d7 and ran ~40 probes across both:
| probe | base ca44b6d7 |
branch 96647a94 |
|---|---|---|
#{x} in a deftype method |
#{7} |
#{7} |
{:k x} in a deftype method |
Can't resolve x |
{:k 7} |
`#{~x} |
(hash-set 1) |
#{1} |
The claim that the map half of item 1 predates this PR checks out: the base binary errors on {:k x} too. go test -count=1 ./test/ ./pkg/... is green, make check-generated is clean, make gogen-diff passes, and every probe returns identical output under -tags gogen_ir.
On the wider reshape: the expand-all set arm closes a divergence where bytecode expanded a set element and native lowering did not, which is worth having on its own. Three things need another pass — a new regression in the reader, and two in the lattice dispatch.
Duplicate element forms now evaluate once
reader.go:819 builds a set of forms, so two =-equal element forms collapse before compilation and only one of them is ever evaluated:
#{(gensym) (gensym)} base: #{G__1 G__2} branch: #{G__1}
#{(swap! c inc) (swap! c inc)} base: counter 2 branch: counter 1
Clojure's reader rejects this outright with Duplicate key, so silently evaluating one of the two is a behavior neither old let-go nor Clojure has. No test covers it. Either throw the way Clojure does, or carry the elements as a slice through the read and build the set at compile time.
The coll? guard cannot fire
core.lg:2187, pipeline.lg:240, and pipeline.lg:423 all end the dispatch with (if (coll? form) (throw ...) form). But coll? is (or (list? x) (vector? x) (map? x) (set? x) (seq? x)) (core.lg:2036), and every one of those is matched by an earlier arm, since list? implies seq?. Nothing reaches the throw. I checked the escape hatch too: a record is coll? false, so it still passes through as an atom.
So the stated rationale — the next collection type added to the reader fails loudly here instead of silently skipping rewrite — does not hold, because coll? would not know that type either. The native isColl at lang.go:2284 tests the vm.Collection interface and would catch records and queues, but core.lg's defn shadows it. For the loud failure, the fallthrough has to test something other than coll?: an allowlist of atom predicates, or the native check.
Dead FormSource call
reader.go:820 still calls vm.FormSource.Set(result, ...), but formSourceMap.Set stores only *List and *Cons (source.go:296), so it is a no-op for *PersistentSet. Nothing degrades: error spans inside a set literal still point at the right line, and the frame label improved to "compiling set element". The call and the startLine/startCol computation above it are now dead.
Before this can merge
- CI has never run on this PR.
statusCheckRollupis empty and there are no workflow runs for96647a94or the branch, while #734 from the same fork is green across 19 checks. mergeStateStatusisDIRTY. The merge is clean locally because the repo's merge driver regeneratescore_compiled.lgbandgenerated.sums; GitHub has no driver, so it needs a rebase onto3d8c9d93.- The base is
main, notwarn-on-core-shadow-fix, so the PR carries #734's three commits and shows 21 files / +395 where the net change is 10 files / +255. Retarget it, or land #734 first; otherwise the squash message describes the wrong change.
|
To clarify, since the above reads flatter than I meant it:
Nothing else in the reshape needs changing from my side. |
(read-string "#{1}") returned the form (hash-set 1) — a constructor call —
where Clojure returns the set #{1}. Maps and vectors already read as data;
sets were the only collection literal that did not, so (set? (read-string
"#{1}")) was false and anything reading EDN got a list back.
readSet now builds the set value. Evaluation is unchanged: compileForm's
vm.SetType case already emitted the same hash-set invocation and compiled each
element, so #{x (f y)} still evaluates its elements.
Three downstream sites had assumed a set literal always arrives as a list,
because before this it always did:
- ir/build.lg — build-form threw `unrecognized form #{entry}` on the first
quoted set it saw. Adds build-set (modelled on build-vector) plus the set
cases in captures-of and free-vars, so a value captured or free only inside a
set literal is still walked.
- ir/lower_go.lg — a set reaching :const had no Go emitter, and the lowered
tree failed to compile with unused locals and a missing return in BuildFn
(build-fn* uses #{} as a reduce init). Adds boxed-set-expr → vm.NewSet.
core.lg's `case` macro needed nothing: it already tested (or (set? test-val)
set-literal?), accepting both shapes.
Verified: make generate + make check-generated clean, the lowered tree compiles
and dispatches natively, ./test and ./pkg/... pass, and
test/set_literal_reader_test.lg pins the data/eval split in both directions —
read-string returns a set, quoted set literals hold unevaluated elements,
unquoted ones still evaluate, and ' @ still expand to forms.
… type
Set literals reading as data exposed traversal gaps at four sites that
dispatched on vector?/seq? and returned every other collection unchanged.
Two of the gaps predate this PR.
core.lg -dt-rewrite map literals have NEVER been rewritten:
{:k field} in a deftype/defrecord method
fails on main with "Can't resolve field in
this context". Sets were rewritten only by
accident, via the seq arm, while they still
read as (hash-set ..) lists.
pipeline.lg dt-rewrite-field the native-lowering twin; its docstring says
it mirrors -dt-rewrite, and it mirrored the
defect too.
pipeline.lg expand-all set elements were never macroexpanded. Also
pre-existing and previously unreachable, so
bytecode and native lowering disagreed on
#{(when true 1)}.
reader.go syntaxQuote had VectorType/MapType/ListType but no
SetType, so `#{~x} produced #{(unquote x)};
unquote, gensym and ns-qualification were all
skipped. Adds flattenSet alongside flattenMap.
Each site now keys off the collection lattice: associative arms BEFORE
vector? (a map entry IS a vector, so descending into it as a 2-vector
rewrites the key as a value), and an unrecognised coll? throws instead of
falling through as an atom. That makes the next collection type added to the
reader fail loudly here rather than silently skipping traversal.
Also corrects two comments in core.lg and pipeline.lg that still asserted
set literals read as (hash-set ..) forms.
Regression coverage, each failing on main:
defrecord_field_scope_test.lg map values, map KEYS, set elements, and
collections nested inside a map
special_forms_test.lg unquote / unquote-splicing / nesting inside
a syntax-quoted set
expand_all_collections_test.lg macroexpansion inside every collection kind
Validation: check-generated clean; go test ./test/ ./pkg/... green;
make gogen-diff (TestGogenAOTDiff) green; make ir-stress-gate 2503/2514
native, 11 failures within the known baseline bucket.
Set literals are sets of forms, so constructing the set in one step silently collapsed equal forms before compilation. Reject duplicates during reading, matching Clojure and preserving the old guarantee that duplicate side-effecting forms are never silently evaluated once. Remove the no-op set FormSource write and the unreachable coll? guards whose comments promised a loud fallback they could not provide.
96647a9 to
f011633
Compare
|
Second-round findings are fixed at |
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
The original findings are fixed, but this new set traversal introduces a scoped native correctness regression.
expand-all collapses distinct source forms after macroexpansion
The new set arm in pkg/rt/core/ir/passes/pipeline.lg rebuilds the expanded elements with (into #{} ...). Two distinct source forms are therefore deduplicated if macroexpansion makes them equal, before either form is evaluated.
Reproducer:
(defn run []
(let [c (atom 0)]
#{(when true (swap! c inc))
(if true (do (swap! c inc)) nil)}
@c))when expands to the second if form. Rebuilding the form collection as a set drops one element, so only one swap! remains in native lowering.
Verified with the repository's gogen trampoline:
| revision | bytecode | native | result |
|---|---|---|---|
base b32acf6 |
2 | 2 | PASS |
PR head f011633 |
2 | 1 | FAIL |
This is therefore introduced by this PR, not a pre-existing map/set traversal defect. The simple #{(when true 1)} test passes because there is no second form to collide with after expansion.
Please preserve the expanded element sequence until runtime construction—e.g. produce a (hash-set ...) call or use another non-deduplicating intermediate representation—instead of rebuilding macroexpanded forms with into #{}. A regression test should assert bytecode/native parity and that both side effects occur.
|
Remediation published at
Evidence: focused reader tests, compiler suite (61), short suite (1,734), compatibility suite (243), generation/check-generated, gogen-diff, IR stress, and independent re-review passed. The published head now has no failing GitHub checks (14 passed; non-applicable checks skipped). |
|
Rechecked the published head That commit correctly addresses reader-conditional splicing inside set literals, but it does not change the blocking The existing changes-requested finding therefore remains open: macroexpanding set elements into |
2 participants
Summary
(read-string "#{1}")returned the form(hash-set 1)where Clojure returns the set#{1}. Maps and vectors already read as data; sets were the only collection literal that did not.Stacked on #734.
Before
Genuine reader macros were never the issue —
'sym,@xand#'fooexpand to forms in Clojure too.#{}was the outlier.Change
readSetbuilds the set value instead of consinghash-setonto a list. Evaluation is unchanged:compileForm'svm.SetTypecase already emitted the samehash-setinvocation and compiled each element, so#{x (f y)}still evaluates its elements.What that surfaced
Three sites had assumed a set literal always arrives as a list, because before this it always did.
ir/build.lg—build-formthrew on the first quoted set it saw:from
test/gogen_nth_non_int_test.lg, whose trigger form containsvisited #{entry}. Addsbuild-set, modelled onbuild-vector, plus theset?cases incaptures-ofandfree-varsso a value captured or free only inside a set literal is still walked — the same reason the map cases exist.ir/lower_go.lg— a set reaching:consthad no Go emitter, so the lowered tree stopped compiling:That is
BuildFn, becausebuild-fn*uses#{}as areduceinit (build.lg:1533). Addsboxed-set-expr→vm.NewSet, alongside the existing vector, list and map emitters.core.lgneeded nothing: thecasemacro already tested(or (set? test-val) set-literal?), so it accepted both shapes.Tests
test/set_literal_reader_test.lg, 15 assertions across three deftests, pinning the data/eval split in both directions:read-stringreturns a set, isset?, equals#{1 2}, and is not a list;#{}reads as an empty set.'symand@xstill expand to forms.#{1 2 x}withxbound,#{(+ 1 1)}).'#{a b}equals#{'a 'b}, and'#{entry}contains the symbol, which is the IR case that was previously unreachable.Validation
make generate+make check-generatedclean; lowered tree compiles and dispatches natively under-tags gogen_ir.go test ./test/ ./pkg/...green.