From e9cfa24f5a885204d45e5403863cd4858b1273f4 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 7 Mar 2026 18:37:07 -0800 Subject: [PATCH 1/4] engine: replace base_from with from_salsa constructor Replace the monolithic `Project::base_from` with `from_salsa` which builds a Project from a pre-synced salsa database. All variable parsing comes from salsa-cached results instead of duplicating the parsing/DB-setup logic. The new constructor takes a salsa DB and SourceProject (already synced by the caller), eliminating the local DB creation and `cached_sources` parameter that `base_from` needed. A convenience wrapper `from_datamodel` handles the common case of creating a local DB and syncing. For stdlib (implicit) models, all variable names are passed as extra module idents so PREVIOUS expansion uses module expansion instead of LoadPrev. This matches the behavior of the old `ModelStage0::new()` for implicit models, where module inputs lack persistent slots in prev_values. --- src/simlin-engine/CLAUDE.md | 2 +- src/simlin-engine/src/model.rs | 1 + src/simlin-engine/src/project.rs | 238 ++++++++++++--------------- src/simlin-engine/src/units_infer.rs | 42 +++-- 4 files changed, 141 insertions(+), 142 deletions(-) diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index 376c9018..323b2216 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -32,7 +32,7 @@ Equation text flows through these stages in order: - **`src/variable.rs`** - Variable variants (`Stock`, `Flow`, `Aux`, `Module`), `ModuleInput`, `Table` (graphical functions). `classify_dependencies()` is the primary API for extracting dependency categories from an AST in a single walk, returning a `DepClassification` with five sets: `all` (every referenced ident), `init_referenced`, `previous_referenced`, `previous_only` (idents only inside PREVIOUS), and `init_only` (idents only inside INIT/PREVIOUS). The older single-purpose functions (`identifier_set`, `init_referenced_idents`, etc.) remain as thin wrappers. `parse_var_with_module_context` accepts a `module_idents` set so `PREVIOUS(module_var)` falls through to module expansion instead of `LoadPrev`. - **`src/dimensions.rs`** - Dimension context and dimension matching for arrays - **`src/model.rs`** - Model compilation stages (`ModelStage0` -> `ModelStage1` -> `ModuleStage2`), dependency resolution, topological sort. `collect_module_idents` pre-scans datamodel variables to identify which names will expand to modules (preventing incorrect `LoadPrev` compilation). `init_referenced_vars` extends the Initials runlist to include variables referenced by `INIT()` calls, ensuring their values are captured in the `initial_values` snapshot. `check_units` is gated behind `cfg(any(test, feature = "testing"))` (production unit checking uses salsa tracked functions). -- **`src/project.rs`** - `Project` struct aggregating models. `From`, `with_ltm()`, and `with_ltm_all_links()` are all gated behind `cfg(any(test, feature = "testing"))` (monolithic construction path, retained only for tests and the AST interpreter cross-validation path); production code uses `db::compile_project_incremental` with `ltm_enabled`/`ltm_discovery_mode` on `SourceProject`. +- **`src/project.rs`** - `Project` struct aggregating models. `from_salsa(datamodel, db, source_project, cb)` builds a Project from a pre-synced salsa database (all variable parsing comes from salsa-cached results). `from_datamodel(datamodel)` is a convenience wrapper that creates a local DB and syncs. `From`, `with_ltm()`, and `with_ltm_all_links()` are all gated behind `cfg(any(test, feature = "testing"))` (retained only for tests and the AST interpreter cross-validation path); production code uses `db::compile_project_incremental` with `ltm_enabled`/`ltm_discovery_mode` on `SourceProject`. - **`src/results.rs`** - `Results` (variable offsets + timeseries data), `Specs` (time/integration config) - **`src/patch.rs`** - `ModelPatch`/`ProjectPatch` for representing and applying model changes diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 01cfc6dd..678c247a 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -872,6 +872,7 @@ pub(crate) fn equation_is_stdlib_call(eqn: &datamodel::Equation) -> bool { } #[cfg(any(test, feature = "testing"))] +#[allow(dead_code)] impl ModelStage0 { pub fn new( x_model: &datamodel::Model, diff --git a/src/simlin-engine/src/project.rs b/src/simlin-engine/src/project.rs index a7646eca..0933f2bb 100644 --- a/src/simlin-engine/src/project.rs +++ b/src/simlin-engine/src/project.rs @@ -12,9 +12,7 @@ use std::sync::Arc; #[cfg(any(test, feature = "testing"))] use { - crate::canonicalize, - crate::model::{ModelStage0, ScopeStage0}, - crate::units::Context, + crate::canonicalize, crate::model::ScopeStage0, crate::units::Context, std::collections::BTreeSet, }; @@ -27,9 +25,9 @@ pub struct Project { pub models: HashMap, Arc>, #[allow(dead_code)] model_order: Vec>, - /// Project-level errors are also accumulated via the salsa accumulator - /// in `project_units_context`. This field is retained for the monolithic - /// `Project::from` construction path used by tests. + /// Project-level errors. With the `from_salsa` construction path, + /// unit definition errors are accumulated via the salsa accumulator + /// in `project_units_context`; this field is always empty. pub errors: Vec, /// Cached dimension context for subdimension lookups pub dimensions_ctx: DimensionsContext, @@ -41,9 +39,10 @@ impl Project { } } -/// Runs unit inference and unit checking for a model during monolithic -/// Project construction. Only used by the test-only `From` -/// impl; the production path uses salsa tracked functions for unit analysis. +/// Runs unit inference and unit checking for a model during +/// `from_salsa` construction. Matches the behavior of the salsa +/// `check_model_units` tracked function but stores results on the +/// model struct instead of accumulating via the salsa accumulator. #[cfg(any(test, feature = "testing"))] fn run_default_model_checks( models: &HashMap, &ModelStage1>, @@ -76,135 +75,117 @@ fn run_default_model_checks( #[cfg(any(test, feature = "testing"))] impl From for Project { fn from(project_datamodel: datamodel::Project) -> Self { - Self::base_from(project_datamodel, None, run_default_model_checks) + Self::from_datamodel(project_datamodel) } } #[cfg(any(test, feature = "testing"))] impl Project { - pub(crate) fn base_from<'a, F>( + /// Convenience constructor: creates a local salsa DB, syncs the + /// datamodel, and builds the Project via `from_salsa` with default + /// unit inference/checking. + pub(crate) fn from_datamodel(project_datamodel: datamodel::Project) -> Self { + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + Self::from_salsa( + project_datamodel, + &db, + sync.project, + run_default_model_checks, + ) + } + + /// Build a `Project` from a pre-synced salsa database. + /// + /// All variable parsing comes from salsa-cached results (no + /// redundant parsing). The caller provides the salsa DB and + /// `SourceProject`; the `model_cb` runs per non-stdlib model + /// after dependency resolution (typically unit inference/checking). + pub(crate) fn from_salsa( project_datamodel: datamodel::Project, - cached_sources: Option<( - &'a dyn crate::db::Db, - crate::db::SourceProject, - &'a HashMap, - )>, + db: &dyn crate::db::Db, + source_project: crate::db::SourceProject, mut model_cb: F, ) -> Self where F: FnMut(&HashMap, &ModelStage1>, &Context, &mut ModelStage1), { - use crate::common::{ErrorCode, ErrorKind, topo_sort}; - use crate::db::{SimlinDb, sync_from_datamodel}; - use crate::model::enumerate_modules; - - let mut project_errors = vec![]; - - let units_ctx = - Context::new_with_builtins(&project_datamodel.units, &project_datamodel.sim_specs) - .unwrap_or_else(|errs| { - for (unit_name, unit_errs) in errs { - for err in unit_errs { - project_errors.push(Error { - kind: ErrorKind::Model, - code: ErrorCode::UnitDefinitionErrors, - details: Some(format!("{unit_name}: {err}")), - }); - } - } - Default::default() - }); - - // Set up salsa database/source handles for per-variable caching. - let mut local_salsa_db: Option = None; - let mut local_source_models: Option> = None; - let (salsa_db, source_project, source_models): ( - &dyn crate::db::Db, - crate::db::SourceProject, - &HashMap, - ) = if let Some((db, source_project, source_models)) = cached_sources { - (db, source_project, source_models) - } else { - let db = local_salsa_db.insert(SimlinDb::default()); - let sync_result = sync_from_datamodel(db, &project_datamodel); - let source_project = sync_result.project; - let source_models_ref = local_source_models.insert( - sync_result - .models - .iter() - .map(|(name, synced_model)| (name.clone(), synced_model.source)) - .collect(), - ); - (db, source_project, source_models_ref) + use crate::common::topo_sort; + use crate::db::{ + model_module_ident_context, parse_source_variable_with_module_context, + project_datamodel_dims, project_units_context, }; - - // Build set of model names present in the datamodel so we can detect - // when a stdlib model has been overridden (e.g., by LTM augmentation - // adding synthetic variables to a stdlib model's definition). - let datamodel_model_names: std::collections::HashSet = project_datamodel - .models - .iter() - .map(|m| canonicalize(&m.name).into_owned()) - .collect(); - - // Pull in stdlib models, skipping any that are overridden in the datamodel. - // Stdlib models use direct parsing (no salsa caching). - let mut models_list: Vec = crate::stdlib::MODEL_NAMES - .iter() - .filter(|name| { - let canonical = canonicalize(&format!("stdlib\u{205A}{name}")).into_owned(); - !datamodel_model_names.contains(&canonical) - }) - .map(|name| crate::stdlib::get(name).unwrap()) - .map(|x_model| { - ModelStage0::new(&x_model, &project_datamodel.dimensions, &units_ctx, true) - }) - .collect(); - - // User models use salsa-cached per-variable parsing when the model - // has a corresponding SourceModel in the sync result. - models_list.extend(project_datamodel.models.iter().map(|m| { - let canonical_name = canonicalize(&m.name); - let is_stdlib_override = crate::stdlib::MODEL_NAMES - .iter() - .any(|name| canonicalize(&format!("stdlib\u{205A}{name}")) == canonical_name); - - if let Some(source_model) = source_models.get(canonical_name.as_ref()) { - ModelStage0::new_cached( - salsa_db, - *source_model, - source_project, - m, - &project_datamodel.dimensions, - &units_ctx, - is_stdlib_override, - ) + use crate::model::{ModelStage0, VariableStage0, enumerate_modules}; + + let units_ctx = project_units_context(db, source_project); + let dm_dims = project_datamodel_dims(db, source_project); + let dims_ctx = DimensionsContext::from(dm_dims.as_slice()); + + // Build ModelStage0 from salsa-parsed variables for all models. + let project_models = source_project.models(db); + let mut all_s0: Vec = Vec::new(); + for (_name, src_model) in project_models.iter() { + let is_stdlib = src_model.name(db).starts_with("stdlib\u{205A}"); + let src_vars = src_model.variables(db); + // For stdlib models, ALL variable names must be module idents + // so PREVIOUS(module_input) uses module expansion instead of + // LoadPrev. Inside a submodule, module inputs are passed via a + // transient array with no persistent slot in prev_values; + // LoadPrev would return the current value, not the previous one. + let extra_module_idents: Vec = if is_stdlib { + src_vars.keys().cloned().collect() } else { - ModelStage0::new( - m, - &project_datamodel.dimensions, - &units_ctx, - is_stdlib_override, - ) + vec![] + }; + let module_ctx = model_module_ident_context(db, *src_model, extra_module_idents); + let mut var_list: Vec = Vec::new(); + let mut implicit_dm: Vec = Vec::new(); + for (_vname, svar) in src_vars.iter() { + let parsed = parse_source_variable_with_module_context( + db, + *svar, + source_project, + module_ctx, + ); + var_list.push(parsed.variable.clone()); + implicit_dm.extend(parsed.implicit_vars.iter().cloned()); } - })); - - let models: HashMap, &ModelStage0> = - models_list.iter().map(|m| (m.ident.clone(), m)).collect(); + // Parse implicit vars (SMOOTH/DELAY expansion). + let mut dummy: Vec = Vec::new(); + var_list.extend(implicit_dm.into_iter().map(|dm_var| { + crate::variable::parse_var(dm_dims, &dm_var, &mut dummy, units_ctx, |mi| { + Ok(Some(mi.clone())) + }) + })); + let variables: HashMap, VariableStage0> = var_list + .into_iter() + .map(|v| (Ident::new(v.ident()), v)) + .collect(); + all_s0.push(ModelStage0 { + ident: Ident::new(src_model.name(db)), + display_name: src_model.name(db).clone(), + variables, + errors: None, + implicit: is_stdlib, + }); + } - let dims_ctx = DimensionsContext::from(&project_datamodel.dimensions); - let mut models_list: Vec = models_list + // ModelStage0 -> ModelStage1 + let models_s0: HashMap, &ModelStage0> = + all_s0.iter().map(|m| (m.ident.clone(), m)).collect(); + let mut models_list: Vec = all_s0 .iter() - .map(|model| { + .map(|ms0| { let scope = ScopeStage0 { - models: &models, + models: &models_s0, dimensions: &dims_ctx, - model_name: model.ident.as_str(), + model_name: ms0.ident.as_str(), }; - ModelStage1::new(&scope, model) + ModelStage1::new(&scope, ms0) }) .collect(); + // Topo-sort by model dependencies. let model_order = { let model_deps: HashMap, BTreeSet>> = models_list .iter_mut() @@ -222,18 +203,14 @@ impl Project { .map(|(i, n)| (n.clone(), i)) .collect::, usize>>() }; - - // sort our model list so that the dependency resolution below works models_list.sort_unstable_by(|a, b| model_order[&a.name].cmp(&model_order[&b.name])); let module_instantiations = { let models = models_list.iter().map(|m| (m.name.as_str(), m)).collect(); - // FIXME: ignoring the result here because if we have errors, it doesn't really matter enumerate_modules(&models, "main", |model| model.name.clone()).unwrap_or_default() }; - // dependency resolution; we need to do this as a second pass - // to ensure we have the information available for modules + // Dependency resolution + model callbacks (unit inference etc.). { let no_instantiations = BTreeSet::new(); let mut models: HashMap, &ModelStage1> = HashMap::new(); @@ -241,12 +218,9 @@ impl Project { let instantiations = module_instantiations .get(&model.name) .unwrap_or(&no_instantiations); - model.set_dependencies(&models, &project_datamodel.dimensions, instantiations); - // things like unit inference happen through this callback - // Skip unit inference for implicit (stdlib) models as they are generic - // templates that only make sense when instantiated with specific inputs + model.set_dependencies(&models, dm_dims.as_slice(), instantiations); if !model.implicit { - model_cb(&models, &units_ctx, model); + model_cb(&models, units_ctx, model); } models.insert(model.name.clone(), model); } @@ -266,7 +240,7 @@ impl Project { datamodel: project_datamodel, models, model_order: ordered_models, - errors: project_errors, + errors: vec![], dimensions_ctx: dims_ctx, } } @@ -294,7 +268,10 @@ impl Project { return Ok(self); } - Ok(Project::from(inject_ltm_vars(self.datamodel, <m_vars))) + Ok(Project::from_datamodel(inject_ltm_vars( + self.datamodel, + <m_vars, + ))) } pub fn with_ltm_all_links(self) -> crate::common::Result { @@ -305,7 +282,10 @@ impl Project { return Ok(self); } - Ok(Project::from(inject_ltm_vars(self.datamodel, <m_vars))) + Ok(Project::from_datamodel(inject_ltm_vars( + self.datamodel, + <m_vars, + ))) } } diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index 2c433fa2..2138d981 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -952,9 +952,12 @@ fn test_inference() { for _ in 0..64 { let mut results: UnitResult, UnitMap>> = Ok(Default::default()); - let _project = crate::project::Project::base_from( + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( project_datamodel.clone(), - None, + &db, + sync.project, |models, units_ctx, model| { results = infer(models, units_ctx, model); }, @@ -1037,9 +1040,12 @@ fn test_inference_negative() { sources: vec![], details: None, }); - let _project = crate::project::Project::base_from( + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( project_datamodel.clone(), - None, + &db, + sync.project, |models, units_ctx, model| { results = infer(models, units_ctx, model); }, @@ -1062,9 +1068,12 @@ fn test_inference_error_has_location() { let project_datamodel = x_project(sim_specs.clone(), &[model]); let mut results: UnitResult, UnitMap>> = Ok(Default::default()); - let _project = crate::project::Project::base_from( + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( project_datamodel.clone(), - None, + &db, + sync.project, |models, units_ctx, model| { results = infer(models, units_ctx, model); }, @@ -1204,9 +1213,12 @@ fn test_multi_metavar_constraint_mismatch() { sources: vec![], details: None, }); - let _project = crate::project::Project::base_from( + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( project_datamodel.clone(), - None, + &db, + sync.project, |models, units_ctx, model| { results = infer(models, units_ctx, model); }, @@ -1443,9 +1455,12 @@ fn test_rank_builtin_unit_inference() { let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); let mut results: UnitResult, UnitMap>> = Ok(Default::default()); - let _project = crate::project::Project::base_from( + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( project_datamodel.clone(), - None, + &db, + sync.project, |models, units_ctx, model| { results = infer(models, units_ctx, model); }, @@ -1489,9 +1504,12 @@ fn test_unify_conflict_detection() { let project_datamodel = x_project(sim_specs.clone(), &[model]); let mut results: UnitResult, UnitMap>> = Ok(Default::default()); - let _project = crate::project::Project::base_from( + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( project_datamodel.clone(), - None, + &db, + sync.project, |models, units_ctx, model| { results = infer(models, units_ctx, model); }, From 341f5ef88618482a4399d976f80adcdffc401079 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 7 Mar 2026 18:56:49 -0800 Subject: [PATCH 2/4] engine: recover unit definition errors from salsa accumulator After calling project_units_context, collect accumulated CompilationDiagnostics for unit definition errors and convert them to Error instances stored in Project.errors. Without this, the from_salsa constructor always produced an empty errors vec, so callers inspecting compiled.errors (e.g. test_common helpers) would silently miss malformed unit definitions. Also restore the implicit-var recursion guard assertion that was present in ModelStage0::new but dropped in the from_salsa path, and rename the opaque "dummy" variable to "nested_implicit". --- src/simlin-engine/src/project.rs | 77 +++++++++++++++++++++++++++----- 1 file changed, 67 insertions(+), 10 deletions(-) diff --git a/src/simlin-engine/src/project.rs b/src/simlin-engine/src/project.rs index 0933f2bb..7b38f5b7 100644 --- a/src/simlin-engine/src/project.rs +++ b/src/simlin-engine/src/project.rs @@ -26,8 +26,8 @@ pub struct Project { #[allow(dead_code)] model_order: Vec>, /// Project-level errors. With the `from_salsa` construction path, - /// unit definition errors are accumulated via the salsa accumulator - /// in `project_units_context`; this field is always empty. + /// unit definition errors are recovered from the salsa accumulator + /// in `project_units_context` so callers can still inspect them. pub errors: Vec, /// Cached dimension context for subdimension lookups pub dimensions_ctx: DimensionsContext, @@ -110,14 +110,30 @@ impl Project { where F: FnMut(&HashMap, &ModelStage1>, &Context, &mut ModelStage1), { - use crate::common::topo_sort; + use crate::common::{ErrorCode, ErrorKind, topo_sort}; use crate::db::{ - model_module_ident_context, parse_source_variable_with_module_context, - project_datamodel_dims, project_units_context, + CompilationDiagnostic, DiagnosticError, model_module_ident_context, + parse_source_variable_with_module_context, project_datamodel_dims, + project_units_context, }; use crate::model::{ModelStage0, VariableStage0, enumerate_modules}; let units_ctx = project_units_context(db, source_project); + + // Recover unit definition errors from the salsa accumulator so + // callers that inspect Project.errors (e.g. tests) still see them. + let project_errors: Vec = + project_units_context::accumulated::(db, source_project) + .into_iter() + .filter_map(|cd| match &cd.0.error { + DiagnosticError::Unit(unit_err) => Some(Error { + kind: ErrorKind::Model, + code: ErrorCode::UnitDefinitionErrors, + details: Some(format!("{unit_err}")), + }), + _ => None, + }) + .collect(); let dm_dims = project_datamodel_dims(db, source_project); let dims_ctx = DimensionsContext::from(dm_dims.as_slice()); @@ -151,12 +167,20 @@ impl Project { implicit_dm.extend(parsed.implicit_vars.iter().cloned()); } // Parse implicit vars (SMOOTH/DELAY expansion). - let mut dummy: Vec = Vec::new(); + let mut nested_implicit: Vec = Vec::new(); var_list.extend(implicit_dm.into_iter().map(|dm_var| { - crate::variable::parse_var(dm_dims, &dm_var, &mut dummy, units_ctx, |mi| { - Ok(Some(mi.clone())) - }) + crate::variable::parse_var( + dm_dims, + &dm_var, + &mut nested_implicit, + units_ctx, + |mi| Ok(Some(mi.clone())), + ) })); + debug_assert!( + nested_implicit.is_empty(), + "implicit vars should not produce further implicit vars" + ); let variables: HashMap, VariableStage0> = var_list .into_iter() .map(|v| (Ident::new(v.ident()), v)) @@ -240,7 +264,7 @@ impl Project { datamodel: project_datamodel, models, model_order: ordered_models, - errors: vec![], + errors: project_errors, dimensions_ctx: dims_ctx, } } @@ -370,6 +394,39 @@ fn abort_if_arrayed(project: &Project) -> crate::common::Result<()> { mod tests { use super::*; + #[test] + fn test_unit_definition_errors_surface_in_project_errors() { + use crate::common::ErrorCode; + use crate::testutils::{sim_specs_with_units, x_aux, x_model, x_project}; + + let model = x_model("main", vec![x_aux("x", "1", None)]); + let sim_specs = sim_specs_with_units("years"); + let mut dm = x_project(sim_specs, &[model]); + // Add a duplicate unit definition to provoke a unit parse error. + dm.units.push(datamodel::Unit { + name: "widget".to_string(), + equation: None, + disabled: false, + aliases: vec![], + }); + dm.units.push(datamodel::Unit { + name: "widget".to_string(), + equation: None, + disabled: false, + aliases: vec![], + }); + + let project = Project::from(dm); + assert!( + project + .errors + .iter() + .any(|e| e.code == ErrorCode::UnitDefinitionErrors), + "Project.errors should contain UnitDefinitionErrors, got: {:?}", + project.errors, + ); + } + #[test] fn test_project_with_ltm() { use crate::testutils::{sim_specs_with_units, x_aux, x_flow, x_model, x_project, x_stock}; From 5401b4dd339d9e3fc1b289d19c43f70c1d43e2ba Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 7 Mar 2026 19:06:27 -0800 Subject: [PATCH 3/4] engine: use canonical key for stdlib model detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the canonical HashMap key (always lowercase) for the starts_with("stdlib⁚") check instead of the raw model name, so non-canonical spellings like Stdlib⁚Delay1 are handled correctly. Also bind model_name once to reduce repeated src_model.name(db) calls. --- src/simlin-engine/src/project.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/simlin-engine/src/project.rs b/src/simlin-engine/src/project.rs index 7b38f5b7..22e50bab 100644 --- a/src/simlin-engine/src/project.rs +++ b/src/simlin-engine/src/project.rs @@ -140,8 +140,11 @@ impl Project { // Build ModelStage0 from salsa-parsed variables for all models. let project_models = source_project.models(db); let mut all_s0: Vec = Vec::new(); - for (_name, src_model) in project_models.iter() { - let is_stdlib = src_model.name(db).starts_with("stdlib\u{205A}"); + for (canonical_name, src_model) in project_models.iter() { + // Use the canonical key (always lowercase) for stdlib + // detection so non-canonical spellings are handled correctly. + let is_stdlib = canonical_name.starts_with("stdlib\u{205A}"); + let model_name = src_model.name(db); let src_vars = src_model.variables(db); // For stdlib models, ALL variable names must be module idents // so PREVIOUS(module_input) uses module expansion instead of @@ -186,8 +189,8 @@ impl Project { .map(|v| (Ident::new(v.ident()), v)) .collect(); all_s0.push(ModelStage0 { - ident: Ident::new(src_model.name(db)), - display_name: src_model.name(db).clone(), + ident: Ident::new(model_name), + display_name: model_name.clone(), variables, errors: None, implicit: is_stdlib, From 2f2a54626007e193f4a2b47de3f0ff44ec23c603 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 7 Mar 2026 19:17:03 -0800 Subject: [PATCH 4/4] engine: preserve unit name in definition errors, tighten stdlib check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Include the failing unit name in UnitDefinitionErrors details so callers can identify which unit definition is broken. The salsa accumulator in project_units_context now stores the unit name in the Diagnostic.variable field; from_salsa recovers it and formats errors as "{name}: {err}" matching the old base_from behavior. Also tighten stdlib model detection to check against the known crate::stdlib::MODEL_NAMES list instead of just the "stdlib⁚" prefix, so user models with that prefix are not incorrectly classified as implicit. --- src/simlin-engine/src/db.rs | 4 ++-- src/simlin-engine/src/project.rs | 41 ++++++++++++++++++++++---------- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/simlin-engine/src/db.rs b/src/simlin-engine/src/db.rs index c6977216..9447d179 100644 --- a/src/simlin-engine/src/db.rs +++ b/src/simlin-engine/src/db.rs @@ -689,11 +689,11 @@ pub fn project_units_context(db: &dyn Db, project: SourceProject) -> crate::unit Err(unit_parse_errors) => { // Accumulate each unit definition parsing error as a // project-level diagnostic (no model / variable). - for (_unit_name, eq_errors) in &unit_parse_errors { + for (unit_name, eq_errors) in &unit_parse_errors { for eq_err in eq_errors { CompilationDiagnostic(Diagnostic { model: String::new(), - variable: None, + variable: Some(unit_name.clone()), error: DiagnosticError::Unit(crate::common::UnitError::DefinitionError( eq_err.clone(), None, diff --git a/src/simlin-engine/src/project.rs b/src/simlin-engine/src/project.rs index 22e50bab..b49d5c0b 100644 --- a/src/simlin-engine/src/project.rs +++ b/src/simlin-engine/src/project.rs @@ -126,11 +126,14 @@ impl Project { project_units_context::accumulated::(db, source_project) .into_iter() .filter_map(|cd| match &cd.0.error { - DiagnosticError::Unit(unit_err) => Some(Error { - kind: ErrorKind::Model, - code: ErrorCode::UnitDefinitionErrors, - details: Some(format!("{unit_err}")), - }), + DiagnosticError::Unit(unit_err) => { + let name = cd.0.variable.as_deref().unwrap_or("unknown"); + Some(Error { + kind: ErrorKind::Model, + code: ErrorCode::UnitDefinitionErrors, + details: Some(format!("{name}: {unit_err}")), + }) + } _ => None, }) .collect(); @@ -141,9 +144,12 @@ impl Project { let project_models = source_project.models(db); let mut all_s0: Vec = Vec::new(); for (canonical_name, src_model) in project_models.iter() { - // Use the canonical key (always lowercase) for stdlib - // detection so non-canonical spellings are handled correctly. - let is_stdlib = canonical_name.starts_with("stdlib\u{205A}"); + // Only treat a model as implicit/stdlib if it matches one + // of the known stdlib model names, not just any model whose + // name starts with the stdlib prefix. + let is_stdlib = canonical_name + .strip_prefix("stdlib\u{205A}") + .is_some_and(|suffix| crate::stdlib::MODEL_NAMES.contains(&suffix)); let model_name = src_model.name(db); let src_vars = src_model.variables(db); // For stdlib models, ALL variable names must be module idents @@ -420,14 +426,25 @@ mod tests { }); let project = Project::from(dm); + let unit_errs: Vec<_> = project + .errors + .iter() + .filter(|e| e.code == ErrorCode::UnitDefinitionErrors) + .collect(); assert!( - project - .errors - .iter() - .any(|e| e.code == ErrorCode::UnitDefinitionErrors), + !unit_errs.is_empty(), "Project.errors should contain UnitDefinitionErrors, got: {:?}", project.errors, ); + // The failing unit name must appear in the error details so + // callers can identify which unit definition is broken. + assert!( + unit_errs + .iter() + .any(|e| e.details.as_deref().unwrap_or("").contains("widget")), + "Error details should include the unit name 'widget', got: {:?}", + unit_errs, + ); } #[test]