Pass already valid class instances through Schema make unchanged - #6917
Pass already valid class instances through Schema make unchanged#6917spencerbeggs wants to merge 4 commits into
Conversation
toConstructorAST rewrote every class Declaration into its constructor form unconditionally, so a field such as Schema.Array(MyClass) rebuilt each element even when it was already an instance of exactly that class, allocating a fresh tree and discarding the caller's references. Constructor-form ASTs are now tracked so makeParser can run the declaration and its checks directly against such an input and skip the encoding chain. A failure from that attempt is discarded rather than reported, so the encoding chain still produces the authoritative issue and error messages are unchanged. Only the constructor direction is affected; decoding and encoding are untouched. Closes Effect-TS#6890 Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
🦋 Changeset detectedLatest commit: f33e3db The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ One minor suggestion — otherwise looks great.
Reviewed changes — a single commit that skips the encoding chain in Schema.make when a class-typed input already satisfies its declaration and checks, preserving identity and avoiding redundant re-construction.
SchemaParserfast-path guard:toConstructorASTregisters constructor-form class ASTs in aWeakSet;makeParserspeculatively callsparseLocaland returns the input unchanged when it passes, falling through to the encoding chain on failure so error messages stay identical.- Tests: identity preservation for
Schema.Array(MyClass), constructor-default application for plain objects, and check-failure messages on nested class fields (check + makeFilter).
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — one merge commit (96444c81f) brings the branch up to date with main; the PR's own content (3 files: SchemaParser.ts, Schema.test.ts, .changeset/) is unchanged since the prior review at 6ca694e44. Prior feedback (non-blocking changeset style suggestion) was resolved.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
…class make test
The test was written against a base that predated the removal of actual values from schema issues, so it expected the old "Expected positive n, got {...}" rendering. The branch now carries that change and the assertion matches it.
Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — one commit (9225e3b) drops the , got {"n":0} suffix from a makeFilter failure message assertion to match the error format from #6912 and the other makeFilter tests in the file.
- Fixed test assertion format: the new class-make test at
Schema.test.ts:6495previously asserted an error message that included a, got {…}suffix; the filter issue format doesn't produce that suffix, so the assertion was wrong. Aligned with the format used by lines 6929–6930.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
1 participant
Closes #6890.
toConstructorASTrewrites every classDeclarationinto its constructor form unconditionally, so a field typedSchema.Array(MyClass)rebuilds each element onmakeeven when the element is already an instance of exactly that class. The result is a fresh tree and the loss of the caller's references.This registers the constructor-form ASTs it produces, and
makeParserconsults that set: when the input already satisfies the declaration and its checks, it is returned unchanged and the encoding chain is skipped. Only the constructor direction is affected, sincetoConstructorASTis reached solely frommakeEffect; decoding and encoding are untouched.The failure from that attempt is deliberately discarded rather than reported. The encoding chain below still runs and produces the authoritative issue, so error messages are byte-identical to before. That detail matters: a first attempt at this modelled the fast path as
Union([declaration, constructorForm]), which worked and was faster, but made every failedmakeon a class field carry a spuriousExpected B, got {...}prefix from the union's other branch. That approach is not in this PR.Measured
Table.make({ rows })over 30,000 pre-builtRowinstances, median of 15 runs after warmup, Node 26.5.0:Schema.Array(Row)For reference the issue's
Schema.Array(Schema.Union([Row]))workaround measures 1.84ms, so the class spelling is now comfortably ahead of it. (These numbers are measured on this branch after merging #6912; an earlier revision of this description quoted 8.69ms/1.51ms from the pre-merge base.)On the issue's second suggestion
#6890 offered an alternative: document the re-construction as the class contract and treat the union spelling as the sanctioned identity-preserving form. That does not hold up. The union spelling is not an equivalent schema in the constructor direction — it rejects plain-object input outright and silently skips constructor defaults inside its members:
The cause is that
toConstructorASThas nocase "Union", so unions fall through todefault: return astand never receive constructor semantics at all. Its identity preservation is a side effect of that gap. I have left that behavior alone here — it looks like a separate bug, and I did not want to widen this PR into it — but it is worth a maintainer's eye.Also worth correcting from the issue: the reported 126x was a cold single-shot measurement on beta.101. On
mainthe steady-state gap is about 5x.Validation
pnpm lint-fix,pnpm check, andvitest run --project effect— 244 files, 7658 passed, 3 skipped, including the full schema suite.On the two new tests: disabling the guard makes
make passes already valid class instances through unchangedfail withValues have same structure but are not reference-equal, so that one discriminates this change directly. The second test does not — it still passes with the guard off, because guard-off is simply the old behavior. It is a regression guard aimed at theUnionshape described above, which is the mutant it actually catches.Behavior change to weigh
makeno longer returns defensive copies of class inputs. Anyone relying on the previous re-construction to isolate caller-held references will now observe the originals. That is the point of the change, but it is a visible semantic shift and belongs in a maintainer's judgement rather than mine — happy to gate it behind an option instead if you would prefer.Signed-off-by: C. Spencer Beggs spencer@beggs.codes