engine: delete the LTM input-side round trip and the last Expr0 classifiers#985
Open
bpowers wants to merge 16 commits into
Open
engine: delete the LTM input-side round trip and the last Expr0 classifiers#985bpowers wants to merge 16 commits into
bpowers wants to merge 16 commits into
Conversation
The ceteris-paribus wrap took the target's equation as printed TEXT and parsed it -- but patch::expr2_to_string IS print_eqn(expr2_to_expr0(e)), so that parse was reconstructing a tree the caller already held in exactly this shape. The wrap and the three per-element / agg generators now take &Expr0 lowered straight from the target's Expr2, which deletes two of the eight Expr0::new sites outright (wrap_changed_first_ast, and shaped_guard_form_text's changed-last re-parse). The reason this is more than parse cost: db::ltm_ir computes every occurrence's SiteId on the Expr2 tree, and the wrap addresses those occurrences by the child-index path it tracks as it descends. Handing the wrap the direct lowering makes the two path spaces the same tree by CONSTRUCTION, where before they coincided only because print-then-parse happened to round-trip -- a property assert_occurrence_stream_aligns could sample but never establish. The new corpus property assert_lowering_matches_reparse states the equivalence that makes the swap byte-neutral: structural equality modulo Loc and modulo raw-ident SPELLING, the two coordinates no consumer reads directly (every head goes through canonicalize/Ident::new on read and print_ident on write, and Ident::to_source_repr renders the module separator back as '.' while the parser returns a quoted name with its quotes). It runs on every alignment/agreement fixture plus a new fixture built from the shapes a printer/parser asymmetry would most plausibly reshape -- negated constants, zero-argument builtins, the word operators, right-associative ^, If in operand position, the variadic builtins, LOOKUP's table slot, and every subscript index form. shaped_guard_form_text's changed-last leg documented its parse as a deliberate cheap re-parse on a rare doomed path. The cost argument was correct and is beside the point: that parse was the only reason the function needed the text at all, and while it stood the two attribution conventions could in principle walk different trees. Both legs now start from the one AST the caller owns. One parse deliberately survives here, moved into scalar_or_a2a_target_expr: a target that failed to LOWER has no Expr2, only the user-authored datamodel equation string. That is a genuine source-format boundary rather than a re-parse of engine output, so it keeps the loud PartialEquationError::Parse channel -- which is now the only way that error can fire on the aux/stock path, since the wrap itself became infallible.
The `PerElement` row-pinning lowering ran as a pass over the ALREADY-WRAPPED `Expr0`. A `SiteId` is computed on the target's original `Expr2`, and the wrap inserts `PREVIOUS(...)` nodes, so the paths no longer line up and the occurrence IR could not drive that pass -- which is why it re-derived every occurrence's per-axis access with its own Expr0 classifier (classify_expr0_per_element_axes / expr0_iterated_axis_lines_up), the last two Expr0-side classifiers left in production. The pinning now runs FROM the wrap as it descends, reading OccurrenceSite::axes by path, and those two are gone from production along with IteratedDimCtx::dim_ctx, the duplicate DimensionsContext that existed only to feed them. The obvious alternative -- pin FIRST, on the original AST, where the paths do line up -- is wrong, and the corpus already says so. Pinning is shape-preserving (it rewrites leaf Var names inside subscript indices), so it looks like it should be fully occurrence-addressable. But the lowering emits two spellings: the occurrence the wrap leaves LIVE gets the row with BARE element names, and every occurrence the wrap FREEZES gets the same row QUALIFIED (region·boston), so the frozen read compiles to a direct LoadPrev rather than an ambiguous bare element name. Which side an occurrence lands on is not a function of its axes: per_element_index_nested_occurrence emits both spellings of ONE site shape in one equation, because subtree_has_live_shape excludes index-nested occurrences. Pin-first cannot see the difference and would spell the nested one bare, and on a model whose element names are ambiguous across dimensions (C-LEARN's regions) the bare spelling is the one that fails to compile -- a silent zero, not a cosmetic diff. So the wrap threads a `frozen` flag beside `path`, and at the two points where it deliberately stops descending -- a pre-existing PREVIOUS/INIT call, and the GH #517 whole-frozen reducer -- it runs a pin-only descent: same path cursor, same occurrence IR, pins (always qualified, since it is inside a freeze) and wraps nothing. That is the shape `86df68bf`'s rustdoc rejected, and the rejection was aimed at something else: it rejected wrap-time occurrence TAGGING, which needs either a descent whose only purpose is to write tags or a Loc-keyed side map -- a second representation of the classification, keyed on a coordinate the wrap rewrites. A pin-only descent carries no map and writes no tag; it reads the one IR by path and performs the actual lowering. Per-axis equivalence is exact, not approximate. The retired classifier accepted an index only when it was an iterated-dimension name lined up with the source's axis at that position, or a literal element of that axis -- and its lineup check consulted the SAME ltm_agg::classify_axis_access the IR uses, gate for gate. So "every axis Iterated or Pinned, arity equal to the source's declared arity" (axes_as_read_slice) is the same predicate; Reduced, MismatchedIterated and Dynamic all fall out as not-describable exactly where the old one returned None, and an over-arity index can never be Iterated because the IR only consults classify_axis_access where the source has an axis at that position. All 16 char goldens are byte-identical. Two of the three mutation probes are caught by them and caught NARROWLY: ignoring the live/frozen distinction fails eight per-element goldens, and dropping the frozen propagation into a wrapped node's indices fails exactly per_element_index_nested_occurrence -- the golden that discriminates the two designs. The third slipped through the entire corpus: deleting the pin-only descent under a whole-frozen reducer left all 5272 tests green, because no fixture reaches a PerElement source occurrence nested inside a reducer that freezes whole. That shape needs an index-nested occurrence (so subtree_has_live_shape excludes it) inside a reducer that therefore carries no live reference, and un-pinned it leaves a dimension-name subscript in a scalar fragment that cannot compile -- the silent zero this track exists to delete. per_element_pin_reaches_inside_a_whole_frozen_reducer closes it and fails on that mutation. The `#[cfg(test)]` Expr0 classifier family moves to ltm_augment_wrap_test_support, beside the occurrence builder that is now its only consumer. That makes its status structural rather than an attribute a reader has to notice, and keeps ltm_augment.rs under the line cap (the in-wrap pinning pushed it to 6078; it is now 5655). The test occurrence builder also gained the Iterated arm it was missing: it only ever synthesized Pinned/Dynamic axes, so it could not have driven the pinning at all -- a divergence from production that was hiding in the tests, which is the worst place for one.
`OccurrenceSite` carried five classified facts with no production reader, documented as such and justified by "the remaining GH #965 generation-half work consumes them". The generation half is this branch, and it consumes exactly one of them -- so the other four go, along with `OccurrenceRouting` and the `matching_aggs_for` closure that existed only to populate `routing`. The decisive one is `routing`, and it is worse than unread: it is a DUPLICATED DERIVATION. `model_ltm_reference_sites` held two copies of the GH #793 narrowing -- route a reference only through aggs minted for one of its ENCLOSING reducers -- one in `matching_aggs_for`, whose only caller was the occurrence finalize, and one inline in the `ClassifiedSite` loop. Deleting the per-occurrence routing deletes one of the two. A second implementation of one rule, inside the IR that exists to end second implementations of one rule, is the exact thing this track is about. `reducer_keys` was needed only to compute that routing, so it goes with it (the coarser `in_reducer` bit it also fed is kept as a bool). `target_element` and `already_lagged` are per-occurrence duplicates of facts their consumers already hold: the generators are handed the target element explicitly, and the wrap recognizes a pre-existing `PREVIOUS`/`INIT` at the node itself. Between them that is an `Option<String>`, a `Vec<String>`, a `Vec<AggRef>` and a `bool` per occurrence removed from a salsa-cached per-model map -- which is not nothing while GH #977 stands. `in_reducer` stays because it HAS a consumer, not because it might get one: the corpus gate reads it to exclude reducer arguments from the per-occurrence shape comparison and to assert both streams agree on reducer context. Deleting it would silently narrow that gate. It is also the cheap half of what `reducer_keys` carried, at the only granularity anything asks for. A prediction this corrects: `already_lagged` was expected to gain its first production reader when the row pinning moved into the wrap, as the `frozen` flag. It did not. `frozen` must also cover the freezes the WRAP ITSELF inserts, which no field of an occurrence can describe, so the wrap computes it structurally and `already_lagged` stayed unread. The IR pins for the deleted fields go too -- that is the point of the "production consumer or IR pin" rule, not a loophole around it. Two of them are rewritten rather than dropped, because the FACT they pinned still exists at a different granularity: the hoisted-reducer test now asserts `ThroughAgg` on the per-EDGE `ClassifiedSite`, where the routing actually lives, and both keep their assertions that the occurrence preserves the RAW walker shape where the per-edge view reclassifies `Wildcard` -> `DynamicIndex`. The `already_lagged` walker pin is deleted outright; the behavior it protected (the wrap never re-wraps already-lagged content) is pinned end-to-end by `char_already_lagged_other_dep`. Not done here, deliberately: `in_reducer` is ALSO the natural signal for the GH #779 bare-reducer-feeder decline, which today re-derives "inside a scalar reducer" with its own walk. Consuming it there is not behavior-neutral, because the two decisions use two different reducer SETS -- `reducer_collapses_to_scalar` (includes SIZE, excludes RANK) versus `builtin_routes_through_agg` (excludes SIZE, includes RANK) -- so a bare source inside `RANK(...)` would flip from scored to loudly declined. That divergence between adjacent decisions is a real latent drift surface and gets an issue, not a silent unification.
Moving the `PerElement` row pinning into the ceteris-paribus wrap made `OccurrenceSite::axes` load-bearing for the emitted equation: the pinning reads the per-axis classification to decide which index is a projected coordinate and which is a fixed literal. The `#[cfg(test)]` occurrence builder synthesizes that same field so the text-in/text-out wrap unit tests can drive the real wrap without a db -- and the corpus gate compared only `shape`, so nothing proved the builder's AXES matched the IR's. The gate now compares them. Writing the comparison immediately found the divergence it was meant to find. The builder had no `MismatchedIterated` arm: a target-iterated dimension name that does NOT line up positionally with the source's axis (the GH #526 case) came out `Dynamic`. Every consumer treats the two identically -- both are "not describable per axis", so the pinning declines either way -- which is exactly why it could sit there unnoticed. It is fixed by mirroring `classify_occurrence_axes`'s arm order rather than by relaxing the assertion, because the value of the gate is that the reconstruction is faithful, not that it is close enough for today's consumers. The Expr0 classifier family stays `#[cfg(test)]` rather than being deleted, and this is the reason: at least three wrap unit tests cannot be expressed as db fixtures at all. Two feed deliberately unparseable and empty text (they exist to pin the loud parse-failure contract). The third is `"SUM(w[from]) + from"` -- the Fig. 2 Q4 index-nested reducer-freeze selection -- which the engine REJECTS as a model (`ArrayReferenceNeedsExplicitSubscripts`, a scalar used as a subscript index; the same reason `char_reducer_index_nested_freeze`'s own model does not compile). That one needs an occurrence stream, so there is no db-backed substitute for it. Deleting the builder would mean deleting the test, and the shape it pins is one where a mutation was already shown to pass the whole corpus while changing the emitted text. So the seam is kept deliberately, and this commit is what makes keeping it honest: one classifier family in production, a reconstruction confined to test support, and a corpus gate that now proves every field of that reconstruction the production wrap actually reads -- `site_id` path, `shape`, `axes`, `in_reducer`.
`ClassifiedReducer` walked the target's `Expr2` to find the reducer applied to the source and then handed back its array argument as printed TEXT, which every consumer immediately parsed back. Since `patch::expr2_to_string` IS `print_eqn(expr2_to_expr0(..))`, that was a parse of our own print of a tree the classifier was holding in exactly that shape. `body_text: String` becomes `body: Expr0`, and with it three more of the eight `Expr0::new` sites go: `pinned_body_row_terms`, `generate_linear_body_partial`, and `expr_reference_idents`. Two of those parses had failure modes that are now structurally impossible rather than merely unreachable, and one of them was SILENT. `expr_reference_idents` documented "returns an empty set when the text does not parse" -- and its result is the freeze set (`model_deps` / `arrayed_dep_dims`) the row partial holds at `PREVIOUS`. An empty freeze set does not fail: it emits a "partial" that freezes nothing, i.e. the target's own equation, whose score magnitude is a constant |Δz/Δz| = 1. That is the GH #311 hazard exactly, reachable through a function whose docstring described the fallback as if it were benign. The other two bailed to the delta-ratio fallback on a parse failure, which was loud enough but is now moot. Nothing in the failure handling was load-bearing, because there is no longer a parse to fail. `ClassifiedReducer` loses `Eq` and gates `Debug` on `debug-derive`: `Expr0` carries an `f64` on `Const`, so it is `PartialEq` only, and its `Debug` is feature-gated. `--no-default-features --lib` still builds, which `src/simlin-engine/Cargo.toml` documents as the constraint feature-gating hygiene must be checked against. Not done here, deliberately: the two remaining agg-equation feeders (`generate_scalar_feeder_to_agg_equation` / `generate_iterated_feeder_to_agg_equation`) parse `AggNode::equation_text`, and carrying an AST there is not the same mechanical change. `AggNode` is salsa-cached, its `Debug` derive is unconditional, and -- the real obstacle -- `Expr0`'s `PartialEq` includes `Loc`, so storing one would make a salsa cache entry position-SENSITIVE where the text key it sits beside is deliberately position-insensitive ("the canonical (`Loc`-insensitive) key the node was deduped on"). That wants a decision about whether to store the AST loc-stripped, not an edit.
Regression from the previous commit, found by a reviewer probe. A `PerElement`
source occurrence inside a `LOOKUP` TABLE argument came out UN-PINNED:
lookup(pop[region, young], PREVIOUS(input))
That dimension-name subscript cannot resolve in a scalar link-score fragment. The
fragment is dropped, the variable keeps a layout slot with no bytecode, and the
score reads a constant 0 -- the silent zero this track exists to delete,
reintroduced by the change that was deleting them.
The cause is a genuine tension, not an oversight in the walk. `db::ltm_ir` records
NO occurrence under a table argument (`BuiltinContents::LookupTable(_) => {}` --
"a graphical-function table reference is static data, not a causal edge"), and it
is right: the table reference carries no causal edge. But the pin such a reference
needs is about COMPILABILITY, not attribution. The previous arrangement pinned it
anyway, because it was a pass over the wrapped tree that re-classified with the
Expr0 classifier and so needed no occurrence. The IR-driven pinning correctly
refuses to guess -- and then said nothing, which is the actual defect.
So it now says so: no occurrence at a SUBSCRIPTED source reference sets
`WrapOutcome::missing_occurrence`, which every wrap caller already converts into
a warned skip. Loud degradation over a silent zero, the same contract the rest of
the wrap follows.
Chosen over the two alternatives deliberately. Pinning it structurally from the
source's declared dimension names would restore the old output exactly, but it
means a small per-axis rule living outside the IR -- reintroducing in miniature
the classifier the previous commit deleted, for a shape whose reachability from a
real model I could not establish (a `LOOKUP` table argument resolves to a
lookup-only variable, which has no value slot and so can never be a link-score
source, or to the WITH-LOOKUP self-reference, whose head is the TARGET rather than
the source). Recording table-argument occurrences in the IR would make them
live-selectable by the wrap, changing what it holds live on unrelated models. If
the warning ever fires on a real model, that is the signal to support the shape
properly rather than to guess at it.
The scope is narrow by construction: the other two pin-only descents (a
pre-existing `PREVIOUS`/`INIT` call, a whole-frozen reducer) ARE walked by the IR,
so they cannot trip this, and a BARE `Var` cannot either -- `pin_bare_source_ref`
pins it structurally from the source's own declared dims and needs no occurrence.
No char golden changes; none has a `LOOKUP` inside a partial at all, which is
exactly why nothing caught this.
The architecture doc still described the ceteris-paribus wrap as working "via `wrap_non_matching_in_previous` and `classify_expr0_subscript_shape`" -- a classifier that is `#[cfg(test)]`-only and now lives in a different file. A stale pointer to a deleted seam in the map is the same drift the seam's deletion was about, and this file's own maintenance note asks for it to be kept current. It now says what is true: production never parses a target equation (the wrap takes an `Expr0` lowered from `Expr2`, and `expr2_to_string` is the print of that same lowering, so the former round trip was a parse of our own output); every per-occurrence decision is an occurrence-IR lookup by structural path, which equals the `SiteId` by construction now that both walk the same tree; there is ONE access-shape classifier family, on `Expr2`; and the Expr0 mirror is test-support only, kept because three wrap unit tests cannot be db-backed fixtures, with the corpus gate proving it matches the IR field for field. It also records the one place the IR deliberately has nothing to say -- a `LOOKUP` table argument -- and that a source reference there is declined LOUDLY rather than pinned by guess. Also documents the four `#[path]`-mounted `ltm_augment` siblings, which the map never mentioned at all -- including that the row pinning is called FROM the wrap as it descends rather than as a pass over its output, since that is the non-obvious part a reader would otherwise have to reconstruct.
`8d3ee4ab` declined the whole per-element partial when a source subscript had no
recorded occurrence, and the only shape that reaches that is a `LOOKUP` TABLE
argument. The decline is wrong, and worse than the un-pinned emission it
replaced: a target can read the source normally AND in a table argument, and the
decline threw away the direct site's scores too.
target[Region] = effect[Region,young] + LOOKUP(effect[Region,old], TIME)
The first term is a perfectly good scoreable `PerElement` site. Because the same
target also mentions `effect` in a table argument, the partial returned
`UnfreezablePartial` and EVERY per-element score of the `effect -> target` edge
was dropped with a warning. The shape is also constructible and deliberately
supported: `codegen::extract_table_info` resolves a table argument by offset ->
owning ident with nothing restricting it to lookup-only variables, and carries an
explicit `StaticSubscript` arm for `LOOKUP(g[<single element>], x)`. A variable
with tables AND a real equation is value-bearing (`var_is_lookup_only` wants an
empty/sentinel equation), has a series, and can be a causal source.
The tension `8d3ee4ab` identified is real but resolves the other way. `db::ltm_ir`
records no occurrence under a table argument ("static data, not a causal edge")
and it is RIGHT: no causal edge, no attribution, no score. The lowering is
separately obliged to emit a COMPILABLE scalar fragment, and a dimension-name
subscript cannot resolve in one. Two obligations, and discharging the second does
not require the first -- so `pin_dimension_name_indices` discharges it by NAME,
consulting no occurrence and inferring no shape: an index spelling the dimension
the source declares AT THAT POSITION becomes this target element's coordinate for
it, and an index that axis declares as an element is qualified by that axis.
`pin_bare_source_ref` has always done exactly this structurally for a bare `Var`.
It is a lowering-completeness rule, not a second classifier, and it cannot make
the reference live-selectable: the pin-only descent records no `live_ref` and
spells every pin qualified. The output is byte-identical to the pre-`391bc3c1`
pass's, which is what a regression fix should be.
The loud net stays BEHIND it, for the class the rule provably cannot describe: an
index naming a mapped or transposed dimension (`pop[State, old]` on a source over
`[Region, Age]`) still sets `missing_occurrence`. Deciding that `State` reads
`Region` through a positional mapping rather than being a mismatch IS the per-axis
classification the IR owns, and it recorded nothing. Fixing the reachable shape
while leaving the class silent is how this survived in the first place.
Probing all three pin-only descents (rather than just the one that changed) found
that the pre-existing `PREVIOUS`/`INIT` descent had NO coverage at all: deleting
it left all 5274 lib tests and all 634 integration tests green, the same hole
`391bc3c1` closed for the whole-frozen reducer. Each descent now has exactly one
dedicated test that fails when it is deleted, and the four share one fixture --
per-test copies of the same `(Region, Age)` scaffolding were 60 lines each, which
also pushed `ltm_augment_tests.rs` past the file cap.
All 16 char goldens are byte-identical; none has a `LOOKUP` inside a partial at
all, which is exactly why nothing caught either the regression or the decline.
The new integration test is the end-to-end guard: a per-element-table source read
both ways compiles with zero assembly warnings, emits both per-element scores with
the table argument pinned to each target element's own row, and simulates to real
non-zero values.
The pin-only descent's index closure walked a `Range`'s two endpoints but passed a plain `Expr` index straight through -- while its own comment said it descended "exactly as the other-variable arm above does", and that arm handles both. So a source reference nested in an EXPRESSION index of a `from`-headed subscript was neither pinned nor flagged: its dimension-name subscript survived into the scalar link-score fragment, which cannot compile, so the fragment was dropped and the score read a constant 0. Same silent zero as the table-argument case, reached through the sibling index kind. It is reachable through the shape the previous commit fixed, which is why it belongs here rather than in a follow-up: a table reference may carry a DYNAMIC element index (`LOOKUP(pop[Region, pop[Region, young]], x)`), which `codegen::extract_table_info`'s `Expr::Subscript` arm computes an element offset for on purpose. The IR records nothing anywhere under a table argument, so BOTH the outer reference and the one inside its index need the structural pin, and before this only the outer one got it. Found by probing all three pin-only descents rather than only the one that changed. The new test fails on the mutation that removes the arm.
Reviewer finding against the previous two commits, and it turns on an asymmetry
those commits missed. Of the three pin-only descents, a `LOOKUP` TABLE argument is
the only one NOT inside a freeze: the wrap holds it verbatim rather than wrapping
it, because a `PREVIOUS` of a table has no value slot. So anything the descent
leaves live in a table argument stays live in the emitted partial.
For a statically-spelled table reference that is fine and is the whole point of
`5ce2aa69`: `LOOKUP(effect[Region, old], TIME)` selects an element by name, which
is not a value read, so pinning it is pure lowering. But an index that READS at
runtime is a different thing:
LOOKUP(pop[Region, pop[Region, old]], x)
LOOKUP(pop[Region, idx], x)
`codegen::extract_table_info` evaluates that index to choose the table element, so
the partial isolating `pop[Region, young]` also varies with the current-step value
of `old` -- one row's influence attributed to another. Pinning made those compile
(`0f1f0e9b`) without making them correct, which is the worse failure: a
compilable-but-wrong score is what GH #311 / #661 / #743 all exist to avoid, and it
is invisible where a dropped fragment at least warns.
A descent that wraps nothing cannot make them ceteris-paribus, so the rule now
discharges ONLY statically resolvable indices -- the axis's own dimension name, an
element that axis declares (or its already-qualified spelling), a numeric literal
-- and declines everything else into the existing loud skip. The declined class is
now uniform: a runtime read, another dimension's name, an over-arity index, an
arithmetic index, a range. Previously a bare name the axis did not declare fell
through unflagged, which is exactly how `pop[Region, idx]` slipped past.
The two descents that ARE inside a freeze need no such refusal, and this is why
`0f1f0e9b`'s expression-index descent stays: under a pre-existing `PREVIOUS`/`INIT`
or a whole-frozen reducer everything is already lagged, index reads included, so
pinning is sufficient and a second freeze would read two steps back. That commit's
test moves onto a `PREVIOUS`-nested fixture, where the descent is both reachable
and correct, and asserts the absence of the nested freeze.
The three decline shapes share one table-driven test rather than three near-copies
(also what keeps `ltm_augment_tests.rs` under the file cap). Both tightenings are
mutation-verified: accepting a non-static index, or accepting a bare name the axis
does not declare, each fails it. All 16 char goldens stay byte-identical.
Not addressed here, and pre-existing: the wrap leaves a table argument's indices
untouched for EVERY caller (`ctx.pin` `None` included), so `LOOKUP(g[idx], x)` in
any link-score partial keeps `idx` live. That is the same attribution defect
outside the `PerElement` path, it predates this branch, and fixing it means giving
the wrap's own index pass the table argument. Filed separately.
Reviewer finding against `db6dc850`, whose refusal was keyed on the wrong thing. It
asked "is this a LOOKUP table argument?" when the question that matters is "is this
subtree already inside a freeze?" -- and those differ, because a `LOOKUP` can sit
INSIDE one of the other two pin-only descents:
growth[Region] = pop[Region, young] + PREVIOUS(LOOKUP(pop[Region, idx], x))
The IR skips a table argument wherever it appears, so this reaches the same
no-occurrence path -- but the enclosing `PREVIOUS` lags everything inside it, index
reads included. Ceteris paribus already holds, and refusing dropped a perfectly
good per-element score plus every loop score through the edge. That is the same
over-decline `5ce2aa69` exists to undo, reintroduced one level down.
So the two loud verdicts are now distinguished, because only one of them depends on
the freeze. `IndexVerdict::Unspellable` -- another dimension's name, an axis this
target does not project, an over-arity index -- is a COMPILABILITY verdict: left
alone it keeps a dimension-name subscript that cannot resolve in a scalar fragment,
so it is loud everywhere, a freeze included. `IndexVerdict::RuntimeRead` -- a
variable or an expression selecting the element -- compiles fine, so it is purely a
CETERIS-PARIBUS verdict: loud in a bare table argument (which stays live), kept
inside a freeze (which lags it). The wrap already threads a `frozen` flag beside
`path` for exactly this kind of question, so the descent takes it too: `true` at the
`PREVIOUS`/`INIT` and whole-frozen-reducer sites, and the wrap's own `frozen` at the
table-argument site, which inherits the enclosing freeze and nothing more.
Naming the four possible verdicts is what makes the split legible; the previous
`Option<String>`-plus-bool encoding is what let two different reasons for declining
get conflated in the first place.
Three mutations, three distinct failures, no overlap: ignoring the freeze (always
decline) fails the frozen test, dropping the refusal entirely fails the bare test,
and gating `Unspellable` on the freeze fails the frozen test's second half. That
third probe initially passed -- nothing covered "unspellable inside a freeze is
still loud" -- so the frozen test carries both halves now.
The pin-descent tests move to their own `ltm_augment_pin_tests.rs`, mounted as a
child of the `tests` module so `use super::*` still resolves every helper. They had
pushed `ltm_augment_tests.rs` past the file cap, and they are a cohesive family:
one test per descent, each failing when its descent is deleted, plus the two
obligations (compile, stay ceteris-paribus) the file header now states.
All 16 char goldens byte-identical.
Every defect this branch fixed in the row-pinning rule was a CELL rather than a
logic error: one index shape landing in the wrong bucket. The `391bc3c1`
regression, and both review findings since, were each fixed after someone supplied
the counterexample -- which means the tests covered the cases people happened to
construct, not the space.
So the space is now stated. Thirteen index kinds x {bare table argument, inside a
freeze} = 26 cells, each with its verdict: pinned to a spelling, or declined into
the warned skip. The four static-selector rows and the four unspellable rows are
identical in both columns; only the three runtime-read rows differ, and that
difference IS the compilability-vs-ceteris-paribus split, now visible as a shape in
the table rather than as prose in a rustdoc.
Every cell matched the current implementation on the first run, so this adds no
production change -- it converts "the reviewed cases are right" into "every case
has a stated verdict". It is load-bearing rather than decorative: all four
mutations of the verdict logic (always-decline a runtime read, never-decline one,
gate `Unspellable` on the freeze, stop qualifying a declared element) fail this
test independently of the seven per-shape tests beside it.
Two rows are marked UNREACHABLE and assert current behavior rather than a derived
requirement, because a compilable model cannot produce them: codegen's
`extract_table_info` rejects a range or wildcard table index with `BadTable`, so
the target's OWN equation fails to compile before its link scores exist. Their
frozen cells are honestly uncertain -- the rule accepts them today, and if they
became reachable the failure would surface as a fragment-compile Warning instead
of a warned skip, loud either way through a different channel. Left as-is
deliberately rather than guessed into `Unspellable`, since no reachable shape
distinguishes the two choices. The numeric-literal row is likewise about the
rule's verdict only: a numeric index into a NAMED dimension does not resolve, but
that is a property of the target's own equation, which carries the same index.
Non-vacuity is asserted per cell, not once: every case re-checks that the IR
records NOTHING for the table argument, so a cell can never silently end up
testing the occurrence walker instead of the rule.
Review — no blocking findingsI reviewed the twelve commits across the six substantive files ( What I checked and cleared:
One minor observation, not a blocker. In Overall correctness verdict: correct. The core claim — that the wrap's child-index path space now equals the IR's |
Owner
Author
|
Review pass 7 (over the verdict enumeration itself) found two P2s, both in
Root cause is shared: the rule hand-rolls per-axis name resolution that already exists in single-source form ( Worth recording the methodological point: mutation testing proved the table constrains the code — four verdict mutations fail it independently of the per-shape tests — but it cannot show the table asserts the right verdicts. A cell matching the implementation because both are wrong is invisible to mutation by construction. That gap is exactly what this pass closed, and it is why the enumeration was reviewed rather than trusted. Not ready to merge until two consecutive clean passes land, at least one after the enumeration extension. |
`pin_dimension_name_indices` -- the lowering-completeness rule that pins a `LOOKUP` table argument's row, the one source reference the occurrence IR deliberately records nothing for -- decided each index's verdict by comparing names itself. Two shapes it could have resolved were therefore filed loud, and a loud index declines the whole partial, dropping every link score on the edge rather than just the table argument's (which earns none anyway). A MAPPED dimension name was judged unspellable purely because it differed from the source axis's name. `target[State] = effect[State,young] + LOOKUP(effect[State,old], time)` over an `effect[Region,Age]` source is a valid model when a positional State/Region mapping exists: the simulation reads Region's element at the position of the State element being computed. Which element that is, is exactly what `per_element_row_for_target` answers -- already the ONE row derivation the score's NAME and the occurrence-driven pin both consume -- through `DimensionsContext::mapped_element_correspondence`. So the identity axis and a mapped axis collapse into one arm, gated on the target actually iterating the dimension, which is `ltm_agg::classify_axis_access`'s own gate. Both bottom out in the same correspondence, so the rule now accepts exactly the mapped pairs the IR does, in either declaration direction (GH #757), and declines an explicit element map for the GH #756 reason. `Unspellable` now means the shared derivation could not resolve the axis, not that a name differed. An `@N` POSITION index reached the catch-all and scored a runtime read, which declines a bare table argument on ceteris-paribus grounds. It selects a FIXED element and reads nothing at the current step, and `compiler::context` resolves `DimPosition` to a concrete element offset in scalar context -- which a link-score fragment is -- so it is kept verbatim. Spelling it out as an element name here would be a second implementation of position syntax outside the compiler that owns it; the new end-to-end test compiles a fragment with `@2` intact rather than taking that on faith. `classify_axis_access` itself is deliberately NOT the helper consulted. It consumes `IndexExpr2` where this descent walks the `Expr0` the wrap lowered, and it answers the hoisting question rather than the compilability one -- which is why it declines `@N`, correctly there and wrongly here, and why it carries a `Reduced` verdict a pin cannot use. The shared ANSWER this rule needs is one level down, at the row derivation and the correspondence. The enumeration grows from 13 rows to 18 over two tables; the mapped rows need their own, since a mapped axis requires a target dimension the source does not declare. Every emitted cell now also asserts the live site kept its bare row, so a fixture that is not a `PerElement` instantiation can no longer pass. Both defects were missing ROWS in an all-green table, which is the lesson: a mutation probe shows that a table constrains the code and nothing about whether its rows cover the space.
Both defects the last commit fixed were missing ROWS, so "no gaps" should be a checkable claim rather than an assertion. `IndexExpr0` has exactly five variants and `StarRange` had no row -- the one variant the enumeration never named. It lands in the same unreachable class as the range and wildcard rows and for the same single reason, now stated once: `codegen::extract_table_info` accepts a table argument only when the subscript selects exactly ONE element (a resolved `Var`, a `StaticSubscript` of `view.size() == 1`, or a `Subscript` whose every index is `SubscriptIndex::Single`), so anything wider is `BadTable` before link scores are generated. That reason also settles subscript ARITY, which the table deliberately does not vary beyond the existing over-arity row: an UNDER-arity subscript leaves its unindexed axes in the view, so it is `BadTable` too. Rowing it would state a verdict about a shape no compilable model can produce, which is the thing this table is supposed to stop doing. A probe confirms the new row is distinguishable rather than a duplicate of the wildcard row: giving `StarRange` a `Static` arm of its own moves that row and nothing else.
Third finding of the same shape as the first two: a static selector filed as a runtime read, so a bare `LOOKUP` table argument declined and every link and loop score on the edge was dropped. `LOOKUP(effect[Region, 1 + 1], time)` reads no variable at any step, so it is exactly as static as the bare `2` the rule already left alone -- the catch-all simply never looked INSIDE a compound index. The target compiles and so does the fragment; only the score went missing, which the new end-to-end test reproduced as a warned skip before the fix. `index_expr_selects_a_fixed_element` is deliberately the narrowest sound predicate rather than a general invariance test. `compiler::invariance::exprs_are_invariant` is the engine's run-invariance derivation, but it consumes LOWERED `Expr` plus an offset-classification callback -- neither of which exists in this position -- and its notion is WIDER than this rule's obligation: `Dt`, `StartTime`, and `INIT(x)` of any variable are all run-invariant, yet whether a table index may read a variable's init buffer is an attribution question, and attribution is the wrap's vocabulary, not this rule's. So the predicate decides only the half it can decide alone, and its match over `Expr0` is exhaustive with no catch-all, so a new variant forces a decision here instead of silently inheriting a refusal. The widening stops at a 0-arity BUILTIN index, and that boundary is stated rather than left as a gap. `reify_0_arity_builtins` turns `time` into an `Expr0::App` before this rule sees it, and telling `TIME` (varies every step) from `PI` (does not) inside `App` would mean a fourth copy of the builtin classification `builtins`/`compiler::invariance` own. The arm stays loud in a bare table argument -- the conservative direction, a warned skip rather than a confident wrong score -- and it now has an enumeration row saying so, so the next reviewer reads a decision instead of an oversight. 18 rows now, and four probes pin the predicate's boundaries: deleting the arm, widening it to `Var`, widening it to `App`, and making it non-recursive are each caught, the last three by rows that already existed.
XMILE lets a dimension declare an element whose name is also a dimension name (`Category = [Region, x]` beside a `Region` dimension), and the table-argument pin resolved such an index as the DIMENSION first. The compiler does the opposite: `normalize_subscripts3`'s `Expr3::Var` arm looks the name up in the axis's own `indexed_elements` and only falls back to the dimension-name `ActiveDimRef` reading if that misses -- its comment says "takes priority". So for `target[Region] = ... LOOKUP(effect[Region, Region, old], TIME)` over an `effect[Region, Category, Age]` source, the pin contradicted the equation's actual meaning. Both wrong outcomes matter, in the two directions this branch cares about. With no `Region`/`Category` mapping the dimension reading finds no correspondence and declines, dropping every score on a valid edge -- the silent-zero direction. With such a mapping it would pin the correspondence's element instead of the literal one: a compilable, confidently WRONG row, which is the outcome worse than none. The fix is to try the axis-element reading first; `qualify_axis_element` already tests exactly the axis's own `indexed_elements`, the same membership the compiler tests, so this is a reordering rather than new knowledge. The previous ordering was justified in the rustdoc as mirroring `ltm_agg::classify_axis_access`, which was mirroring the wrong thing -- `classify_axis_access` checks `target_iterated_dims` first and so disagrees with the compiler in the same way. That is filed as GH #986 rather than fixed here: it is a precision gap today (its dimension-first reading then finds no mapping and declines, so the reference falls to the conservative path instead of being mis-resolved) and it feeds reducer hoisting, the reference-shape IR, and element-edge emission, so reordering it moves decisions well outside this rule. The rustdoc now records the divergence as deliberate and points at the issue. The new enumeration carries a CONTROL row -- the same axis's other, non-colliding element -- so a fix that merely special-cased the colliding name would not pass both. The probe is a precise inversion (let the dimension arm claim a name it shares with an axis element) rather than a deletion, and it moves exactly the collision rows and nothing else.
Review — no blocking issues foundTwo independent review passes across the ~10.8k-line diff, focused on the areas the PR description flagged as having taken multiple rounds:
Path/ Verdict: correct. No P0/P1/P2 findings. |
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.
Part 2 of Track A, stacked on #980. #980 deleted LTM's
AST -> text -> ASTround trip on theoutput side (generated equations stored as typed ASTs) and killed the classify-twice contract
that produced silent-zero link scores. This deletes the round trip on the input side: the
ceteris-paribus wrap no longer parses the target equation's printed text.
Part of #965. Does not close it — see "What remains" below.
The result worth leading with
expr2_to_stringis literallyprint_eqn(expr2_to_expr0(e)). So every one of the eightExpr0::newsites inltm_augment.rswas parsing our own printed output of a tree we alreadyheld in that exact shape. Deleting them is "pass the
Expr0instead of theString".The consequence is stronger than the parse count.
db::ltm_ircomputes every occurrence'sSiteIdon theExpr2tree; the wrap addresses occurrences by the child-index path it tracks asit descends. Feeding the wrap the direct lowering makes those two path spaces the same tree by
construction. Before, they coincided only because print-then-parse happened to round-trip — a
property the corpus alignment sweep could sample but never establish. Given that five of the
six defects #980's review found were silent zeros arising from exactly this class of
"coincides in practice" assumption, moving it to by-construction is the point of the change.
Parse-site tally: 5 deleted, 1 relocated to a legitimate source-format boundary, 2 remain
(with reasons, below).
ltm_augment.rs: 5921 -> 5644 lines.Commits
c57cca0c391bc3c10d730b75b151a60cf54919cd8d3ee4ab123a2cd05ce2aa690f1f0e9bdb6dc850fb0863b0fa34e26bb11c182a566088a4172abc1b902b1f27The crux: row pinning moved inside the wrap
391bc3c1is the load-bearing commit. Deleting the last two production Expr0-side classifiersrequired relocating
PerElementrow pinning, and the obvious approach — a shape-preservingpre-pass on the original AST — is wrong, which we established from the corpus rather than by
argument.
Row pinning emits two spellings of the same site shape: the occurrence the wrap left live
gets the row with bare element names; every occurrence it froze gets the same row qualified
(
region·boston), so the frozen read compiles to a directLoadPrevinstead of an ambiguous bareelement name. That live-vs-frozen fact exists only in the wrapped tree, and it is not derivable
from the occurrence — two occurrences with identical
axescan land on opposite sides.per_element_index_nested_occurrence.txtalready pins both spellings inside one equation. On amodel whose element names are ambiguous across dimensions — C-LEARN's regions — the bare spelling
fails to compile, so a pre-pass's divergence would be a silent zero, not cosmetic.
So the pin runs inside the wrap, which is the one traversal that knows both the occurrence (by
path) and whether it is about to freeze. This reverses a documented decision in #980's
86df68bf, whose rustdoc rejected wrap-time occurrence tagging — that needs a tag-only descentor a
Loc-keyed side map, i.e. a second representation with its own drift surface. A pin-onlydescent writes no tag and carries no map; it reads the one IR by path and performs the actual
lowering. Different thing, admissible for that reason.
Pinned narrowly rather than incidentally: mutating
child_frozen = frozen || will_wrapdown tofrozen— the exact information loss a pre-pass suffers — fails precisely one golden, the onenamed in advance. Ignoring live/frozen entirely fails eight.
One classifier family, and the surviving mirror is gated
classify_expr0_per_element_axesandexpr0_iterated_axis_lines_upare out of production. Per-axisequivalence is exact, not approximate: the retired classifier consulted the same
ltm_agg::classify_axis_accessthe IR uses, gate for gate.A
#[cfg(test)]Expr0 mirror survives, moved beside its only consumer inltm_augment_wrap_test_support.rs. Full deletion is not achievable: three wrap unit tests cannotbe db-backed fixtures — two feed deliberately unparseable and empty text (they pin the loud
parse-failure contract), and the third is
SUM(w[from]) + from, which the engine rejects as amodel (
ArrayReferenceNeedsExplicitSubscripts) yet still needs an occurrence stream.Per the plan's own rule — mirrors are acceptable when drift is loudly gated; where one must stay,
the gate is the deliverable —
b151a60cextends the corpus gate from comparingshapetocomparing
axesas well, so it now proves every field of the reconstruction the productionwrap reads (
SiteIdpath,shape,axes,in_reducer). AnIndexExpr0 -> IndexExpr2lift wasrejected: it would add a mirror faithful only to the forms
classify_axis_accesshappens toinspect, in order to delete a mirror.
OccurrenceSite: four fields deleted, one kepttarget_element,routing,reducer_keys,already_laggedhad no production reader. Thestrongest case is
routing, which was not merely unread but a duplicated derivation:model_ltm_reference_sitesheld two copies of the #793 agg-narrowing rule, and deleting the fielddeletes one of them. Duplicate logic inside the IR built to remove duplicate logic.
in_reducerstays — its reader is the corpus gate's reducer-context assertion, and deleting itwould silently narrow that gate. Its rustdoc says so explicitly, and records why it is not
consumed by the #779 bare-feeder decline:
reducer_collapses_to_scalarincludesSIZEandexcludes
RANKwhilebuiltin_routes_through_aggdoes the opposite, so wiring it up would flip abare source inside
RANK(...)from scored to loudly declined. Filed as #982.Deleting
already_laggedalso removed a parameter still threaded through every recursive call ofwalk_all_in_exprcomputing only itself — dead code the compiler cannot see, because it isgenuinely "used" at each hop.
A regression introduced and fixed inside this branch
391bc3c1introduced a silent zero, and the story is disclosed rather than tidied because the wayit was found matters.
A
PerElementsource occurrence inside aLOOKUPtable argument came out un-pinned:lookup(pop[region, young], ...). That dimension-name subscript cannot resolve in a scalarfragment, so the fragment is dropped, the variable keeps a layout slot with no bytecode, and the
score reads a constant 0. The cause is a genuine tension:
db::ltm_irrecords no occurrence undera table argument ("static data, not a causal edge") and it is right — but the pin such a
reference needs is about compilability, not attribution. The pre-
391bc3c1pass pinned itanyway because it re-classified the wrapped tree and so needed no occurrence. The IR-driven
pinning correctly refused to guess, and then said nothing, which was the defect.
8d3ee4abmade it loud. That was the wrong fix, on a premise that turned out false: theclaim was that a table head can only be a lookup-only variable (no value slot, never a causal
source) or the WITH-LOOKUP self-reference. In fact
compiler/codegen.rs::extract_table_inforesolves a table argument by ident against any table-carrying variable and has an explicit
Expr::StaticSubscriptarm deliberately supportingLOOKUP(g[<single element>], x); andvar_is_lookup_onlyrequires an empty/sentinel equation, so a table-carrying variable with anequation is value-bearing and can be a causal source. A review pass then produced the case that
shows the blast radius:
That edge is scoreable from the first term alone — but because the same target reads
effectin atable argument, the whole partial returned
UnfreezablePartialand every per-element score forthe edge was dropped.
5ce2aa69replaces the decline with a structural pin (pin_dimension_name_indices): an indexnaming one of the source's own declared dimensions becomes this element's coordinate, and a literal
element index is qualified by its axis. It consults no occurrence — enforced by its signature,
which takes only indices and a context — infers no shape, makes no live/frozen decision, and
cannot make a table argument live-selectable. The loud net is kept behind it: an index naming a
different dimension still sets
missing_occurrence, because resolving that needs the mappingclassifier the IR owns.
0f1f0e9bcompletes it — a table reference may carry a dynamic elementindex (
LOOKUP(pop[Region, pop[Region,young]], x)), whichextract_table_infocomputes an offsetfor on purpose, so the reference inside the index needs the same pin. The
IndexExpr0::Exprarmthe closure's own comment already claimed to have.
Then two more review passes each found a defect in the previous fix, and the end state is worth
more than the individual fixes.
0f1f0e9bmade the shape compile without making it correct:the table argument is the only one of the three pin-only descents not inside a freeze — the
wrap holds it verbatim precisely because a
PREVIOUSof a table has no value slot — so a runtimeindex left live there stays live in the emitted partial, and a sibling occurrence's partial then
varies with it.
db6dc850declined runtime indices;fb0863b0then found that over-declined,because a
LOOKUPcan itself sit inside aPREVIOUS/INITor a whole-frozen reducer, where theindex is already lagged and refusing drops a valid score.
The resolution is a three-way split, which is the real shape of the problem:
UnspellableRuntimeReadBoth earlier defects were the same mistake: one predicate trying to answer two independent
questions. A lowering has two obligations — it must compile and it must preserve ceteris
paribus — and the pin-only descent satisfies the second only because the enclosing node freezes
everything. The table argument is the one site where that premise fails, which is exactly where
both defects landed. That premise is now in
pin_dimension_name_indices' rustdoc in those terms.The verdict enumeration
fa34e26baddsper_element_pin_index_verdict_enumeration: 13 index kinds × {bare tableargument, inside a freeze} = 26 cells, each stating the expected verdict. Every cell matched the
implementation on the first run, so it is test-only.
It exists because both of the findings above were cells — a shape in the wrong bucket — not
logic errors, and each was supplied by a reviewer's counterexample. An enumeration states a verdict
for every shape rather than the ones someone thought of.
The table's shape is itself the result: the four static rows and four unspellable rows are
identical across both columns, and only the three runtime-read rows differ — so the
compilability-vs-ceteris-paribus split is a visible property of the table rather than a claim in a
comment. Three of the unspellable rows were not previously tested at all (an own dimension at a
position the target doesn't project, transposed own dimensions, and an over-arity index).
It is a gate, not documentation: four mutations of the verdict logic fail it independently of
the seven per-shape tests, so an edit that moves a cell goes red even without breaking a named
shape test. Non-vacuity is asserted per cell — every case re-checks that the IR records nothing
for the table argument — so no cell can drift into testing the occurrence walker instead of the rule.
Two cells are marked uncertain rather than asserted. A range and a wildcard table index
are unreachable from a compilable model: codegen's
extract_table_inforejects them withBadTable, so the target's own equation fails to compile before its link scores exist. The rulecurrently accepts them when frozen. If they ever became reachable, the failure would surface as a
fragment-compile Warning rather than a warned skip — loud either way, different channel. They were
left marked rather than guessed into
Unspellable, because no reachable shape distinguishes the twochoices and a speculative production change was not worth the blast radius. One further caveat: the
numeric literal cell asserts the rule's verdict (leave a static selector alone), not downstream
compilability — a numeric index into a named dimension does not resolve, but that is a property of
the target's own equation, which carries the same index.
Verification
0f1f0e9b), 236.73s (db6dc850), 236.72s (fb0863b0) — three runs within 0.4s of each other. Also green at 242.30s on391bc3c1, the commit that could falsify the pinning design, on the model whose element names are ambiguous across dimensions. Used as a negative control as well as a smoke test: had any real C-LEARN partial contained a runtime table index, the refusal indb6dc850would have dropped its score and the pinned-loop assertion would have gone to zero. It didn't, so the tightening is narrow in practice.cargo test -p simlin-engine --lib: 5280 passed / 0 failed / 3 ignored; integration 637 passed / 0 failed.scripts/lint-project.sh,cargo clippy --all-targets, andcargo check -p simlin-engine --lib --no-default-featuresall clean; full pre-commit hook green on all 9 commits.git checkout.What remains on #965
Criteria 1-4 are met. Criterion 5 is substantially met with one documented exception, so this does
not close the issue:
subscript_idents_at_element's re-parse (site 2147) is byte-load-bearing. The wrap constructsPREVIOUSuppercase; four goldens carry lowercaseprevious(only because print -> re-parselowercases function names, while
arrayed_target_slot_scores.txtcarries the uppercase spellingon a path where no re-parse happens. Both spellings are pinned in the corpus today. Deleting the
site therefore requires normalizing the generators to one spelling and recapturing those goldens —
a case-only change, mechanically checkable, and the right end state since it makes print -> parse a
genuine no-op. Deferred deliberately rather than done at the end of a long branch; the decision is
made and the four conditions (case-fold-identical diff, one uniform spelling, semantic-inertness
proof, recorded as the single intentional golden change) are recorded.
AggNodefeeders (sites 1916/1981) are blocked on a design question, not effort.Expr0'sPartialEqincludesLoc, andAggNodeis salsa-cached with itsequation_textdocumented as "the canonical (
Loc-insensitive) key the node was deduped on". Storing anExpr0beside it makes the cache entry position-sensitive where the key is deliberately
position-insensitive — an incremental-invalidation change. Decide explicitly whether to store
it loc-stripped.
format!templates by choice. Converting them to AST builderswould re-parenthesise and re-space every emitted equation, churning all 16 goldens to produce
equivalent text — trading the corpus's entire value for an aesthetic.
Filed from this work
emit_source_to_agg_link_scoresre-parses and re-lowers an agg's equation text through the whole pipeline just to reachclassify_reducerfor a kind and a name. A deeper round trip than any of the eight addressed here. Two call sites (the second indb/ltm/loops.rsneeds the whole lowered AST), and both fallible stepsreturnearly, so its failure mode is a silent zero.reducer_collapses_to_scalarvsbuiltin_routes_through_aggdivergence onSIZE/RANK.classify_axis_accessresolves a subscript index dimension-name-first, opposite to the compiler's element-first precedence. Disclosed above; not fixed here because it feeds four downstream consumers and can move goldens.LOOKUParm returns the table argument untouched for every caller, soLOOKUP(g[idx], x)leavesidxlive in any link-score partial, not only thePerElementones. It surfaced only through this branch's code, because this path is the one that now reaches compilability. Not fixed here deliberately: it predates the branch, the real fix (give the wrap's own index pass the table argument's indices rather than holding the whole argument verbatim) touches every link score and can move goldens, and "hold the table argument verbatim" is a deliberate decision that deserves its own diff, review passes, and numeric gate rather than riding along in a regression fix.The table-argument rule: six findings, and what closed the class
The
LOOKUPtable-argument pin absorbed six review findings. Reported in full because the shape ofthe iteration is the useful part, and because a reviewer should know which residual remains.
The findings, in order: not pinned at all (silent zero); a runtime index left live (misattribution);
over-declining inside a freeze;
@Nand a mapped dimension name misfiled as runtime reads; a missingStarRangerow (self-found); a constant-expression index (1 + 1) misfiled; and subscriptname-collision precedence. Every one after the second was a cell — a shape in the wrong bucket —
not a flaw in the design.
Three structural changes closed the class, rather than the individual cells:
RuntimeRead. Thematch over
Expr0is now exhaustive with no catch-all, so a new variant is a compile errorinstead of a silent refusal.
IndexExpr0has exactly fivevariants; the table now names all five, which makes "no gaps" checkable. Subscript arity beyond
the over-arity row is a documented non-row:
codegen::extract_table_infoaccepts a tableargument only when the subscript selects exactly one element, so an under-arity subscript is
BadTablebefore link scores exist, and rowing it would state a verdict about a shape nocompilable model can produce.
Residual risk, stated plainly: this rule may still loudly refuse a statically-resolvable index no
row covers. The consequence is a warned skip and a dropped score, never a wrong number. The
silent direction — a runtime read left live, which misattributes — is guarded by the
RuntimeReadverdict plus the freeze gate and is pinned in both directions by mutation. Two deliberate boundaries
are stated rather than hidden: a 0-arity builtin index stays loud (telling
TIMEfromPIinsideAppwould be a fourth copy of the builtin classification thatbuiltins/compiler::invarianceown), and three unreachable rows (range, wildcard, star-range) are marked uncertain for one traced
reason rather than three hand-waves.
Verdict enumeration: 3 tables, 52 cells. Main 18 rows × 2 freeze columns, mapped 6 row-runs × 2
(3 rows across both declaration directions), collision 2 × 2. Ten mutation probes, all caught, and
seven of the ten are caught by rows that already existed — the table doing the job a per-shape
test cannot. Every emitted cell also asserts the live site kept its bare row, so a fixture that is
not a genuine
PerElementinstantiation cannot pass. The collision table carries a control row(the same axis's non-colliding element) so a fix that merely special-cased the colliding name fails
one of the two.
Single-sourcing eliminates drift; it does not establish truth
This branch's thesis is that one derivation should serve every consumer. One finding sharpened it,
and the correction matters more than the fix.
Subscript index precedence was resolved dimension-name-first, on the stated grounds of mirroring
classify_axis_access. But XMILE lets a dimension declare an element whose name is also adimension name, and
compiler/subscript.rs'snormalize_subscripts3is element-first — its owncomment says "takes priority". So the analysis-layer classifier disagrees with the compiler, and
"mirror the shared classifier" is what propagated the defect into the new rule.
A shared derivation that disagrees with the compiler is a single source of a consistent error.
The authority for what an equation means is the compiler; the correct instruction is not "match the
shared derivation" but "match the compiler, and share that." Fixed here by ordering the rule the
compiler's way.
classify_axis_accessitself is left wrong and filed as #986 — it feeds reducerhoisting, the reference-shape IR, element-edge emission and link-score naming, so reordering it can
move goldens and needs its own diff and verification. Today its dimension-first reading is
accidentally safe (it finds no mapping, declines, and falls to the conservative path); #986 records
that the collision-plus-mapping case is a genuine wrong-answer hazard there, not mere imprecision.
What C-LEARN does and does not gate here
Worth being precise, because a green numeric gate is easy to overclaim.
clearn_pinned_climate_loop_is_scoredasserts Discovery mode, three partition entries, and that the deterministic series is finite,
not identically zero, and negative where non-zero. It asserts no magnitudes and no warning count.
So for a tightening it is a strong control — a refusal that dropped a real score zeroes the series
and trips the non-zero assertion. For a widening it is nearly blind, and three of the four fixes
here widen what gets scored.
What actually makes it inert is structural, not luck: C-LEARN's 199 subscripted lookup-table calls
(
Historical forestry LOOKUP[COP](…)and siblings) all use literal element indices — the oneindex form this arc never changed — and those heads are table-only holders, excluded from every
runlist, so they cannot be causal sources and the rule is never reached for them. Five C-LEARN runs
across the arc landed in a 2.3s band (235.78 / 236.39 / 236.72 / 236.73 / 238.03s), all green.
The honest conclusion: C-LEARN does not exercise the shapes these four fixes changed. Their
coverage is the three new end-to-end tests (
@N, mapped dimension, name collision), each of whichcompiles a real model and asserts zero assembly warnings plus a finite non-zero score. C-LEARN's role
here is a consistency check that nothing else moved.
Method note
Worth recording honestly, because it is the reusable part.
Stage
391bc3c1created three pin-only descents (a pre-existingPREVIOUS/INIT, awhole-frozen reducer, and a
LOOKUPtable argument). Two were probed; the third was not, and thatis where the regression lived. It surfaced only because a review pass left a probe test behind. A
later probe of the
PREVIOUS/INITdescent then found that deleting it left all 5274 lib and all634 integration tests green — zero coverage on a second descent. The rule: derive the probe count
from the branch's branch structure, not from when probing starts to feel sufficient.
Four test-scaffolding divergences surfaced across this work, every one by mutation rather than by
review:
cfg(test)occurrence builder synthesized onlyPinned/Dynamicand neverIterated, so those tests leaned on the classifier rather than the occurrence stream they claimed to build;MismatchedIteratedarm, so a non-aligning dimension name came outDynamic— invisible because every consumer treats the two identically;expr_reference_identsdocumented "returns an empty set when the text does not parse", but that result is the freeze set — an empty one emits a partial that freezes nothing, i.e. the target's own equation, scoring a constant|Δz/Δz| = 1. The ltm-augment: parse fallback can silently break ceteris-paribus logic #311 hazard, behind a docstring calling the fallback benign.Review passes used
--base origin/roundtrips-track-aso each read this branch's commits ratherthan re-reading #980's 14; the #981 finding in
equation.rsbelongs to #980's stack and isdisclosed there.
Review is a sampling process, and this branch measured it rather than assuming it. Six passes:
LOOKUPpin regressionGreen here required two consecutive completed clean passes, and that bar is justified by
measurement, not argument: pass 2 read the tree clean and pass 3 found a P2 in it. A single
clean pass in this area has been demonstrated worthless once. (The same thing happened on #980,
where pass 1 read
dea42188clean and pass 3 found a P2 in that same file.) An aborted orquota-blocked pass counted as neither clean nor dirty — one pass was verified as a genuine
completion via
turn.completedand exit code rather than by grepping for a quota string, since theobvious pattern
quotafalse-matched this branch's own test nameunquotable_source_name.