engine: delete the diagnostics round trip (row B of the round-trips plan)#990
Merged
Conversation
The "Tracking Discovered Issues" section told an agent that finding a problem mid-task obliged it to FILE the problem. That defers the cost without reducing it, and it spends the one moment when the context needed to fix the thing is already loaded. Filing becomes the exception, and both exceptions are explicit conversations rather than silent decisions: a fix too large to fold in (say so, with a cost estimate, and sequence it) and a fix that is not the agent's to make. The track-issue agent survives as the mechanism for those. The address-feedback skill's deferred-feedback step said the same thing about review comments, so it now defaults to fixing and points at the root policy.
check_model_units is tracked per model, but its body rebuilt a ModelStage0 for every model in the project and lowered all of them to ModelStage1 before picking the one it came for. Collecting whole-project diagnostics therefore cost M x (whole-project reconstruction), and unit checking runs on the interactive path: editor error panels, MCP read_model/edit_model, and simlin_project_get_errors. The two stages become salsa-tracked returns(ref) queries in a new db::stages submodule, and check_model_units reads them. Cross-module unit inference still needs the full models map, so the read pattern stays O(M^2) -- but the BUILDS are now one per model per revision. A 12-model fixture builds 12 of each where the deleted code built 36, pinned by execution counters rather than memo pointer identity: salsa backdates a re-executed query whose value compares equal and can keep the memo address, so a pointer-equal memo does not prove the body did not run. The salsa-native Stage0 construction existed in three places that had silently diverged on three fields: whether a stdlib-prefixed name with an unknown suffix counts as implicit, whether a stdlib model contributes all its variable names as extra module idents, and whether duplicate-canonical-ident errors are recorded. This takes Project::from_salsa's behaviour in all three cases -- the more careful one each time -- and each was verified inert for unit checking rather than assumed: `implicit` is never read on the unit path, no stdlib body calls PREVIOUS/INIT so the module-ident set cannot change a parse, and check_model_units never reads either stage's `errors`. Forcing the old empty module-ident set red-lights exactly one test and nothing else across the suite. is_stdlib and the module-ident set are derived TOGETHER as a Stage0Context, so model_stage0 cannot obtain one without the other and the wiring has a single place to go wrong -- the earlier shape let the call site silently drop the extra idents with the whole suite staying green. The strict predicate also replaces check_model_units' bare-prefix skip gate, so a stdlib-prefixed model with an unknown suffix (a user model, not a generic template) is now unit checked. build_stdlib_models names every model from stdlib::MODEL_NAMES, so no real template can fall through the strict rule; asserted end to end. The tradeoff is memory: returns(ref) retains one Stage0 and one Stage1 per model per revision -- roughly two lowered copies of every equation in the project, name-keyed and pre-layout -- where the old code paid CPU instead. The per-variable mini ModelStage0 built during fragment lowering is deliberately NOT migrated: it is scoped to one variable's dependencies, and pointing it at a project-wide cached stage would add a project-wide dependency edge to every fragment compile. Fixes #966 Fixes #988 Fixes #989
Two project-level facts -- the macro-registry build error and the unit definition errors -- were accumulated from inside the bodies of project_macro_registry and project_units_context. Both had the same two defects, and the second one is a silent drop. They were reported once per MODEL that happened to reach the query: 15 copies of DuplicateMacroName in a 15-model project, for a fact that belongs to no model. Every existing test asserted with .any(), so N copies and one copy were indistinguishable and the suite could not see it. Worse, they VANISHED after any unrelated edit. Measured on a 13-model fixture: 13 reports before an unrelated input bump, zero after -- through simlin_project_get_errors, the CLI, both MCP surfaces, and simlin-serve. A project with a broken macro set simply stopped being reported. The cause is that salsa's accumulated() walk prunes a dependency subtree whose root memo reports no accumulated inputs, so an error accumulated by a query that some unrelated subtree happens to reach is not a diagnostic anyone can rely on. Both errors become memoized VALUES that collect_all_diagnostics emits once, the way emit_duplicate_variable_diagnostics already handles duplicate idents. project_units_context_result carries the context and its definition errors together so the two cannot come from separate builds, and Project::from_salsa reads that value instead of draining the accumulator -- it had the same vanishing bug in Project.errors. project_units_context stays a TRACKED PROJECTION over that result rather than becoming a plain accessor, which is load bearing. A malformed unit equation is dropped from the context entirely, so editing one bad equation into a different bad equation changes the error list while the context stays byte-identical. As a plain accessor every one of its ~30 callers would then be invalidated -- every keystroke while typing a unit equation re-parsing every variable in the project. As a tracked projection salsa backdates the equal context and readers keep exactly the dependency width they had. Adding no_eq to the projection fails the regression test even though the returned context is still equal, which isolates value-equality backdating as the mechanism. The module-reference cycle stays per-model on purpose (GH #806): a diagnostic naming the model that reaches the cycle, not one project-level row, so an unrelated draft cycle elsewhere cannot hide a valid model's errors. Every production surface that reports project errors was traced by call chain and still receives both: libsimlin (get_errors, the three apply_patch passes), simlin-cli, simlin-mcp-core read_model/edit_model, simlin-serve, the wasm/TypeScript path, pysimlin, compile_project_incremental's hard fail, and Project::from_salsa. collect_model_diagnostics has no production caller.
The salsa-native ModelStage0 build existed in three places. #966 made db::stages the cached one; this deletes the other two: Project::from_salsa's inline whole-project build (which parsed every variable, re-parsed the SMOOTH/DELAY implicit vars, and lowered the whole project a second time) and the test-only ModelStage0::new_cached. from_salsa now reads model_stage1 and clones. The clone is mandatory rather than stylistic: set_dependencies takes the errors, fills instantiations, and pushes equation errors onto the variables themselves, so it needs an owned value the cache must not see. Equality with the deleted build was measured, not argued. The inline build was temporarily restored as an oracle and compared field by field against the queries across three fixtures and every model of each -- including the stdlib templates -- then removed; the review reproduced it independently over 36 model-instances. The two builds agree because they were already computing the same thing from the same inputs: the map key from_salsa tested for `implicit` IS canonicalize(display_name), both read the same variables map, and model_module_ident_context sorts and dedups so list order cannot matter. The whole-project scope map in model_stage1 turned out to have NO test coverage: replacing it with an empty map left all 5299 tests green, while every arrayed cross-module reference silently lowered as if it were scalar -- `get_dimensions` returns None for a model missing from the scope, and a None dimension is not an error. That matters because narrowing this map is the next box, so it would have been narrowed with nothing watching. Three fixtures now constrain it: an arrayed read through one module hop (catches an empty map), a two-level chain `main -> sub_a -> sub_c` reducing over the grandchild's arrayed output (catches a direct-targets-only closure, which the one-hop fixture cannot see), and a tripwire asserting no synced stdlib Stage0 is arrayed or holds a module instance -- which is WHY a stdlib-omitting closure is safe today, and which fails the moment that stops being true. ModelStage0::new gained a new_in_project sibling that builds its MacroRegistry from all project models rather than from the single model, so an oracle cannot silently disagree with the query about whether a sibling-defined macro call expands. `new` is the single-model wrapper, so its eight callers are unchanged. Project::from_salsa was also nondeterministic run to run -- model order and each ModuleStage2's initials runlist flipped between two builds in one process, because topo_sort was seeded from HashMap keys and the initials runlist from a HashSet. This is the Project-path analogue of GH #595, fixed the same way, by sorting both seeds. Verified test-only before touching an ordering: from_salsa is the only non-test caller of set_dependencies, and the only reader of ModelStage1::instantiations is compiler::Module::new, whose single caller is a cfg(test) helper. Pinned by a 64-run determinism test; 8 runs was measurably flaky. from_salsa itself is test-only (plus one engine integration test that feeds ltm_finding::discover_loops); the plan calls it the live-oracle monolith, and its rustdoc now says so, since "is this production code" was not answerable from the call sites.
Two walks over the module graph recursed with no guard against a cycle. On a project where A instantiates B and B instantiates A, both overflowed the stack -- which is not a catchable panic: libsimlin builds with panic=abort, so it takes the host process with it (a WASM tab, an MCP server, a Python session). units_infer::gen_all_constraints now threads an InstantiationPath, a cons list of the models on the CURRENT path, and declines a submodel already on it. A cons list rather than a push/pop Vec because there is no pop to forget: an entry lives exactly as long as its stack frame, so the invariant is structural rather than maintained by discipline. The root model is seeded onto the path, so a model instantiating ITSELF is declined at the first edge. The guard sits beside the existing dangling-model_name guard because it means the same thing: decline this submodel, keep everything else. Unit inference is partial by design throughout -- one bad declaration never discards the rest -- so a cycle degrades rather than failing, and it produces no unit diagnostic of its own: project_module_graph already reports CircularDependency, and a second unit-flavoured message for the same structural fact is noise. The test that matters is not the cycle test but the DIAMOND: A instantiates B and C, both instantiate D. A model reached twice on different paths is instantiated twice and must be constrained twice, so the naive "visited anywhere" fix silences the crash while silently dropping the second instantiation's constraints -- a plausible wrong answer, which is the failure class this work exists to delete. Two diamond tests pin it, and they PASS before this change on purpose: they are guards against the fix, not reproductions of the bug. Swapping the path guard for a visited-anywhere set reds both, with `c·d·out` resolving to None instead of widget. model::enumerate_modules_inner had the same class of defect from a different cause: it recorded a model only AFTER walking it, so two models instantiating each other were each unrecorded when the other looked. A cycle through `main` terminated (main is recorded up front); one below it did not. Now it records before descending, which is what its salsa twin db::assemble::enumerate_module_instances_inner already did -- this was the stale copy, not a new rule. Recording early cannot lose an instantiation: the line runs at every module site regardless, and the values are sets of input sets, so arrival order is not observable. Establishing the pre-fix behaviour is worth recording: a stack overflow aborts the test process, so libtest prints no `test result:` line at all. Each of the four tests was run individually against the unfixed code and observed to SIGABRT. Declining the back edge drops the module-INPUT constraint as well as the body, and that has a real cost: a parent's own equations mint constraints at the skipped prefix whenever they read back a port they feed (gen_constraints' Expr2::Var arm renders the ident verbatim under the active prefix), so the callee-side metavariable IS constrained elsewhere and a genuine cross-module dimensional contradiction inside a cycle now goes unreported. That is accepted -- the project is already rejected as CircularDependency, and resurrecting a unit conflict on a model that cannot compile helps nobody -- but it is pinned by a test rather than left silent, because the first version of this code carried an argument that the omission was information-free, and that argument was false. collect_model_diagnostics gained the module-cycle gate that collect_all_diagnostics already had: without it, the pub per-model entry panicked with a salsa dependency-graph cycle inside model_module_map. The gate is now one shared module_cycle_diagnostic that both callers use, rather than a second copy -- two callers disagreeing is what produced this hole. Sharing it also fixes a silent drop. The inline gate built its diagnostic with the canonical map key (`sub_a`) where every other diagnostic in the crate uses the display name (`Sub A`) -- it was the sole outlier. simlin-mcp-core filters diagnostics by an exact `e.model_name == model_name` compare against the datamodel's display spelling (read_model.rs, and three more times in edit_model.rs), so a CircularDependency on a model whose name is not already canonical -- `Sub A` is just a name with a space -- was silently filtered out of what those tools reported. The filter is `is_none_or`, so project-level rows carrying `model_name: None` always survived; only per-model rows could be dropped this way, and this was the only one spelling them canonically. Not addressed, deliberately: db::layout::compute_layout and db::query::model_module_map are recursive salsa queries, where a cycle drives salsa's own dependency-graph cycle panic (the GH #806 class). All three production entry points gate on project_module_graph first. Making those individually cycle-tolerant is not a bug fix -- a cyclic module graph has no finite flattened layout, so the recursion IS the semantics, and what a partial layout would mean is a design question per query.
Caching the two model stages (#966) stopped whole-project diagnostics being quadratic, but each cached stage still READ every model in the project: the Stage1 lowering built its scope from the whole-project Stage0 map, and check_model_units built its inference map the same way. So an edit anywhere invalidated every model's stage and every model's unit check -- the follow-on the plan named but did not fix. Both now read `model_scope_models`: the model plus the transitive closure of the models its module variables reach. An edit to an unrelated model re-executes neither the stage nor the unit check; an edit to a model that IS reached still re-executes both. Both directions are pinned by exact assert_eq! on the per-query execution counters, because the second direction is the stale-results direction and a narrowing that gets it wrong returns answers computed from a model it never re-read. The closure's edges come from each model's Stage0 variables, NOT from db::project_module_graph, for two reasons that each cost a silent mis-lowering if missed. First: that graph records EXPLICIT module edges only. Its rustdoc claimed the implicit ones can only reach leaf stdlib models and so can never close a cycle, and that is false -- a macro call expands into an implicit module targeting the macro's own model, which is an ordinary user model and can be arrayed. A closure derived from that graph would have dropped it, and a model missing from the scope does not fail: `ArrayContext::get_dimensions` returns None and the reference lowers as though it were scalar. The rustdoc is corrected here; the query itself is left alone, since adding implicit edges would put every variable's parse on the dependency list of every compile, diagnostic and analysis entry point. Second: the scope map has TWO consumers, not one. Besides the dimension lookup, `resolve_relative` walks a module input's dotted `src` treating each path component as a MODEL name, so a module variable's own IDENT is also an edge whenever it names a project model. Without that edge a project whose module ident differs from its target starts reporting BadModuleInputSrc where the whole-project map lowered cleanly. Stdlib templates ride the same rule rather than being spliced in wholesale: a model instantiating none stages none, and one instantiating smth1 gets exactly stdlib:smth1. Dropping them wholesale is no longer safe now that unit inference shares this closure -- it loses the stdlib argument-group check -- so the tripwire test that documented "omitting stdlib is inert" is corrected rather than left to mislead. The walk is an iterative worklist over a visited set, so a module cycle yields a finite scope and no third recursive-tracked-query divergence joins the two that 61d467d just fixed. Two of the narrowings this could plausibly have been are pinned by their CONSEQUENCE rather than by a name list, because a name-list assertion cannot show that losing an edge mis-lowers anything. Dropping the macro edge changes how a reducing caller of an arrayed macro output lowers, and a scalar-output control proves that difference is dimension resolution through the edge rather than incidental sensitivity to the map's contents. A direct-targets-only closure loses a two-hop cross-module unit mismatch, which is the unit-side twin of the lowering fixtures -- before it, the transitive gap was pinned only by lowering tests while the stdlib gap had a unit-side guard. One behaviour delta rides along: check_conveyor_param_units now gets the self-entry repair it did not have when it read the whole-project map, so its throwaway augmented stage agrees with the scope it is lowered in.
The XMILE writer guarded on `self.model_name.is_some()` and then pushed `self.name` into `simlin:model_name`. Since `From<datamodel::Module>` always sets `model_name`, that guard always fires, so EVERY exported module recorded its own instance ident as the model it instantiates. It was silently correct whenever the two coincide, which is the common case and the only case any existing fixture covers -- `x_module` sets ident equal to model_name. A renamed instance is what exposes it: `u_hop` instantiating `u` exported as `simlin:model_name="u_hop"` and re-imported pointing at a model that need not exist, with the failure surfacing far from the export as a dangling module reference. `simlin:model_name` is the only place the target is recorded -- the standard `name` attribute carries the instance ident -- so there is nothing else to recover it from. The regression test pins the emitted attribute as well as the round-trip: a round-trip-only assertion would also pass if the writer and the reader were wrong in matching ways, and the emitted file is what other tools consume. Found while adversarially reviewing the macro-module cycle fix.
A macro-marked model whose body holds an explicit Variable::Module, targeting a model that CALLS that macro, closes a module cycle through an IMPLICIT edge: the macro call expands into a synthetic module targeting the macro's own model. project_module_graph records EXPLICIT module edges only, so cycle_error_from returns None from every root, and the recursive model_module_map then panics inside salsa -- an abort under panic=abort, which takes the host process with it. All three production entry points reach it: collect_all_diagnostics, compile_project_incremental, and analyze_model. This is not redundant with Pass 3's recursion check. The macro set here is VALID and must be for the bug to fire: `mac` calls no macro, so there is no macro-to-macro cycle to find. The loop runs `mac ->(explicit module)-> u ->(macro call)-> mac`, through a NON-macro model, and Pass 3's macro-to-macro graph cannot express that edge. Pass 4 rejects the construct at MacroRegistry::build. It is placed last so no existing error message or ordering shifts, and its offending idents are sorted because the salsa reconstruction path walks a HashMap. The rejection is deliberately BROADER than the bug: an ACYCLIC macro holding a module is rejected too, confirmed by ablation not to panic. That is a real behaviour change on a REACHABLE input, and the first draft of this commit justified it with a claim that turned out to be false. The MDL path genuinely cannot produce the shape -- not because "Vensim macros have no modules", which is wrong as stated (mdl/convert/multi_output.rs does mint modules for the multi-output `:`-list invocation), but because the scoped macro-body context hard-codes an empty materialization (mdl/convert/macros.rs). The XMILE reader CAN: the `<macro>` content model is shared with `<model>` and a `Var::Module` passes through unfiltered, so a hand-authored file produces exactly this shape. It is rejected anyway, for two reasons. A reachability-scoped rule would need a second reachability analysis inside MacroRegistry::build that has to agree with project_module_graph's, and two reachability implementations disagreeing is the failure mode 61d467d documents for the two diagnostic collectors. And a macro is a template: instantiating a sub-model inside one is dubious on its own terms, so AC5.7 is a language rule with a cycle-safety motivation rather than a workaround. The estimate that no shipping tool authors the shape (Stella emits no <macro>; xmutil emits them only from Vensim :MACRO: blocks, which cannot contain modules) is judgement, not measurement. A module TARGETING a macro model is untouched -- that is how a multi-output macro invocation works, and the pass tests macro_spec on the model that HOLDS the variable. Why this CLOSES the hole rather than narrowing it, each step verified rather than assumed: the edges model_module_map follows that project_module_graph does not are exactly the implicit ones, and they have a single synthesis site (expand_module_function) whose descriptor has exactly two producers -- a stdlib template or the macro's own model. A stdlib model is a sink (it holds no module variable, asserted over the SYNCED stages so implicit vars are covered too). A macro model's outgoing edges are therefore three kinds: an explicit module in its body (this rejects it), an implicit macro-to-macro edge (Pass 3 rejects the cycle), or an implicit stdlib edge (a sink). So every remaining cycle lies entirely in explicit edges, which is exactly what the gate records. Three conditions would reopen it, each recorded where the next reader will find it. The stdlib-sink property is TEST-asserted rather than structural. The empty-registry-on-failure behaviour of MacroRegistry::build is now load-bearing for CYCLE SAFETY and not merely for error quality -- a "keep a partial registry alongside the error" refactor reopens the abort with Pass 4 fully in place, demonstrated by mutation rather than argued, and for two of the three entry points that empty registry is the ONLY thing stopping the walk. The third is an honest residual: that builtin and macro expansion are the only synthesisers of implicit module variables was enumerated from the code, not proved, and no test guards it.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #990 +/- ##
==========================================
+ Coverage 91.66% 91.70% +0.03%
==========================================
Files 238 239 +1
Lines 157293 157770 +477
==========================================
+ Hits 144185 144680 +495
+ Misses 13108 13090 -18 ☔ 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.
Row B of the "Deleting the Round Trips" plan: the third round trip, where
check_model_unitsis tracked per model yet rebuildsModelStage0andModelStage1for every project model on each call, then uses one.Fixes #966
Fixes #988
Fixes #989
What the plan asked for
salsa::Updateon the two stages6893927c49551322What it actually took
Four of the six commits here are things the plan did not know about. Each was
found by an adversarial reviewer refusing to accept a plausible argument, and
the recurring shape was a writer reasoning correctly from a premise that turned
out to be false.
6893927c— cache the two stages (#966). Whole-project diagnostics stopbeing quadratic in model count: a 12-model fixture builds 12 of each stage
where the deleted code built 36. Pinned by execution counters, not memo pointer
identity — salsa backdates a re-executed query whose value compares equal and
can keep the memo address, so a pointer-equal memo does not prove the body did
not run. Also unifies two disagreeing "is this a stdlib model" predicates
(#988) and adds a missing
Debugimpl (#989).bbe3d756— take project-level diagnostics off the accumulator. Themacro-registry build error and the unit-definition errors were accumulated from
inside query bodies. They were reported once per model — 15 copies in a
15-model project — and they VANISHED entirely after any unrelated edit
(measured: 13 reports before an unrelated input bump, 0 after), across
simlin_project_get_errors, the CLI, both MCP surfaces and simlin-serve. Noexisting test could see either half, because every one asserted with
.any().project_units_contextstays a tracked projection rather than becoming aplain accessor, which is load-bearing: a malformed unit equation is dropped
from the context entirely, so editing one bad equation into a different bad one
changes the error list while the context stays byte-identical — as a plain
accessor that invalidated all ~30 of its callers, i.e. every keystroke while
typing a unit equation re-parsed every variable in the project.
f3599a42— one salsa-native construction of the model stages. There werethree, and they had silently disagreed on three fields. Equality with the
deleted build was measured across every model of three fixtures and reproduced
independently by review over 36 model-instances. Along the way: the
whole-project scope map turned out to have no test coverage at all —
replacing it with an empty map left 5,299 tests green while every arrayed
cross-module reference silently lowered as a scalar. Three artifacts now
constrain it. Also fixes
Project::from_salsabeing nondeterministic run torun (the
Project-path analogue of #595).61d467d2— make the cross-model walks cycle-safe. Two walks over themodule graph recursed with no cycle guard and overflowed the stack — not a
catchable panic: libsimlin builds
panic=abort, so it takes the host process.The test that matters is not the cycle test but the DIAMOND: a naive
visited-anywhere fix silences the crash while silently dropping a legal second
instantiation's constraints. Sharing the module-cycle gate between its two
callers also fixed a silent drop, where a cycle diagnostic's canonical model
name was filtered out of MCP
read_model/edit_model, which compare againstthe display spelling.
49551322— narrow the lowering scope (B3). An edit to an unrelated modelno longer invalidates another model's stage or unit check; an edit to a model
that IS reached still does. The closure's edges come from Stage0, not from
project_module_graph, for two reasons that each cost a silent mis-lowering:that graph's rustdoc claimed implicit modules can only reach leaf stdlib models
(false — a macro call expands into an implicit module targeting the macro's own,
possibly arrayed, user model), and the scope map has a second consumer,
resolve_relative, which treats a module's own ident as a model name. Verifiedby a dual-lowering oracle: every model lowered under both the narrowed and the
whole-project scope, diffed, across the corpus and all lib tests — zero
user-model divergence.
e1d95a97— reject a module inside a macro body (macros.AC5.7). Amacro-marked model holding an explicit module, targeting a model that CALLS
that macro, closes a cycle through an IMPLICIT edge.
project_module_graphrecords explicit edges only, so the gate returns None from every root and the
recursive queries then panic inside salsa — an abort. Not redundant with Pass
3: the macro set is VALID and must be for the bug to fire, and the loop runs
through a NON-macro model, which Pass 3's macro-to-macro graph cannot express.
Both recursive cross-model queries are live abort paths —
collect_all_diagnosticshits
model_module_map, whilecompile_project_incrementalandanalyze_modelboth hit
compute_layout.a87885e8— export a module's target model, not its own ident. Found whilereviewing the above. The XMILE writer pushed
self.nameintosimlin:model_nameunder an always-true guard, so every exported modulerecorded its own ident as the model it instantiates — silently correct only
when the two coincide, which is every existing fixture. A renamed instance
re-imports pointing at a model that need not exist.
12c5f01a— doc.CLAUDE.mdtold an agent that finding a problem obligedit to FILE the problem. It now says fix it, with two named exceptions that are
explicit conversations rather than silent deferrals.
Deliberate decisions
ModelStage0s are untouched. They are scoped to onevariable's dependencies on purpose; pointing them at a cached project-wide
stage would add a project-wide dependency edge to every fragment compile.
returns(ref)retains one Stage0 and one Stage1 per model per revision —roughly two lowered copies of every equation in the project. That is the
trade: the old code paid CPU instead. Documented at the source.
genuine cross-module unit conflict inside a cycle goes unreported. Accepted
(the project is already rejected as
CircularDependency) but pinned by atest, because the first version of that code carried an argument that the
omission was information-free, and that argument was false.
compute_layout/model_module_mapare NOT made individuallycycle-tolerant. A cyclic module graph has no finite flattened layout, so the
recursion is the semantics; all production entry points gate on
project_module_graphfirst.macro-holding-a-module is reachable from an ordinary XMILE file (the
<macro>content model is shared with
<model>, andVar::Modulepasses throughunfiltered), it compiles and simulates correctly, and our own writer
round-trips it. It is rejected anyway: narrowing the rule to the cyclic case
needs a second reachability analysis whose back edge — the macro call — is
only discoverable by parsing every model's equation, and two reachability
implementations disagreeing is the exact failure mode
61d467d2documents.The estimate that no shipping tool authors the shape is judgement, not
measurement, and is labelled as such at each site. The first version of this
argument claimed no importer could produce the shape at all; that was false,
and both the implementer and the reviewer found it independently.
Disclosed residuals
Expr0–Expr3'sConstholds a baref64under a derivedPartialEq, soany memo containing a NaN literal — every stdlib SMOOTH/DELAY/TREND — can
never compare equal to itself. It is an incrementality cliff rather than a
live correctness bug today, and the fix (a bitwise-equality literal newtype)
touches 163 construction sites across 34 files, plus 148 more to extend it to
compiler::Exprfor engine: salsa::Update on compiled bytecode uses PartialEq over f64/NaN (benign missed-backdate) #642. That belongs in its own PR, not bundled here.6893927candbbe3d756were verified together, notindependently. Three files carried hunks belonging to both, so the split was
staged hunk-by-hunk; the pre-commit hook necessarily tests the working tree.
Project::from_salsa's model order is deterministic now, but the path remainstest-only; production compiles through
db::compile_project_incremental.