feat(interp): implement deftype — continuation of #356, with the merge damage repaired - #366
Merged
Conversation
…od/defrecord
`(defprotocol ^:private Driver ...)` reads as Meta(:private, Symbol("Driver")),
and require_sym matched only FormKind::Symbol, so the interpreter rejected a
form Clojure accepts. malli.impl.regex opens with five such protocols, which
made the whole of malli unloadable on cljrs.
Four defining forms shared the one helper and so shared the one bug. require_sym
becomes require_sym_meta, which peels nested Meta wrappers (`^:a ^:b x`, outer
mark winning) and RETURNS the metadata rather than discarding it: defprotocol
and defmulti attach it to the var they intern, so `^:private` survives instead
of being silently dropped. defmethod resolves an existing multi and defrecord
names a generated type, so neither has a var to carry it; both unwrap and
discard explicitly.
…s own name
Two lexical-scope defects, both silent until call time.
1. A defrecord's fields were unbound as bare symbols in a protocol method body
((mutable? [_] (valid-sha? sha)) threw "unbound symbol: sha"), because a
method impl is built as an ordinary fn whose only bindings are its params,
while Clojure compiles the fields as instance fields in scope. build_impl_fn
now wraps the body in (let* [f (:f this) ...] body) for each field a param
does not already shadow, threaded from defrecord through register_impls_for_tag.
reify (no fields) and extend-type/extend-protocol are unaffected.
2. A defn/fn param sharing the function's OWN name did not shadow it: the
self-reference was bound AFTER the params in the same frame, overwriting the
param. (defn text [text] {:text text}) returned the function as its :text.
Binding the self-reference before the params, in both the interp and tiered
apply paths, lets the param shadow it as Clojure does while the name stays
visible in the body.
Verified against the JVM: bare-field access, mixed field/this, param shadowing,
assoc-then-read, empty-field records, reify, extend-type, self-recursion,
name-in-body identity, arity-specific and destructured and rest params.
defrecord/reify/extend-type/extend-protocol looked up the protocol name with lookup_in_ns(current_ns, s), passing the whole symbol string. A qualified name — mp/IThing or mini.proto/IThing — is neither interned nor referred under that string in the current ns, so a cross-namespace protocol impl failed with "mp/IThing is not a protocol" even though the protocol was loaded and (resolve 'mini.proto/IThing) was truthy. This sank every DIP/port design on cljrs, where the protocol and its implementations live in different namespaces by definition. Added resolve_protocol_sym, which parses the symbol, resolves the namespace part through the current ns's :require :as aliases (falling back to the literal ns), and looks the name up there — the same resolution eval uses for any other qualified symbol. All three impl sites route through it; an unqualified name still resolves in the current ns. Verified against the JVM: defrecord (alias and fully-qualified), reify, extend-type, extend-protocol across a namespace boundary, plus the same-file control, all identical. cljrs-runtime suite 431/431.
deftype was a builtin wired to a nil stub, so (deftype T [x y]) took the function-application path, evaluated its args, and died on the unbound symbol T with "Unable to resolve symbol: T" — an error that names a symbol and points nowhere near the real cause. deftype is genuinely unimplemented (it needs mutable/volatile fields, set! over them, and array interop). Made it a special form that returns "deftype is not implemented (defining T); use defrecord where a map-backed type suffices" without evaluating its args, so the failure lands on the deftype line and says what is actually missing. Removed the unreachable builtin registration. defrecord and reify unaffected; suite 431/431.
…tocol impls) deftype was a loud "not implemented" special form. Make it real: a positional ->T constructor, protocol/interface method impls with the fields in scope (the machinery defrecord/reify already share), the type symbol interned so instance? resolves, and .-field access on type instances via dispatch_method. Extract the constructor / field-parse / symbol-intern helpers so deftype and defrecord share one definition rather than two divergent copies; defrecord now also accepts per-field metadata as a side effect. Immutable deftype only — mutable/volatile fields + set! land separately. clojure-test-suite: 5486/5486.
…tile-mutable) + set! A deftype field marked ^:unsynchronized-mutable or ^:volatile-mutable now carries an interior-mutable slot on the instance — an Atom cell over a keyword→value map. Method bodies read it live through (.-field this); (set! field v) inside a method and (set! (.-field inst) v) externally update the cell, and the in-method local snapshot is refreshed so read-after-write within one method is correct. Instances are !Send, so volatile and unsynchronized-mutable behave identically. - TypeInstance gains `mutable: Option<GcPtr<Atom>>`; assoc/assoc-in/with-meta preserve the cell; make-type-instance/reify/deserialize leave it None (serialize folds the slot values into the field map). - New make-type-instance-mut builtin; the deftype ->T ctor routes to it when the type declares mutable fields. - The IR lowerer declines to lower a set! whose target is a local binding (a mutable-field write), so a hot mutable-field method tree-walks instead of miscompiling to a global var store. - defrecord now shares the field-spec parser (Single-Source Lever). clojure-test-suite: 5486/5486; verified interactively (instance independence, >300-iter hot path, read-after-write).
… tests The merge of main into upstream/deftype resolved the conflict in interp/special.rs by keeping BOTH sides of eval_deftype: a truncated copy of the new one — its body cut off after the field-spec parse and an orphan fragment of intern_type_symbol's body spliced in, referencing `globals` and `ns` that do not exist there — followed by #355's old "not implemented" stub. That is E0428 plus two E0425, so cljrs-runtime did not compile at all: both the WASM build and Build/test/lint went red, the WASM job on the compile errors and the lint job on rustfmt drift the same merge left behind. Three things the merges silently reverted, restored: 1. eval_deftype's body. It registers the protocol impls with the mutable field names, builds the positional ->T constructor (routing to make-type-instance-mut when the type declares mutable fields) and interns the type symbol. The duplicate stub is gone. 2. defrecord's field parse, back onto the shared parse_field_specs rather than a second hand-rolled copy. parse_field_specs now reads the vector through `as_vector`, so a marker on the field vector itself — (defrecord R ^:marker [x]) — stays transparent for deftype as it already was for defrecord. 3. resolve_protocol_sym's three call sites (PR #354). The merges dropped all three back to lookup_in_ns(current_ns, "mp/IThing"), leaving the helper dead — which also fails `clippy -D warnings` — and cross-namespace protocol impls broken again with "mp/IThing is not a protocol". Everything outside special.rs survived and is unchanged: TypeInstance.mutable, the serialize fold, rt_assoc, the set! lowering declines, .-field dispatch and the make-type-instance-mut builtin. Nothing caught any of this, because nothing tested the behaviour — so: - deftype_types.rs: the constructor, .-field, instance?, the absent map->T, fields in scope in a method body, multi-protocol dispatch, set! in both forms, instance independence, and mixed immutable/mutable fields. - deftype_mutable_tiered.rs: its own binary so it can force eager IR lowering process-wide. Three of its four tests fail when the set!-on-a-local decline is removed ("IR interpreter: var not found user/n"), and the immutable control still passes — the tree-walking tests cannot discriminate there. - qualified_protocol_impl.rs: all five impl sites across a namespace boundary, by alias and fully qualified, plus the same-ns control. Six of the seven fail when resolve_protocol_sym is neutered. Docs: differences.md and TODO.md said deftype is not implemented; the crate READMEs now carry TypeInstance.mutable, the shared deftype/defrecord helpers, the .-field dispatch rule and the set! lowering decline. Verified: cargo fmt --check, clippy --workspace -D warnings, cargo test --workspace, the wasm32-unknown-unknown release build, and the AOT clojure-test-suite (310 tests / 5543 assertions, 0 failures). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wdq2umKkdALhZbbGMxM8n1
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL raised four high "cleartext logging of sensitive information" alerts on the three new test files, at the panic arms of eval_pr/eval_err. The finding is a false positive in substance — these are test helpers whose panic fires only when the test is already failing, and there is no secret in an interpreter value — but the dataflow behind it is real: `Value::Uuid(u128)` is a variant, so Debug-formatting ANY `Value` into a panic reads as writing a UUID to a log. Which type came back instead of a string is what these assertions are actually about, so report `type_name()` and the alert has nothing to follow. The same line exists in tests/defrecord_method_fields.rs, which this was copied from; CodeQL reports only alerts new in a PR's diff, so it is untouched here rather than widening this PR. 27 tests across the three binaries still pass; fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wdq2umKkdALhZbbGMxM8n1
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
4 participants
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.
Continuation of #356 (@BuddhiLW's
deftypeimplementation). This branch is #356's head (939d5cc) plus one commit that repairs what themainmerges into it broke, so it carries the original feature in full — merging this lands #356.Opened as a separate PR only because #356's head lives on a fork branch I cannot push to. The author's commits are unchanged and retain their authorship; my single commit sits on top.
Why a continuation was needed
#356 is red on both CI jobs. The two
mainmerges (a86d5fc,939d5cc) resolved the conflict incrates/cljrs-runtime/src/interp/special.rsby keeping both sides ofeval_deftype: a truncated copy of the new one — its body cut off after the field-spec parse, with an orphan fragment ofintern_type_symbol's body spliced in referencingglobalsandnsthat don't exist in that scope — followed by #355's old "not implemented" stub.That is
E0428plus twoE0425, socljrs-runtimedid not compile at all. The WASM build failed on those compile errors; Build, test, lint failed separately on rustfmt drift the same merge left behind (},match arms, a re-wrappedglobals.intern(…)).What the merges silently reverted
Three things, all confined to
special.rs:eval_deftype's body. Restored: it registers the protocol impls with the mutable field names, builds the positional->Tconstructor (routing tomake-type-instance-mutwhen the type declares mutable fields), and interns the type symbol. The duplicate stub is gone.defrecord's field parse, reverted off the sharedparse_field_specsonto a second hand-rolled copy — undoing the author's single-source consolidation.resolve_protocol_sym's three call sites (from fix(interp): resolve a qualified protocol name in impl position #354, included in this stack). All three went back tolookup_in_ns(current_ns, "mp/IThing"), leaving the helper dead — which also failsclippy -D warnings— and cross-namespace protocol impls broken again with"mp/IThing is not a protocol". This one was also lost onmainitself, where it is papered over with#[expect(dead_code)] // until next PR uses this; that attribute is no longer needed.Everything outside
special.rssurvived the merges intact and is unchanged here:TypeInstance.mutable, theserializefold,rt_assoc, theset!lowering declines inanf.rs,.-fielddispatch, and themake-type-instance-mutbuiltin.One deliberate deviation from the original
Restoring
parse_field_specsfordefrecordbrokemain'smeta_transparency::defrecord_name_and_field_vector: the author's parser matched&form.kinddirectly and so rejected a^:markeron the field vector itself, whichmain's hand-rolled parser accepted viaas_vector.parse_field_specsnow reads throughas_vector, satisfying both the author's single-source intent andmain's metadata transparency — and closing the same latent hole fordeftype, where(deftype T ^:marker [x])previously failed.Tests
None of this was caught, because nothing tested the behaviour — only compilation. Three new files, each verified to actually discriminate by reverting the fix underneath it:
deftype_types.rs(16 tests) — constructor,.-field,instance?, the absentmap->T, fields in scope in a method body, multi-protocol dispatch, bothset!forms, instance independence, mixed immutable/mutable fields.deftype_mutable_tiered.rs(4 tests, its own binary so it can force eager IR lowering process-wide) — 3 of 4 fail when theset!-on-a-local decline is removed (IR interpreter: var not found user/n), while the immutable control still passes. The tree-walking tests cannot reach that path at all, so this is where the tiered-safety half is actually pinned.qualified_protocol_impl.rs(7 tests) — all five impl sites across a namespace boundary, by alias and fully qualified, plus the same-namespace control. 6 of 7 fail whenresolve_protocol_symis neutered.Docs
docs/book/src/language/differences.mdandTODO.mdboth still saiddeftypeis not implemented. The crate READMEs now carryTypeInstance.mutableand its clone/serialize semantics, the shareddeftype/defrecordhelpers, the.-field-only dispatch rule on aTypeInstance, and theset!lowering decline.Verification
All four CI steps run locally on this head:
cargo fmt --check— cleancargo clippy --workspace -- -D warnings— cleancargo test --workspace— exit 0, no failureswasm-pack's underlyingcargo build --lib --release --target wasm32-unknown-unknown— buildsclojure-test-suite— 310 tests / 5543 assertions, 0 failures, 0 errorsGenerated by Claude Code