Summary
The #885 fix gates duplicate canonical variable idents at the salsa layer (SourceModel.declared_variable_idents + db/diagnostic.rs model_duplicate_variables, hard DuplicateVariable error in compile_project_incremental, an identical pre-expansion datamodel-level check in queue_compile::build_compiled, and Error-severity diagnostics via model_all_diagnostics). That covers every production compile/simulate/diagnostic surface.
Residual: the legacy/monolithic ModelStage0 construction still collects variables into a canonical-keyed HashMap with last-wins semantics, silently dropping the earlier twin:
src/simlin-engine/src/model.rs:997 (ModelStage0::new)src/simlin-engine/src/model.rs:1101 (ModelStage0::new_cached)
Both do .map(|v| (Ident::new(v.ident()), v)) into a HashMap<Ident<Canonical>, _>, so net flow / net_flow twins collapse with no diagnostic on this path.
Reachability
This path is reached via crate::project::Project::from(datamodel) / Project::from_datamodel consumers -- e.g. TestProject::compile and some non-simulating model-query surfaces -- NOT by the production compile pipeline, which now rejects duplicates upstream. So the exposure is test-only / non-simulating surfaces.
Why it matters
Defense in depth: any future direct consumer of ModelStage0::new/new_cached that bypasses the salsa gate silently inherits the last-wins collapse, reintroducing the #885 class (silently-wrong results, or latent panics of the #870 flavor in code that assumes ident uniqueness). Today the invariant "callers must have already rejected duplicate canonical idents" is implicit and undocumented.
Suggested direction
Either:
- Add a duplicate-canonical-ident check in
ModelStage0::new/new_cached that records a DuplicateVariable error on the model (cheap: compare variable_list.len() against the resulting map's len, then find the colliding pair for the message), or
- At minimum, document the invariant in rustdoc on both constructors: the caller is responsible for rejecting duplicate canonical idents before construction, and last-wins collapse is the unchecked fallback behavior.
Option 1 closes the class entirely and is preferred; severity is low because the production pipeline is already gated.
Context
Identified during the #885 fix on the conveyor-engine branch (PR #869 follow-ups). #885 documented these exact lines but is being closed by the salsa-layer gate, which deliberately does not touch the legacy path. Related: #870 (conveyor expansion panic from this class), #568 (the broader salsa-vs-legacy divergence pattern, for cycle gates).
Summary
The #885 fix gates duplicate canonical variable idents at the salsa layer (
SourceModel.declared_variable_idents+db/diagnostic.rsmodel_duplicate_variables, hardDuplicateVariableerror incompile_project_incremental, an identical pre-expansion datamodel-level check inqueue_compile::build_compiled, and Error-severity diagnostics viamodel_all_diagnostics). That covers every production compile/simulate/diagnostic surface.Residual: the legacy/monolithic
ModelStage0construction still collects variables into a canonical-keyedHashMapwith last-wins semantics, silently dropping the earlier twin:src/simlin-engine/src/model.rs:997(ModelStage0::new)src/simlin-engine/src/model.rs:1101(ModelStage0::new_cached)Both do
.map(|v| (Ident::new(v.ident()), v))into aHashMap<Ident<Canonical>, _>, sonet flow/net_flowtwins collapse with no diagnostic on this path.Reachability
This path is reached via
crate::project::Project::from(datamodel)/Project::from_datamodelconsumers -- e.g.TestProject::compileand some non-simulating model-query surfaces -- NOT by the production compile pipeline, which now rejects duplicates upstream. So the exposure is test-only / non-simulating surfaces.Why it matters
Defense in depth: any future direct consumer of
ModelStage0::new/new_cachedthat bypasses the salsa gate silently inherits the last-wins collapse, reintroducing the #885 class (silently-wrong results, or latent panics of the #870 flavor in code that assumes ident uniqueness). Today the invariant "callers must have already rejected duplicate canonical idents" is implicit and undocumented.Suggested direction
Either:
ModelStage0::new/new_cachedthat records aDuplicateVariableerror on the model (cheap: comparevariable_list.len()against the resulting map's len, then find the colliding pair for the message), orOption 1 closes the class entirely and is preferred; severity is low because the production pipeline is already gated.
Context
Identified during the #885 fix on the
conveyor-enginebranch (PR #869 follow-ups). #885 documented these exact lines but is being closed by the salsa-layer gate, which deliberately does not touch the legacy path. Related: #870 (conveyor expansion panic from this class), #568 (the broader salsa-vs-legacy divergence pattern, for cycle gates).