engine: burn down the round-trips plan's deferred issues - #992
Merged
Conversation
`ast::needs_quoting` is the single "can this canonical name be spelled bare in an equation" predicate, but it tested only the XID character classes and the leading-character rule. The lexer resolves a bare word against KEYWORDS before it ever considers an identifier, so a variable legally named `"if"` -- neither XMILE nor Vensim reserves anything, and both our readers admit one with zero diagnostics -- printed BARE and no longer re-parsed as a reference to itself. A `patch` rename of an unrelated variable reprints every dependent equation through `print_ident` and persists the result, so an edit that touched neither the variable nor its reader silently rewrote a valid saved model into an unparseable one. That predicate has three consumers, one per producer of equation text, and all three were reached: `print_ident`, `ltm_augment::quote_ident`, and the MDL importer's reference formatter. The importer had its own rule that quoted only on `.`, which left every other unreadable name bare -- `data revenue k$` emitted a bare `$`, `Marg Capital'Energy per Capital` emitted a bare `'` that our lexer reads as the TRANSPOSE operator, and seven of the eight keywords imported to an `UnrecognizedToken` on every reference (all are names in the checked-in corpus). It now delegates to `needs_quoting` like the other two; the shared `quoted_space_to_underbar` is untouched, because it spells a definition-side ident rather than equation text. `nan` is excluded, and that is a disclosed residual rather than an oversight: the importer represents Vensim's `A FUNCTION OF(...)` placeholder as the stored equation text `NAN`, so quoting `nan` would bind that placeholder to any like-named variable. The writer cannot spell it differently either -- Vensim documents `A FUNCTION OF` as not intended for use in writing equations, and as precluding simulation. So a bare `nan` reference naming a declared variable still reads as the NaN literal: pre-existing, silent, narrower than the seven this fixes, pinned by a characterization test, and properly fixed by changing how the importer represents "no equation". Renaming a variable TO a name containing `"` is refused at the same front door, because it is the same class arriving through the same entry point. `Lexer::quoted_identifier` ends at the first `"` and the equation grammar has no escape, so such a name can be written neither bare nor quoted -- yet both readers will define one from `<aux name="a"b">`. The rename used to return Ok and persist reprinted dependents that no longer compile. The check is on the target name only, so renaming AWAY from such a name still works and remains how an affected model is repaired; it reuses `ErrorCode::UnclosedQuotedIdent`, the code recompiling would have produced, and an `x$y` control pins that the rejection stays narrow. The new proptest states the completeness property against the LEXER rather than a second copy of the character rules, which is the check both past holes here -- leading digit, keyword -- would have failed.
The doc on `Ident<State>` said its derived `salsa::Update` delegates to `CanonicalStorage`'s manual impl. It could not: salsa 0.26's derive adds an `Update` bound to every type parameter, and the `Canonical`/`Raw` markers had no impl, so `Ident<Canonical>: salsa::Update` was unsatisfiable and a container's per-field dispatch resolved the field through salsa's `'static + PartialEq` fallback instead. That fallback is value-correct here -- `Ident`'s derived `PartialEq` bottoms out in the same interned comparison -- so this was never a wrong answer, only a comment describing a path that did not exist. The markers now derive `Update` themselves. On a field-less struct the derive generates the "nothing can differ, report no change" body, structurally identical to salsa's own `PhantomData<T>` impl, so the unsafe trait's obligation is discharged vacuously and no `unsafe` is hand-written. Scope, stated so nobody over-reads it: salsa calls a field's `maybe_update` only for tracked STRUCTS, while a tracked FUNCTION's memo backdates through `values_equal`/`PartialEq` and an input setter overwrites outright -- and this crate declares no tracked structs, only tracked functions. So this changes which impl the compiler selects and changes no runtime behavior at all today; it is a correctness-of- documentation fix plus forward-compatibility for the first tracked struct. A static assertion instantiates the bound at both marker states so the doc claim is a build failure if it stops holding, and a test names `UpdateDispatch::<Ident<Canonical>>::maybe_update` with `UpdateFallback` deliberately out of scope, so it can only resolve to the inherent `Update`-backed method -- direct evidence of which impl runs rather than an argument that it runs.
Each of these cost real rework on the branch that added them, so they are written as rules rather than as advice. "Claims About Other Tools" is the expensive one. A premise about what Vensim does -- asserted, never checked -- carried a design decision through three rounds of adversarial review, and every round found genuine defects in the code built on top of it while none questioned the premise. That is the shape of the failure: review checks code against the stated claim, not the claim against the world, so a wrong premise about an external system survives exactly the process meant to catch it. The section names the primary sources, including the XMILE spec already checked in at docs/reference/xmile-v1.0.html, which settles a subscript-precedence question the same branch nearly answered by reasoning from our own compiler instead. The test rule generalizes a mistake that recurred three times in one change at three different altitudes: a test covering one arm of a two-arm gate, then one position of a two-position decision, then one of six call sites. Each read as though it pinned the decision, and each left the untested arm free to compute a wrong number. Deriving the rows from an enumeration -- the variant list, the call-site list -- makes "no gaps" checkable instead of assumed. The scope signal is the cheapest of the three to apply: when a second attempt at the same discovered issue produces a new defect, the understanding behind it has now failed twice, and further attempts are designed against it. Splitting at that point would have ended the episode above four rounds earlier.
LTM occurrence identity is a `SiteId`: a path of u16 child indices. Three of the walk's push AXES take a count the AST node's own shape does not bound -- an Ast::Arrayed target's equation slots, one builtin call's contents (Mean is the only variadic BuiltinFn), and one subscript's index list, which Expr2 lowering does NOT narrow to the subscripted variable's declared arity. Past 65,536 children, two references share an identity and the ceteris-paribus wrap holds or freezes the wrong one, emitting a plausible, wrong link score. Only the builtin axis was handled, by a reserved u16::MAX sentinel, a suppression depth counter with a deliberately asymmetric two-view contract, and a checked sentinel-mapping conversion in the consumer -- machinery that also left module_output_ref_in_document_order unable to distinguish a width-suppressed module-output reference from "the target reads no output of this module". Replace all of it with one check run per target equation before the walk pushes a single path component. A rejected equation records no occurrence at all, so no recorded SiteId can be a path two children share, and model_ltm_variables refuses the whole model with a Warning naming the variable. The refusal is model-level rather than per-edge because the trigger is a property of a target equation and because consumers read a MISSING per-edge entry as a real Bare classification -- so a per-site drop would misclassify rather than decline. The name-keyed, path-free per-edge view stays complete, so loop detection is unaffected: a refusal costs the scores, not the structure. One ordering consequence, benign and deliberate: the refusal sits after the stateless early return, so a stateless model that is also over-wide returns no variables and no width warning -- it emits no scores either way, so there is nothing to warn about. It is a separate pre-pass rather than a counter carried on the walk because its descent has to be a SUPERSET of the walk's. The walk pushes a path component for a LOOKUP table argument and then does not recurse, while the ceteris-paribus wrap and pin_only_source_refs both descend through it with child_path -- so a builtin nested under a table argument is reachable by consumer path indices and by no producer path at all. Subscript indices are the same shape of problem with a different skip predicate on each side. The check's arms are derived from the walk's own push sites and its Expr2 match is exhaustive with no catch-all, so a new variable-width variant is a compile error rather than an unchecked axis. Tests cover all eleven descent edges, the accept side of the boundary on each axis, and that a refusal leaves model_edge_shapes and model_detected_loops untouched; they use a #[cfg(test)] limit override and tiny fixtures. The 16 LTM char goldens and 12 fragment goldens are byte-identical, since the deleted machinery only activated past 65,535.
emit_source_to_agg_link_scores and source_to_agg_hop_polarity recovered the reducer kind, name, body and polarity by printing an AggNode's reducer subexpression, re-parsing it, re-lowering it against a freshly built scope and re-classifying the result -- a closed round trip through our own printer and parser, run once per (agg, source) pair and once per agg-touching loop link. Each failed silently in its own way: a failed reconstruction made the emitter skip the edge's link scores entirely, zeroing the synthetic agg's loop score, and made the polarity hop return Unknown, degrading every loop through the agg to Undetermined. AggNode now carries the BuiltinFn<Expr2> the enumerator classified when it decided the hoist, and reconstruct_ltm_var_lowered is deleted along with its last helper; nothing on the emission path parses. The equation-shape reconstruction both call sites documented as a hazard (an arrayed agg had to be rebuilt as an ApplyToAll over result_dims) is removed rather than moved: there is no shape to rebuild. Carrying an Expr2 puts an f64 inside a salsa-cached, PartialEq-compared value, which has three consequences and the stored form only addresses two. Stripping Loc is load-bearing: raw, an edit that only moves an equation's byte offsets stopped enumerate_agg_nodes backdating, which a test now pins. Stripping ArrayBounds is a guard, inert today because the ASTs this query walks are lowered against an empty model scope and carry no bounds at all. The f64 itself cannot be normalized away: a derived PartialEq is not reflexive on NaN, so a model whose hoisted reducer holds a nan literal never backdates this query -- an incrementality regression this commit introduces, disclosed on the field, in CLAUDE.md, and in a characterization test asserting today's broken behaviour. That is the GH #987/#981 class, fixed at the root by making AST float literals compare by bit pattern, and that fix must invert the test rather than delete it. The same f64 is why AggNode loses Eq and its unconditional Debug. The carried builtin is read only for synthetic aggs; the variable-backed arm's copy is unread today and is kept so AggNode has one shape. The same change settles GH #982, which read reducer_collapses_to_scalar and builtin_routes_through_agg as one question answered twice, inverted on SIZE and RANK. They are two questions: the first is about the reducer's result type (does the subtree fit in a scalar slot -- SIZE does, RANK does not) and gates the freeze/capture paths; the second is about LTM routing (did enumerate_agg_nodes mint a node -- SIZE is Constant and never is, RANK gets an array-valued agg) and sets OccurrenceSite::in_reducer. Both already derive from the single reducer_kind_from_name table, so there was no second table to collapse; what was missing was the argument and a test. Mutating each set in each direction showed three of the four cells already pinned and one not: the SIZE membership of the GH #779 decline, whose inertness rests on the freezability gate reading the same predicate -- move one and it becomes reachable with nothing noticing. A shared cfg(test) decision table now covers every arm of reducer_kind_from_name against all three derived predicates, including the agreement gate's own name-keyed restatement of the routing rule, whose drift would have quietly weakened that gate rather than failing it. RANK is deliberately not flipped: that would turn a scored bare source into a loud decline, a user-visible change with no evidence behind it.
Expr0..Expr3's `Const` held a bare `f64`, so any value containing a NaN literal was not equal to ITSELF. Salsa decides whether to backdate a re-executed query's memo by comparing it with its own rebuild -- the `salsa::Update` derive walks fields and falls back to `PartialEq` for a field type with no `Update` impl -- so such a memo never backdated and the whole downstream query cone re-ran on every revision bump. Every stdlib SMOOTH/DELAY/TREND template declares `initial_value = NAN` and `db::sync` splices those into every project, so this needed no user action; a user model with a `nan` upstream of a cached stage paid it per keystroke on the interactive diagnostics path. The literal is now an `ast::Literal`, a Copy newtype comparing and hashing `f64::to_bits`. It has no `salsa::Update` impl on purpose: the derive's dispatch resolves to salsa's `PartialEq` fallback, so equality is defined in exactly one place and the crate gains no new `unsafe`. What defends that is `#![deny(unsafe_code)]`, which stops an `Update` impl being added at all, plus `-D warnings` on the now-unused `UpdateFallback` import if someone opts out -- an `Update` impl would otherwise take over silently through the inherent dispatch candidate. The four AST enums now also derive `Eq`, which is what keeps the fix structural: a future float-bearing variant cannot be a bare `f64`, because `Eq` rejects one at compile time, transitively through intermediate types. Bit comparison costs this layer nothing, because both distinctions it draws are unreachable in an AST literal rather than accepted. There is one `nan` spelling at the language surface yielding one canonical `f64::NAN`, so no payload is authorable; and the lexer takes no leading sign, so `-0` is a negation of the literal `0` and never a `Const`. Both premises are tripwire tests, and the only production code comparing two ASTs structurally (`mdl::writer`) reads the value out and compares with its own tolerance. `float.rs` gains the reason that is the right posture rather than a shortcut, next to the `:NA:` sentinel it contrasts with: a NaN in a system dynamics model is a diagnostic, not a value. What a practitioner sees is a line on a graph that stops, and their next task is tracing it backward through the dependency graph, because NaN is absorbing and every variable downstream shows the same symptom. So attributing a NaN to its origin is worth a lot, a NaN the engine manufactures is noise in a channel debugged by hand, and nothing needs machinery treating NaN as a structured value. `ast::Literal` carries the caching half: bit comparison wherever a float feeds a cache key, and the types still outside it -- the compiled bytecode pools and `variable::Table`'s lookup points -- keep IEEE comparison as an accepted cost of not converting them, with #642's false "only consumer is non-tracked" premise corrected there so it is not re-litigated. Two tests that had to work around the defect now do not: `db::stages_tests`' stdlib oracle uses the representative `smth1` rather than `npv`, and its whole-project oracle covers all twelve synced models instead of the three user ones. `ltm_agg`'s characterization test of the broken behaviour is inverted rather than deleted.
Three silent-wrong-answer defects in the LTM per-element machinery, each of which produced a degraded link score indistinguishable from a real one. GH #974 -- a per-element link-score partial pinned every arrayed dep of the target with the target's FULL element tuple, which is only right when the dep declares exactly the target's dimensions in the target's order. A dep declaring a strict subset got an over-arity subscript whose fragment failed to compile, so the score read a constant 0 behind four Assembly warnings; a dep declaring the same dimensions in another order got a subscript that COMPILED and read the transposed element, with no diagnostic at all. A bare reference to the live source had the mirror defect: it projected by dimension name only, so a positionally-mapped target found no coordinate, left the reference bare, and froze it into a multi-slot PREVIOUS that cannot compile in a scalar fragment. Both arms now go through one projection, which asks the existing single row derivation which element each of the DEP's own axes reads -- so an identity axis and a positionally-mapped one are one arm, and the rule accepts exactly the mapped pairs the occurrence-driven pin does. A dep whose every axis resolves may subscript a bare reference; a dep with an unresolved axis still gets the dimension-name indices of an already-subscripted reference substituted (that only needs the axes the reference spells) but keeps its bare references bare, which fails loudly rather than reading a wrong element. The pin table also replaces deriving the dimension names by parsing the separator out of our own printed element tuple. ----- The ceteris-paribus wrap held a LOOKUP's table argument verbatim, which is right for the table HEAD -- a graphical-function table has no value slot, so wrapping it cannot compile and zeroes the whole fragment -- but wrong for its subscript INDEX expressions, which codegen evaluates at the current step to select the element. Every partial of such a target therefore varied with the index's own movement, attributing it to whichever source the partial isolates. The indices now go through the wrap's own index pass, which already carries the element and dimension-name guards, so only genuine value reads change spelling. That pass freezes an ident only when it is in the wrap's dep set, and the dependency extractor never walks a table expression -- so an index variable referenced ONLY inside a table argument is not a dependency and would have been left live. The descent therefore widens its own dep set with the idents under these indices, scoped to that argument, with the element and dimension-name guards running first so a selector still cannot wrap. Leaving the extractor itself alone is deliberate: an index dropped from a variable's dependencies is a runlist-ordering question rather than an LTM one. Because the freeze now covers any index ident, and an enclosing freeze covers the descents the arm does not reach, the per-element path's separate REFUSAL of a runtime table index is deleted along with its frozen plumbing, one verdict variant and the static-index predicate it needed -- a refusal that also threw away the scoreable sites on the same edge. The descent uses an empty occurrence lookup because the IR records nothing under a table argument by design, so the desync guard's premise does not hold there. Widening the dep set also made an existing gap reachable: an already-qualified dim-element index had no standalone guard on the paths that suppress qualification, so it froze; the guard now sits beside the element and dimension-name ones. Note for anyone re-reading this later: a table argument carrying a runtime index does not compile on this engine at all -- the target's own fragment fails with DoesNotExist first -- so the wrong score this prevents is not reachable today. Five constructions were tried and all five fail that way, which is why the model-level fixture asserts generated text rather than a score series, and asserts that upstream refusal alongside it. ----- The LTM per-axis classifier asked whether a subscript index named one of the target's iterated dimensions BEFORE asking whether the axis declares it as an element, the opposite of what the compiler does. XMILE lets a dimension declare an element whose name is also a dimension name, and there the orders disagree: without a mapping the classifier declined and the reference fell to the conservative cross-product, but WITH one it described the axis as iterated over a dimension the compiler never iterates there, so read slices and element edges named rows the simulation does not read. The precedence moves into one shared function that both the classifier and the per-element pin rule read, so a colliding name cannot be resolved two ways; the pin rule's rustdoc no longer documents the divergence because there is none. Element-first is what the engine executes, and the binding evidence is a run rather than the spec: a new oracle test compiles the collision and asserts the VM reads the element, then asserts the reference-site IR describes that same read. The spec points the same way but settles only the adjacent variable-vs-element pair outright, and the shared function's rustdoc says which passages are a rule and which are an argument from the namespace rule. ltm_agg.rs crossed the per-file line cap, so its test module moves to a #[path]-mounted sibling, matching ltm_augment's four.
Two independent defects in how the LTM ceteris-paribus wrap treats a subscript index, both silent wrong numbers. GH #977's collapse was implemented alongside these and withdrawn; see the PR body and the issue. -- GH #975: seed a frozen subscript index with the un-lagged index -- The ceteris-paribus wrap emitted a UNARY PREVIOUS around a frozen dynamic subscript index, which the parser desugars to PREVIOUS(idx, 0). The XMILE spec (section 3.5.6) makes that second parameter the value returned in the first DT, and zero is a sound default for a value but is out of range for every 1-based subscript -- so at t=0 the synthesized capture helper read past the end of its source and computed NaN, and the outer PREVIOUS served that NaN as the score's first live step. A NaN the engine manufactures is indistinguishable on a graph from the modeller's own division by zero, which is what makes this a defect rather than a wart. An index-position freeze now names its own un-lagged operand as its first-DT initial value: at the first DT "the index one step ago" IS the current index, and since it is the index the target's own equation reads at that step, the freeze is never out of range where the model's own read is not. Value positions keep the unary spelling, because 0 is a valid VALUE -- not because it is unobservable; a value-position freeze inside a capture helper is read at step 1 by exactly this route. Only the two walkers that descend into subscript indices need it; the other two document that they never do. Note the issue's own diagnosis was wrong on two counts, both checked rather than assumed: the capture helper already participates in the initials phase, and PREVIOUS does not read a prev_values snapshot seeded from initials -- it returns its declared initial value, exactly as the spec says. The head-lag pin gets a strictly stronger discriminator and its own fixture. The NaN used to be the only evidence the lag existed at all; now the guard checks the helper is finite at t=0 AND reads the LAGGED element at every step, against the model's own simulated series. That needs two Age rows that actually differ, which the shared fixture does not have. -- GH #986's third consumer: resolve against the axis it indexes -- The ceteris-paribus wrap decided whether a bare-identifier subscript index was a static element selector or a runtime read with two PROJECT-WIDE predicates -- dimension_uniquely_containing_element and is_element_of_any_dimension, which range over every dimension in the project. The compiler does not: compiler::subscript::normalize_subscripts3 resolves such an index against the indexed variable's OWN axis, which is what dimensions::resolve_axis_index_name implements and what GH #986 unified two other LTM consumers onto. This was the third consumer and it was missed. The disagreement is a wrong number, not a missed optimization. A model variable whose canonical name happens to be an element of an UNRELATED dimension -- a Scenario = [base, high, low] beside a variable named base -- is a runtime read to the simulation and a static selector to the wrap, so the partial leaves it LIVE and moves with it: a link score reporting real influence for an edge with no causal dependence at all. The qualification step then rewrites the index to an otherdim-qualified form, naming an element of a dimension the indexed variable is not declared over, which still compiles and reads a different slot than the PREVIOUS(target) anchor did. Declaring a dimension that NO equation references is enough to change a score, with no diagnostic in either mode. THE UNIFICATION IS PARTIAL AND THIS DOES NOT PRETEND OTHERWISE. Resolving an index needs the declared Dimension at that index's position, and the wrap takes it from IteratedDimCtx::dep_dims -- the dep-to-declared-dimensions table the db layer already threads for the GH #526 other-dep correspondence check, so this adds a consumer rather than a second table built by a second route. But dep_dims is a table of the TARGET'S ARRAY DEPENDENCIES, not of declared dimensions, and reading "absent from dep_dims" as "no axis exists" is the convenient reading rather than the sound one. Two production paths therefore still take the project-wide fallbacks and still reproduce the defect, and both are named at their call sites rather than left to be rediscovered: every LOOKUP table index, because a graphical-function holder is by construction absent from dep_dims (classify_dependencies records it in referenced_tables and keeps it off the dependency graph, GH #606), so the axis lookup can never resolve there -- it is passed a literal None, because a call that could only ever return None reads as coverage for a path that is not fixed; and generate_per_element_link_equation, which threads dep_dims: None on a comment that was true when other_dep_verdict was that table's only consumer. GH #984 stays open on the first, with its reproduction on the issue. Closing either needs the wrap to get its dimension truth from where the compiler gets it, which is a decision about a function family that deliberately carries none, and so is its own change. What IS fixed is pinned on the emitted text AND on the simulated series, over the arrayed / scalar / A2A / stock-flow emitters, with a control proving the fix does not degenerate into freezing every bare index. The test says in its own rustdoc which path it covers and which it does not, because an earlier revision of it claimed a LOOKUP fixture it never had -- and that false claim is how the LOOKUP gap shipped unnoticed in the first place. One older defect at this site is disclosed rather than folded in: an index frozen inside an already-frozen head is DOUBLE-lagged, reading the head at t-1 indexed at t-2 where the anchor indexed at t-1, so such a partial is not fully ceteris-paribus even once every name resolves. That behaviour is pinned but was never adjudicated -- the pin that freezes it was written to catch a blanket skip of the whole index pass and uses the lag only as its discriminator, while this function's own GH #759 comment calls reading an index two steps back "semantically wrong for a genuinely-dynamic index". It is deferred because it is a semantics question that interacts with GH #975's head-lag pin, not because the current answer is settled.
Batch 4 shipped a fix that could not fire: both its tests supplied a dependency set the extractor never generates, so they passed on an input that does not occur and the defect survived untouched. The rule sits beside the N-way-decision rule because they are the same failure wearing different clothes -- a test that reads as coverage without being coverage.
A Vensim model built with the Sketch tool carries `A FUNCTION OF(...)` for every variable the modeller has drawn but not yet written a formula for. Per Vensim's documentation -- the quote already recorded in keyword_ident_tests.rs -- that construct "precludes simulation", and Vensim refuses to run the model; our MDL importer stores it as the equation text `NAN`, which we compile, simulate, and hand back as NaN. So we returned garbage where the source tool declined to answer at all, and the cost lands on the practitioner: per src/float.rs a NaN is absorbing, so what they see is a line on a graph that stops -- identically for every variable downstream of the origin -- and their next task is a hand search backward through the dependency graph. This is the case where the engine knows the origin structurally before the model runs, so a warning naming the variable replaces that whole search. Emitted engine-side rather than in the MDL reader: open_vensim has no warnings channel, while db::diagnostic's accumulator already reaches the CLI, MCP, the FFI and pysimlin -- and it covers a hand-authored XMILE <eqn>NAN</eqn>, which is the same claim however the variable got that way. Warning, not Error, so FormattedErrors::push cannot raise the failure flags a passing CLI run or get_errors check depends on. Two shapes are deliberately NOT reported, because for them the warning would assert something the simulation contradicts. A module input port's own equation is dead once a caller binds the port -- the compiler substitutes the binding, so the port is not NaN and neither is anything downstream -- recognized by equation_is_a_module_input_fallback. That predicate needs two clauses and the second is not redundant: can_be_module_input covers XMILE access="input", while our five SMOOTH/DELAY/TREND templates declare initial_value = NAN yet carry Compat::default(), because they detect binding with the isModuleInput() builtin instead. Since db::sync splices those templates into every project, dropping either clause misreports. Likewise an EXCEPT default is read only when has_except_default marks it live, since the MDL converter keeps a dead default as round-trip metadata and the compiler never evaluates one. The accepted cost is an under-report: an input port that no caller binds falls back to its own equation, and a NaN one now goes unwarned. The atom is decided by lexing rather than by comparing against the string "nan", so the literal's spelling stays in lexer::KEYWORDS alone -- which also makes a quoted reference to a variable named `nan` an ordinary equation rather than a missing one. An arrayed variable with only some unfilled arms is reported too, and named per arm: its elements are separate series, so an unfilled x[b] stops x[b]'s line exactly the way an unfilled scalar stops its own. It stays at most one finding per variable. Four checked-in Vensim sketch fixtures newly warn, 94 findings in total; each file's count equals its A FUNCTION OF count exactly.
Two corrections to the UnfilledEquation advisory added in b283d2b, both found by review, both cases where the warning stated something the simulation contradicts. First, the verdict turned on (which arms are unfilled, what the EXCEPT default says) when it also needs to know whether the arms COVER the variable's dimension. An armless slot's value is not something the arms' text can answer, and both directions were wrong on models that run today. A NaN default that no slot can reach was reported: with arms a=1,b=2 over D=[a,b] and default NAN, expand_arrayed_with_hoisting never consults the default and the compiled model holds 1 and 2 with no NaN anywhere. And a sparse array whose listed arms were all NaN was reported as WHOLLY unfilled, though its omitted slots compile to a finite 0. unfilled_arms now takes a variable::ArmCoverage; db::diagnostic computes it, since resolving dimension names against the project is a database read and the classifier stays pure. The membership test is the compiler's own whole-string element lookup rather than the sibling advisory's union rule, because that lookup IS the fact being asked about. The two arrayed advisories now share one dimension resolver instead of two copies. Second, "NaN is absorbing, so every variable downstream is NaN too" is false, and b283d2b asserted it in the emitted message, in the ErrorCode doc, in src/float.rs's module documentation and in its own commit body. NaN is absorbing through ARITHMETIC; every IEEE comparison against a NaN is false, so `IF x > 0 THEN 1 ELSE 0` reading a NaN x returns a finite 0 and everything below it is finite. That is not a footnote -- it is exactly what bites during the backward search for a NaN's origin that float.rs argues is the real cost of a NaN, so the module doc now states the two consequences a modeller needs: a finite variable does not clear its inputs, and a NaN can vanish partway down a chain instead of reaching the output that would have revealed it. The message now scopes the spread to arithmetic. The decision table gains coverage as a third axis (2 + 2 + [4 x 4 x 2 - 4 impossible] = 32 cells). Its fixtures are now generated from the axis states and checked to land in the cell they are filed under, while the verdicts stay authored as flat literals -- a grouped expectation would be a second copy of the rule under test. Worth recording why neither the original 20-cell product nor the review's 9-cell analysis caught this: both enumerated the axes the code branched on, and the code branched on two because the third was never passed in. The corpus warning count is unchanged at 94 across 4 files.
The per-element pin projection decided which target axis supplied each axis of a bare arrayed dependency by searching the target's dimension NAMES, and a name is not an axis identity. Two shapes came out wrong, both compiling and both reading a row the simulation does not: a target that repeats a dimension collapsed to one map entry, so `target[D,D] = driver * w` over a `w[D]` pinned the second axis where the simulation reads the first; and two dependency axes that could each map to either target axis both claimed the first, because an independent per-axis search tracks no used set. Neither had a diagnostic. The compiler already resolves exactly this reference, so its allocation moves into `dimensions::allocate_implicit_axes_partial` and both callers read it -- positional, and one-to-one because each active axis is consumed once. The compiler takes the total projection of that answer and errors without it; LTM takes the partial one, because a subscripted reference spells some of its own axes. The explicit over-arity bail and the equal-arity reordering fast path are deleted rather than moved: the first is pigeonhole and the second can only fire where the general first pass agrees. What could not be shared is declined instead. The other per-element row derivations are asked their question as a dimension name -- that is what an AxisRead::Iterated carries -- so a repeated dimension is not expressible in their interface, and making it so means changing the reference-shape IR. That edge now gets a Warning and no score, which is the trade this area has taken every time a plausible wrong number was the alternative. Three checked-in models declare a repeated dimension, so this is a shape users write. Also fixes a test helper that built element indices 0-based where production is 1-based; it went unnoticed until the projection started calling get_offset on a fixture dimension and read the wrong mapped partner.
Third and last correction to the UnfilledEquation advisory, and the same root as the second: the gap between the arms AS WRITTEN and the arms that REACH A SLOT. ae71ed0 handled an arm the compiler never selects because the other arms already cover the dimension; this handles one it never selects because the subscript names nothing. Given arms a=1, b=2 and typo=NAN over D=[a,b], expand_arrayed_with_hoisting drops the typo'd arm and both slots compile finite, yet the advisory claimed typo simulates as NaN -- on top of the UnknownElementSubscript warning that correctly reported the subscript itself. Both facts come from one place. arm_coverage already built the declared combination keys and the arm keys to answer "does every slot have an arm"; the effective-arm test is that same lookup read the other way, so ArmCoverage now carries the effective set alongside the coverage bit and unfilled_arms classifies over the arms the compiler will select. No second scan and no second membership rule -- the rule stays expand_arrayed_with_hoisting's own element lookup. The UnknownElementSubscript advisory is untouched: a subscript naming nothing is still worth reporting, and what was wrong was the second warning claiming a value that no slot has. Ineffective arms are deliberately NOT a fifth decision-table axis. They are not a state the verdict depends on but a thing that must not matter, so the table stays a 32-cell product and one invariant test asserts that appending an ignored arm -- NaN or filled -- changes no verdict in any cell. That is a stronger claim than the 24 rows a fifth axis would have added, and a smaller one. The corpus count is unchanged at 94 findings across 4 files: exactly one checked-in model has unknown-subscript arms at all, and none of them is a NaN literal, so the defect was reachable by construction rather than by any fixture. ArmCoverage's Debug derive is feature-gated like CanonicalElementName's -- an unconditional one compiles under the test profile and breaks the release build pysimlin uses.
Two review findings on the UnfilledEquation advisory, one a defect class and one an interactive-path cost. The class first. Three findings on this diagnostic were the same mistake: an arm shadowed because the others already cover the dimension, an arm whose subscript names nothing, and an arm a later duplicate overrides. Each was patched as a case. They are one question -- which arm does the compiler SELECT for a slot -- so the classifier now takes an ArmSelection and db::diagnostic::arm_selection answers it by performing expand_arrayed_with_hoisting's own walk: build the arm map, then for each declared element combination look it up and fall to the EXCEPT default only on a miss. Arms are identified by index rather than canonical key, which is what makes the duplicate case fall out instead of needing handling: a=NAN and A=1 share a key, so a key-based set marks the key selected and leaves both source entries to be classified, reporting the shadowed arm though the compiled slot holds 1. Note parse_equation's map alone is not the answer -- it is canonical-keyed and last-wins but keys unknown subscripts too, so the drop of those happens only in the declared-slot walk. The cost second. emit_unfilled_equation_warnings ran that walk for every arrayed variable before checking whether any arm could report, so an ordinary per-element array paid an O(declared slots) expansion on every diagnostics pass -- and model_all_diagnostics runs on the interactive path. variable::may_have_unfilled_arms is a cheap superset test that now gates it. Measured with a fresh database per iteration, since within one salsa revision the accumulated diagnostics come from cache and a warm loop measures DFS overhead instead of the emitters: on a 40-variable 20x20x20 model (320,000 declared slots) the pass drops from ~6.77s to ~6.06s, about 2.2us of walk per slot; on C-LEARN, the largest arrayed corpus model at 3,255 slots, ~40ms or 1.5%. The corpus was never going to show this; a modeller's own 3-D array on every keystroke would. Probing the gate found that its superset property was unpinned -- every end-to-end fixture with a NaN default also had a NaN arm, so a version scanning only the arms passed everything. A variable whose only NaN is its EXCEPT default now has a test. The corpus warning count is unchanged at 94 across 4 files: the duplicate shape needs subscripts differing in spelling but not canonically, which no checked-in model has.
The UnfilledEquation advisory read a stored equation of `NAN` and asserted the modeller had written no formula. In a model that declares a variable NAMED `nan` that assertion is undecidable, and it was wrong on a model this branch itself makes reachable: importing MDL `nan = 3` / `b = nan` stores b's equation as the bare text, so `b has no equation` named a formula the modeller did write. This is two changes on this branch meeting rather than a defect in either. Batch 1 deliberately excluded `nan` from the MDL importer's keyword quoting, because quoting it would bind Vensim's `A FUNCTION OF(...)` placeholder -- which we store as `NAN` -- to any like-named variable, and a round-tripped model would compute a value for a variable that has none. That trade is disclosed and pinned by keyword_ident_tests::a_bare_nan_reference_in_mdl_is_still_the_literal. Its consequence is that the two shapes are stored identically. So the diagnostic declines to make a claim: may_have_unfilled_arms returns false when the model declares a variable named `nan`. Note this is not the declared-name resolution rule batch 1 rejected -- that resolved the ambiguity in favour of one reading and would have shipped a wrong value. This ships no claim at all, which is the correct behaviour for a warning whose entire content is an assertion about the model: its value is that a practitioner can trust it and skip the backward hunt through the dependency graph, so a warning that might be false is worse than one that is absent. Only the bare literal is affected, and only within the model that declares the name, since that is the scope a bare reference resolves in; a sub-model's `nan` cannot silence its parent. The corpus count is unchanged at 94 findings across 4 files -- no checked-in model declares a variable named `nan`. Note the cost of declining: in such a model a genuine placeholder now goes unwarned. Recovering it would need import provenance carried in the datamodel.
Fourth review finding on the UnfilledEquation advisory, and the first FALSE NEGATIVE: an arm with an empty equation was treated as covering its slot, so a live NAN default looked unreachable and nothing was reported while the slot really did simulate as NaN. A spurious warning costs a moment; a missing one costs the backward hunt this diagnostic exists to remove. The three earlier findings were each fixed by re-deriving one more stage of the pipeline between the arms as written and the arms the compiler evaluates. That approach is what failed here -- the hand-built selection was missing a stage, and missed it silently. So this stops re-deriving. unfilled_arms now takes the parsed Ast<Expr0>, which IS the result of that pipeline: empty and unparseable arms dropped, duplicate canonical subscripts collapsed last-wins, dimensions resolved. ArmSelection and arm_selection are deleted; the only stage still performed here is the declared-slot walk, which cannot come from the Ast because the parser does not know about slots, and which is the compiler's own SubscriptIterator/elements.get/default-on-miss sequence. Reading the parse costs nothing: model_all_diagnostics already triggers compile_var_fragment for every variable, which parses under the same module-ident context key, so this is a memo read. The cheap text gate added for the interactive path still runs first, so an ordinary variable reaches neither the parse nor the slot walk. One behaviour change to note: arrayed element names in the message are now canonical and in row-major declared order, because that is how the parsed map is keyed. Recovering the as-written spelling would require re-deriving the last-wins rule to know which spelling survived, which is exactly what this removes. The corpus count is unchanged at 94 findings across 4 files -- no checked-in model contains an empty arm.
The ceteris-paribus wrap qualified a resolved subscript element as `dim·elem` whatever kind of dimension the axis was. DimensionsContext::lookup resolves that spelling only for a NAMED dimension, so an index into an indexed axis came out as `d·1`, which resolves to nothing: the PREVIOUS-capture helper holding it failed to compile and both of that target's link scores read a constant 0 behind Assembly warnings. The qualification now goes through qualify_axis_element, the one qualifier, which leaves an indexed axis's position verbatim -- and verbatim is what resolves. The resolution itself was right and is unchanged, which is worth recording because it is easy to read the other way. A quoted numeric-named variable used as an index -- `q["1"]` -- is not a runtime read of that variable: normalize_subscripts3 declines it, but declining routes the subscript to the dynamic lowering, whose first move is Dimension::get_offset, and that parses an indexed axis's identifier text into a position. Measured on a running model, with the variable holding two different values and the result unchanged. Refusing to resolve it instead would make the wrap freeze the index, and a frozen index is a dynamic one -- the partial would read whatever the variable held last step where the equation reads position 1, turning a loud zero into a silent wrong row. DimName.N remains a deliberate gap in the shared resolver, now pinned by a test asserting that shape does not compile at all, with instructions for the day it does. ltm_augment.rs sat exactly on the per-file line cap, so the wrap's subscript-index pass moves to a #[path]-mounted sibling -- a cohesive unit and a pure move.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #992 +/- ##
==========================================
- Coverage 91.72% 91.70% -0.03%
==========================================
Files 241 244 +3
Lines 157779 157154 -625
==========================================
- Hits 144729 144114 -615
+ Misses 13050 13040 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Burns down the issues deferred out of the "Deleting the Round Trips" plan (rows A/B/C, PRs #973/#980/#985/#990/#991). Each was filed rather than fixed at the time because folding it in would have swamped the branch it was found on.
Fixes #976
Fixes #972
Fixes #978
Fixes #979
Fixes #983
Fixes #982
Fixes #987
Fixes #981
Fixes #642
Fixes #974
Fixes #986
Fixes #975
What changed, and why it is smaller than the diffstat suggests
The guiding goal was a simpler compiler, so the work is weighted toward deleting mechanisms rather than guarding them:
SiteIdunaddressable-child machinery is gone (ltm: consider rejecting over-arity builtins at the LTM front door to delete the SiteId unaddressable-child machinery #978, ltm: unguardedslot as u16cast on the SiteId slot axis can alias occurrences for a >65,535-element arrayed target #979). A reservedu16::MAXsentinel, a suppression depth counter with a deliberately asymmetric two-view contract, and a checked sentinel-mapping conversion in the consumer collapse into one front-door width check — which covers three axes where the old machinery covered one. ltm: unguardedslot as u16cast on the SiteId slot axis can alias occurrences for a >65,535-element arrayed target #979's claim that the subscript-index axis is "bounded by the reference's declared arity" is false (Expr2::fromkeeps every index), and two further unchecked casts inArrayedSlotMapwere not named by either issue.AggNode's subexpression, re-parsing it, re-lowering it against a fresh scope and re-classifying the result.AggNodenow carries theBuiltinFn<Expr2>the enumerator classified when it decided the hoist. A second round trip the issue did not name (agg-hop polarity inloops.rs) went with it.Silent wrong answers fixed
Four of these produced a number with no diagnostic, which is the worst failure class in this codebase because a degraded link score is indistinguishable from a real one:
LOOKUPtable argument's subscript index, which fixed the shape the issue describes — but the index resolution underneath it is still the project-wide one, so a runtime table index that collides with any dimension's element name is still left live.y = g[idx](src) + 0*idxbeside an unreferencedB: idx, sparecompiles with zero diagnostics and moves the score by up to 1.0. The+ 0*idxterm is what buys the index into scope pastclassify_dependencies, which is also why the earlier "verified in five constructions" claim was wrong rather than merely conservative. Reproductions and the structural analysis are on GH #984.prev_valuesfrom initials. The real cause is a synthesized unaryPREVIOUS(idx)desugaring toPREVIOUS(idx, 0), where0is not a valid 1-based subscript.Decisions worth reviewing
float.rsnow records the project's position on NaN: it is a diagnostic signal, not a value. A practitioner cannot author a NaN payload (onenanspelling, one canonical bit pattern) and the lexer takes no leading sign, so the two distinctions bit-comparison draws are unreachable in a literal rather than accepted costs. The issue's own "the only consumer is non-tracked" premise is corrected in place, so it is not re-litigated on a false argument.N^2+N+1to2N+1arms, 2.17s to 520ms at N=400 -- but six adversarial review rounds turned up seven distinct models whose supposedly-trivial arm was not zero, the last one after the predicate had already been rebuilt as a conservative allowlist over exhaustive matches with a verdict type that makes a silently-skipping arm a compile error. The seventh was a model that compiles with zero diagnostics in both modes. All seven boundaries, their measured magnitudes, their fixtures, and the taxonomy separating "the arm establishes nothing" (a shape a type can forbid) from "the arm establishes the wrong thing" (a shape only a fixture catches) are written up on GH #977 so the next attempt starts from round seven. What the attempt did leave behind is ltm: classify_axis_access resolves a subscript index dimension-name-first, opposite to the compiler's element-first precedence #986's third consumer, below.wrap_index_non_matching_in_previousdecides whether a bare-identifier index is a static element selector or a runtime read with two project-wide predicates, where the compiler ranges over the indexed variable's own axis. A model variable colliding with an unrelated dimension's element name is therefore a runtime read to the simulation and a selector to the wrap: the partial leaves it live and moves with it, and the qualification step rewrites the index onto a dimension the variable is not declared over — which compiles and reads a different slot than the anchor. Declaring a dimension no equation references is enough to change a score, silently, in both modes. The arrayed / scalar / A2A / stock-flow emitters are fixed and pinned on the emitted text and the simulated series. Two production paths are not, and are named in the code rather than left to be rediscovered: everyLOOKUPtable index (a graphical-function holder is by construction absent from the dep→declared-dimensions table the fix reads, per GH engine: standalone lookup-only variable is lowered to gf(Time) (unit error; phantom Time-indexed series for a bare graphical function) #606) andgenerate_per_element_link_equation(which threads no such table). Both need the wrap to get its dimension truth from where the compiler gets it, which is a design decision and its own change.Disclosed residuals
variable::Table'sVec<f64>andGraphicalFunctionScalekeep IEEE comparison, so a hand-authored NaN in a lookup table still defeats backdating on the same three memos. Zero corpus hits; the fix belongs with the bytecode pools.t-1indexed att-2, where thePREVIOUS(target)anchor indexed att-1), so such a partial is not fully ceteris-paribus even once every name resolves correctly. The behaviour is pinned but was never adjudicated:per_element_dynamic_index_scores_preserve_head_lagfreezes the lag, but it was written to catch a blanket skip of the whole index pass and uses the lag only as its discriminator — and the wrap's own GH ltm-augment: partial builders PREVIOUS-wrap dimension-name subscript indices (Wildcard and Bare shapes), silently stubbing scores #759 comment calls reading an index two steps back "semantically wrong for a genuinely-dynamic index". Deferred because it is a semantics question that interacts with ltm: first-live-step NaN in dynamic-index per-element link scores (uninitialized PREVIOUS(index) capture helper) #975's own head-lag pin, not because the current answer is settled. (The "5 red tests" figure from a crude probe disables the entire index pass, so it is an upper bound on the narrow change, not a measurement.)classify_dependenciesdrops aLOOKUPtable-argument index from a variable's dependencies. Arguably a genuine engine bug (it is a same-step dependency, so omitting it is a runlist-ordering question), but nothing that reaches it compiles today.normalize_subscripts3'sDimName.Narm is dead code — it compares a canonical ident against a literal.prefix, so no indexed-dimension qualified spelling resolves.db::ltm::compile's tworeconstruct_single_variablecalls parse and lower the whole N-slot target from scratch once per score. Measured with the withdrawn collapse in place, that was 439ms of the 520ms that remained at N=400 -- i.e. once the arm count is linear, ltm: C-LEARN LTM compile-time dominated by per-fragment fixed overhead (DimensionsContext clone, O(D^2) intern_name, repeated parse) #655 is nearly all of what is left. It needsvariable::Variable: salsa::Updateand has non-LTM callers, and it may well be the larger and safer win of the two.mdl_equivalencefails onSIR.mdl(view element counts, xmutil 29 vs native 32). Pre-existing, confirmed against a cleanmain, and gated behind the optionalxmutilfeature that is in neither CI nor pre-commit.Review history
Six whole-branch
codex-reviewpasses ran after the per-batch work was committed; they found eight P2s that per-batch review could not see, because each needed a whole-tree view. Two were silent wrong link scores in the pinning projection (a map keyed by dimension name where axis identity was needed) and were fixed by extracting the compiler's own implicit-axis allocation so there is no second implementation left to diverge. Four were in the new diagnostic and shared one root -- it re-derived the pipeline between an equation as written and the arms the compiler evaluates, and each pass found a different stage it got wrong; that ended by enumerating the pipeline's six stages and taking five of them from the parsed AST, which deleted four hand-written mirrors. One was an interaction between two batches on this branch (batch 1's deliberatenanquoting residual made a stored equation ambiguous, and the new diagnostic asserted a claim the text cannot support). The last one is worth reading in the commit: the review's stated mechanism was falsified by measurement, the fix it recommended was implemented as a probe and shown to convert a loud zero into a silent wrong row, and the actual defect was a second copy of a spelling rule one line away.Process changes
Three rules were added to the root
CLAUDE.md, each written with the defect that paid for it: