engine: delete the LTM classify-twice round trip and type its synthetic equations#980
Open
bpowers wants to merge 14 commits into
Open
engine: delete the LTM classify-twice round trip and type its synthetic equations#980bpowers wants to merge 14 commits into
bpowers wants to merge 14 commits into
Conversation
The reference-site IR (model_ltm_reference_sites) classified access shapes per (from, to) edge, which serves causal-edge emission but not the ceteris-paribus transform: live-ref selection re-derives shapes on printed-and-reparsed Expr0 via the mirror classifier family. The settled Fig. 2 analysis of the round-trips plan established that both consumers ask one per-occurrence question, provided the IR carries context the transform needs and the edge emitter ignores. This extends the walker (one pass, unchanged edge view) with a per-occurrence view over the whole target equation: stable structural SiteIds (deterministic child-index paths), reducer-enclosure context surfaced rather than folded into routing, already-lagged markers (inside PREVIOUS/INIT; suppresses re-wrapping, not selection), index-position markers (the occurrences the transform structurally excludes from live selection), a position-mismatched-iterated per-axis arm distinct from a dynamic index (so the other-dep Collapse/Mismatch/NotIterated verdict is derivable from the IR), and document-order enumeration of module-output composites including implicit SMOOTH/DELAY expansions (the deterministic source A2b needs to fix GH #971's arbitrary HashSet pick). One walker-side correction: a subscript index token that names an element of the source's own dimensions is an element selector, not a causal reference, matching execution (normalize_subscripts3 resolves element-first, position-strict). No consumer-visible edge changes -- variable-level dep extraction already filtered such idents, so the old site was an orphan no keyed consumer read; the fix ends the walker/transform disagreement so A2b can select live occurrences from the IR. Consumers are unchanged in this step; the switch to the occurrence view (and deletion of the Expr0 mirror family) is the follow-on. Track A step 2a of the round-trips plan; part of the #965 arc.
module_link_score_equation's module-to-variable branch holds one output of the module instance live in the ceteris-paribus partial and freezes the rest. The live output was chosen by .find() over identifier_set's HashSet, whose iteration order is per-process random, so when a target reads two outputs of one module instance the emitted link score -- and every loop score through it -- flapped between processes (GH #971). The reference-site IR now enumerates module-output composites as per-occurrence ModuleOutput records in stable left-to-right walk order, so the pick becomes the document-order-first output that names the module: deterministic across processes, over the same candidate set the old scan considered. The identifier_set scan survives only as a defensive fallback when the IR recorded no matching occurrence, preserving the historical set of edges that receive a real partial; the typed-equation work is expected to delete it. The pin builds two models differing only in the order the target reads the two outputs and asserts they hold different sources live, distinguishing document order from both an alphabetical sort and the old order-independent hash pick.
The ceteris-paribus transform was applied to three categories of input text, two of them DERIVED: generate_per_element_link_equation wrapped a row-rewritten equation against a synthesized FixedIndex(row) live shape, and the agg-to-scalar / scalar-to-element generators wrapped agg-substituted text where hoisted reducers had already been replaced by synthetic names. Derived text has no reference-site occurrence stream, which blocked moving live-ref selection onto the per-occurrence IR: a partial switch would have left three classification paths (see the Category A/B/C analysis recorded in the track-A PR). This inverts the composition to rewrite-of-a-transform: every wrap now runs on the target's own equation (or its own Arrayed slot text) with the site's actual shape as the live shape, and the row-pinning and reducer-substitution rewrites become free-standing post-transform lowerings (rewrite_per_element_source_refs on the wrapped AST, substitute_reducers_in_expr0 mapping frozen PREVIOUS(SUM(..)) and live reducer subexpressions to their synthetic agg names in place). The lowerings live in a new sibling module, ltm_augment_post_transform.rs, both because they are the coherent post-wrap unit and to keep ltm_augment.rs under the per-file line cap. Net new Expr0 classification call sites: zero -- the follow-on step can swap the classifier family for occurrence lookups wholesale. Behavior is pinned by a new characterization suite (ltm_char_tests.rs with checked-in goldens plus runtime guards): generated texts are byte-identical to the prior composition for the entire battery except one adjudicated case -- a frozen occurrence reached through another reference's subscript index is now element-qualified (other[pop[region-dot-boston, ..]]) where the old ordering left the bare element spelling; both resolve to the same element and the full compiled series is bitwise identical. Guards also deliberately pin two pre-existing degradations (a first-live-step NaN from an uninitialized PREVIOUS(index) capture helper, and loud warned-zero on nested live reducers) so this change provably preserves rather than fixes them; they are tracked separately.
…gnment Groundwork for retiring the Expr0 mirror classifier family. Two pieces, both mechanism-verified: The other-dep iterated-subscript verdict (Collapse / Mismatch / NotIterated, which decides whether a non-live dep's subscript collapses to a bare PREVIOUS(dep), dooms the changed-first partial, or wraps normally) now has one implementation: the transform delegates to db::ltm_ir::derive_other_dep_verdict, derived from the occurrence IR's per-axis vocabulary. The delegation is arm-by-arm equivalent to the deleted duplicate, including the arity-dominance corner where the two families labeled an over-declared-arity axis differently: the arity guard returns Mismatch before any per-axis arm is consulted, so the labeling difference was unobservable (pinned by verdict_ignores_over_arity_axis_labeling). The classifier-agreement gate additionally asserts, for every fixture, that the occurrence IR's per-occurrence stream aligns one-to-one with the wrap walk's reference visits on the printed-and-reparsed text -- the premise a SiteId-keyed selection bridge depends on -- with new fixtures covering already-lagged, index-nested, LOOKUP, reducer, and module-output shapes. Misalignment fails with a diagnostic naming the path, turning the printer/parser-asymmetry class loud in the gate before any production switch. Two Fig. 2 selection semantics (index-nested exclusion from live capture, PREVIOUS/INIT verbatim-return) gain direct mutation-verified pins. The wrap itself still decides shapes on reparsed Expr0; the follow-on flips those decisions onto the IR and deletes the remaining classifiers. The GH #971 identifier_set fallback now marks itself loudly if it ever fires (corpus-unreachable today), so its planned deletion can proceed with confidence.
The ceteris-paribus wrap machine re-derived every per-occurrence decision -- live-shape matching, literal/element resolution, iterated-dim recognition, already-lagged skipping, element-selector and dimension-name index handling -- by classifying the printed-and-reparsed Expr0, mirroring the Expr2 classifiers under a "must stay in sync" docstring contract. Those decisions now come from the reference-site occurrence IR, threaded through the wrap with a structural path cursor whose alignment with the IR walker is proven corpus-wide by the gate's occurrence-alignment sweep. A missing or misaligned occurrence at any decision site degrades loudly (skip-and-warn), never to a silent guess. The sync contract is deleted. classify_expr0_subscript_shape, resolve_literal_element_index, is_live_source_iterated_dim_subscript, is_literal_element_index, and expr0_contains_live_match are gone from production (the first four survive cfg(test)-only, feeding synthetic non-compilable fixtures that cannot be built through a db); classify_other_dep_iterated_dim_subscript is deleted outright. The two lowering-side classifiers (classify_expr0_per_element_axes, expr0_iterated_axis_lines_up) survive lowering-only -- they classify the already-wrapped AST for row-pinning, which original-AST SiteIds cannot address -- behind a rustdoc'd seam that the typed-equation step deletes; wrap-time occurrence tagging was rejected because it would need a tag-only descent into PREVIOUS/INIT content the wrap never visits plus a Loc-keyed parallel map with its own drift surface. Every wrap call site threads a real occurrence stream; the empty- lookup shortcut and its incorrect justification are deleted. That also aligns the legacy and shaped link-score queries on the bare-live-source-inside-reducer class, where they had derived different equations (the compiled fragment came from one, the reported equation from the other): both now take the changed-last form, which is what shipped from both queries before this arc (the changed-first freeze form never shipped; pinned by legacy_and_shaped_bare_score_agree_when_source_bare_in_reducer). Un-hoisted-reducer and module-output-inside-reducer shapes keep their loud warned-skip degradation, pinned against the silent-zero variant. The characterization goldens are unchanged; the classifier-agreement gate becomes single-family classification pins plus the alignment sweep, with every expected-shape fixture retained.
LTM synthetic variables were stored as datamodel::Equation text: the transform built a wrapped Expr0 AST, printed it, and db/ltm/parse.rs minted a transient datamodel aux and re-parsed the text for compilation -- 2-3 parses per equation and a printer/parser asymmetry surface at the compile boundary. LtmSyntheticVar now carries the typed equation (a new LtmEquation in db/ltm/equation.rs holding the wrapped Expr0 arms for scalar, apply-to-all, and arrayed forms); producers hand over the AST they already built, and compilation consumes it directly. Printing survives only as a diagnostics projection (warnings, debug dumps, the characterization goldens), which is why the goldens are byte-identical. Two long-lived seams die at the same boundary. The legacy link_score_equation_text query is deleted: compile.rs's sub-case (a) previously compiled fragments from the legacy text while emitters reported the shaped query's text (with a known dim_ctx divergence between them); compiled and reported equations now come from one stored derivation by construction. module_link_score_equation's identifier_set fallback (and its loud marker) is deleted: the occurrence IR is the only source for module-output live selection, and a missing occurrence takes the loud missing-occurrence path. Salsa incrementality is preserved: the shaped text query backdates on the PartialEq-equal typed equation, so unrelated edits reuse the expensive fragment (pinned by the existing per-link caching test); the units-context dependency was dropped from the LTM parse path entirely. Mutation probes confirm the compiled representation is test-bound: a perturbed stored AST trips numeric pins even though text goldens are blind to it. This is the storage/compile half of the GH #965 typed-equation work; the generation half (the transform still parses the target equation from printed text once at the source boundary) is the follow-on.
Comment-only. Inserting other_dep_verdict between wrap_non_matching_in_previous and its rustdoc left the wrap undocumented and gave the verdict helper a doc block describing live_source / other_deps / iter_ctx / out.live_ref / dims_ctx -- none of which are its parameters. The same accident already existed for classify_expr0_subscript_shape, whose doc sat on the bool-returning is_literal_element_index (so that predicate was documented as "Rules: any Wildcard -> RefShape::Wildcard"); this branch's edits appended new cfg(test) rationale into that wrong block, so it is corrected here rather than left to rot. Both blocks are also brought in step with the code they now describe: the wrap takes a &WrapCtx rather than positional parameters, its iterated-dim normalization is driven by the occurrence IR's shape (iter_ctx now supplies only the other-dep arity/mapping facts and the dimension-name index guard's fallback), and it threads a structural path; the shape classifier's rule list was missing the #511 Bare and #525 PerElement arms it actually implements. Two narrower staleness fixes ride along. A link_scores comment still pointed at `substituted`, the caller-side local this branch deleted when reducer substitution became a post-transform lowering. And OccurrenceSite's "every field has a named A2b/A3 consumer" claim was false once the wrap turned out to reach the already_lagged and in_reducer decisions structurally: target_element, routing, in_reducer, reducer_keys, and already_lagged have no production reader today. Saying so plainly -- and recording that each is pinned at the IR level, which is what keeps a walker regression catchable without one -- is more useful to the next reader than a claim they would disprove in a grep.
quote_ident spelled an identifier bare whenever every character was alphanumeric or underscore. A leading digit satisfies that but cannot START an identifier: the equation lexer reads `1stock` as the number 1 followed by the identifier `stock`. `"1stock"` is a legal quoted XMILE name, so a valid model produced a link score whose partial carried print_eqn's quoted `"1stock"` beside a bare `1stock` in the guard form's SIGN factor -- one equation, two spellings of one name, and the whole thing unparseable. Before the typed-equation work that meant a silently-degraded score; after it, the eager arm parse also tripped the augmentation-bug debug_assert, so a debug build aborted inside a salsa query on ordinary user input. quote_ident now also consults ast::needs_quoting -- the leading-character rule print_ident already applies on the print_eqn path -- so the two spellings cannot disagree. It deliberately does NOT delegate outright: U+00B7 IS XID_Continue, so needs_quoting alone would spell a module-output composite bare (`mod·out1`). That parses, but it would rewrite the emitted text of every module-composite link score for no bug. Requiring BOTH predicates can only ever add quotes, which is why every characterization golden is byte-identical. The debug_assert in LtmArm::new is removed rather than kept alongside the root-cause fix. Its premise -- a generated equation is a print_eqn re-print, so a parse failure can only be an augmentation bug -- was false, and asserting on it converted the pre-existing warn-and-skip degradation into an abort on a valid model. The loud path is the diagnostic that was already there: no AST means no bytecode, so model_ltm_fragment_diagnostics warns that the variable holds a layout slot reading constant 0. Generator bugs still fail loudly in tests via the numeric pins and the goldens. The regression pin asserts the arm has a parsed AST, not merely that the text contains a quote, so it catches any unparseable generated equation rather than this one spelling; mutation-verified by restoring the old predicate.
Three findings from a review pass over the typed-equation boundary. Arrayed arm parse errors were silently dropped, and this completes the degradation story 17d4e7c claimed. That commit deleted a debug_assert on the grounds that "no AST means no bytecode, so model_ltm_fragment_diagnostics warns". True for a scalar equation; FALSE for an arrayed one. LtmArm::new collapsed a parse failure into a bare expr: None, to_flow_ast's slot map dropped it, and any sibling arm that parsed kept the fragment alive with bytecode -- so the compiler zero-filled the missing slot and no diagnostic fired at all. A silent per-element zero: the exact class this track exists to remove, reintroduced at the new boundary. LtmArm now retains the parse error, which is what distinguishes a FAILED arm from a legitimately EMPTY one (the latter must still drop, as variable::parse_equation drops it), and to_flow_ast rejects the whole equation before any shape-specific assembly. The pin covers both shapes and both mutation directions. Only the first error is kept: the diagnostic names the variable and its text, not a parse position, so a Vec inflated LtmSyntheticVar past clippy's large_enum_variant threshold for data nothing reads. Occurrence lookup was quadratic in an arrayed target's element count: for_slot rescanned the whole occurrence stream once per slot and allocated N temporary vectors. SlotOccurrences groups once and indexes per slot, and is now the ONLY way to obtain an OccurrenceLookup -- the lookup borrows from the index, so the type system forces callers to hoist it out of their per-element loop instead of rebuilding it inside. Measured, because the win is not where the finding implied. Timing model_ltm_variables on a width-N per-element target (the checked-in #[ignore]d harness per_element_generation_scaling): N=50 37.7ms -> 37.3ms N=200 564ms -> 568ms N=100 141ms -> 141ms N=400 2.317s -> 2.304s About 2%, still 4x per doubling. The rescan was a genuine asymptotic term and is gone, but a LARGER pre-existing quadratic dominates: this shape emits N+1 link scores each arrayed over the target's N+1 slots, so (N+1)^2 equation arms per edge -- 160,401 at N=400. The queries it reads are all linear and total ~7ms of that 2.3s, so the cost is the emission shape, not classification. Filed separately; not this branch's to fix, and the harness prints the arm counts that show it. A SiteId child index could overflow on a variadic builtin. Builtin arity is the one child count the AST shape does not bound, so a 65,536-argument MEAN panicked in debug and WRAPPED in release -- re-using an earlier child's SiteId, which makes the wrap's path lookup return a different occurrence and freeze the wrong reference: a plausible wrong score, silent. site_child_index returns None past the addressable range rather than a wrapped value, and the walker records no occurrence for such a child, which routes into the existing loud path (a live-source subscript with no occurrence sets missing_occurrence, so the partial is abandoned and the emitter warns). The per-edge ReferenceSite view still gets its site, so no causal edge is lost. Widening the path element to u32 was rejected: every occurrence carries a boxed path in a salsa-cached per-model IR, so it would double that footprint on every model to serve an arity no real model reaches, and it would move the boundary rather than remove it. Pinned on the boundary function, not on a 65,536-argument fixture that would strain the suite's wall-clock cap. The occurrence read side (SlotOccurrences / OccurrenceLookup) moves to its own file: ltm_augment.rs had reached the 6000-line cap, and this is the coherent unit to extract.
b0a2a6f guarded only the PRODUCER. Two halves of the same defect survived, both of them the silent-corruption class the guard exists to prevent. The consumer still aliased. ltm_augment::child_path built its path with `i as u16`, so a builtin child at index 65,536 wrapped to child 0's path. The IR walker deliberately records no occurrence for that child, but the wrap would then look up child 0's -- and if that sibling HAS an occurrence, the lookup succeeds with an unrelated shape, missing_occurrence never fires, and the wrap freezes the wrong reference and emits a plausible wrong score. Omitting the occurrence on one side is only safe if the other side cannot manufacture a colliding path. So the sentinel is now reserved rather than merely hoped-for: UNADDRESSABLE_CHILD is a value site_child_index NEVER returns (it costs one child index out of 65,536), and child_path maps an over-arity child to it with a checked conversion. A path containing it therefore cannot equal any recorded SiteId, so every lookup at or below such a child provably misses instead of aliasing. The App arms additionally set missing_occurrence at the overflow point, so the partial is abandoned where the problem is rather than depending on a miss further down. The producer dropped too much. The early return skipped the whole contents match, so it suppressed push_ref_site along with the occurrence -- which my commit message for b0a2a6f explicitly claimed it did not do. That claim was wrong. The two views are not interchangeable: the occurrence view is SiteId-keyed and must stay silent for a child it cannot address, but the per-edge view is name-keyed and feeds model_edge_shapes, where a MISSING entry is not read as "no reference" -- consumers default it to a single Bare site. So dropping the edge site would misclassify a FixedIndex/DynamicIndex reference and emit wrong element edges and link scores. Suppression is now a depth counter consulted only by push_occurrence, covering the unaddressable child's whole subtree (a nested occurrence at a truncated path would alias a sibling) while every reference keeps its real shape in the edge view. Both halves needed their own pins, and neither was covered by the producer-side boundary test: it passes whether or not child_path's conversion is checked, and the suppression counter is never raised below 65,536 arguments, so no fixture exercises it. Each is now pinned on the exact invariant -- child_path's sentinel mapping and WalkAccum's record-the-edge-anyway contract -- rather than through a 65,536-argument fixture that would strain the suite's wall-clock cap. Both are mutation-verified: restoring `as u16` fails the consumer pin, and suppressing ref sites fails the producer pin.
Three findings, two fixed and one documented. The per-element row-pinning lowering skipped subscript RANGE endpoints. rewrite_per_element_source_refs matched only IndexExpr0::Expr when recursing through another variable's subscript, so a source reference in a range bound -- other[pop[region]:3] -- passed through untouched. The IR walker DOES record an occurrence there (IndexExpr2::Range pushes children 0 and 1) and the sibling lowering substitute_reducers_in_expr0 in the same file already descends both endpoints, so this was an internal inconsistency rather than a deliberate boundary: the recorded occurrence was left un-pinned and its dimension-name subscript survived into the scalar per-element equation, which either fails to compile (a PREVIOUS-of-dim-name capture helper) or reads the wrong element. Both index branches now descend range endpoints. The partially-classifiable branch descends only where no per-axis substitution applied, since recursing after a substitution would double-pin the source's own axis. generate_per_element_link_equation did not doom on missing_occurrence. It checked only other_dep_mismatch, so a live-source occurrence the IR could not record left the source looking non-live: the wrap froze it at PREVIOUS and the score came out a clean ZERO. Every other wrap caller already dooms on it (shaped_guard_form_text, generate_scalar_to_element_equation, build_partial_equation_shaped_with_live_ref); this was the one hole through which that silent zero could still reach an emitter, so the doom contract is now uniform across all of them. module_output_ref_in_document_order cannot distinguish a SUPPRESSED module-output occurrence (one inside an unaddressable builtin child) from "the target reads no output of this module": both yield None and fall through to the signed magnitude-1 unit transfer. That is documented rather than fixed. The outcome is the same fallback an unlocatable reference has always taken -- an explicit approximation, not a plausible-looking wrong number -- and distinguishing the two means threading an unaddressable marker through the IR and this query to serve a 65,536-argument builtin, an arity far past where LTM generation is tractable at all (GH #977). The rustdoc records what the None conflates and says that if such a marker ever becomes cheap, the honest behavior is to skip the edge loudly rather than silently approximate it. The range fix is pinned by a focused test on the lowering and mutation-verified: removing the descent leaves other[pop[region]:3] verbatim and fails the pin.
A char golden pins generated equation TEXT, so it is structurally blind to a generated equation that is STABLY unparseable: such an equation compiles to no bytecode, the variable reads a constant 0, and the golden stays green forever. Only 5 of the 15 fixtures asserted the runtime consequence, and they did it in separate test functions, so the other 10 contributed no coverage of that class. Two independent unquotable-generation bugs turned up in one day (the 1stock leading digit, and the bare-keyword class of GH #976), neither of which any text golden would have caught -- a defect class with a demonstrated recurrence rate against a check that was 1/3 deployed. The assertion now rides the shared harness. assert_char_fixture pins the golden AND the fragment-compile expectation in one call, and the expectation is a REQUIRED argument, so a new fixture cannot forget to declare it and quietly contribute nothing. Thirteen fixtures declare AllCompile. The two that legitimately produce failures declare them exactly, each with a `why`: - agg_nested_reducer: the GH #517 live-agg-inside-a-declined-outer- reducer degradation b789869 deliberately preserves -- the partial freezes a wildcard slice, which has no LoadPrev-of-array-view path. Six entries (two target elements, each with its score plus the two PREVIOUS-capture helpers its partial synthesizes). - reducer_index_nested_freeze: this fixture's OWN model is rejected (ArrayReferenceNeedsExplicitSubscripts -- it uses a scalar as a subscript index, deliberately, to pin the Fig. 2 Q4 index-nested selection semantics at the text level), so its PREVIOUS-capture helper inherits the un-compilable subscript. Zero failures is not reachable here without changing what the fixture pins. An exact set comparison rather than a subset: a NEW failure is a silent-zero regression, and a DISAPPEARED one means the annotation went stale and should be tightened. An annotation nothing can forget to set beats a global assert with invisible carve-outs. Non-vacuity was demonstrated rather than assumed: running all fifteen as AllCompile first, exactly the two genuinely-failing fixtures failed, and loosening either annotation back to AllCompile is rejected. One honest limit, since it bears on what this buys. The guard catches a generation change that stops compiling on the shapes the fixtures cover; it does NOT catch the quoting class specifically, because no fixture carries a name the lexer cannot read bare -- verified by re-introducing the quote_ident regression, which leaves all fifteen green while the dedicated 1stock pin fails. Covering that class through the corpus would need a fixture with such a name, which is deliberately out of scope here. The dedicated pins remain the coverage for it. Also documents the producer-side asymmetry on both accumulator methods: absence in the per-edge view is defaulted to Bare by consumers (so skipping a site MISCLASSIFIES the reference), while absence in the occurrence view is safe and sometimes required. That is why suppress_occurrences gates only push_occurrence, and it is the trap worth naming so the two are not collapsed later.
The corpus was blind to the highest-recurrence generation bug class we have. Two independent unquotable-generation bugs turned up in one day -- the leading-digit `1stock` fixed in 17d4e7c, and the bare-keyword class of GH #976 -- and neither was catchable by any of the fifteen existing fixtures, because a fixture only exercises the class if it CARRIES such a name. Demonstrated, not assumed: re-introducing the quote_ident regression left all fifteen green while only the dedicated 1stock unit pin failed. The corpus is the net that runs on every commit, so a class with that recurrence rate resting entirely on one unit pin -- which a future refactor could delete with nothing going red -- was the wrong place to be. This is Model A (the PerElement mixed iterated+literal shape) cloned with the source renamed `pop` -> `1pop`: an existing shape with one identifier changed, not a new model shape. `"1pop"` is a legal quoted XMILE name that canonicalizes to `1pop`, which the equation lexer cannot read bare (it only starts an identifier on XID_Start, so bare `1pop` lexes as the number 1 followed by `pop`). A leading digit and deliberately NOT a keyword: a keyword-named source would fail today against open GH #976 (`ast::needs_quoting` has no keyword check), and a fixture that is red on correct code is worse than no fixture. Both halves of the fixture's assertion are load-bearing, verified separately under the regression. The golden catches the text change (`"1pop"` -> bare `1pop`). Independently -- with the golden temporarily recaptured so the text assertion could not mask it -- the AllCompile expectation catches the runtime consequence: both per-element scores fail to compile, so each keeps a layout slot with no bytecode and reads a constant 0. That second half is the one the fifteen text-only fixtures could never have provided. The new golden file is a behavior RECORD, not a behavior change: the fifteen existing goldens are byte-identical (`git diff dea4218..HEAD -- ltm_char_golden/` is empty).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
==========================================
+ Coverage 91.53% 91.61% +0.08%
==========================================
Files 234 238 +4
Lines 156305 157050 +745
==========================================
+ Hits 143070 143888 +818
+ Misses 13235 13162 -73 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ReviewTraced the core surfaces where a regression would hide:
No blocking issues found. Everything I probed either matched the author's own commit-message reasoning or a disclosed residual (#977/#978/#979/#981). Two non-qualifying observations left unfiled: a test-only walker in Overall verdictCorrect. Approve. |
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.
Track A of the "Deleting the Round Trips" plan: the LTM subsystem decided each reference's
access shape twice — once on
Expr2for causal-edge emission, once on a reparsedExpr0to pick which occurrence stays live — and the two implementations were held together by a
docstring saying they "must stay in sync". This deletes that round trip and stores LTM
synthetic equations as typed ASTs instead of text.
The failure mode is why this was worth doing: when the two classifier families disagreed, the
intended live reference got wrapped in
PREVIOUS()and the link score came back a cleanzero. No error, no panic, no diagnostic — a plausible number. Two shipped bugs (#759, #913)
came through that seam.
Part of #965. Fixes #971.
What changed
f44097b7f057ef38b78986926993b9b686df68bfdea4218839093964…2b72c67dAfter
86df68bf,classify_expr0_subscript_shape,resolve_literal_element_index,is_live_source_iterated_dim_subscript,is_literal_element_indexandexpr0_contains_live_matchare gone from production, andclassify_other_dep_iterated_dim_subscriptis deleted outright. Decisions now come from thereference-site occurrence IR, threaded through the wrap by a structural path cursor whose
alignment with the IR walker is proven corpus-wide by the gate's occurrence-alignment sweep.
A missing or misaligned occurrence degrades loudly (skip-and-warn) at every decision site,
never to a silent guess.
dea42188makesLtmSyntheticVar.equationa typedLtmEquationholding parsedExpr0arms.Printing survives only as a diagnostics projection (warnings, debug dumps, the goldens).
Design decisions worth knowing
b7898692). Wraps run on original equations; row-pinning andagg-substitution became post-transform lowerings. This is what makes original-AST
SiteIdsaddressable by the wrap at all.
classify_expr0_per_element_axesandexpr0_iterated_axis_lines_upclassify thealready-wrapped AST for row pinning, which original-AST
SiteIds cannot address. They donot mirror an
Expr2classifier for the same decision, so the dangerous mirror is goneeven though these remain. Wrap-time occurrence tagging was rejected: it would need a
tag-only descent into
PREVIOUS/INITcontent the wrap never visits, plus aLoc-keyedparallel map with its own drift surface.
UNADDRESSABLE_CHILDis a reserved sentinel, not a widened type. Builtin arity is theone child count the AST shape does not bound. Rather than widen the path element to
u32—doubling a boxed-path footprint on every model in a salsa-cached IR to serve an arity no real
model reaches — one child index out of 65,536 is reserved as a value the producer never
emits, so a path containing it provably cannot equal any recorded
SiteId. Producer andconsumer both derive from that single constant.
Absence in the name-keyed per-edge view is defaulted to
Bareby consumers, so skipping asite misclassifies the reference; absence in the
SiteId-keyed occurrence view is safe andsometimes required. That is why suppression gates only
push_occurrence. Collapsing the twois the trap that produced one of the review findings below.
debug_assertwas removed rather than kept (17d4e7c0). Its premise — that agenerated equation is a
print_eqnre-print, so a parse failure can only be an augmentationbug — was falsified by a valid model. Aborting inside a salsa query on ordinary user input is
the wrong failure mode for a library whose debug builds reach pysimlin/MCP/serve consumers.
The loud path is warn-and-skip.
Defects found and fixed during review
Six review passes over the branch diff found six real defects, five of them the silent-zero
class this track exists to delete. None would have thrown or failed a test.
17d4e7c0).quote_identspelled anyall-alphanumeric name bare, which a leading digit satisfies — but the lexer only starts an
identifier on
XID_Start, so a stock legally named"1stock"produced one equationcarrying both
"1stock"and bare1stock, unparseable. Fixed at the root by consultingast::needs_quotingtoo. Requiring both predicates can only ever add quotes, which is whyevery existing golden stayed byte-identical.
b0a2a6f0). One unparseable arm amongparseable siblings: the error became
None, the slot was zero-filled, the fragment kept itsbytecode, and no diagnostic fired — a silent per-element zero.
LtmArmnow retains theerror, distinguishing a failed arm from a legitimately empty one.
b0a2a6f0).SlotOccurrencesgroups once and is now theonly route to an
OccurrenceLookup, so the borrow forces callers to hoist it out of theirper-element loop. See the honest note on its measured effect below.
SiteIdchild index overflow (b0a2a6f0, completed ind5eed63b). Both halvesmattered: the consumer aliased an over-arity child onto an earlier sibling's path (so a
lookup could succeed with an unrelated shape and freeze the wrong reference), and the
producer's early return suppressed
push_ref_sitealongside the occurrence, whichmisclassifies rather than omits.
2f56c512).other[pop[region]:3]passed through untouched, leaving a recorded occurrence un-pinned —an internal inconsistency, since the sibling lowering in the same file already descended
both endpoints.
generate_per_element_link_equationdid not doom onmissing_occurrence(2f56c512).It checked only
other_dep_mismatch, so a live source the IR could not record lookednon-live, got frozen at
PREVIOUS, and the score came out a clean zero. Every other wrapcaller already doomed on it; the contract is now uniform.
New deterministic gates
2ae6cb33lifts the zero-fragment-compile-failure assertion into the shared char-goldenharness. A char golden pins generated text, so it is structurally blind to a stably
unparseable equation: no bytecode, variable reads constant 0, golden green forever. Only 5 of
15 fixtures asserted the runtime consequence.
The expectation is now a required argument to
assert_char_fixture, so a new fixture cannotforget to declare it. Thirteen declare
AllCompile; two declare their real failures exactly,each with a
why. Comparison is an exact set, not a subset — a new failure is a regression,a disappeared one means the annotation went stale. Non-vacuity was demonstrated, not assumed.
2b72c67dadds the sixteenth fixture: an existing per-element shape with its source renamedpop->1pop, a legal quoted XMILE name the lexer cannot read bare. Both halves of itsassertion were verified load-bearing separately — with the golden temporarily recaptured so
the text assertion could not mask it, the
AllCompilehalf independently catches the runtimeconsequence. Deliberately a leading digit and not a keyword: a keyword-named source would fail
today against open #976, and a fixture that is red on correct code is worse than no fixture.
Verification
clearn_pinned_climate_loop_is_scored ... ok, 241.47s. Real arrayed Vensim model, productionSetLoopNamepatch path, discovery-mode auto-flip, pinned climate loop scored finite/non-zero/negative.cargo test -p simlin-engine --lib: 5271 passed / 0 failed / 3 ignored (baseline 5262; +9 new tests).cargo clippy --all-targets -- -D warningsclean;scripts/lint-project.shpassed; full pre-commit hook green on every commit.Disclosed residuals
Stated here rather than left for the next person to discover.
Occurrence pre-indexing moved wall-clock ~2%, not the win the finding implied: 37.7→37.3ms at N=50, 2.317→2.304s at N=400, still 4x per doubling. The rescan was a genuine asymptotic term and is gone, but a larger pre-existing quadratic dominates — N+1 scores each arrayed over N+1 slots, (N+1)² equation arms per edge, 160,401 at N=400. Sub-phase timings prove it is not classification (~7ms of that 2.3s). Filed as ltm: link-score generation is quadratic in an arrayed target's element count ((N+1)^2 equation arms per edge) #977; not fixed here.
The corpus guard does not cover the quoting class; the new fixture and the dedicated unit pin do. Two mechanisms, both explicit, neither pretending to be the other.
reducer_index_nested_freeze's own model does not compile (ArrayReferenceNeedsExplicitSubscripts— it uses a scalar as a subscript index, deliberately, to pin index-nested selection semantics at the text level). Sound for what it pins, but no numeric backstop is possible for that fixture; text is all it can ever assert.slot as u16inltm_augment.rsis the same aliasing class as finding 4 on the slot axis. Pre-existing and unreachable in practice because ltm: link-score generation is quadratic in an arrayed target's element count ((N+1)^2 equation arms per edge) #977 makes that width intractable first. Left unwidened rather than changed silently; filed as ltm: unguardedslot as u16cast on the SiteId slot axis can alias occurrences for a >65,535-element arrayed target #979. Note this also corrects ltm: consider rejecting over-arity builtins at the LTM front door to delete the SiteId unaddressable-child machinery #978's premise: builtin arity is not the only unbounded child count, so a front-door reject would have to cover both axes to actually delete the sentinel machinery (correction posted on ltm: consider rejecting over-arity builtins at the LTM front door to delete the SiteId unaddressable-child machinery #978).A
NaNliteral in a scored equation defeats salsa backdating (ltm: a NaN literal in a scored equation defeats salsa backdating for typed LtmEquation (avoidable fragment recompiles) #981).Expr0::Constholds anf64,nanis a lexer keyword, andLtmArm/LtmEquationderivePartialEq+salsa::Update— so two arms built from identical text compare unequal and<LtmEquation as salsa::Update>::maybe_updatereturns CHANGED. Verified empirically, not by inspection. This falsifies a claim indea42188's commit message: incrementality is preserved for ordinary equations, not for NaN-bearing ones.Disclosed rather than fixed, deliberately. The effect is recomputation, not a wrong answer — salsa only requires CHANGED when a value genuinely changed, so a spurious CHANGED is conservative. The sound fix compares on
text(provably canonical, sinceexpr = Expr0::new(text)and the two never drift), but that requires a hand-writtensalsa::Update, an unsafe trait whose failure mode is returning UNCHANGED on a real change — i.e. stale results, a correctness bug strictly worse than the performance residual it would fix. It deserves its own change with its own review. The trigger also needs a hand-writtenNaNliteral: Vensim:NA:is the finitefloat::NAsentinel, so MDL imports are unaffected. engine: salsa::Update on compiled bytecode uses PartialEq over f64/NaN (benign missed-backdate) #642 is the same root cause on bytecode types, where it was argued benign because the consumer is non-tracked — that mitigation does not transfer here, sinceLtmEquation's consumer is a tracked query.A module-output occurrence suppressed inside an unaddressable builtin child is indistinguishable from "the target reads no output of this module" — both fall to the signed magnitude-1 unit transfer, the same explicit approximation an unlocatable reference has always taken. Documented in rustdoc, including that the honest fix is to skip the edge loudly if a marker ever becomes cheap. Simplification option filed as ltm: consider rejecting over-arity builtins at the LTM front door to delete the SiteId unaddressable-child machinery #978.
OccurrenceSitehas five fields with no production reader (target_element,routing,in_reducer,reducer_keys,already_lagged) — the wrap reaches those decisions structurally. Rustdoc now says so. The follow-on typed-generation work either consumes them or deletes them.What remains on #965
This does not close #965. Criteria 1 (one authoritative representation) and 3 (typed
storage, no print/reparse to compile) are met. What remains is the generation half: eight
Expr0::newtext-parse sites inltm_augment.rs, text-levelpinned_body_row_terms, the twolowering-only classifiers above, and two
cfg(test)mirrors. That is parse cost andcleanliness — the correctness payoff is banked here — and it is deliberately a separate PR
rather than more diff on top of this one.
Follow-ups filed
if/or/modprints bare and fails to re-parse #976 —ast::needs_quotinghas no keyword check, so a variable namedif/mod/orprints bare and re-lexes as a keyword. User-reachable, and
patch.rsrename writes thecorruption into the saved model — materially worse than the LTM symptom. Pre-existing.
UNADDRESSABLE_CHILD, the suppression counter, and the conflation above (with a correction:it must cover the slot axis too, per ltm: unguarded
slot as u16cast on the SiteId slot axis can alias occurrences for a >65,535-element arrayed target #979).slot as u16cast on the SiteId slot axis can alias occurrences for a >65,535-element arrayed target #979 — the slot-axisas u16cast.NaNbackdating defeat above; related to engine: salsa::Update on compiled bytecode uses PartialEq over f64/NaN (benign missed-backdate) #642.A note on how this was reviewed
Six
scripts/codex-reviewpasses, each over the whole branch diff. The pass history was0, 1, 3, 2, 3, 0, 0 findings — pass 1 read
dea42188and reported no regressions, and pass 3then found a P2 silent-zero in that same file; the range-endpoint bug lived in
b7898692,which four passes read and missed. Sampling review is not a proof, which is why green here
required two consecutive clean completed passes, and why the deterministic in-repo gates above
are the load-bearing evidence rather than the review itself.
dc20563e(pinningscripts/codex-reviewto gpt-5.6-sol) is an unrelated tooling commit thatlanded on this branch; it can be dropped or kept at your preference.