From 1fc0c4cb8ee0ccf683bd6869d73dac4e5f532b5c Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Thu, 21 May 2026 21:08:23 -0700 Subject: [PATCH 1/7] engine: make unit constraint generation total gen_constraints only ever returned Ok, threading a vestigial UnitResult whose .unwrap() in gen_all_constraints was a latent panic the day any arm became fallible. Constraint generation is total: you can always walk a well-formed AST and emit linear constraints; dimensional inconsistency is detected later, during solving. Change gen_constraints to return Units, deleting the unwrap and the dead Err arms in the arrayed path. This also closes a propagation gap: the None => continue arm (no parsed equation) skipped the rest of the loop body, including the var.units() declared-units constraint. A variable with declared units but no equation -- e.g. the editor's units-typed-but-equation-not-yet-written state -- now still contributes its units to inference so dependents resolve. --- src/simlin-engine/src/units_infer.rs | 209 ++++++++++++++++----------- 1 file changed, 127 insertions(+), 82 deletions(-) diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index 4a21b69cc..2de4de4f5 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -362,25 +362,30 @@ impl UnitInferer<'_> { prefix: &str, current_var: &str, constraints: &mut Vec, - ) -> UnitResult { + ) -> Units { + // Constraint generation is total: every well-formed expression yields + // a `Units` value (and pushes zero or more `1 == UnitMap` constraints). + // Dimensional *inconsistency* is detected later, during solving + // (`unify`/`find_constraint_mismatch`), never here -- so this function + // does not return a `Result` and cannot fail. match expr { - Expr2::Const(_, _, _) => Ok(Units::Constant), + Expr2::Const(_, _, _) => Units::Constant, Expr2::Var(ident, _, _loc) => { let units: UnitMap = [(format!("@{prefix}{ident}"), 1)].iter().cloned().collect(); - Ok(Units::Explicit(units)) + Units::Explicit(units) } Expr2::App(builtin, _, _) => match builtin { - BuiltinFn::Inf | BuiltinFn::Pi => Ok(Units::Constant), + BuiltinFn::Inf | BuiltinFn::Pi => Units::Constant, BuiltinFn::Time | BuiltinFn::TimeStep | BuiltinFn::StartTime - | BuiltinFn::FinalTime => Ok(Units::Explicit( - self.time.units().cloned().unwrap_or_default(), - )), + | BuiltinFn::FinalTime => { + Units::Explicit(self.time.units().cloned().unwrap_or_default()) + } BuiltinFn::IsModuleInput(_, _) => { // returns a bool, which is unitless - Ok(Units::Explicit(UnitMap::new())) + Units::Explicit(UnitMap::new()) } BuiltinFn::Lookup(table_expr, _, _loc) | BuiltinFn::LookupForward(table_expr, _, _loc) @@ -389,14 +394,14 @@ impl UnitInferer<'_> { let table_name = match table_expr.as_ref() { Expr2::Var(name, _, _) => name.as_str(), Expr2::Subscript(name, _, _, _) => name.as_str(), - _ => return Ok(Units::Constant), + _ => return Units::Constant, }; let units: UnitMap = [(format!("@{prefix}{table_name}"), 1)] .iter() .cloned() .collect(); - Ok(Units::Explicit(units)) + Units::Explicit(units) } BuiltinFn::Abs(a) | BuiltinFn::Arccos(a) @@ -418,10 +423,10 @@ impl UnitInferer<'_> { let args = args .iter() .map(|arg| self.gen_constraints(arg, prefix, current_var, constraints)) - .collect::>>()?; + .collect::>(); if args.is_empty() { - return Ok(Units::Constant); + return Units::Constant; } // find the first non-constant argument @@ -441,16 +446,16 @@ impl UnitInferer<'_> { )); } } - Ok(Units::Explicit(arg0)) + Units::Explicit(arg0) } - Some(Units::Constant) => Ok(Units::Constant), - None => Ok(Units::Constant), + Some(Units::Constant) => Units::Constant, + None => Units::Constant, } } BuiltinFn::Max(a, b) | BuiltinFn::Min(a, b) => { - let a_units = self.gen_constraints(a, prefix, current_var, constraints)?; + let a_units = self.gen_constraints(a, prefix, current_var, constraints); if let Some(b) = b { - let b_units = self.gen_constraints(b, prefix, current_var, constraints)?; + let b_units = self.gen_constraints(b, prefix, current_var, constraints); if let Units::Explicit(ref lunits) = a_units && let Units::Explicit(runits) = b_units @@ -463,7 +468,7 @@ impl UnitInferer<'_> { )); } } - Ok(a_units) + a_units } BuiltinFn::Quantum(a, _) => { self.gen_constraints(a, prefix, current_var, constraints) @@ -472,7 +477,7 @@ impl UnitInferer<'_> { self.gen_constraints(bottom, prefix, current_var, constraints) } BuiltinFn::Pulse(_, _, _) | BuiltinFn::Ramp(_, _, _) | BuiltinFn::Step(_, _) => { - Ok(Units::Constant) + Units::Constant } BuiltinFn::SafeDiv(a, b, c) => { let div = Expr2::Op2( @@ -482,13 +487,13 @@ impl UnitInferer<'_> { None, a.get_loc().union(&b.get_loc()), ); - let units = self.gen_constraints(&div, prefix, current_var, constraints)?; + let units = self.gen_constraints(&div, prefix, current_var, constraints); // the optional argument to safediv, if specified, should match the units of a/b if let Units::Explicit(ref result_units) = units && let Some(c) = c && let Units::Explicit(c_units) = - self.gen_constraints(c, prefix, current_var, constraints)? + self.gen_constraints(c, prefix, current_var, constraints) { constraints.push(LocatedConstraint::new( combine(UnitOp::Div, c_units, result_units.clone()), @@ -497,15 +502,15 @@ impl UnitInferer<'_> { )); } - Ok(units) + units } BuiltinFn::Rank(a, _) => { - let a_units = self.gen_constraints(a, prefix, current_var, constraints)?; + let a_units = self.gen_constraints(a, prefix, current_var, constraints); // The direction argument is unitless control input; RANK's // result keeps the array's units for now. - Ok(a_units) + a_units } BuiltinFn::VectorSelect(_, expr_array, _, _, _) => { self.gen_constraints(expr_array, prefix, current_var, constraints) @@ -513,7 +518,7 @@ impl UnitInferer<'_> { BuiltinFn::VectorElmMap(source, _) => { self.gen_constraints(source, prefix, current_var, constraints) } - BuiltinFn::VectorSortOrder(_, _) => Ok(Units::Constant), + BuiltinFn::VectorSortOrder(_, _) => Units::Constant, BuiltinFn::AllocateAvailable(req, _, _) | BuiltinFn::AllocateByPriority(req, _, _, _, _) => { self.gen_constraints(req, prefix, current_var, constraints) @@ -521,8 +526,8 @@ impl UnitInferer<'_> { // Previous(x, fallback) and Init(x) preserve the units of the // lagged/current argument; the fallback must be compatible. BuiltinFn::Previous(a, b) => { - let a_units = self.gen_constraints(a, prefix, current_var, constraints)?; - let b_units = self.gen_constraints(b, prefix, current_var, constraints)?; + let a_units = self.gen_constraints(a, prefix, current_var, constraints); + let b_units = self.gen_constraints(b, prefix, current_var, constraints); // Constrain fallback to match the lagged argument's units, // analogous to Max/Min handling. if let Units::Explicit(ref a_map) = a_units @@ -535,7 +540,7 @@ impl UnitInferer<'_> { Some(loc), )); } - Ok(a_units) + a_units } BuiltinFn::Init(a) => self.gen_constraints(a, prefix, current_var, constraints), }, @@ -545,18 +550,18 @@ impl UnitInferer<'_> { .iter() .cloned() .collect(); - Ok(Units::Explicit(units)) + Units::Explicit(units) } Expr2::Op1(_, l, _, _) => self.gen_constraints(l, prefix, current_var, constraints), Expr2::Op2(op, l, r, _, _) => { - let lunits = self.gen_constraints(l, prefix, current_var, constraints)?; - let runits = self.gen_constraints(r, prefix, current_var, constraints)?; + let lunits = self.gen_constraints(l, prefix, current_var, constraints); + let runits = self.gen_constraints(r, prefix, current_var, constraints); match op { BinaryOp::Add | BinaryOp::Sub => match (lunits, runits) { - (Units::Constant, Units::Constant) => Ok(Units::Constant), + (Units::Constant, Units::Constant) => Units::Constant, (Units::Constant, Units::Explicit(units)) - | (Units::Explicit(units), Units::Constant) => Ok(Units::Explicit(units)), + | (Units::Explicit(units), Units::Constant) => Units::Explicit(units), (Units::Explicit(lunits), Units::Explicit(runits)) => { let loc = l.get_loc().union(&r.get_loc()); constraints.push(LocatedConstraint::new( @@ -564,26 +569,26 @@ impl UnitInferer<'_> { current_var, Some(loc), )); - Ok(Units::Explicit(lunits)) + Units::Explicit(lunits) } }, - BinaryOp::Exp | BinaryOp::Mod => Ok(lunits), + BinaryOp::Exp | BinaryOp::Mod => lunits, BinaryOp::Mul => match (lunits, runits) { - (Units::Constant, Units::Constant) => Ok(Units::Constant), + (Units::Constant, Units::Constant) => Units::Constant, (Units::Explicit(units), Units::Constant) - | (Units::Constant, Units::Explicit(units)) => Ok(Units::Explicit(units)), + | (Units::Constant, Units::Explicit(units)) => Units::Explicit(units), (Units::Explicit(lunits), Units::Explicit(runits)) => { - Ok(Units::Explicit(combine(UnitOp::Mul, lunits, runits))) + Units::Explicit(combine(UnitOp::Mul, lunits, runits)) } }, BinaryOp::Div => match (lunits, runits) { - (Units::Constant, Units::Constant) => Ok(Units::Constant), - (Units::Explicit(units), Units::Constant) => Ok(Units::Explicit(units)), + (Units::Constant, Units::Constant) => Units::Constant, + (Units::Explicit(units), Units::Constant) => Units::Explicit(units), (Units::Constant, Units::Explicit(units)) => { - Ok(Units::Explicit(combine(UnitOp::Div, UnitMap::new(), units))) + Units::Explicit(combine(UnitOp::Div, UnitMap::new(), units)) } (Units::Explicit(lunits), Units::Explicit(runits)) => { - Ok(Units::Explicit(combine(UnitOp::Div, lunits, runits))) + Units::Explicit(combine(UnitOp::Div, lunits, runits)) } }, BinaryOp::Gt @@ -595,13 +600,13 @@ impl UnitInferer<'_> { | BinaryOp::And | BinaryOp::Or => { // binary comparisons result in unitless quantities - Ok(Units::Explicit(UnitMap::new())) + Units::Explicit(UnitMap::new()) } } } Expr2::If(_, l, r, _, _) => { - let lunits = self.gen_constraints(l, prefix, current_var, constraints)?; - let runits = self.gen_constraints(r, prefix, current_var, constraints)?; + let lunits = self.gen_constraints(l, prefix, current_var, constraints); + let runits = self.gen_constraints(r, prefix, current_var, constraints); if let Units::Explicit(ref lunits) = lunits && let Units::Explicit(runits) = runits @@ -614,7 +619,7 @@ impl UnitInferer<'_> { )); } - Ok(lunits) + lunits } } } @@ -723,55 +728,50 @@ impl UnitInferer<'_> { let array_var: UnitMap = [(format!("@{prefix}{id}"), 1)].iter().cloned().collect(); - let mut result: UnitResult = Ok(Units::Constant); for (_element, expr) in asts.iter() { - match self.gen_constraints(expr, prefix, ¤t_var, constraints) { - Ok(expr_units) => { - // Add a constraint tying this element's units to the array variable - if let Units::Explicit(units) = expr_units { - let element_var = format!("{current_var}[element]"); - constraints.push(LocatedConstraint::new( - combine(UnitOp::Div, array_var.clone(), units), - &element_var, - Some(expr.get_loc()), - )); - } - } - Err(e) => { - result = Err(e); - break; - } + let expr_units = + self.gen_constraints(expr, prefix, ¤t_var, constraints); + // Add a constraint tying this element's units to the array variable + if let Units::Explicit(units) = expr_units { + let element_var = format!("{current_var}[element]"); + constraints.push(LocatedConstraint::new( + combine(UnitOp::Div, array_var.clone(), units), + &element_var, + Some(expr.get_loc()), + )); } } if let Some(default_expr) = default_expr { - match self.gen_constraints( + let expr_units = self.gen_constraints( default_expr, prefix, ¤t_var, constraints, - ) { - Ok(expr_units) => { - if let Units::Explicit(units) = expr_units { - constraints.push(LocatedConstraint::new( - combine(UnitOp::Div, array_var.clone(), units), - &format!("{current_var}[default]"), - Some(default_expr.get_loc()), - )); - } - } - Err(e) => result = Err(e), + ); + if let Units::Explicit(units) = expr_units { + constraints.push(LocatedConstraint::new( + combine(UnitOp::Div, array_var.clone(), units), + &format!("{current_var}[default]"), + Some(default_expr.get_loc()), + )); } } - // Return Constant since we've added constraints directly above - // (the constraint from var_units below would be redundant) - result + // We added the per-element constraints directly above, so the + // array variable itself contributes no further equation + // constraint here (the `Units::Explicit` branch below would be + // redundant). + Units::Constant } None => { - // TODO: maybe we should bail early? If there is no equation we will fail - continue; + // No parsed equation -- e.g. an empty/not-yet-written equation + // or a module-input placeholder. There is no equation-derived + // constraint to add, but we must NOT skip the variable: we fall + // through to the `var.units()` constraint below so a variable + // with declared units but no equation still informs inference of + // its dependents. + Units::Constant } - } - .unwrap(); + }; // Constants don't generate constraints - they adopt units from context // (e.g., in "x + 1", the 1 has the same units as x) if let Units::Explicit(units) = var_units { @@ -1007,6 +1007,51 @@ fn test_inference() { } } +/// A variable can have declared units but no parsed equation -- e.g. in the +/// editor when units are entered before the equation is written (the same +/// half-built state that powers unit fill-in). Such a variable must still +/// contribute its declared units to inference so that dependents can be +/// inferred. Regression test for the `None => continue` gap in +/// `gen_all_constraints`, which skipped the `var.units()` constraint entirely +/// for equation-less variables. +#[test] +fn test_declared_units_without_equation_propagate() { + let sim_specs = sim_specs_with_units("parsec"); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + + // `base` has declared units but an empty equation (so `ast()` is None). + // `derived = base` has no declared units; inference should propagate + // `widget` to it through the reference. + let vars = vec![ + x_aux("base", "", Some("widget")), + x_aux("derived", "base", None), + ]; + let model = x_model("main", vars); + let project_datamodel = x_project(sim_specs.clone(), &[model]); + + let mut results: UnitResult, UnitMap>> = Ok(Default::default()); + 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(), + &db, + sync.project, + |models, units_ctx, model| { + results = infer(models, units_ctx, model); + }, + ); + let results = results.unwrap(); + + let widget: UnitMap = crate::units::parse_units(&units_ctx, Some("widget")) + .unwrap() + .unwrap(); + assert_eq!( + results.get(&*canonicalize("derived")), + Some(&widget), + "derived should inherit base's declared units via inference even though base has no equation" + ); +} + #[test] fn test_inference_negative() { let sim_specs = sim_specs_with_units("parsec"); From 7834191e251c6854efd88b80a99c8b16a41567c3 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Thu, 21 May 2026 21:15:45 -0700 Subject: [PATCH 2/7] engine: RANK result units are dimensionless RANK returns an ordinal position (a 1-based index), which is a dimensionless quantity -- like a comparison result -- not the units of the array being ranked. Both the inference and checking paths previously inherited the ranked array's units, so a RANK result silently took on e.g. dollars and declaring it dimensionless was a false-positive mismatch. Walk the ranked array for its internal constraints, but return an explicit empty unit map for the RANK result itself. --- src/simlin-engine/src/unit_checking_test.rs | 21 ++++++++ src/simlin-engine/src/units_check.rs | 8 ++- src/simlin-engine/src/units_infer.rs | 56 ++++++++++----------- 3 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/simlin-engine/src/unit_checking_test.rs b/src/simlin-engine/src/unit_checking_test.rs index 65d75b1c8..e9cb90599 100644 --- a/src/simlin-engine/src/unit_checking_test.rs +++ b/src/simlin-engine/src/unit_checking_test.rs @@ -60,6 +60,27 @@ mod tests { .assert_unit_error_vm(); } + #[test] + fn test_rank_result_dimensionless_in_checking() { + // A RANK result is a dimensionless ordinal position, not the units of + // the ranked array. Declaring the result with the array's units + // (`widgets`) is therefore a genuine dimensional error that the unit + // checker must flag -- it must not silently inherit `widgets` from the + // ranked source. + TestProject::new("rank_units_test") + .with_sim_time(0.0, 0.0, 1.0) + .unit("widgets", None) + .indexed_dimension("D", 3) + .array_const_with_units("source[D]", 10.0, "widgets") + .array_aux_direct( + "result", + vec!["D".to_string()], + "RANK(source[D], 1)", + Some("widgets"), + ) + .assert_unit_error_vm(); + } + #[test] fn test_delay1_with_units() { // Test DELAY1 function diff --git a/src/simlin-engine/src/units_check.rs b/src/simlin-engine/src/units_check.rs index dfc91d568..7b8aef7bc 100644 --- a/src/simlin-engine/src/units_check.rs +++ b/src/simlin-engine/src/units_check.rs @@ -217,7 +217,13 @@ impl UnitEvaluator<'_> { Ok(units) } - BuiltinFn::Rank(a, _) => self.check(a), + BuiltinFn::Rank(a, _) => { + // Check the ranked array for internal consistency, but a + // RANK result is a dimensionless ordinal position, not + // the units of the array being ranked. + self.check(a)?; + Ok(Units::Explicit(UnitMap::new())) + } BuiltinFn::VectorSelect(_, expr_array, _, _, _) => self.check(expr_array), BuiltinFn::VectorElmMap(source, _) => self.check(source), BuiltinFn::VectorSortOrder(_, _) => Ok(Units::Constant), diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index 2de4de4f5..9d7b3b0f4 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -505,12 +505,13 @@ impl UnitInferer<'_> { units } BuiltinFn::Rank(a, _) => { - let a_units = self.gen_constraints(a, prefix, current_var, constraints); - - // The direction argument is unitless control input; RANK's - // result keeps the array's units for now. - - a_units + // Walk the ranked array so any constraints inside `a` are + // generated, but discard its units: a RANK result is a + // dimensionless position/index (like a comparison result), + // not the units of the array being ranked. The direction + // argument is a unitless control input. + self.gen_constraints(a, prefix, current_var, constraints); + Units::Explicit(UnitMap::new()) } BuiltinFn::VectorSelect(_, expr_array, _, _, _) => { self.gen_constraints(expr_array, prefix, current_var, constraints) @@ -1625,21 +1626,17 @@ fn test_located_constraint_is_empty() { #[test] fn test_rank_builtin_unit_inference() { - // Test that RANK builtin is handled correctly in unit inference - // RANK returns the same units as its first argument (the array being ranked) + // RANK returns a dimensionless position/index, NOT the units of the + // ranked array. So `ranking` below must infer to dimensionless (an empty + // unit map) even though the ranked `values` is in dollars. let sim_specs = sim_specs_with_units("year"); - let vars = [ - (x_aux("values", "10", Some("dollar")), "dollar"), - (x_aux("ranking", "RANK(values, 1)", None), "dollar"), + let vars = vec![ + x_aux("values", "10", Some("dollar")), + x_aux("ranking", "RANK(values, 1)", None), ]; - let expected: HashMap<&str, &str> = vars - .iter() - .map(|(var, units)| (var.get_ident(), *units)) - .collect(); - let model_vars: Vec<_> = vars.iter().map(|(var, _)| var.clone()).collect(); - let model = x_model("main", model_vars); + let model = x_model("main", vars); let project_datamodel = x_project(sim_specs.clone(), &[model]); let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); @@ -1656,18 +1653,19 @@ fn test_rank_builtin_unit_inference() { ); let results = results.expect("RANK inference should succeed"); - for (ident, expected_units) in expected.iter() { - let expected_units: UnitMap = crate::units::parse_units(&units_ctx, Some(expected_units)) - .unwrap() - .unwrap(); - if let Some(computed_units) = results.get(&*canonicalize(ident)) { - assert_eq!( - expected_units, *computed_units, - "Units for {} should match", - ident - ); - } - } + + // `values` keeps its declared dollar units... + let dollar: UnitMap = crate::units::parse_units(&units_ctx, Some("dollar")) + .unwrap() + .unwrap(); + assert_eq!(results.get(&*canonicalize("values")), Some(&dollar)); + + // ...but `ranking` is dimensionless, not dollars. + assert_eq!( + results.get(&*canonicalize("ranking")), + Some(&UnitMap::new()), + "RANK result should be dimensionless, not inherit the ranked array's units" + ); } #[test] From 6f66fd3671e4b824a82a983ba38e823096c41350 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Thu, 21 May 2026 22:18:25 -0700 Subject: [PATCH 3/7] engine: unit inference keeps partial results on conflict units_infer::infer returned a single Result, so the first dimensional conflict aborted inference and discarded every unit it had already resolved; db_units then fell back to an empty inferred-units map, blinding the per-variable unit check for the rest of the model (GH #614). infer now returns InferenceResult { resolved, conflicts }: solving continues past a conflict keeping the first binding -- a contradiction is confined to its connected component of the constraint graph, since substitution only flows along shared metavariables, so this can never corrupt an independent component -- and find_constraint_mismatches collects every residual contradiction rather than the first. check_model_units keeps the resolved units (so the rest of the model is still checked) and surfaces conflicts as one umbrella warning rather than one-per-conflict, since a macro-heavy model can produce hundreds of internal contradictions. Keeping the resolved units unmasked a pre-existing leak: a Vensim macro may annotate body variables' units with the formal parameter names (e.g. `~ xfrom` inside RAMP FROM TO) -- a polymorphic unit, not a concrete base unit. Inference recurses into macro instances, so those parameter names leaked as literal units into every instantiation and conflicted with the real argument units (C-LEARN's xfrom/xto storm). ModelStage0/1 gain an is_macro flag and inference now skips declared-units constraints for macro bodies, letting those units be inferred polymorphically from the equation and cross-module parameter bindings (mirroring check_model_units, which already skips macro models). This restores C-LEARN to its documented 14-diagnostic residual. --- src/simlin-engine/src/db_units.rs | 57 +-- src/simlin-engine/src/db_var_fragment.rs | 2 + src/simlin-engine/src/model.rs | 13 + src/simlin-engine/src/project.rs | 1 + src/simlin-engine/src/units_infer.rs | 425 ++++++++++++++++------- 5 files changed, 348 insertions(+), 150 deletions(-) diff --git a/src/simlin-engine/src/db_units.rs b/src/simlin-engine/src/db_units.rs index 05745e277..3da212466 100644 --- a/src/simlin-engine/src/db_units.rs +++ b/src/simlin-engine/src/db_units.rs @@ -215,6 +215,7 @@ pub fn check_model_units(db: &dyn Db, model: SourceModel, project: SourceProject variables, errors: None, implicit: is_stdlib, + is_macro: src_model.macro_spec(db).is_some(), } }; @@ -263,27 +264,41 @@ pub fn check_model_units(db: &dyn Db, model: SourceModel, project: SourceProject .values() .any(|var| var.units().is_some()); - // Run unit inference. - let inferred_units = crate::units_infer::infer(&models_s1, units_ctx, target_model) - .unwrap_or_else(|err| { - if has_declared_units - && let crate::common::UnitError::InferenceError { code, .. } = &err - && *code == ErrorCode::UnitMismatch - { - CompilationDiagnostic(Diagnostic { - model: model_name.clone(), - variable: None, - error: DiagnosticError::Model(crate::common::Error { - kind: ErrorKind::Model, - code: *code, - details: Some(format!("{}", err)), - }), - severity: DiagnosticSeverity::Warning, - }) - .accumulate(db); - } - Default::default() - }); + // Run unit inference. Inference is partial: it returns the units it could + // resolve together with any dimensional conflicts it found. We keep the + // resolved units -- so the rest of the model is still unit-checked even when + // one equation conflicts, rather than discarding the whole inferred-units + // map on the first conflict (GH #614). + // + // Conflicts are surfaced as a single umbrella model-level warning rather + // than one diagnostic per conflict: a large macro-instantiated model can + // produce hundreds of internal constraint contradictions, and emitting one + // warning each would flood the report. The full conflict list remains + // available on the `InferenceResult` for callers that want it. + let inference = crate::units_infer::infer(&models_s1, units_ctx, target_model); + if has_declared_units && !inference.conflicts.is_empty() { + let detail = if inference.conflicts.len() == 1 { + format!("{}", inference.conflicts[0]) + } else { + format!( + "{} dimensional unit conflicts found during inference; first: {}", + inference.conflicts.len(), + inference.conflicts[0] + ) + }; + CompilationDiagnostic(Diagnostic { + model: model_name.clone(), + variable: None, + error: DiagnosticError::Model(crate::common::Error { + kind: ErrorKind::Model, + code: ErrorCode::UnitMismatch, + details: Some(detail), + }), + severity: DiagnosticSeverity::Warning, + }) + .accumulate(db); + } + let inferred_units = inference.resolved; // Check stdlib module argument unit compatibility. // diff --git a/src/simlin-engine/src/db_var_fragment.rs b/src/simlin-engine/src/db_var_fragment.rs index 51461bc35..2c633fd83 100644 --- a/src/simlin-engine/src/db_var_fragment.rs +++ b/src/simlin-engine/src/db_var_fragment.rs @@ -703,6 +703,8 @@ pub(crate) fn lower_var_fragment( variables: stage0_vars, errors: None, implicit: false, + // Single-variable fragment compilation only; not a macro template. + is_macro: false, }; let mut models: HashMap, &crate::model::ModelStage0> = HashMap::new(); diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index b4250b751..8eb044985 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -47,6 +47,12 @@ pub struct ModelStage0 { /// implicit is true if this model was implicitly added to the project /// by virtue of it being in the stdlib (or some similar reason) pub implicit: bool, + /// is_macro is true if this model is a macro definition. A macro is a + /// polymorphic template: its body variables' declared units may name the + /// macro's formal parameters (a Vensim idiom -- e.g. `~ xfrom` inside + /// RAMP FROM TO), so unit inference must treat those as polymorphic rather + /// than concrete base units. + pub is_macro: bool, } #[cfg_attr(feature = "debug-derive", derive(Debug))] @@ -73,6 +79,10 @@ pub struct ModelStage1 { /// implicit is true if this model was implicitly added to the project /// by virtue of it being in the stdlib (or some similar reason) pub implicit: bool, + /// is_macro is true if this model is a macro definition; see + /// `ModelStage0::is_macro`. Inference treats a macro body's declared units + /// as polymorphic rather than concrete base units. + pub is_macro: bool, } #[cfg_attr(feature = "debug-derive", derive(Debug))] @@ -964,6 +974,7 @@ impl ModelStage0 { variables, errors: None, implicit, + is_macro: x_model.macro_spec.is_some(), } } @@ -1065,6 +1076,7 @@ impl ModelStage0 { variables, errors: None, implicit, + is_macro: x_model.macro_spec.is_some(), } } } @@ -1110,6 +1122,7 @@ impl ModelStage1 { model_deps: Some(model_deps), instantiations: None, implicit: model_s0.implicit, + is_macro: model_s0.is_macro, } } diff --git a/src/simlin-engine/src/project.rs b/src/simlin-engine/src/project.rs index 02d5b8baa..ce899f4b9 100644 --- a/src/simlin-engine/src/project.rs +++ b/src/simlin-engine/src/project.rs @@ -159,6 +159,7 @@ impl Project { variables, errors: None, implicit: is_stdlib, + is_macro: src_model.macro_spec(db).is_some(), }); } diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index 9d7b3b0f4..fac869fd2 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -6,7 +6,7 @@ use std::collections::HashMap; use crate::ast::{Ast, BinaryOp, Expr2}; use crate::builtins::{BuiltinFn, Loc}; -use crate::common::{Canonical, ErrorCode, Ident, UnitError, UnitResult, canonicalize}; +use crate::common::{Canonical, ErrorCode, Ident, UnitError, canonicalize}; use crate::datamodel::UnitMap; use crate::model::ModelStage1; #[cfg(test)] @@ -88,6 +88,22 @@ impl LocatedConstraint { } } +/// The result of unit inference for a model. +/// +/// Inference is *partial*: `resolved` holds every metavariable the solver +/// could pin to a concrete unit, and `conflicts` holds every dimensional +/// contradiction it found. A conflict in one connected component of the +/// constraint graph cannot affect another (substitution only flows along +/// shared metavariables), so a single bad equation no longer discards the +/// units resolved for the rest of the model -- and the conflict set is +/// complete rather than just the first contradiction encountered (GH #614). +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Default)] +pub(crate) struct InferenceResult { + pub resolved: HashMap, UnitMap>, + pub conflicts: Vec, +} + struct UnitInferer<'a> { ctx: &'a Context, models: &'a HashMap, &'a ModelStage1>, @@ -280,10 +296,28 @@ fn split_constraint(u: &UnitMap) -> (UnitMap, UnitMap) { /// /// This is O(n) by grouping constraints by their metavariable signature using a HashMap, /// rather than O(n²) pairwise comparison. -fn find_constraint_mismatch(constraints: &[LocatedConstraint]) -> Option { +/// Find every dimensional contradiction among the residual (post-solve) +/// constraints, rather than just the first. Collecting them all -- instead of +/// short-circuiting on the first -- gives a complete diagnostic set in one pass +/// and makes the reported set independent of which contradiction the solver +/// happens to reach first (GH #614, and mitigates the order-dependence in +/// GH #474). +/// +/// Two kinds of contradiction: +/// +/// 1. A constraint with only concrete units (no metavariables) that isn't +/// dimensionless -- e.g. `meters == seconds`, which is impossible. +/// 2. Two constraints with the same metavariable "signature" but different +/// concrete "residuals" -- e.g. `@a/@b * meters == 1` and +/// `@a/@b * seconds == 1` imply `meters == seconds`. +/// +/// Grouping by signature keeps this O(n) rather than O(n^2) pairwise. +fn find_constraint_mismatches(constraints: &[LocatedConstraint]) -> Vec { use std::collections::HashMap; use std::fmt::Write; + let mut mismatches: Vec = Vec::new(); + // Group constraints by their metavariable signature. // Key: sorted string representation of metavar signature (for HashMap key) // Value: reference to the first LocatedConstraint with this signature, plus its residual @@ -296,7 +330,7 @@ fn find_constraint_mismatch(constraints: &[LocatedConstraint]) -> Option Option Option Option { @@ -800,7 +838,19 @@ impl UnitInferer<'_> { )); } } - if let Some(units) = var.units() { + // A macro is a polymorphic template: its body variables' declared + // units may name the macro's formal parameters (a Vensim idiom, + // e.g. `~ xfrom` inside RAMP FROM TO), which would otherwise leak + // the parameter name as a literal base unit into every + // instantiation and conflict with the real argument units. So we + // skip declared-units constraints for macro bodies and let those + // units be inferred polymorphically from the equation and the + // cross-module parameter bindings instead. (This mirrors + // `check_model_units`, which skips unit-checking macro models + // entirely.) + if !model.is_macro + && let Some(units) = var.units() + { let mv: UnitMap = [(format!("@{prefix}{id}"), 1)].iter().cloned().collect(); // User-defined unit declarations don't have equation locations constraints.push(LocatedConstraint::new( @@ -812,20 +862,30 @@ impl UnitInferer<'_> { } } + #[allow(clippy::type_complexity)] + /// Solve the constraint system by Gaussian-elimination-style substitution, + /// returning the resolved metavariable units, the residual (still + /// metavariable-bearing) constraints, and any dimensional conflicts found + /// while solving. + /// + /// A conflict is recorded and solving continues with the *first* binding + /// kept, rather than aborting (GH #614). Because substitution only ever + /// flows along shared metavariables, a contradiction is confined to its own + /// connected component of the constraint graph: keeping going can never + /// corrupt the units resolved for an independent component. #[allow(clippy::type_complexity)] fn unify( &self, constraints: Vec, - ) -> std::result::Result< - ( - HashMap, UnitMap>, - Option>, - ), - UnitError, - > { + ) -> ( + HashMap, UnitMap>, + Vec, + Vec, + ) { let mut resolved_fvs: HashMap, UnitMap> = HashMap::new(); // Track sources for each resolved variable in case of conflict let mut resolved_sources: HashMap, Vec> = HashMap::new(); + let mut conflicts: Vec = Vec::new(); let mut pending = ConstraintSet::from_vec(constraints); let mut finalized = ConstraintSet::default(); @@ -855,7 +915,10 @@ impl UnitInferer<'_> { } } } - return Err(UnitError::InferenceError { + // Record the conflict but keep the first binding and + // keep solving: a contradiction for one metavariable + // must not discard the units resolved for the rest. + conflicts.push(UnitError::InferenceError { code: ErrorCode::UnitMismatch, sources: all_sources, details: Some(format!( @@ -879,41 +942,32 @@ impl UnitInferer<'_> { } } - let final_constraints = finalized.into_vec(); - let final_constraints = if final_constraints.is_empty() { - None - } else { - Some(final_constraints) - }; - - Ok((resolved_fvs, final_constraints)) + (resolved_fvs, finalized.into_vec(), conflicts) } - fn infer(&self, model: &ModelStage1) -> UnitResult, UnitMap>> { - // use rand::seq::SliceRandom; - // use rand::thread_rng; - + fn infer(&self, model: &ModelStage1) -> InferenceResult { let mut constraints = vec![]; self.gen_all_constraints(model, "", &mut constraints); - // mostly for robustness: ensure we don't inadvertently depend on - // test cases iterating in a specific order. - // constraints.shuffle(&mut thread_rng()); - let (results, constraints) = self.unify(constraints)?; + let (resolved, leftover, mut conflicts) = self.unify(constraints); - if let Some(constraints) = constraints { - // Check if any unresolved constraint represents an actual mismatch - let mismatch = find_constraint_mismatch(&constraints); + // Leftover constraints that still contain metavariables just mean the + // model is under-constrained (e.g. undeclared units) -- not an error. + // Only a concrete contradiction among them is a real mismatch. + conflicts.extend(find_constraint_mismatches(&leftover)); - if let Some(err) = mismatch { - Err(err) - } else { - // Unresolved constraints with metavariables just mean the model - // is under-constrained (e.g., no units declared). Return partial results. - Ok(results) + // The same contradiction can be reached both while solving and as a + // leftover constraint; drop exact duplicates so each is reported once. + let mut deduped: Vec = Vec::with_capacity(conflicts.len()); + for conflict in conflicts { + if !deduped.contains(&conflict) { + deduped.push(conflict); } - } else { - Ok(results) + } + + InferenceResult { + resolved, + conflicts: deduped, } } } @@ -980,8 +1034,7 @@ fn test_inference() { // there is non-determinism in inference; do it a few times to // shake out heisenbugs for _ in 0..64 { - let mut results: UnitResult, UnitMap>> = - Ok(Default::default()); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -992,7 +1045,12 @@ fn test_inference() { results = infer(models, units_ctx, model); }, ); - let results = results.unwrap(); + assert!( + results.conflicts.is_empty(), + "expected no conflicts for a fully-inferrable model, got: {:?}", + results.conflicts + ); + let results = results.resolved; for (ident, expected_units) in expected.iter() { let expected_units: UnitMap = crate::units::parse_units(&units_ctx, Some(expected_units)) @@ -1030,7 +1088,7 @@ fn test_declared_units_without_equation_propagate() { let model = x_model("main", vars); let project_datamodel = x_project(sim_specs.clone(), &[model]); - let mut results: UnitResult, UnitMap>> = Ok(Default::default()); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1041,7 +1099,7 @@ fn test_declared_units_without_equation_propagate() { results = infer(models, units_ctx, model); }, ); - let results = results.unwrap(); + let results = results.resolved; let widget: UnitMap = crate::units::parse_units(&units_ctx, Some("widget")) .unwrap() @@ -1109,12 +1167,7 @@ fn test_inference_negative() { // there is non-determinism in inference; do it a few times to // shake out heisenbugs for _ in 0..64 { - let mut results: UnitResult, UnitMap>> = - Err(UnitError::InferenceError { - code: ErrorCode::UnitMismatch, - sources: vec![], - details: None, - }); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1125,7 +1178,10 @@ fn test_inference_negative() { results = infer(models, units_ctx, model); }, ); - assert!(results.is_err()); + assert!( + !results.conflicts.is_empty(), + "expected a dimensional conflict to be reported" + ); } } } @@ -1142,7 +1198,7 @@ fn test_inference_error_has_location() { let model = x_model("main", vars); let project_datamodel = x_project(sim_specs.clone(), &[model]); - let mut results: UnitResult, UnitMap>> = Ok(Default::default()); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1154,47 +1210,167 @@ fn test_inference_error_has_location() { }, ); - // Verify that we got an error with location information - match results { - Err(UnitError::InferenceError { + // Verify that at least one reported conflict carries source + location info. + assert!( + !results.conflicts.is_empty(), + "expected at least one conflict to be reported" + ); + let found = results.conflicts.iter().any(|conflict| { + if let UnitError::InferenceError { code, sources, details, - }) => { - assert_eq!(code, ErrorCode::UnitMismatch); - // Should have at least one source with the variable name - assert!( - !sources.is_empty(), - "inference error should have at least one source" - ); - // At least one source should reference "bad" (the variable with the mismatch) - let has_bad = sources.iter().any(|(var, _)| var == "bad"); - assert!( - has_bad, - "sources should contain 'bad' variable, got: {:?}", - sources - ); - // The source should have a location (non-None) for the equation-based constraint - // Note: some sources may have None loc (e.g., from declarations without equations) - let has_loc = sources.iter().any(|(_, loc)| loc.is_some()); - assert!( - has_loc, - "at least one source should have a location, got: {:?}", - sources - ); - // Verify there's a meaningful details message - assert!(details.is_some(), "error should have details"); + } = conflict + { + *code == ErrorCode::UnitMismatch + // at least one source references "bad" (the mismatched variable) + && sources.iter().any(|(var, _)| var == "bad") + // at least one source carries an equation location (some sources, + // e.g. bare unit declarations, legitimately have None) + && sources.iter().any(|(_, loc)| loc.is_some()) + && details.is_some() + } else { + false } - Ok(_) => panic!("expected inference error, got Ok"), - Err(e) => panic!("expected InferenceError, got {:?}", e), - } + }); + assert!( + found, + "expected an InferenceError mentioning 'bad' with a location and details, got: {:?}", + results.conflicts + ); +} + +/// Inference is partial: a dimensional conflict in one part of a model must +/// not discard the units inference resolved elsewhere, and every independent +/// conflict must be reported -- not just whichever one happens to be found +/// first (see GH #614). +#[test] +fn test_inference_partial_results_survive_conflict() { + let sim_specs = sim_specs_with_units("year"); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + + let vars = vec![ + // A clean, independently-inferrable chain. + x_aux("clean_src", "6", Some("widget")), + x_aux("clean_dst", "clean_src", None), // inferred: widget + // Conflict A: `a` is forced to two incompatible units. + x_aux("a", "10", None), + x_aux("ay", "a", Some("meter")), + x_aux("az", "a", Some("second")), + // Conflict B: independent of A -- `b` forced to two incompatible units. + x_aux("b", "10", None), + x_aux("bp", "b", Some("gram")), + x_aux("bq", "b", Some("ampere")), + ]; + let model = x_model("main", vars); + let project_datamodel = x_project(sim_specs.clone(), &[model]); + + let mut result = InferenceResult::default(); + 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(), + &db, + sync.project, + |models, units_ctx, model| { + result = infer(models, units_ctx, model); + }, + ); + + // The clean chain is resolved despite conflicts elsewhere in the model. + let widget: UnitMap = crate::units::parse_units(&units_ctx, Some("widget")) + .unwrap() + .unwrap(); + assert_eq!( + result.resolved.get(&*canonicalize("clean_dst")), + Some(&widget), + "an unrelated dimensional conflict must not discard resolved units" + ); + + // Both independent conflicts are reported, not just the first one found. + assert!( + result.conflicts.len() >= 2, + "expected at least two independent conflicts, got {}: {:?}", + result.conflicts.len(), + result.conflicts + ); +} + +/// A Vensim macro can annotate a body variable's units with the macro's formal +/// parameter *names* (e.g. `~ xfrom` inside C-LEARN's `RAMP FROM TO`) -- a +/// symbolic, polymorphic unit, NOT a concrete base unit. Inference must treat +/// such a macro-body unit as polymorphic; otherwise the parameter name leaks as +/// a literal unit into every instantiation and conflicts with the real argument +/// units (the source of C-LEARN's `xfrom`/`xto` unit-error storm once #614 stops +/// the all-or-nothing behavior from masking it). +#[test] +fn test_macro_body_units_naming_parameters_are_polymorphic() { + let sim_specs = sim_specs_with_units("year"); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + + // A macro `scaleit(amount)` whose output is the parameter `amount`, with the + // output's units declared as the parameter name `amount` (the polymorphic + // idiom). Instantiated with a `widget` argument, the result must infer to + // `widget`, not conflict against a bogus `amount` base unit. + let macro_model = crate::datamodel::Model { + name: "scaleit".to_string(), + sim_specs: None, + variables: vec![ + x_aux("scaleit", "amount", Some("amount")), + x_aux("amount", "0", None), + ], + views: vec![], + loop_metadata: vec![], + groups: vec![], + macro_spec: Some(crate::datamodel::MacroSpec { + parameters: vec!["amount".to_string()], + primary_output: "scaleit".to_string(), + additional_outputs: vec![], + }), + }; + let root = x_model( + "main", + vec![ + x_aux("source", "10", Some("widget")), + x_aux("scaled", "scaleit(source)", None), + ], + ); + let project_datamodel = x_project(sim_specs.clone(), &[root, macro_model]); + + let mut result = InferenceResult::default(); + 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(), + &db, + sync.project, + |models, units_ctx, model| { + if model.name.as_str() == "main" { + result = infer(models, units_ctx, model); + } + }, + ); + + assert!( + result.conflicts.is_empty(), + "a macro body unit naming a parameter must be polymorphic, not leak as a literal unit; got conflicts: {:?}", + result.conflicts + ); + let widget: UnitMap = crate::units::parse_units(&units_ctx, Some("widget")) + .unwrap() + .unwrap(); + assert_eq!( + result.resolved.get(&*canonicalize("scaled")), + Some(&widget), + "the macro result should infer to the argument's units" + ); } pub(crate) fn infer( models: &HashMap, &ModelStage1>, units_ctx: &Context, model: &ModelStage1, -) -> UnitResult, UnitMap>> { +) -> InferenceResult { let time_units_name = canonicalize(units_ctx.sim_specs.time_units.as_deref().unwrap_or("time")).into_owned(); // Resolve through the alias map so the synthetic `time` variable's units @@ -1254,7 +1430,7 @@ fn test_previous_infers_units_from_lagged_arg() { let project_datamodel = x_project(sim_specs.clone(), &[model]); for _ in 0..64 { - let mut results: UnitResult, UnitMap>> = Ok(Default::default()); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1265,7 +1441,7 @@ fn test_previous_infers_units_from_lagged_arg() { results = infer(models, units_ctx, model); }, ); - let results = results.unwrap(); + let results = results.resolved; for (ident, expected_units) in expected.iter() { let expected_units: UnitMap = crate::units::parse_units(&units_ctx, Some(expected_units)) @@ -1311,12 +1487,7 @@ fn test_previous_constrains_fallback_units() { let project_datamodel = x_project(sim_specs, &[model]); for _ in 0..64 { - let mut results: UnitResult, UnitMap>> = - Err(UnitError::InferenceError { - code: ErrorCode::UnitMismatch, - sources: vec![], - details: None, - }); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1328,7 +1499,7 @@ fn test_previous_constrains_fallback_units() { }, ); assert!( - results.is_err(), + !results.conflicts.is_empty(), "PREVIOUS(widget, wallop) should fail unit inference" ); } @@ -1397,12 +1568,7 @@ fn test_multi_metavar_constraint_mismatch() { let model = x_model("main", vars); let project_datamodel = x_project(sim_specs.clone(), &[model]); - let mut results: UnitResult, UnitMap>> = - Err(UnitError::InferenceError { - code: ErrorCode::UnitMismatch, - sources: vec![], - details: None, - }); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1414,9 +1580,10 @@ fn test_multi_metavar_constraint_mismatch() { }, ); - // The inference should fail because x and y have inconsistent unit declarations + // The inference should report a conflict because x and y have inconsistent + // unit declarations. assert!( - results.is_err(), + !results.conflicts.is_empty(), "Should detect multi-metavar constraint mismatch" ); } @@ -1439,8 +1606,8 @@ fn test_find_constraint_mismatch_direct() { .cloned() .collect::(), )]; - let result = find_constraint_mismatch(&constraints); - assert!(result.is_some(), "Should detect direct concrete mismatch"); + let result = find_constraint_mismatches(&constraints); + assert!(!result.is_empty(), "Should detect direct concrete mismatch"); // Case 2: Pairwise mismatch with shared metavariables let constraints = vec![ @@ -1465,9 +1632,9 @@ fn test_find_constraint_mismatch_direct() { .collect::(), ), ]; - let result = find_constraint_mismatch(&constraints); + let result = find_constraint_mismatches(&constraints); assert!( - result.is_some(), + !result.is_empty(), "Should detect pairwise constraint mismatch" ); @@ -1494,10 +1661,10 @@ fn test_find_constraint_mismatch_direct() { .collect::(), ), ]; - let result = find_constraint_mismatch(&constraints); + let result = find_constraint_mismatches(&constraints); // The ratio of these two would be @a/@b * @d/@c which still has metavariables assert!( - result.is_none(), + result.is_empty(), "Should not detect mismatch for different metavar structures" ); @@ -1516,9 +1683,9 @@ fn test_find_constraint_mismatch_direct() { .collect::(), ), ]; - let result = find_constraint_mismatch(&constraints); + let result = find_constraint_mismatches(&constraints); assert!( - result.is_none(), + result.is_empty(), "Should not detect mismatch for purely under-constrained case" ); } @@ -1640,7 +1807,7 @@ fn test_rank_builtin_unit_inference() { let project_datamodel = x_project(sim_specs.clone(), &[model]); let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); - let mut results: UnitResult, UnitMap>> = Ok(Default::default()); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1652,7 +1819,7 @@ fn test_rank_builtin_unit_inference() { }, ); - let results = results.expect("RANK inference should succeed"); + let results = results.resolved; // `values` keeps its declared dollar units... let dollar: UnitMap = crate::units::parse_units(&units_ctx, Some("dollar")) @@ -1690,7 +1857,7 @@ fn test_unify_conflict_detection() { let model = x_model("main", model_vars); let project_datamodel = x_project(sim_specs.clone(), &[model]); - let mut results: UnitResult, UnitMap>> = Ok(Default::default()); + let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); let _project = crate::project::Project::from_salsa( @@ -1702,25 +1869,25 @@ fn test_unify_conflict_detection() { }, ); - // This should fail because 'a' can't be both meters and seconds + // This should report a conflict because 'a' can't be both meters and seconds. assert!( - results.is_err(), + !results.conflicts.is_empty(), "Should detect conflict when same variable has different unit constraints" ); - // Verify we get an InferenceError with the right code - match results { - Err(UnitError::InferenceError { code, sources, .. }) => { - assert_eq!(code, ErrorCode::UnitMismatch); - // Should have sources indicating which variables are involved - assert!( - !sources.is_empty(), - "Should have source information for conflict" - ); - } - Err(e) => panic!("Expected InferenceError, got {:?}", e), - Ok(_) => panic!("Expected error, got success"), - } + // Verify we get an InferenceError with the right code and source info. + let found = results.conflicts.iter().any(|conflict| { + matches!( + conflict, + UnitError::InferenceError { code, sources, .. } + if *code == ErrorCode::UnitMismatch && !sources.is_empty() + ) + }); + assert!( + found, + "expected an InferenceError with source information, got: {:?}", + results.conflicts + ); } #[test] From e0e8642b5cbb7e512ce2fd20b89d0c6b72a8532c Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Thu, 21 May 2026 22:31:15 -0700 Subject: [PATCH 4/7] engine: unit Context construction returns partial context Context::new accumulated unit-definition errors in a Vec but then threw away the fully-built context whenever that Vec was non-empty, returning Err; project_units_context then fell back to Default::default() -- an empty context with no units. So a single duplicate or contradictory unit declaration anywhere in a project lost ALL alias normalization (yr/year, person/people, model-defined equivalences) project-wide, re-creating a spurious unit-mismatch flood. This is the context-layer parallel of the inference all-or-nothing discard -- the long-standing units.rs:263 TODO, the same shape as GH #614. new/new_with_builtins now return (Context, Vec<...>): the context is always the one built from the valid declarations, with any conflicting declarations reported alongside it. project_units_context keeps the context and accumulates the errors as diagnostics. Callers that expected success take .0; the tests that exercise the conflict path assert on the returned error vec. --- src/simlin-engine/src/ast/mod.rs | 4 +- src/simlin-engine/src/db.rs | 42 ++++---- src/simlin-engine/src/db_analysis.rs | 2 +- src/simlin-engine/src/db_implicit_deps.rs | 2 +- src/simlin-engine/src/db_tests.rs | 2 +- src/simlin-engine/src/ltm_augment.rs | 4 +- src/simlin-engine/src/model.rs | 12 +-- src/simlin-engine/src/testutils.rs | 6 +- src/simlin-engine/src/units.rs | 122 +++++++++++++++------- src/simlin-engine/src/units_infer.rs | 12 +-- src/simlin-engine/src/variable.rs | 2 +- 11 files changed, 127 insertions(+), 83 deletions(-) diff --git a/src/simlin-engine/src/ast/mod.rs b/src/simlin-engine/src/ast/mod.rs index 52481e394..cf55f74f5 100644 --- a/src/simlin-engine/src/ast/mod.rs +++ b/src/simlin-engine/src/ast/mod.rs @@ -1188,7 +1188,7 @@ mod ast_tests { macro_spec: None, }; - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let model_s0 = ModelStage0::new( &model_datamodel, std::slice::from_ref(&dim), @@ -1278,7 +1278,7 @@ mod ast_tests { macro_spec: None, }; - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let model_s0 = ModelStage0::new( &model_datamodel, &[dim1.clone(), dim2.clone()], diff --git a/src/simlin-engine/src/db.rs b/src/simlin-engine/src/db.rs index b48e22c5a..520f7d20c 100644 --- a/src/simlin-engine/src/db.rs +++ b/src/simlin-engine/src/db.rs @@ -750,28 +750,30 @@ impl std::fmt::Debug for ParsedVariableResult { pub fn project_units_context(db: &dyn Db, project: SourceProject) -> crate::units::Context { let dm_units = source_units_to_datamodel(project.units(db)); let dm_sim_specs = source_sim_specs_to_datamodel(project.sim_specs(db)); - match crate::units::Context::new_with_builtins(&dm_units, &dm_sim_specs) { - Ok(ctx) => ctx, - 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 eq_err in eq_errors { - CompilationDiagnostic(Diagnostic { - model: String::new(), - variable: Some(unit_name.clone()), - error: DiagnosticError::Unit(crate::common::UnitError::DefinitionError( - eq_err.clone(), - None, - )), - severity: DiagnosticSeverity::Error, - }) - .accumulate(db); - } - } - Default::default() + // Construction is partial: keep the context built from the valid unit + // declarations and surface each conflicting/duplicate declaration as a + // project-level diagnostic, rather than discarding every unit definition on + // the first conflict. An empty context would lose all alias normalization + // project-wide (yr/year, person/people, model-defined equivalences) and + // re-create a spurious unit-mismatch flood -- the context-layer parallel of + // the inference partial-results fix (GH #614). + let (ctx, unit_parse_errors) = + crate::units::Context::new_with_builtins(&dm_units, &dm_sim_specs); + for (unit_name, eq_errors) in &unit_parse_errors { + for eq_err in eq_errors { + CompilationDiagnostic(Diagnostic { + model: String::new(), + variable: Some(unit_name.clone()), + error: DiagnosticError::Unit(crate::common::UnitError::DefinitionError( + eq_err.clone(), + None, + )), + severity: DiagnosticSeverity::Error, + }) + .accumulate(db); } } + ctx } /// Cached datamodel dimensions -- computed once per project. diff --git a/src/simlin-engine/src/db_analysis.rs b/src/simlin-engine/src/db_analysis.rs index efeef41d8..f1743fbeb 100644 --- a/src/simlin-engine/src/db_analysis.rs +++ b/src/simlin-engine/src/db_analysis.rs @@ -2244,7 +2244,7 @@ fn reconstruct_implicit_variable( }; } - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap_or_default(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut dummy_implicits = Vec::new(); let parsed_imp = crate::variable::parse_var( dims, diff --git a/src/simlin-engine/src/db_implicit_deps.rs b/src/simlin-engine/src/db_implicit_deps.rs index 10f54a6ae..c70ca8e54 100644 --- a/src/simlin-engine/src/db_implicit_deps.rs +++ b/src/simlin-engine/src/db_implicit_deps.rs @@ -33,7 +33,7 @@ pub(super) fn extract_implicit_var_deps( return Vec::new(); } - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap_or_default(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let converted_dims: Vec = dims .iter() .map(crate::dimensions::Dimension::from) diff --git a/src/simlin-engine/src/db_tests.rs b/src/simlin-engine/src/db_tests.rs index 2203d4041..a49f2f6da 100644 --- a/src/simlin-engine/src/db_tests.rs +++ b/src/simlin-engine/src/db_tests.rs @@ -607,7 +607,7 @@ fn test_parse_source_variable_matches_direct_parse() { // Parse directly via parse_var for comparison let dm_var = &project.models[0].variables[0]; - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars = Vec::new(); let direct_result = parse_var( &project.dimensions, diff --git a/src/simlin-engine/src/ltm_augment.rs b/src/simlin-engine/src/ltm_augment.rs index d3682602b..3511e5908 100644 --- a/src/simlin-engine/src/ltm_augment.rs +++ b/src/simlin-engine/src/ltm_augment.rs @@ -4297,7 +4297,7 @@ mod tests { }) }; - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars = Vec::new(); let stage0 = crate::variable::parse_var::( dims, @@ -4568,7 +4568,7 @@ mod tests { // ApplyToAll target. let dims = vec![region_dm_dimension()]; - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit = Vec::new(); let a2a_dm = crate::datamodel::Variable::Aux(crate::datamodel::Aux { ident: "a2a_target".to_string(), diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 8eb044985..6e285b2e7 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -1522,7 +1522,7 @@ fn test_module_parse() { ); let mut implicit_vars: Vec = Vec::new(); - let units_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let owned_models: HashMap, ModelStage0> = vec![ ("main".to_string(), &main_model), @@ -1553,7 +1553,7 @@ fn test_module_parse() { #[test] fn test_errors() { - let units_ctx = Context::new(&[], &Default::default()).unwrap(); + let units_ctx = Context::new(&[], &Default::default()).0; let main_model = x_model( "main", vec![x_aux("aux_3", "unknown_variable * 3.14", None)], @@ -1608,7 +1608,7 @@ fn test_errors() { #[test] fn test_new_cached_preserves_previous_helper_rewrite() { - let units_ctx = Context::new(&[], &Default::default()).unwrap(); + let units_ctx = Context::new(&[], &Default::default()).0; let main_model = x_model( "main", vec![ @@ -1722,7 +1722,7 @@ fn test_init_expression_vm() { #[test] fn test_previous_module_input_var_uses_helper_rewrite() { - let units_ctx = Context::new(&[], &Default::default()).unwrap(); + let units_ctx = Context::new(&[], &Default::default()).0; let module_input = datamodel::Variable::Aux(datamodel::Aux { ident: "input".to_string(), equation: datamodel::Equation::Scalar("0".to_string()), @@ -1990,7 +1990,7 @@ fn test_all_deps() { x_aux("aux_4", "mod_1.output", None), ], ); - let units_ctx = Context::new(&[], &Default::default()).unwrap(); + let units_ctx = Context::new(&[], &Default::default()).0; let owned_x_models: HashMap, ModelStage0> = vec![ ("mod_1".to_owned(), &mod_1_model), ("main".to_owned(), &main_model), @@ -2039,7 +2039,7 @@ fn test_all_deps() { }; let mut implicit_vars: Vec = Vec::new(); - let unit_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let unit_ctx = crate::units::Context::new(&[], &Default::default()).0; let mod_1_orig = &main_model.variables[0]; assert_eq!("mod_1", mod_1_orig.get_ident()); let mod_1 = parse_var(&[], mod_1_orig, &mut implicit_vars, &unit_ctx, |mi| { diff --git a/src/simlin-engine/src/testutils.rs b/src/simlin-engine/src/testutils.rs index ae64878d2..c9615e12d 100644 --- a/src/simlin-engine/src/testutils.rs +++ b/src/simlin-engine/src/testutils.rs @@ -30,7 +30,7 @@ pub(crate) fn x_aux(ident: &str, eqn: &str, units: Option<&str>) -> datamodel::V pub(crate) fn aux(ident: &str, eqn: &str) -> Variable { let var = x_aux(ident, eqn, None); let mut implicit_vars: Vec = Vec::new(); - let unit_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let unit_ctx = crate::units::Context::new(&[], &Default::default()).0; let var = parse_var(&[], &var, &mut implicit_vars, &unit_ctx, |mi| { Ok(Some(mi.clone())) }); @@ -70,7 +70,7 @@ pub(crate) fn x_stock( pub(crate) fn stock(ident: &str, eqn: &str, inflows: &[&str], outflows: &[&str]) -> Variable { let var = x_stock(ident, eqn, inflows, outflows, None); let mut implicit_vars: Vec = Vec::new(); - let unit_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let unit_ctx = crate::units::Context::new(&[], &Default::default()).0; let var = parse_var(&[], &var, &mut implicit_vars, &unit_ctx, |mi| { Ok(Some(mi.clone())) }); @@ -159,7 +159,7 @@ pub(crate) fn x_flow(ident: &str, eqn: &str, units: Option<&str>) -> datamodel:: pub(crate) fn flow(ident: &str, eqn: &str) -> Variable { let var = x_flow(ident, eqn, None); let mut implicit_vars: Vec = Vec::new(); - let unit_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let unit_ctx = crate::units::Context::new(&[], &Default::default()).0; let var = parse_var(&[], &var, &mut implicit_vars, &unit_ctx, |mi| { Ok(Some(mi.clone())) }); diff --git a/src/simlin-engine/src/units.rs b/src/simlin-engine/src/units.rs index f458b458b..f972df255 100644 --- a/src/simlin-engine/src/units.rs +++ b/src/simlin-engine/src/units.rs @@ -75,7 +75,7 @@ impl Context { pub fn new_with_builtins( units: &[Unit], sim_specs: &SimSpecs, - ) -> StdResult)>> { + ) -> (Self, Vec<(String, Vec)>) { // Built-in unit equivalences for common singular/plural variations. // These ensure that "person" and "people" (etc.) are treated as the same unit. // We only add a built-in if the model doesn't already define a unit with that name. @@ -126,10 +126,7 @@ impl Context { Self::new(&combined_units, sim_specs) } - pub fn new( - units: &[Unit], - sim_specs: &SimSpecs, - ) -> StdResult)>> { + pub fn new(units: &[Unit], sim_specs: &SimSpecs) -> (Self, Vec<(String, Vec)>) { let mut unit_errors: Vec<(String, Vec)> = Vec::new(); // Vensim's MDL files routinely repeat the same `22:` unit-equivalence @@ -260,18 +257,14 @@ impl Context { } } - // TODO: we shouldn't discard the whole context if there are errors - if unit_errors.is_empty() { - Ok(ctx) - } else { - // for (id, errors) in unit_errors.iter() { - // eprintln!("unit errors for '{}'", id); - // for err in errors.iter() { - // eprintln!(" {}", err); - // } - // } - Err(unit_errors) - } + // Construction is partial: always return the context we built, with any + // conflicting/duplicate declarations reported alongside it. A single bad + // unit declaration must not discard every other (valid) unit -- callers + // surface the errors as diagnostics and keep using the context. This is + // the context-layer parallel of the inference partial-results fix + // (GH #614): an empty context would lose all alias normalization + // (yr/year, person/people) and re-create a spurious mismatch flood. + (ctx, unit_errors) } pub(crate) fn lookup(&self, ident: &str) -> Option<&UnitMap> { @@ -457,7 +450,7 @@ fn self_aliased_prime_unit_is_registered() { // `lookup` returned None and callers fell back to whatever (un-normalized) // name they queried with -- the source of the C-LEARN `year` vs `yr` // mismatch flood. - let ctx = Context::new( + let (ctx, errors) = Context::new( &[Unit { name: "Yr".to_owned(), equation: None, @@ -471,8 +464,11 @@ fn self_aliased_prime_unit_is_registered() { ], }], &Default::default(), - ) - .expect("a self-aliased unit group must not produce a DuplicateUnit error"); + ); + assert!( + errors.is_empty(), + "a self-aliased unit group must not produce a DuplicateUnit error" + ); let yr = ctx.lookup("yr"); let year = ctx.lookup("year"); @@ -486,6 +482,55 @@ fn self_aliased_prime_unit_is_registered() { ); } +/// Context construction is partial: one conflicting unit declaration must not +/// discard the whole context. The unrelated valid units (and their alias +/// normalization) still resolve, and the conflict is reported alongside them +/// rather than throwing every definition away. This is the context-layer +/// parallel of the inference all-or-nothing fix (GH #614); it closes the +/// long-standing "we shouldn't discard the whole context if there are errors" +/// TODO. +#[test] +fn context_construction_keeps_valid_units_despite_a_conflict() { + let units = [ + // A valid alias group: `yr` normalizes to `year`. + Unit { + name: "year".to_owned(), + equation: None, + disabled: false, + aliases: vec!["yr".to_owned()], + }, + // A genuine conflict: `m` is claimed as an alias of two different units. + Unit { + name: "meter".to_owned(), + equation: None, + disabled: false, + aliases: vec!["m".to_owned()], + }, + Unit { + name: "second".to_owned(), + equation: None, + disabled: false, + aliases: vec!["m".to_owned()], + }, + ]; + let (ctx, errors) = Context::new(&units, &Default::default()); + + assert!( + !errors.is_empty(), + "the conflicting `m` alias must be reported" + ); + // ...but the unrelated valid alias group still resolves. + assert!( + ctx.lookup("yr").is_some(), + "valid units must survive a conflict elsewhere in the unit list" + ); + assert_eq!( + ctx.lookup("yr"), + ctx.lookup("year"), + "`yr` must still normalize to `year` despite the conflict" + ); +} + #[test] fn test_pretty_print_unit() { let context = Context::new( @@ -517,7 +562,7 @@ fn test_pretty_print_unit() { ], &Default::default(), ) - .unwrap(); + .0; let positive_cases: &[(&str, &str); 9] = &[ ("m^2/s", "meter^2/second"), @@ -584,10 +629,7 @@ fn test_context_creation() { .collect(), }; - assert_eq!( - expected, - Context::new(simple_units, &Default::default()).unwrap() - ); + assert_eq!(expected, Context::new(simple_units, &Default::default()).0); let more_units = &[ Unit { @@ -625,10 +667,7 @@ fn test_context_creation() { .collect(), }; - assert_eq!( - expected2, - Context::new(more_units, &Default::default()).unwrap() - ); + assert_eq!(expected2, Context::new(more_units, &Default::default()).0); } #[test] @@ -662,7 +701,7 @@ fn test_basic_unit_parsing() { ], &Default::default(), ) - .unwrap(); + .0; let positive_cases: &[(&str, UnitMap); 6] = &[ ( @@ -738,7 +777,7 @@ fn test_basic_unit_checks() { ], &Default::default(), ) - .unwrap(); + .0; // from a set of datamodel::Units build a Context // with a context, check if a set of variables unit checks @@ -806,7 +845,7 @@ fn test_unit_canonicalization() { ], &Default::default(), ) - .unwrap(); + .0; // All of these should resolve to the same canonical unit let test_cases = &["meter", "Meter", "METER", "m", "M", "meters", "METERS"]; @@ -848,7 +887,7 @@ fn test_unit_canonicalization() { fn test_year_years_builtin_alias() { // Test that the year/years builtin alias works correctly // This tests the `new_with_builtins` path which adds built-in aliases - let context = Context::new_with_builtins(&[], &Default::default()).unwrap(); + let context = Context::new_with_builtins(&[], &Default::default()).0; // Test singular form let expr = Expr0::new("year", LexerType::Units).unwrap().unwrap(); @@ -902,7 +941,7 @@ fn test_year_years_builtin_alias() { #[test] fn test_builtin_dollar_equivalences() { - let context = Context::new_with_builtins(&[], &Default::default()).unwrap(); + let context = Context::new_with_builtins(&[], &Default::default()).0; let expected: UnitMap = [("$".to_owned(), 1)].iter().cloned().collect(); @@ -933,7 +972,7 @@ fn test_builtin_dollar_equivalences() { #[test] fn test_builtin_unit_equivalences() { - let context = Context::new_with_builtins(&[], &Default::default()).unwrap(); + let context = Context::new_with_builtins(&[], &Default::default()).0; let expected: UnitMap = [("unit".to_owned(), 1)].iter().cloned().collect(); @@ -966,8 +1005,11 @@ fn test_redundant_duplicate_unit_declarations_are_benign() { aliases: vec!["Resource units".to_string()], }, ]; - let context = Context::new_with_builtins(&units, &Default::default()) - .expect("duplicate identical unit declarations must be tolerated; got an error"); + let (context, errors) = Context::new_with_builtins(&units, &Default::default()); + assert!( + errors.is_empty(), + "duplicate identical unit declarations must be tolerated; got an error" + ); // Both spellings must still resolve through the alias to the same canonical unit. let expr = Expr0::new("Resource_unit", LexerType::Units) @@ -1002,9 +1044,9 @@ fn test_conflicting_unit_declarations_are_still_errors() { aliases: vec!["FB".to_string()], // same alias mapped to a different primary }, ]; - let result = Context::new(&units, &Default::default()); + let (_ctx, errors) = Context::new(&units, &Default::default()); assert!( - result.is_err(), + !errors.is_empty(), "conflicting alias declarations must still produce an error" ); } @@ -1021,7 +1063,7 @@ fn debug_user_alias_with_underscore_identifiers() { disabled: false, aliases: vec!["Resource units".to_string()], }]; - let context = Context::new_with_builtins(&units, &Default::default()).unwrap(); + let context = Context::new_with_builtins(&units, &Default::default()).0; let expr = Expr0::new("Resource_unit", LexerType::Units) .unwrap() diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index fac869fd2..34e77b25f 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -975,7 +975,7 @@ impl UnitInferer<'_> { #[test] fn test_inference() { let sim_specs = sim_specs_with_units("parsec"); - let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).0; // test cases where we should be able to infer all units let test_cases: &[&[(crate::datamodel::Variable, &'static str)]] = &[ @@ -1076,7 +1076,7 @@ fn test_inference() { #[test] fn test_declared_units_without_equation_propagate() { let sim_specs = sim_specs_with_units("parsec"); - let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).0; // `base` has declared units but an empty equation (so `ast()` is None). // `derived = base` has no declared units; inference should propagate @@ -1247,7 +1247,7 @@ fn test_inference_error_has_location() { #[test] fn test_inference_partial_results_survive_conflict() { let sim_specs = sim_specs_with_units("year"); - let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).0; let vars = vec![ // A clean, independently-inferrable chain. @@ -1306,7 +1306,7 @@ fn test_inference_partial_results_survive_conflict() { #[test] fn test_macro_body_units_naming_parameters_are_polymorphic() { let sim_specs = sim_specs_with_units("year"); - let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).0; // A macro `scaleit(amount)` whose output is the parameter `amount`, with the // output's units declared as the parameter name `amount` (the polymorphic @@ -1407,7 +1407,7 @@ pub(crate) fn infer( #[test] fn test_previous_infers_units_from_lagged_arg() { let sim_specs = sim_specs_with_units("parsec"); - let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).0; // position has explicit units "widget". prev_pos has no declared // units; inference should propagate "widget" from position @@ -1806,7 +1806,7 @@ fn test_rank_builtin_unit_inference() { let model = x_model("main", vars); let project_datamodel = x_project(sim_specs.clone(), &[model]); - let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).0; let mut results = InferenceResult::default(); let db = crate::db::SimlinDb::default(); let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); diff --git a/src/simlin-engine/src/variable.rs b/src/simlin-engine/src/variable.rs index 0d133c704..d2230a9bb 100644 --- a/src/simlin-engine/src/variable.rs +++ b/src/simlin-engine/src/variable.rs @@ -1964,7 +1964,7 @@ fn test_tables() { } let mut implicit_vars: Vec = Vec::new(); - let unit_ctx = crate::units::Context::new(&[], &Default::default()).unwrap(); + let unit_ctx = crate::units::Context::new(&[], &Default::default()).0; let output = parse_var(&[], &input, &mut implicit_vars, &unit_ctx, |mi| { Ok(Some(mi.clone())) }); From 16d00b670c027ec09994cefeb23d868de4a9228a Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Thu, 21 May 2026 22:35:52 -0700 Subject: [PATCH 5/7] doc: document unit-inference partial-result discipline Update the engine CLAUDE.md Unit analysis section and the Last-updated note to describe the partial-result contracts: infer -> InferenceResult, Context::new -> (Context, errors), gen_constraints total, RANK dimensionless, and the is_macro polymorphic-units skip. --- src/simlin-engine/CLAUDE.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index 46c54dc6d..a51188a93 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -2,7 +2,7 @@ Core simulation engine for system dynamics models. Compiles, type-checks, unit-checks, and simulates SD models. See the root `CLAUDE.md` for full development guidelines; this file maps where functionality lives. -**Last updated: 2026-05-21 (#606: a standalone lookup-only variable -- a graphical-function holder with no functional input -- is now a non-value-bearing **static table** (`Variable::Var::is_table_only` / `db::source_var_is_table_only`), NOT a runtime variable: excluded from the runlist and the saved output (produces no series), its data reached only via `LOOKUP(table, x)` call sites. A table reference is kept off the data-flow dependency graph by a dedicated `builtins::BuiltinContents::LookupTable` walk variant; `referenced_tables` on `VariableDeps`/`ImplicitVarDeps` re-supplies the fragment compiler's metadata + tables map. A bare reference (no argument) is a compile error (`ErrorCode::LookupReferencedWithoutArgument`, emitted in `db_var_fragment::lower_var_fragment`). The MDL importer emits the canonical empty-equation form; the `"0+0"` `LOOKUP_SENTINEL` is still ACCEPTED on read (now produced only for empty-RHS vars). The shared lookup-only predicate moved to `src/variable.rs`. Retired the `gf(Time)` lowering (`lookup_only_index_expr`, `LookupOnlyLayout`). C-LEARN `EXPECTED_VDF_RESIDUAL` shrank 13->4. SUPERSEDES the #590 `gf(Time)` primitive (1) below.) Earlier (2026-05-20): C-LEARN residual closure (#590/#591), as five general Vensim import/simulation primitives, not model-specific patches: (1) a standalone lookup-only variable -- a graphical-function holder with no functional input -- lowers uniformly to `gf(Time)` across scalar/A2A/arrayed shapes (`src/compiler/mod.rs`: `var_is_lookup_only`/`is_lookup_only`/`lookup_only_index_expr` + `LookupOnlyLayout { PerElement, Shared }`); (2) a genuine passthrough macro `:MACRO: INIT(x) = INITIAL(x)` collapses to the builtin opcode at the call site (`module_functions.rs`: `classify_passthrough` + a `passthrough: Option` field on `ModuleFunctionDescriptor`, classified at `MacroRegistry::build`), instead of expanding a buggy per-element synthetic module; (3) the import-time XMILE formatter no longer linearizes a shadowed `RAMP FROM TO` macro -- `xmile_compat.rs::format_call_ctx` carries a macro-shadowing audit and the `ramp from to` restructuring arm was removed so the call survives as `RAMP_FROM_TO(...)` and resolves through the macro path; (4) the simulation **initials runlist is now deterministic** (`db_dep_graph.rs` sorts the init set before `topo_sort_str`, GH #595) and the dt stock-submodel-output chain-break applies to ALL readers (parity with the legacy `model.rs::module_output_deps` gate); (5) the VDF reader re-binds a **standalone** graphical-function descriptor to its forward-link output OT (`record_results.rs::standalone_descriptor_rebinds`). New `#[cfg(test)] lookup_only_tests.rs`; the C-LEARN `EXPECTED_VDF_RESIDUAL` carve-out (`tests/simulate.rs`) is now an exact, taxonomy-attributed remainder pinned by `clearn_residual_exactness`. simlin-cli now resolves `GET DIRECT *` external data via a `FilesystemDataProvider` -- see its CLAUDE.md.) Earlier: Element-level cycle resolution + genuine-Vensim VECTOR ELM MAP/SORT ORDER + `:NA:` sentinel, the work that made C-LEARN compile via the incremental path, run to FINAL TIME, and match genuine Vensim (`Ref.vdf`) within the 1% cross-simulator tolerance on the matched floor. The whole-variable `model_dependency_graph` cycle gate now refines a recurrence SCC to an element-acyclic verdict over a cross-member-comparable symbolic `SymVarRef` element graph (`db_dep_graph.rs`: `resolve_recurrence_sccs`/`refine_scc_to_element_verdict`/`symbolic_phase_element_order`, GH #575); a resolved SCC's per-element symbolic segments are interleaved into one combined fragment along the SCC's `element_order` and injected at `assemble_module` (`db.rs`: `combine_scc_fragment`/`var_phase_symbolic_fragment_prod`). Per-variable lowering moved to a new sibling module `db_var_fragment.rs` (`lower_var_fragment`). `crate::float::NA` is the finite Vensim `:NA:` sentinel (`-2^109`, NOT IEEE NaN); both `:NA:` paths route to it. New top-level VM-adjacent modules `vm_vector_sort_order.rs` (arrayed VECTOR SORT ORDER, per-iterated-slice 0-based ranks, #585) and `vm_vector_elm_map.rs` (base+full-source, OOB→NaN, no modulo). New `Opcode::LookupArray` (per-element arrayed-GF apply → array view, #580); `src/compiler/symbolic.rs` gains cross-fragment GF de-duplication (`GfDedup`, #582) and `TempStrategy { Recycle, Sum }`. Per-element graphical-function tables now lay out by element-name → declared dimension index (`variable.rs::reorder_arrayed_element_tables`, `db.rs::extract_tables_from_source_var`), not `Equation::Arrayed` Vec position. New tests: `db_dep_graph_tests.rs`, `db_combined_fragment_tests.rs`, `per_element_gf_tests.rs`; `tests/simulate.rs` `simulates_clearn` is un-stubbed (`#[ignore]` for runtime only) with a hardened `ensure_vdf_results` comparator + `EXPECTED_VDF_RESIDUAL` carve-out.) Earlier: Vensim macro support, Phases 1-7 complete. `:MACRO:`/`` definitions import as macro-marked `datamodel::Model`s (`Model.macro_spec: Option`, persisted through protobuf/JSON/schema); single-output macros inline through `BuiltinVisitor`, multi-output (`:`-list) ones materialize at import. New top-level modules: `module_functions.rs` (the unified `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator for stdlib functions *and* macros, the shared `is_renamed_*` collision predicates) and `db_macro_registry.rs` (the `project_macro_registry` salsa query + sync-time `macro_registry_build_error`); `SourceProject` gains `macro_registry_build_error`, `SourceModel` gains `macro_spec`; `ErrorCode::DuplicateMacroName`; new `tests/metasd_macros.rs` (gated on `file_io`). LTM arrays hardening, Phases 1-8 complete. Phase 7: #502 per-element graphical-function static link polarity -- when an arrayed source feeds an arrayed graphical-function target, `lookup_table_polarity` folds the per-element `tables` list on `Variable::Var` into one link polarity, falling back to `Unknown` for the multi-dim case; #492 the GF strict-monotonicity check uses a y-range-relative epsilon (`max(EPSILON, range_rel * (y_max - y_min))`) so numeric-import noise no longer flips a monotone lookup table to `Unknown`. Phase 6: #483 analytic STDDEV ceteris-paribus partial -- `generate_nonlinear_partial` builds the unrolled population-variance `sqrt` formula for STDDEV (divisor `N`, matching `vm.rs::Opcode::ArrayStddev`) instead of the delta-ratio stand-in; RANK keeps the delta-ratio (an order statistic, unreachable via real models since it returns an array) with a documented justification, pinned by `test_generate_rank_keeps_delta_ratio`. Phase 5: #515 budgeted cross-element-through-aggregate loop recovery -- `recover_cross_agg_loops` drops the old `MAX_AGG_PETALS = 8` hard drop for a deterministic petal priority + a threaded `agg_loop_budget` loop-count budget (`MAX_CROSS_AGG_LOOPS = 256`, `#[cfg(test)]`-overridable via `AggLoopBudgetGuard`; `MAX_AGG_PETALS` survives as a soft per-agg petal cap), surfaces truncation on `LtmVariablesResult.agg_recovery_truncated` + a `Warning`, and enumerates each disjoint petal subset's distinct *cyclic orderings* (`cyclic_orderings(m)` -- (m-1)!/2 for m≥3, mirror reversals skipped, via Heap's algorithm) instead of one ordering per subset. Phase 4 (2026-05-12): #514 sliced-reducer hoisting -- `AggNode.read_slice`, read-slice-driven element graph / link scores, dynamic-index carve-out reclassified as `DynamicIndex`; arrayed synthetic aggs route through agg-half link-score emitters with subscripted agg names and a subscripted Δsource denominator in the diagonal case (strict-prefix broadcast over-subscribes -- GH #528); mapped-dimension sliced reducers stay conservative; a scalar feeder of a hoisted reducer emits a bare element-graph node. Phases 1-3: the `model_ltm_reference_sites` classification IR (`db_ltm_ir.rs`), the consolidated `reducer_kind`/`ReducerKind` table in `ltm_agg.rs`, element-level A2A `Loop::stocks` + per-slot `loop_partitions` (#487), iterated-dimension subscripts ⇒ `Bare` (#511), disjoint-dim arrayed→arrayed per-source-element link scores + the unscoreable-edge `Warning` (#510).)** +**Last updated: 2026-05-21 (unit-inference robustness, GH #614 + the `units.rs:263` TODO: the unit subsystem no longer fails all-or-nothing. `units_infer::infer` returns `InferenceResult { resolved, conflicts }` -- a dimensional conflict no longer discards the units already resolved (a contradiction is confined to its connected component, since substitution only flows along shared metavariables), and `find_constraint_mismatches` collects every residual contradiction rather than the first; `db_units::check_model_units` keeps the resolved units (so the rest of the model is still checked) and surfaces conflicts as ONE umbrella warning, not one-per-conflict. `gen_constraints` is now total (returns `Units`, not a vestigial always-`Ok` `UnitResult`; removed the `.unwrap()` panic landmine and the `None`-arm gap that dropped declared-units propagation for equation-less variables). `units::Context::new`/`new_with_builtins` return `(Context, Vec)` instead of discarding the whole built context on the first duplicate/conflicting declaration (an empty context lost project-wide alias normalization). RANK results are dimensionless in both `units_infer` and `units_check` (an ordinal index, not the ranked array's units). `ModelStage0`/`ModelStage1` gain `is_macro`: a Vensim macro's body-variable units may name the formal parameters (`~ xfrom` inside RAMP FROM TO -- a polymorphic unit, not a base unit), so inference skips declared-units constraints for macro bodies; without this, keeping the resolved map re-floods C-LEARN (the `xfrom`/`xto` leak), with it C-LEARN holds its documented 14-diagnostic residual. New tests in `units_infer.rs` (partial-results-survive-conflict, declared-units-without-equation propagation, macro-polymorphic-units, RANK dimensionless), `units.rs` (partial-context), `unit_checking_test.rs` (RANK-in-checking).) Earlier 2026-05-21 (#606: a standalone lookup-only variable -- a graphical-function holder with no functional input -- is now a non-value-bearing **static table** (`Variable::Var::is_table_only` / `db::source_var_is_table_only`), NOT a runtime variable: excluded from the runlist and the saved output (produces no series), its data reached only via `LOOKUP(table, x)` call sites. A table reference is kept off the data-flow dependency graph by a dedicated `builtins::BuiltinContents::LookupTable` walk variant; `referenced_tables` on `VariableDeps`/`ImplicitVarDeps` re-supplies the fragment compiler's metadata + tables map. A bare reference (no argument) is a compile error (`ErrorCode::LookupReferencedWithoutArgument`, emitted in `db_var_fragment::lower_var_fragment`). The MDL importer emits the canonical empty-equation form; the `"0+0"` `LOOKUP_SENTINEL` is still ACCEPTED on read (now produced only for empty-RHS vars). The shared lookup-only predicate moved to `src/variable.rs`. Retired the `gf(Time)` lowering (`lookup_only_index_expr`, `LookupOnlyLayout`). C-LEARN `EXPECTED_VDF_RESIDUAL` shrank 13->4. SUPERSEDES the #590 `gf(Time)` primitive (1) below.) Earlier (2026-05-20): C-LEARN residual closure (#590/#591), as five general Vensim import/simulation primitives, not model-specific patches: (1) a standalone lookup-only variable -- a graphical-function holder with no functional input -- lowers uniformly to `gf(Time)` across scalar/A2A/arrayed shapes (`src/compiler/mod.rs`: `var_is_lookup_only`/`is_lookup_only`/`lookup_only_index_expr` + `LookupOnlyLayout { PerElement, Shared }`); (2) a genuine passthrough macro `:MACRO: INIT(x) = INITIAL(x)` collapses to the builtin opcode at the call site (`module_functions.rs`: `classify_passthrough` + a `passthrough: Option` field on `ModuleFunctionDescriptor`, classified at `MacroRegistry::build`), instead of expanding a buggy per-element synthetic module; (3) the import-time XMILE formatter no longer linearizes a shadowed `RAMP FROM TO` macro -- `xmile_compat.rs::format_call_ctx` carries a macro-shadowing audit and the `ramp from to` restructuring arm was removed so the call survives as `RAMP_FROM_TO(...)` and resolves through the macro path; (4) the simulation **initials runlist is now deterministic** (`db_dep_graph.rs` sorts the init set before `topo_sort_str`, GH #595) and the dt stock-submodel-output chain-break applies to ALL readers (parity with the legacy `model.rs::module_output_deps` gate); (5) the VDF reader re-binds a **standalone** graphical-function descriptor to its forward-link output OT (`record_results.rs::standalone_descriptor_rebinds`). New `#[cfg(test)] lookup_only_tests.rs`; the C-LEARN `EXPECTED_VDF_RESIDUAL` carve-out (`tests/simulate.rs`) is now an exact, taxonomy-attributed remainder pinned by `clearn_residual_exactness`. simlin-cli now resolves `GET DIRECT *` external data via a `FilesystemDataProvider` -- see its CLAUDE.md.) Earlier: Element-level cycle resolution + genuine-Vensim VECTOR ELM MAP/SORT ORDER + `:NA:` sentinel, the work that made C-LEARN compile via the incremental path, run to FINAL TIME, and match genuine Vensim (`Ref.vdf`) within the 1% cross-simulator tolerance on the matched floor. The whole-variable `model_dependency_graph` cycle gate now refines a recurrence SCC to an element-acyclic verdict over a cross-member-comparable symbolic `SymVarRef` element graph (`db_dep_graph.rs`: `resolve_recurrence_sccs`/`refine_scc_to_element_verdict`/`symbolic_phase_element_order`, GH #575); a resolved SCC's per-element symbolic segments are interleaved into one combined fragment along the SCC's `element_order` and injected at `assemble_module` (`db.rs`: `combine_scc_fragment`/`var_phase_symbolic_fragment_prod`). Per-variable lowering moved to a new sibling module `db_var_fragment.rs` (`lower_var_fragment`). `crate::float::NA` is the finite Vensim `:NA:` sentinel (`-2^109`, NOT IEEE NaN); both `:NA:` paths route to it. New top-level VM-adjacent modules `vm_vector_sort_order.rs` (arrayed VECTOR SORT ORDER, per-iterated-slice 0-based ranks, #585) and `vm_vector_elm_map.rs` (base+full-source, OOB→NaN, no modulo). New `Opcode::LookupArray` (per-element arrayed-GF apply → array view, #580); `src/compiler/symbolic.rs` gains cross-fragment GF de-duplication (`GfDedup`, #582) and `TempStrategy { Recycle, Sum }`. Per-element graphical-function tables now lay out by element-name → declared dimension index (`variable.rs::reorder_arrayed_element_tables`, `db.rs::extract_tables_from_source_var`), not `Equation::Arrayed` Vec position. New tests: `db_dep_graph_tests.rs`, `db_combined_fragment_tests.rs`, `per_element_gf_tests.rs`; `tests/simulate.rs` `simulates_clearn` is un-stubbed (`#[ignore]` for runtime only) with a hardened `ensure_vdf_results` comparator + `EXPECTED_VDF_RESIDUAL` carve-out.) Earlier: Vensim macro support, Phases 1-7 complete. `:MACRO:`/`` definitions import as macro-marked `datamodel::Model`s (`Model.macro_spec: Option`, persisted through protobuf/JSON/schema); single-output macros inline through `BuiltinVisitor`, multi-output (`:`-list) ones materialize at import. New top-level modules: `module_functions.rs` (the unified `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator for stdlib functions *and* macros, the shared `is_renamed_*` collision predicates) and `db_macro_registry.rs` (the `project_macro_registry` salsa query + sync-time `macro_registry_build_error`); `SourceProject` gains `macro_registry_build_error`, `SourceModel` gains `macro_spec`; `ErrorCode::DuplicateMacroName`; new `tests/metasd_macros.rs` (gated on `file_io`). LTM arrays hardening, Phases 1-8 complete. Phase 7: #502 per-element graphical-function static link polarity -- when an arrayed source feeds an arrayed graphical-function target, `lookup_table_polarity` folds the per-element `tables` list on `Variable::Var` into one link polarity, falling back to `Unknown` for the multi-dim case; #492 the GF strict-monotonicity check uses a y-range-relative epsilon (`max(EPSILON, range_rel * (y_max - y_min))`) so numeric-import noise no longer flips a monotone lookup table to `Unknown`. Phase 6: #483 analytic STDDEV ceteris-paribus partial -- `generate_nonlinear_partial` builds the unrolled population-variance `sqrt` formula for STDDEV (divisor `N`, matching `vm.rs::Opcode::ArrayStddev`) instead of the delta-ratio stand-in; RANK keeps the delta-ratio (an order statistic, unreachable via real models since it returns an array) with a documented justification, pinned by `test_generate_rank_keeps_delta_ratio`. Phase 5: #515 budgeted cross-element-through-aggregate loop recovery -- `recover_cross_agg_loops` drops the old `MAX_AGG_PETALS = 8` hard drop for a deterministic petal priority + a threaded `agg_loop_budget` loop-count budget (`MAX_CROSS_AGG_LOOPS = 256`, `#[cfg(test)]`-overridable via `AggLoopBudgetGuard`; `MAX_AGG_PETALS` survives as a soft per-agg petal cap), surfaces truncation on `LtmVariablesResult.agg_recovery_truncated` + a `Warning`, and enumerates each disjoint petal subset's distinct *cyclic orderings* (`cyclic_orderings(m)` -- (m-1)!/2 for m≥3, mirror reversals skipped, via Heap's algorithm) instead of one ordering per subset. Phase 4 (2026-05-12): #514 sliced-reducer hoisting -- `AggNode.read_slice`, read-slice-driven element graph / link scores, dynamic-index carve-out reclassified as `DynamicIndex`; arrayed synthetic aggs route through agg-half link-score emitters with subscripted agg names and a subscripted Δsource denominator in the diagonal case (strict-prefix broadcast over-subscribes -- GH #528); mapped-dimension sliced reducers stay conservative; a scalar feeder of a hoisted reducer emits a bare element-graph node. Phases 1-3: the `model_ltm_reference_sites` classification IR (`db_ltm_ir.rs`), the consolidated `reducer_kind`/`ReducerKind` table in `ltm_agg.rs`, element-level A2A `Loop::stocks` + per-slot `loop_partitions` (#487), iterated-dimension subscripts ⇒ `Bare` (#511), disjoint-dim arrayed→arrayed per-source-element link scores + the unscoreable-edge `Warning` (#510).)** **Maintenance note**: Keep this file up to date when adding, removing, or reorganizing modules. @@ -85,9 +85,11 @@ The primary compilation path uses salsa tracked functions for fine-grained incre ## Unit analysis -- **`src/units.rs`** - Unit parsing and `UnitMap` representation -- **`src/units_check.rs`** - Dimensional consistency checking across equations -- **`src/units_infer.rs`** - Unit inference for variables without explicit declarations +The unit subsystem is partial-result throughout: a single bad declaration or one dimensional conflict never discards the rest (GH #614 and the former `units.rs:263` TODO). + +- **`src/units.rs`** - Unit parsing and `UnitMap` representation. `Context::new`/`new_with_builtins` return `(Context, Vec<(unit_name, errors)>)`: the context always holds every *valid* declaration, with conflicting/duplicate ones reported alongside -- never an empty context (which would lose project-wide alias normalization like yr/year and re-create a spurious mismatch flood). +- **`src/units_check.rs`** - Dimensional consistency checking across equations. A reference's units are `declared OR inferred`; unknown units are skipped (not an error). RANK is dimensionless (an ordinal index, not the ranked array's units). +- **`src/units_infer.rs`** - Hindley-Milner-style unit inference: constraint generation (`gen_constraints`, total -- returns `Units`, no fallible `Result`) + unification over the free abelian group of units (`unify`/`solve_for`/`substitute`). `infer` returns `InferenceResult { resolved, conflicts }` and keeps solving past a conflict (keeping the first binding -- a contradiction is confined to its connected component, since substitution only flows along shared metavariables), collecting *every* residual contradiction via `find_constraint_mismatches`. A macro body's declared units may name the formal parameters (a polymorphic Vensim idiom, e.g. `~ xfrom` inside RAMP FROM TO), so inference skips declared-units constraints for `is_macro` models (`ModelStage0`/`ModelStage1`) and lets those units be inferred from the equation + cross-module parameter bindings (mirroring `check_model_units`, which skips macro models); without this, keeping the resolved map re-floods C-LEARN with the `xfrom`/`xto` leak. ## Special features From 26dfffc9837968731a1754088a314f786225cca9 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Fri, 22 May 2026 06:45:06 -0700 Subject: [PATCH 6/7] engine: substitute only the accepted unit binding in unify Addresses the PR #618 review: unify substituted the newly-derived units into the remaining constraints BEFORE checking whether the metavariable already had a kept binding, so a conflicting re-derivation would have propagated the rejected units -- inconsistent with the documented keep- the-first-binding behavior. Move substitution into the first-resolution branch so only the accepted binding is ever propagated. This is a defensive correctness fix: the conflict arm is currently unreachable -- substitute removes the metavariable from every remaining constraint, so a metavariable is solved at most once and resolved_fvs never already contains it here; a probe across the full lib suite + C-LEARN + wrld3 confirmed zero hits. A genuine over-constraint surfaces instead as a residual concrete contradiction reported by find_constraint_mismatches. No behavior change on any reachable path; the reorder keeps keep-first correct if that invariant ever changes. --- src/simlin-engine/src/units_infer.rs | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index 34e77b25f..e3da13c28 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -899,10 +899,22 @@ impl UnitInferer<'_> { let var = var.to_owned(); let units = solve_for(&var, c.unit_map.clone()); let sources = c.sources.clone(); - pending.substitute(&var, &units, &sources); - finalized.substitute(&var, &units, &sources); let var_key = var.strip_prefix('@').unwrap(); let var_ident = Ident::::from_str_unchecked(var_key); + // Decide whether to accept this binding BEFORE substituting it, + // so a rejected (conflicting) re-derivation can never overwrite + // the kept binding in the remaining constraints -- substitution + // only ever propagates the binding we actually accept. + // + // The conflict arm is currently unreachable: `substitute` + // removes the metavariable from every remaining constraint, so a + // metavariable is solved at most once and `resolved_fvs` never + // already contains it here. A genuine over-constraint (the same + // metavariable forced to two different units) instead surfaces as + // a residual concrete contradiction (e.g. `meter == second`) + // reported by `find_constraint_mismatches`. The arm is kept, and + // ordered so it never substitutes the rejected units, so "keep + // the first binding" stays correct if that invariant ever changes. if let Some(existing_units) = resolved_fvs.get(&var_ident) { if *existing_units != units { let mut all_sources: Vec<(String, Option)> = @@ -915,9 +927,6 @@ impl UnitInferer<'_> { } } } - // Record the conflict but keep the first binding and - // keep solving: a contradiction for one metavariable - // must not discard the units resolved for the rest. conflicts.push(UnitError::InferenceError { code: ErrorCode::UnitMismatch, sources: all_sources, @@ -927,7 +936,11 @@ impl UnitInferer<'_> { )), }); } + // Keep the first binding either way: do NOT substitute the + // re-derived units into the remaining constraints. } else { + pending.substitute(&var, &units, &sources); + finalized.substitute(&var, &units, &sources); resolved_fvs.insert(var_ident.clone(), units); resolved_sources.insert(var_ident, sources); } From b8fd13ef3c7a802df8234c71bdd8873964fdb7b7 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Fri, 22 May 2026 06:57:30 -0700 Subject: [PATCH 7/7] engine: drop redundant conflict detection from unify The previous commit established that unify's in-loop conflict detection is unreachable: substitute removes a metavariable from every remaining constraint, so it is solved at most once and resolved_fvs never already contains it at that check. That detection was therefore dead and fully redundant with find_constraint_mismatches, which already reports every over-constraint as a residual concrete contradiction. Remove it for the simpler end state: unify returns (resolved, leftover) -- no conflicts vec, no resolved_sources -- and keeps the first binding via a vacant-entry guard that also never substitutes a rejected re-derivation. infer collects conflicts from find_constraint_mismatches alone (a single source now, so the dedup just drops identical findings). Behavior is unchanged on every reachable path; full suite + C-LEARN green, C-LEARN holds its documented 14-diagnostic residual. --- src/simlin-engine/src/units_infer.rs | 106 +++++++++------------------ 1 file changed, 34 insertions(+), 72 deletions(-) diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index e3da13c28..bc1f2d732 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -862,30 +862,25 @@ impl UnitInferer<'_> { } } - #[allow(clippy::type_complexity)] /// Solve the constraint system by Gaussian-elimination-style substitution, - /// returning the resolved metavariable units, the residual (still - /// metavariable-bearing) constraints, and any dimensional conflicts found - /// while solving. + /// returning the resolved metavariable units and the residual (still + /// metavariable-bearing) constraints. /// - /// A conflict is recorded and solving continues with the *first* binding - /// kept, rather than aborting (GH #614). Because substitution only ever - /// flows along shared metavariables, a contradiction is confined to its own - /// connected component of the constraint graph: keeping going can never - /// corrupt the units resolved for an independent component. + /// A metavariable is solved at most once: `substitute` removes it from every + /// remaining constraint (and the metavar index), so it can never reappear as + /// a single free variable. `unify` therefore does not detect conflicts + /// itself -- a genuine over-constraint (the same metavariable forced to two + /// different units) reduces to a residual concrete contradiction (e.g. + /// `meter == second`) that `find_constraint_mismatches` reports. The + /// vacant-entry guard keeps the first binding -- and never propagates a + /// rejected re-derivation -- if that solved-at-most-once invariant is ever + /// weakened (GH #614). #[allow(clippy::type_complexity)] fn unify( &self, constraints: Vec, - ) -> ( - HashMap, UnitMap>, - Vec, - Vec, - ) { + ) -> (HashMap, UnitMap>, Vec) { let mut resolved_fvs: HashMap, UnitMap> = HashMap::new(); - // Track sources for each resolved variable in case of conflict - let mut resolved_sources: HashMap, Vec> = HashMap::new(); - let mut conflicts: Vec = Vec::new(); let mut pending = ConstraintSet::from_vec(constraints); let mut finalized = ConstraintSet::default(); @@ -898,51 +893,19 @@ impl UnitInferer<'_> { if let Some(var) = single_fv(&c.unit_map) { let var = var.to_owned(); let units = solve_for(&var, c.unit_map.clone()); - let sources = c.sources.clone(); let var_key = var.strip_prefix('@').unwrap(); let var_ident = Ident::::from_str_unchecked(var_key); - // Decide whether to accept this binding BEFORE substituting it, - // so a rejected (conflicting) re-derivation can never overwrite - // the kept binding in the remaining constraints -- substitution - // only ever propagates the binding we actually accept. - // - // The conflict arm is currently unreachable: `substitute` - // removes the metavariable from every remaining constraint, so a - // metavariable is solved at most once and `resolved_fvs` never - // already contains it here. A genuine over-constraint (the same - // metavariable forced to two different units) instead surfaces as - // a residual concrete contradiction (e.g. `meter == second`) - // reported by `find_constraint_mismatches`. The arm is kept, and - // ordered so it never substitutes the rejected units, so "keep - // the first binding" stays correct if that invariant ever changes. - if let Some(existing_units) = resolved_fvs.get(&var_ident) { - if *existing_units != units { - let mut all_sources: Vec<(String, Option)> = - c.sources.iter().map(|s| (s.var.clone(), s.loc)).collect(); - if let Some(existing_sources) = resolved_sources.get(&var_ident) { - for s in existing_sources { - if !all_sources.iter().any(|(v, l)| v == &s.var && *l == s.loc) - { - all_sources.push((s.var.clone(), s.loc)); - } - } - } - conflicts.push(UnitError::InferenceError { - code: ErrorCode::UnitMismatch, - sources: all_sources, - details: Some(format!( - "conflicting units for {}: {} vs {}", - var, existing_units, units - )), - }); - } - // Keep the first binding either way: do NOT substitute the - // re-derived units into the remaining constraints. - } else { - pending.substitute(&var, &units, &sources); - finalized.substitute(&var, &units, &sources); - resolved_fvs.insert(var_ident.clone(), units); - resolved_sources.insert(var_ident, sources); + // Record the first (and, by the invariant above, only) binding + // for this metavariable and propagate it. The vacant-entry + // guard keeps the first binding -- and never substitutes a + // rejected re-derivation into the remaining constraints -- even + // if the solved-at-most-once invariant is ever weakened. + if let std::collections::hash_map::Entry::Vacant(e) = + resolved_fvs.entry(var_ident) + { + pending.substitute(&var, &units, &c.sources); + finalized.substitute(&var, &units, &c.sources); + e.insert(units); } } else { finalized.push(c); @@ -955,32 +918,31 @@ impl UnitInferer<'_> { } } - (resolved_fvs, finalized.into_vec(), conflicts) + (resolved_fvs, finalized.into_vec()) } fn infer(&self, model: &ModelStage1) -> InferenceResult { let mut constraints = vec![]; self.gen_all_constraints(model, "", &mut constraints); - let (resolved, leftover, mut conflicts) = self.unify(constraints); + let (resolved, leftover) = self.unify(constraints); // Leftover constraints that still contain metavariables just mean the // model is under-constrained (e.g. undeclared units) -- not an error. - // Only a concrete contradiction among them is a real mismatch. - conflicts.extend(find_constraint_mismatches(&leftover)); - - // The same contradiction can be reached both while solving and as a - // leftover constraint; drop exact duplicates so each is reported once. - let mut deduped: Vec = Vec::with_capacity(conflicts.len()); - for conflict in conflicts { - if !deduped.contains(&conflict) { - deduped.push(conflict); + // Only a concrete contradiction among them is a real mismatch; this is + // now the single source of conflicts, so we just dedup identical + // findings (the same contradiction can be reported from more than one + // residual constraint) to report each once. + let mut conflicts: Vec = Vec::new(); + for conflict in find_constraint_mismatches(&leftover) { + if !conflicts.contains(&conflict) { + conflicts.push(conflict); } } InferenceResult { resolved, - conflicts: deduped, + conflicts, } } }