Conversation
defmulti, defmethod, hierarchy ops (isa?, derive, underive, parents, ancestors, descendants, make-hierarchy), and method-table ops (get-method, methods, remove-method, prefer-method, prefers) live in a new src/squint/multi.js module, imported only when one of those forms appears in user code — programs that don't use multimethods pay zero bundle cost.
core.js and multi.js both need the keyword/map-as-fn coercion; instead of duplicating, move it to a new src/squint/internal.js module clearly marked as not-public-API, and import from both sides. Bundle cost is unchanged — programs that pull in core.js already transitively loaded toFn's logic.
Switch core.js and multi.js to import via relative './internal.js' paths instead of the bare 'squint-cljs/…' specifier, and drop internal.js from package.json's exports map. Consumers can no longer reach it with `import 'squint-cljs/src/squint/internal.js'`, making the not-public-API intent enforceable rather than aspirational. The file still ships via `files` so the relative imports resolve inside installed node_modules.
Users can now extend reporting the same way they would in cljs.test: (defmethod cljs.test/report [:cljs.test/default :begin-test-var] [m] ...) Dispatch is on [*current-reporter* (:type m)], defaulting to the built-in :cljs.test/default reporter. Unknown combinations fall through to a no-op :default method so custom reporters don't crash on events they don't handle. test-var now also emits :begin-test-var / :end-test-var events (previously never fired), so reporters can bracket per-var output. Counter increments move from an unconditional top-of-report block into the :pass/:fail/:error default methods — matching cljs.test, where reporters that don't want the default counter behavior can opt out. Regenerates src/squint/test.js and adds a smoke test exercising the extension point end-to-end.
Add squint-cljs/src/squint/multi.js to the playground's importmap so compiled code using defmulti/defmethod can resolve the runtime module, and extend the bb init copy list to stage multi.js + internal.js alongside the existing runtime files in public/public/src/squint/. internal.js is not in the importmap — core.js and multi.js reach it via a relative import, so it just needs to sit next to them on disk.
The playground's dev-mode rewrite sent 'squint-cljs/core.js' through
/js/squint-local/core.js (the 38-byte root shim that re-exports from
./src/squint/core.js). Vite transformed that shim's relative import
into a /@fs/.../src/squint/core.js URL.
Meanwhile the same import inside already-served runtime files
(test.js, multi.js, etc.) was left as a bare specifier and resolved by
the browser via the importmap to /src/squint/core.js. Two URLs, same
file — and ES-module caching treats them as two separate modules.
Each gets its own _metaSym; with-meta written through the user side
becomes invisible to meta reads inside the runtime, which is why
deftest'd tests surfaced as 'anonymous' in :begin-test-var events.
Rewrite 'squint-cljs/{,src/squint/}X' straight to /src/squint/X so
user code and importmap-resolved imports converge on one URL, one
module instance. Confirmed against a live Vite+Playwright repro:
before, sameModule=false; after, sameModule=true and deftest names
surface correctly end-to-end.
new Map(h.parents) copies the outer Map but shares the inner Set values, so _deriveInto on the 'next' hierarchy mutates Sets that the caller's original hierarchy still points at — a silent cross-contamination. Add a cloneHierarchy helper that rebuilds each field with fresh Sets, and use it in derive. core.js already has a copy() helper, but it's shallow in exactly the same way and wouldn't help here. Regression test in multi_test.cljs.
2-arg derive mutates *global-hierarchy* in place, so MultiFn.getMethod's identity-based cache check (cachedHierarchy !== hierarchy.deref()) never fires. A value cached when only one isa match existed keeps resolving to the stale fn even after a subsequent derive introduces ambiguity. Test currently fails — fix in a follow-up commit. Also saves the PR-review follow-up list to todo.md.
Hunting the stale-cache repro surfaced a second bug that was hiding
it: _deriveInto iterated tag's ANCESTORS when propagating the new
relation, so (derive :x :a) followed by (derive :x :b) would also
make :a isa :b. With :a isa :b the ambiguity check in findBest
resolved cleanly ("a dominates b") instead of throwing, masking the
cache issue. Clojure's derive propagates to tag's DESCENDANTS, not
its ancestors.
Two fixes:
- 2-arg derive now rebuilds-and-swaps _globalHierarchy (like 3-arg
already did). Identity changes, so MultiFn.getMethod's hierarchy
identity check fires and the cache invalidates after each global
derive.
- _deriveInto's tagChain now walks h.descendants[tag] (the set the
new parent chain should apply to) instead of h.ancestors[tag].
Regression tests for both cases.
- #5: defmulti auto-wraps a plain (make-hierarchy) passed as :hierarchy. Previously only deref-able refs worked; a direct hierarchy object crashed on first dispatch with "deref is not a function". A plain hierarchy now gets wrapped into a frozen snapshot. Regression test defmulti-accepts-plain-hierarchy-test. - #6: the global hierarchy moves from a module-local `let` onto globalThis under Symbol.for('squint.multi.hierarchy'). If multi.js ever loads twice under different URLs (npm + CDN, symlink quirks, monorepo dedupe misses — same shape of bug as the playground _metaSym episode), all instances now share one hierarchy. Purely defensive; no user-facing test since observing it requires reaching into internals. - #3: softened the CHANGELOG zero-cost claim to "programs that use neither multimethods nor cljs.test" — test.js transitively pulls in multi.js now. - #4: CHANGELOG now flags the cljs.test/report defn→defmulti change as potentially breaking and explains the migration path. todo.md reorganised: #1/#3/#4/#5/#6 moved to Done, #2 (defmulti redef in REPL) remains under Defer.
Hoisting _globalHierarchy onto globalThis[Symbol.for(…)] was over-defensive. The playground bug we used as precedent was fixed by unifying URLs, not by papering over duplicate loads with shared state. Normal ESM semantics — one module instance per realm per URL — is the correct behavior, and every other module-level state in squint (including core.js's _metaSym) lives that way. cljs.core's *global-hierarchy* is module-scoped for the same reason. A user who ends up with two instances of multi.js under different URLs almost certainly has two versions of squint in their graph; sharing state across versions via globalThis would hide real version-mismatch bugs instead of surfacing them. Move #6 from Done to Wontfix in todo.md with the reasoning.
CI failed on ubuntu-latest (pass locally): global hierarchy is shared across tests in this ns, and two tests were both deriving :x against :a and :b. In whatever order shadow-cljs emitted them on CI, derive-does-not-leak-to-tag-ancestors-test ran first and left :x isa both :a and :b in the global hierarchy. Then two-arg-derive-busts-cache-test's first (k :x) — outside its try/catch — hit the ambiguity immediately → unhandled promise rejection → process exited non-zero after the summary was already printed. Switch to namespaced keywords per test (:leak/* and :cache/*) so the shared global hierarchy doesn't cross-pollinate between tests.
Clojure's signatures are (f tag) and (f h tag) — hierarchy FIRST when provided. The JS impl had them reversed (x, h), and the existing hierarchy test wrote (ancestors :a h0) — which happened to match the buggy shape, so the bug hid behind the test that was supposed to pin it down. A caller following the Clojure signature (ancestors h :foo) would compile to ancestors(h, :foo) — the impl would then try ":foo".parents and crash. - Extract a hAnd(a, b, field) helper that dispatches on arity: 1-arg reads the global hierarchy, 2-arg puts the passed hierarchy first. - Rewrite the existing hierarchy-accessor call sites in the immutable 3-arg derive test to the correct (h tag) form. - Add hierarchy-accessors-follow-clojure-signature-test pinning both arities with intentionally unambiguous expectations.
preferTable, ancestors, parents, and descendants are all Map<any, Set<any>> with dispatch values as keys — frequently vectors like [:km :m] that get freshly constructed by the caller. The existing impl used JS reference equality (Map.get / Set.has), so: - prefer-method with a freshly-read vector pair wouldn't see the earlier preference, and the cycle check wouldn't fire. - isa? on the same vector from two different call sites disagreed. - derive would create duplicate parent entries for the same tag if called twice with fresh vectors. addRel, _prefers, _isa, and _deriveInto now route their key/member lookups through findKeyByEquiv / setHasEquiv so value-equality decides map membership. isPrimitive + findKeyByEquiv move up near the other helpers and get a setHasEquiv sibling. Regression test prefer-method-vector-dispatch-equiv-test covers the cycle-detection and read-back paths.
Previously methodCache was populated only for primitive dispatch values (isPrimitive guard). Every vector dispatch — the common (defmulti f (fn [a b] [a b])) shape — re-ran findBest from scratch, which is O(methods × _isa-cost) per call. For workloads where the same [:from :to] tuple dispatches repeatedly, that work was pure waste. Split the read path in two: primitives stay on Map.get (O(1)); non-primitives scan the cache with _EQ_ so freshly-allocated structurally-equal vectors hit prior entries. Cache writes run unconditionally now, so the miss path populates the cache for any dispatch value. Deliberately not using JSON.stringify canonicalization — it collapses symbol/bigint/undefined/NaN to identical strings (correctness bug), and native Map keying via _EQ_ handles every primitive type correctly by language semantics. A trie or typed canonicalization is the next step if the linear scan becomes a bottleneck in practice.
…export The cycle between core.js (imports toFn) and internal.js (imports get) was safe today — ES modules handle cycles via hoisted function bindings, and `get` was only referenced from inside closures, not at top level — but the reviewer's concern was pattern hygiene: any future top-level reference on either side would TDZ at load time. Rather than invert the dep tree (which would mean moving get and its transitive deps List/LazyIterable/SortedSet into internal.js), just fold toFn back into core.js, renamed to __toFn so the double-underscore prefix signals "implementation-helper, not public API". multi.js imports it from 'squint-cljs/core.js' alongside _EQ_. Update bump-core-vars to exclude __-prefixed names so the compiler doesn't resolve a bare (__toFn …) in user CLJS as a core var. Names with __ embedded (IIterable__iterator, _GT__EQ_, clj__GT_js, …) are unaffected because the filter only matches the prefix. Drops: - src/squint/internal.js - its entries in package.json "files" and playground/bb.edn
The duck-typed .get fallback in core/get checked g via `instanceof Function`, which fails cross-realm — a function from a different realm (iframe, vm sandbox) is NOT an instance of the current realm's Function constructor. typeof is a language-level operator that works regardless of origin realm, and also skips the prototype-chain walk. Only one live instanceof Function remained (__toFn was already on typeof). Commented-out arg-validation block in map/1 left as-is since it's inert.
getMethod's no-match branch called findKeyByEquiv(methodTable, defaultDispatchVal) on every dispatch that fell through to the default — even though defaultDispatchVal is immutable after construction and the resolved fn only changes when methodTable does. resetCache already runs on every methodTable mutation (addMethod, removeMethod, preferMethod), so memoize the resolved default fn there once and have getMethod just return this.defaultFn. No-match path is now O(1) instead of O(methods) per dispatch.
Round-2 review caught that hAnd — the reader behind parents / ancestors / descendants — was doing raw h[field].get(tag) while the rest of multi.js had been switched to findKeyByEquiv. Result: (derive h [:km :m] :length) followed by (parents h [:km :m]) returned nil, even though (isa? h [:km :m] :length) saw the relation. Route hAnd's lookup through findKeyByEquiv so a freshly-allocated vector tag reads back the structurally-equal stored entry. hierarchy-accessors-equiv-on-vector-tags-test pins all four (isa?, parents, ancestors, descendants) of the reader paths with fresh-identity vectors. Uses `some` + explicit `=` instead of `contains?` because squint Sets are reference-equal for non-primitives — orthogonal gap worth noting but not in scope for this PR.
Everything in the file is either done, rejected in this PR, or a gap captured in commit messages / code comments / CHANGELOG. The one deferred item (defmulti redef wipes methods in REPL) is speculative — if a real user hits it we'll file a proper issue with context.
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
1 participant
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.
No description provided.