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/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/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..b49d5c0b 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 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, @@ -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,150 @@ 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::db::{ + 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}; - // 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(); + let units_ctx = project_units_context(db, source_project); - // 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, - ) + // 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) => { + 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(); + 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 (canonical_name, src_model) in project_models.iter() { + // 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 + // 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 nested_implicit: Vec = Vec::new(); + var_list.extend(implicit_dm.into_iter().map(|dm_var| { + 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)) + .collect(); + all_s0.push(ModelStage0 { + ident: Ident::new(model_name), + display_name: model_name.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 +236,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 +251,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); } @@ -294,7 +301,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 +315,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, + ))) } } @@ -390,6 +403,50 @@ 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); + let unit_errs: Vec<_> = project + .errors + .iter() + .filter(|e| e.code == ErrorCode::UnitDefinitionErrors) + .collect(); + assert!( + !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] fn test_project_with_ltm() { use crate::testutils::{sim_specs_with_units, x_aux, x_flow, x_model, x_project, x_stock}; 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); },