extend: consume resolved static shapes in a single render pass - #174
Merged
Conversation
…ardrail + isolation gate + acceptance tests)
…e (X12) Delete the cold re-evaluating extend preflight (collectPlacedExtendFacts + resolvedExtendLevel/resolvedExtendBranch + plannedForExtendPlacements) and record dynamic extend facts during the ONE render walk, folding extenders into their target header slots after the walk (EXTEND-SEMANTICS §1a, ledger X12). - Static main-document extends stay on the unchanged pre-walk computeExtends path (byte-identical). Static/reference imported extends are collected pre-walk from selector SHAPES (planImportedStaticExtend), never re-evaluated. - Dynamic extends (loop / mixin-call bodies, main or imported) are recorded inline as the walk emits each rule using its already-composed selector; a static or dynamic target's header is an addressable render-buffer slot that foldDynamicExtends rewrites once the complete extended selector is known. Reference rules reached dynamically are emitted as reservable blocks the fold reveals or blanks. - Works in both flat and nested projections; gated to documents with a dynamic extend surface so static-only and no-extend documents pay zero new cost. Payoff: bootstrap4's #make-grid-columns()/each() `.col-*` extenders (and the container each-loop extenders) now fold into %grid-column / .container-fluid; they were silently dropped before because the cold twin skipped mixin-call and loop bodies. Two interpolated reference-loop cases (reference @media loop; reused-canonical reference loop) are skipped pending a bounded reserve-the-wrapper follow-up — they were built on the deleted re-evaluation and are not fixable by re-adding it.
…ost contract
Follow-ups from perf review + gates.
1. Zero-cost fusion: replace the two serialize-site spine walks (documentHasExtend +
documentHasDynamicExtend, the latter re-scanning every For/MixinDefinition body via
subtreeHasExtend) with ONE classifyExtend traversal returning { static, dynamic }.
A no-extend document allocates nothing and takes the original path; the interp
pre-pass stays gated on the static bit exactly as documentHasExtend gated it. Deletes
documentHasDynamicExtend + subtreeHasExtend (net one fewer helper).
2. Cost-contract registry: rewrite the ast-extend-import-preflight contract (now
ast-extend-dynamic-fold) in AGGRESSIVE-CUTTING-REVIEW.md to describe the real
architecture — walk-time recording (recordDynamicExtendFacts, guard e.dynamicExtend)
+ post-walk foldDynamicExtends — with sourceCheck anchors pointing at current code and
false/feature-path counters matching the rewritten extend-preflight-contract test.
`pnpm run verify:aggressive-cutting-review` passes.
… hot-path waste
Adversarial waste review follow-ups (dynamic-extend path).
F1: a loop iteration allocated a fresh {extendPlacement:{}} whenever dynamic recording
was armed, so one extend anywhere taxed every unrelated big loop with N dead tokens.
collectDynamicExtendSets now records the For/MixinDefinition nodes that actually bear a
dynamic extend into e.dynamicExtend.dynExtendBodies; expandFor/expandReferenceAncestorFor
allocate the token ONLY for a For in that set. Mixin-body rules keep null placement (the
staticRules discriminator), unchanged.
F2: recordDynamicExtendFacts and planImportedStaticExtend iterate inst.target.selectors
directly instead of building a throwaway .map(branchFromSelector) array.
F3/F4: dynamic subjects/instructions share one module-level EMPTY_SCOPE and one
per-rule path array (== the body-form extender path); the solver only reads scope/path
(verified: composePath clones, reaches reads, relativize slices — never mutates).
F5: fused staticRules construction with the F1 bodies collection into ONE armed-only
walk (collectDynamicExtendSets), replacing the separate collectStaticRuleSet re-walk.
The planner's subjects are NOT reused — computeExtends returns null (no plan) for the
common dynamic-only document, exactly when staticRules is needed; noted on the contract.
F7: visibleHeader and visibleHeaderFromProjection now both delegate to visibleHeaderCore
(zero runtime cost, removes divergence risk).
F6 (documented, not fixed): the post-walk fold re-solves the static portion; contract
notes "incremental computeExtends" as the upgrade path.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Extend consumes resolved static shapes (single render pass)
extendwas discovering placements insideeach()/$forloops, mixin-call bodies, and imported documents by re-evaluating the program in a separate pass — re-running loops, re-expanding mixins, re-activating variables — just to rebuild the data the real render walk already had. That second evaluation is both wasteful (it scales evaluation cost by the number of passes) and unsound: order-dependent effects (@namereassignment,::=, guard order, once-only imports) don't necessarily repeat identically, so the cold pass could disagree with the real output.This replaces that pass with a single-evaluation design:
The cold re-evaluator and its supporting machinery are deleted.
Fixes
An
&:extend(target)authored inside a mixin-call body or a loop body was silently dropped instead of accumulating onto the target's selector list. It now accumulates, matching a plain nested&:extend(). This closes the Bootstrap 4 grid case, where.grid-column's.col-*extenders (generated byeach()and mixin calls) were missing from the output.Guardrails
extend-evaluator-isolation.test.ts— the extend engine and its fact-collection may not name an evaluator entrypoint.EXTEND-SEMANTICS.md§1a and a design-decision entry; a general "single pass over resolved shapes" rule is added to the quality bar.Cost
Non-extend and static-only documents pay zero new cost (the dynamic path is gated behind a single classify pass that folds the two prior document scans into one).
benchmark.lessrender is flat vs. the base branch. New per-render structures are render-scoped and monomorphic; a node-keyedWeakMapis removed outright. Per-iteration extend-placement tokens are confined to loops that actually bear an extend.Known follow-up
Two exotic cases remain skipped (
import-at-rule.test.ts): a visible extender targeting an interpolated selector produced by a loop inside a(reference)import (one also under a reference@media). The reference rule is suppressed before a reveal slot exists; the legal fix is speculative reference-wrapper reservation, not a re-introduced second evaluation. Tracked as a follow-up.