You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
engine: SourceVariable.model_name is a misnamed correctness trap (it is the Module reference target, empty for non-Module vars -- not the owning model) #558
SourceVariable.model_name (salsa input field, src/simlin-engine/src/db.rs, definition at ~line 244) is misleadingly named. It does not hold the name of the model that owns/contains the variable. It holds a Module variable's referenced target model name, and is String::new() (empty) for every non-Module variable.
The population site already names the local correctly -- source_variable_from_datamodel (db.rs:2582-2591) computes referenced_model_name:
let(module_refs, referenced_model_name) = match var {
datamodel::Variable::Module(m) => (
m.references.iter().map(SourceModuleReference::from).collect(),
m.model_name.clone(),// <- Module's *target* model),
_ => (Vec::new(),String::new()),// <- empty for everything else};
...but it is then stored into the salsa field named model_name, with no doc comment at the field definition. The name is the only signal a future reader gets, and it points the wrong way.
The trap is reinforced by the rest of the codebase, where model_name consistently means "referenced/target model": SourceModuleReference.model_name, datamodel::Module.model_name, bytecode.rs module descriptors, errors.rs, etc. So source_variable.model_name reads very naturally as "the model this variable belongs to" -- which is exactly what it is not.
Existing readers are only correct because they defensively guard on kind/emptiness, e.g. db_analysis.rs:1013-1016:
let model_name = source_var.model_name(db);if !model_name.is_empty(){// only meaningful for Module vars
dynamic_modules.insert(name.clone(), model_name.clone());}
A future caller that reasonably reads source_variable.model_name(db) expecting the owning model will get the empty string for all ordinary stocks/flows/auxes and be silently wrong -- no panic, no type error, just a plausible-looking empty value.
Why it matters
Silent correctness trap: a wrong-but-plausible field whose misuse fails quietly (empty string for non-Module vars) rather than loudly. This is the worst failure mode for a shared low-level layer.
The salsa source layer (db.rs) is a foundational, widely-touched module; anyone extending LTM, analysis, or compilation may reach for this field.
src/simlin-engine -- SourceVariable salsa input in src/simlin-engine/src/db.rs (field def ~line 244; populated in source_variable_from_datamodel at db.rs:2582-2618).
All readers of SourceVariable::model_name (currently all correctly kind-guarded): db.rs (multiple), db_analysis.rs:1013, db_ltm.rs:783, db_ltm.rs:1847.
Possible approaches for resolution
Either of:
Rename the field to something accurate, e.g. referenced_model_name (it is the Module reference target). This matches the already-correct local variable name at the population site and makes the empty-for-non-Module behavior self-documenting. Update the ~handful of .model_name(db) call sites.
Add a precise doc comment at the field definition (src/simlin-engine/src/db.rs ~line 244) stating that it is the Module-reference target model name, is empty (String::new()) for all non-Module variables, and is NOT the model that owns/contains this variable -- and point readers who need the owning/enclosing model at the appropriate accessor (e.g. the macro enclosing_macro_for_var / macro_body_owner path, or the model that lists the variable in its variable_names).
Option 1 (rename) is preferred -- a name that can't be misread beats a comment that can be skipped -- but either resolves the trap.
Context / how discovered
Pre-existing tech debt surfaced (not introduced) during the GH #554 macro fix (false-recursion-cycle / C-LEARN macro expansion work) when code needed a variable's owning macro model and this field looked correct but was not. Filed for tracking only; out of scope for the macro epic and intentionally not fixed as part of #554.
Problem
SourceVariable.model_name(salsa input field,src/simlin-engine/src/db.rs, definition at ~line 244) is misleadingly named. It does not hold the name of the model that owns/contains the variable. It holds a Module variable's referenced target model name, and isString::new()(empty) for every non-Module variable.The population site already names the local correctly --
source_variable_from_datamodel(db.rs:2582-2591) computesreferenced_model_name:...but it is then stored into the salsa field named
model_name, with no doc comment at the field definition. The name is the only signal a future reader gets, and it points the wrong way.The trap is reinforced by the rest of the codebase, where
model_nameconsistently means "referenced/target model":SourceModuleReference.model_name,datamodel::Module.model_name,bytecode.rsmodule descriptors,errors.rs, etc. Sosource_variable.model_namereads very naturally as "the model this variable belongs to" -- which is exactly what it is not.Existing readers are only correct because they defensively guard on kind/emptiness, e.g.
db_analysis.rs:1013-1016:A future caller that reasonably reads
source_variable.model_name(db)expecting the owning model will get the empty string for all ordinary stocks/flows/auxes and be silently wrong -- no panic, no type error, just a plausible-looking empty value.Why it matters
db.rs) is a foundational, widely-touched module; anyone extending LTM, analysis, or compilation may reach for this field.model_namelooked like exactly the right field but is not, costing a debugging cycle. The workaround was to introduce a separate salsa-trackeddb_macro_registry::macro_body_ownermap plus anenclosing_macro_for_varaccessor -- machinery that exists partly because this field could not be (and must not be) used for "owning model."Component(s) affected
src/simlin-engine--SourceVariablesalsa input insrc/simlin-engine/src/db.rs(field def ~line 244; populated insource_variable_from_datamodelatdb.rs:2582-2618).SourceVariable::model_name(currently all correctly kind-guarded):db.rs(multiple),db_analysis.rs:1013,db_ltm.rs:783,db_ltm.rs:1847.Possible approaches for resolution
Either of:
referenced_model_name(it is the Module reference target). This matches the already-correct local variable name at the population site and makes the empty-for-non-Module behavior self-documenting. Update the ~handful of.model_name(db)call sites.src/simlin-engine/src/db.rs~line 244) stating that it is the Module-reference target model name, is empty (String::new()) for all non-Module variables, and is NOT the model that owns/contains this variable -- and point readers who need the owning/enclosing model at the appropriate accessor (e.g. the macroenclosing_macro_for_var/macro_body_ownerpath, or the model that lists the variable in itsvariable_names).Option 1 (rename) is preferred -- a name that can't be misread beats a comment that can be skipped -- but either resolves the trap.
Context / how discovered
Pre-existing tech debt surfaced (not introduced) during the GH #554 macro fix (false-recursion-cycle / C-LEARN macro expansion work) when code needed a variable's owning macro model and this field looked correct but was not. Filed for tracking only; out of scope for the macro epic and intentionally not fixed as part of #554.