Fix/ exponential compile time in builder chains - #2
Merged
Conversation
…en comments - usingNonEmptyImpl now requires TARGET_TYPE =:= Option[payload]; a strict subtype like Some[String] previously escaped the quoted pattern's conformance check and crashed the expansion with an ExprCastException. Covered by a new compile-time test. - getHandledFields identifies *: by symbol instead of by name string. - ExternalAccessSpec asserts the "cannot be accessed" diagnostic, so the tests cannot pass on an unrelated error mentioning the member name. - Comment pass: dropped the addHandlerCall splice-once note (contradicted by the PR's own analysis), stale narration in compileImpl, and the doc blocks that restated signatures; trimmed the survivors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The three impls each hand-assembled extract -> duplicate-check -> emit, and the pre-3.4 `t & Tuple` bound recovery had two emit sites. registerField now owns the pipeline (handler absent = ignoring), absorbing newHandledType and addHandlerCall. getHandledFields takes Handled directly instead of a Type[?] round-trip, and its terminal EmptyTuple check compares symbols instead of invoking the type comparer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ameter extractFieldName accepted any Select, so `using(_.a.b)` registered the source type's field "b" while the handler read a.b -- marking b handled with no real handler and defeating the completeness check. The receiver must now be the lambda's own parameter. Also: usingNonEmpty's non-Option error prints short type names (String, not scala.Predef.String), its scaladoc no longer claims the method is unavailable for non-Option targets, and the duplicate check gains an ignoring-then-using test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- registerField splices `name *: Handled` directly; the `'[t]` re-match with the `t & Tuple` bound recovery was never needed since `Handled`'s Tuple bound is statically known, and getHandledFields loses its AndType-stripping case with it. - extractFieldNameOrAbort uses `underlyingArgument` and the `Lambda` extractor instead of hand-rolled Inlined/Typed/Block stripping and DefDef dissection. - checkNotAlreadyHandled inlined into registerField (single caller). - CaseCompleteImpl drops pair-destructuring ceremony. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nostics
Review fixes:
- Selectors must name a case field: `_.productArity` or a body val is a
Select on the lambda parameter too, and previously registered a phantom
handler that ran on every eval outside the completeness check's universe.
- usingNonEmpty's error no longer reads as self-contradicting for
Some[String] ("must be exactly Option[...]" instead of "an Option").
- getHandledFields states the invariant its structural decoding relies on,
and the abort distinguishes a user-widened builder type (with actionable
advice) from a genuine internal error. The widened case is reachable by
ascribing a chain to `CaseCompleteBuilder[..., ?]` and compiling.
- ExternalAccessSpec now pins all four package-private members
(addHandler and handlers joined markHandled and the constructor).
Cleanups:
- caseFieldNames is the single definition of the handleable-field universe,
consulted by both registerField's gate and compileImpl's completeness check.
- registerField takes the handler by name, so usingNonEmpty's target-type
check runs after the shared selector/case-field/duplicate checks and all
three entry points report selector errors with the same precedence.
- The Option-target check matches the type constructor's symbol structurally,
replacing the quoted pattern + `=:=` guard pair.
- The 14 copies of the typeCheckErrors assertion scaffold collapsed into
inline helpers whose failures print the actual compiler messages.
- Two new tests pin that ignoring and usingNonEmpty route through the shared
pipeline rather than only using.
Tests 17 -> 24.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Handling a body val or parameterless method is harmless: it cannot share a name with a constructor field, so it can never satisfy another field's completeness obligation, and compileImpl only demands the constructor fields. Drop registerField's case-field gate and pin the new contract: extra members can be handled and evaluated, but do not count toward completeness. Nested selectors stay rejected. Also, from review: - Fix the self-recursive MovieFilter test fixture (its object initializer called its own companion apply, blowing the stack on any runtime instantiation). - Share one compile-error assertion helper between the specs. - Hoist the duplicated four-step handler chain in the extra-fields tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Motivation
A downstream project (~3,000 files) reported that files with long
CaseCompletebuilder chainsdominated its build: ~21 s of a 130 s build was spent in the compiler's
posttyperphase acrossfour files, versus ~2.5 s of posttyper for the other ~3,000 files combined. A 16-step chain cost
13.5 s; the same file at 10 steps had cost 0.3 s a few months earlier.
Chain length had become a compile-time cliff — every field added to a long chain roughly doubled
that file's compile time.
Root cause
usingNonEmptywas atransparent inlineextension method.A
transparent inlineextension binds its receiver to a parameter proxy carrying thetransparent-refined type of the entire preceding chain. Chaining those makes compilation
exponential in chain length (~1.9× per step), even though the generated tree stays linear — the cost
is in repeated traversal of the resulting nested singleton-type chain, which is why it lands in
posttyperrather than in macro expansion.Measured per-variant (posttyper, ms), which isolates it cleanly:
ignoring(method on the class)using(method on the class)usingNonEmpty(extension)The blowup is independent of handler body size — a chain of
usingNonEmpty(_.f)(v => v)coststhe same as one with multi-line handler bodies.
A previously suspected cause that turned out to be wrong
usingImplspliced$buildertwice, which looked like it would duplicate the receiver tree per stepand give
O(2ⁿ). It does not: the inliner binds the inline receiver to aCaseCompleteBuilder_thisval, so a second splice copies an
Ident, not the chain. Verified three ways — the step-1 handlermarker appears exactly once in a 16-step
-Xprint:posttyperdump; an ablation bindinghandlersto a single val changed nothing (4033 ms → 5086 ms); and plain.usingchains measuredflat from N=4 to N=20 against the pre-change library.
The fix
usingNonEmptymoves from a companionextensionblock ontoCaseCompleteBuilder, using a matchtype so the handler keeps exactly the same expected result type it had before:
An evidence parameter (
Option[P] <:< TARGET_TYPE) was rejected: it would infer the payload fromthe lambda body instead of propagating it as the expected type, silently changing inference for
targets like
Option[Any]. TheAnyfallback is deliberate — it keeps the match type reducible fornon-
Optiontargets so the macro can report an actionable error instead of the raw"match type reduction failed".
The existing
usingNonEmptytest passes unmodified — that's the drop-in check.Results
posttyper, ms:
Linear through N=40. At 20 steps: 54 s → 0.11 s.
Two further scaling fixes found along the way
getHandledFieldsdecodedHandledwith quoted type patterns. Every chain step ran'[head *: tail]+Type.valueOfConstantover the whole accumulated tuple to check for duplicatefields — one type-comparer invocation per element, so quadratic over a chain. It is now a structural
TypeReprwalk. Measured on a 96-stepusingchain, 3 runs each, fresh compiler per run:Noise at 32 steps; ~31% at 96. The explicit "not a tuple" abort is preserved — silently returning
partial results would turn a malformed
Handledinto a spurious missing-handler error.CaseCompleteImpl.evalre-sorted the handler map on every call, recomputing an ordering fixed atconstruction. It now sorts once. Measured at 32 fields, 1M iterations after 200k warmup, JVM 21:
5.3× faster, 2.8 KB less garbage per call. Ordering is unchanged and now has an explicit test.
Hardening:
Handledcan no longer be forgedThe macro plumbing (
addHandler/markHandled) isprivate[casecomplete], and so are theconstructor and
handlers. Without the latter two the guard was decorative — a user outside thepackage could write
new CaseCompleteBuilder[F, T, ("a", "b")](map).compileand claim fields thathave no handler, defeating the library's core guarantee. Generated code still reaches these because
quoted calls resolve at macro-definition site rather than at the splice site.
Nested selectors are rejected. The selector's receiver must now be the lambda's own parameter.
Previously
using(_.a.b)accepted anySelectand registered the innermost name — so it markedthe source type's field
bas handled while the handler reada.b, silently defeating thecompleteness check (
using(_.a.b) … .ignoring(_.a).compilepassed withbunhandled). Suchselectors now fail with the existing "expected a field selector" error.
Also in this change
usingImplsplit intousingImpl/usingNonEmptyImpl/ignoringImpl, all delegating to asingle
registerFieldthat owns the shared pipeline: selector extraction, duplicate check, and theemit under the extended
Handledtype. The old shape encodedignoringasusingImplwith'{ None }plus a runtime.foldin the generated code to express a purely compile-timedistinction; that and the per-step
Some/Noneallocation are gone. EveryExpris splicedexactly once.
usingNonEmptyrejects target types that are strict subtypes ofOption(e.g.Some[String]) with the same readable error. The quoted pattern'[Option[payload]]matches byconformance, so without the
=:=guard those types passed the match and crashed the expansionwith an
ExprCastException.AppliedType/@uncheckeddestructuring replaced with the quoted type API.revision briefly rejected such selectors, but registering e.g. a body val is harmless: it can
never share a name with a constructor field, so it can never satisfy another field's completeness
obligation. Both directions are now pinned by tests: extra members can be handled and evaluated,
and they do not count toward completeness.
timeout-minutes: 15on the CI job.LongChainSpecfails by hanging, so under GitHub's360-minute default a regression would burn a 6-hour job and read as flaky infra rather than as this
test failing.
Tests
4 → 24.
LongChainSpec— a 32-step regression guard interleavingusingNonEmpty,usingandignoring(all three are equally at risk). Pre-fix this would take hours to compile; post-fix thefile costs ~0.25 s.
ExternalAccessSpec— every other test lives insideio.github.stivens.casecomplete, whereprivate[casecomplete]is indistinguishable from public, so the access story was untested fromwhere users actually sit. This spec sits outside the package and pins both directions: a full chain
compiles and evaluates, while
markHandledand the constructor are unreachable — asserted on the"cannot be accessed" diagnostic, so an unrelated error mentioning the member name cannot pass.
CaseCompleteSpec— missing field, duplicate field (bothusing-then-usingandignoring-then-using), non-selector expression, nested selector (forusingandignoringalike), non-Optiontarget, strict-Option-subtype target, selectorerrors taking precedence over
usingNonEmpty's target check, a body val not counting towardcompleteness, a readable diagnostic for a builder ascribed a widened type, plus a positive
control so the negatives cannot pass vacuously. This path had zero coverage before. The negatives assert the
error message text, not merely that compilation fails: the
Anyfallback inOptionPayloadexists only to make that message readable, and could be deleted with a failure-only test still
green.
evalcalls, the invariantthe sort-once change must preserve.
Compatibility
usingNonEmptymoved from the companion object to the class,usingImpl's signature changed, and the builder constructor andhandlersbecame package-private— source-compatible for normal usage, but not binary/TASTy-compatible. Already-compiled call sites
are unaffected (they are already expanded); users must recompile. The README keeps 0.2.2 until
0.3.0 is published.
usingNonEmptyon a non-Optiontarget previously emitted twocopies of
Match type reduction failed since selector String; it now emitsusingNonEmpty requires the target type to be an Option, but it is String. Use ``using`` instead._.a.bno longer compile (see the hardening section) — previouslyaccepted, but with semantics that broke the completeness guarantee.
evalordering,ignoringdfields still contribute nothing.