Skip to content

engine: clear C-LEARN unit-error flood (481 -> 14) #615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bpowers merged 1 commit into from
May 22, 2026
Merged

engine: clear C-LEARN unit-error flood (481 -> 14) #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
379 changes: 4 additions & 375 deletions src/simlin-engine/src/db.rs
View file
Open in desktop

Large diffs are not rendered by default.

422 changes: 422 additions & 0 deletions src/simlin-engine/src/db_units.rs
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/simlin-engine/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ mod db_macro_registry;
// (`scripts/lint-project.sh` rule 2); `db.rs` reaches it via
// `crate::db_dep_graph::...`, mirroring `db_ltm_ir` / `db_macro_registry`.
mod db_dep_graph;
mod db_units;
mod db_var_fragment;
pub mod diagram;
mod dimensions;
Expand Down
37 changes: 37 additions & 0 deletions src/simlin-engine/src/macro_expansion_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1260,3 +1260,40 @@ sibling=
);
}
}

// ── unit checking: macro-marked models are templates, like stdlib ──────────

/// F4: a macro-marked model is a generic template -- its formal parameters are
/// unitless, so unit checking it in isolation produces only spurious errors.
/// Like a stdlib model, it must be SKIPPED by unit checking. Even a blatant
/// dimensional inconsistency in the macro body (here `meters + seconds`) must
/// not surface a diagnostic attributed to the macro model. (Macro correctness
/// is validated at instantiation via cross-module unit constraints; this is
/// the source of C-LEARN's spurious `ramp_from_to`/`sshape` unit warnings.)
#[test]
fn macro_body_units_are_not_checked() {
let source = mdl(r#":MACRO: BADUNITS(a, b)
BADUNITS = lhs + rhs
~ widgets
~ |
lhs = a
~ meters
~ |
rhs = b
~ seconds
~ |
:END OF MACRO:
y =
BADUNITS(3, 4)
~ widgets
~ |
"#);

let diags = diagnostics_for(&source);
let macro_diags: Vec<_> = diags.iter().filter(|d| d.model == "badunits").collect();
assert!(
macro_diags.is_empty(),
"unit checking must skip macro-marked models; diagnostics attributed \
to the `badunits` macro model:\n{macro_diags:#?}",
);
}
12 changes: 12 additions & 0 deletions src/simlin-engine/src/test_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,18 @@ impl TestProject {
self
}

/// Add a unit definition with alias names (a Vensim `22:` equivalence
/// group). The first name is the primary; the rest are aliases.
pub fn unit_with_aliases(mut self, name: &str, aliases: &[&str]) -> Self {
self.units.push(datamodel::Unit {
name: name.to_string(),
equation: None,
disabled: false,
aliases: aliases.iter().map(|s| s.to_string()).collect(),
});
self
}

/// Add an indexed dimension (e.g., for numeric indices)
pub fn indexed_dimension(mut self, name: &str, size: u32) -> Self {
self.dimensions
Expand Down
60 changes: 51 additions & 9 deletions src/simlin-engine/src/units.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,12 @@ impl Context {
// A primary name that is already an alias of *another* unit is a
// genuine conflict; a primary name that's already itself a prime
// unit is a benign re-declaration as long as the inferred unit
// map matches.
if let Some(target) = aliases.get(&unit_name) {
if target != &unit_name {
unit_errors.push(dup_err(&unit_name));
}
// map matches. A name that appears among its OWN aliases (a
// self-alias -- e.g. `22:Yr,...,yr,...` where both `Yr` and `yr`
// canonicalize to `yr`) is also benign: we must still register the
// prime unit so that the name and its aliases resolve via lookup.
if matches!(aliases.get(&unit_name), Some(target) if target != &unit_name) {
unit_errors.push(dup_err(&unit_name));
} else {
let new_map: UnitMap = [(unit_name.clone(), 1)].iter().cloned().collect();
match parsed_units.entry(unit_name.clone()) {
Expand Down Expand Up @@ -241,10 +242,10 @@ impl Context {
None => [(unit_name.clone(), 1)].iter().cloned().collect(),
};

if let Some(target) = ctx.aliases.get(&unit_name) {
if target != &unit_name {
unit_errors.push(dup_err(&unit_name));
}
// As in step 1: only an alias of *another* unit is a conflict; a
// self-alias is benign and the prime unit must still be registered.
if matches!(ctx.aliases.get(&unit_name), Some(target) if target != &unit_name) {
unit_errors.push(dup_err(&unit_name));
} else {
match ctx.units.entry(unit_name.clone()) {
Entry::Vacant(e) => {
Expand Down Expand Up @@ -444,6 +445,47 @@ pub fn parse_units(
}
}

#[test]
fn self_aliased_prime_unit_is_registered() {
// A Vensim `22:` equivalence group can list the canonical form of its own
// primary name among its aliases. C-LEARN declares
// `22:Yr,year,years,yr,Year,Years`: the primary `Yr` canonicalizes to `yr`,
// and `yr` ALSO appears in the alias list, so `yr` becomes an alias of
// itself. The prime unit must still be registered so that the primary
// name and every alias resolve to the same UnitMap. Before the fix the
// self-alias caused `Context::new` to skip registering the prime unit, so
// `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(
&[Unit {
name: "Yr".to_owned(),
equation: None,
disabled: false,
aliases: vec![
"year".to_owned(),
"years".to_owned(),
"yr".to_owned(),
"Year".to_owned(),
"Years".to_owned(),
],
}],
&Default::default(),
)
.expect("a self-aliased unit group must not produce a DuplicateUnit error");

let yr = ctx.lookup("yr");
let year = ctx.lookup("year");
assert!(
yr.is_some(),
"lookup(\"yr\") must resolve the registered prime unit, got None"
);
assert_eq!(
yr, year,
"\"yr\" and \"year\" must resolve to the same UnitMap"
);
}

#[test]
fn test_pretty_print_unit() {
let context = Context::new(
Expand Down
133 changes: 36 additions & 97 deletions src/simlin-engine/src/units_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,111 +502,50 @@ pub fn check(
check_flows(inflows);
check_flows(outflows);
}
// Compare a sub-expression's computed units against the variable's
// declared (`expected`) units. Returns a mismatch error, or None
// when the units match, the expression is constant, or a
// dependency's units are unknown. Unknown units (DoesNotExist) are
// NOT a dimensional inconsistency -- they arise from module outputs
// or synthesized helpers that inference left unresolved -- so we
// skip the check, exactly as the arrayed-element consistency check
// above does (and as Vensim does: it warns that the dependency
// lacks units rather than erroring on every reader).
let check_against_expected = |expr: &Expr2| -> Option<UnitError> {
match units.check(expr) {
Ok(Units::Explicit(actual)) if actual != *expected => {
let loc = expr.get_loc();
Some(ConsistencyError(
ErrorCode::UnitMismatch,
Loc::new(loc.start.into(), loc.end.into()),
Some(format!(
"computed units '{actual}' don't match specified units"
)),
))
}
Ok(_) => None,
Err(ConsistencyError(ErrorCode::DoesNotExist, _, _)) => None,
Err(err) => Some(err),
}
};

if let Some(ast) = var.ast() {
match ast {
Ast::Scalar(expr) => match units.check(expr) {
Ok(Units::Explicit(actual)) => {
if actual != *expected {
let details = format!(
"computed units '{}' don't match specified units",
&actual,
);
let loc = expr.get_loc();
errors.push((
ident.clone(),
ConsistencyError(
ErrorCode::UnitMismatch,
Loc::new(loc.start.into(), loc.end.into()),
Some(details),
),
))
}
}
Ok(Units::Constant) => {
// definitionally we're fine
}
Err(err) => {
Ast::Scalar(expr) | Ast::ApplyToAll(_, expr) => {
if let Some(err) = check_against_expected(expr) {
errors.push((ident.clone(), err));
}
},
Ast::ApplyToAll(_, expr) => match units.check(expr) {
Ok(Units::Explicit(actual)) => {
if actual != *expected {
let details = format!(
"computed units '{}' don't match specified units",
&actual,
);
let loc = expr.get_loc();
errors.push((
ident.clone(),
ConsistencyError(
ErrorCode::UnitMismatch,
Loc::new(loc.start.into(), loc.end.into()),
Some(details),
),
))
}
}
Ok(Units::Constant) => {
// definitionally we're fine
}
Err(err) => {
errors.push((ident.clone(), err));
}
},
}
Ast::Arrayed(_, asts, default_expr, _) => {
// Check each element expression in the arrayed variable
for (_element, expr) in asts.iter() {
match units.check(expr) {
Ok(Units::Explicit(actual)) => {
if actual != *expected {
let details = format!(
"computed units '{}' don't match specified units",
&actual,
);
let loc = expr.get_loc();
errors.push((
ident.clone(),
ConsistencyError(
ErrorCode::UnitMismatch,
Loc::new(loc.start.into(), loc.end.into()),
Some(details),
),
))
}
}
Ok(Units::Constant) => {
// definitionally we're fine
}
Err(err) => {
errors.push((ident.clone(), err));
}
if let Some(err) = check_against_expected(expr) {
errors.push((ident.clone(), err));
}
}
if let Some(default_expr) = default_expr {
match units.check(default_expr) {
Ok(Units::Explicit(actual)) => {
if actual != *expected {
let details = format!(
"computed units '{}' don't match specified units",
&actual,
);
let loc = default_expr.get_loc();
errors.push((
ident.clone(),
ConsistencyError(
ErrorCode::UnitMismatch,
Loc::new(loc.start.into(), loc.end.into()),
Some(details),
),
))
}
}
Ok(Units::Constant) => {}
Err(err) => {
errors.push((ident.clone(), err));
}
}
if let Some(default_expr) = default_expr
&& let Some(err) = check_against_expected(default_expr)
{
errors.push((ident.clone(), err));
}
}
}
Expand Down
40 changes: 30 additions & 10 deletions src/simlin-engine/src/units_infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,19 @@ impl UnitInferer<'_> {
prefix: &str,
constraints: &mut Vec<LocatedConstraint>,
) {
let time_units =
let time_units_name =
canonicalize(self.ctx.sim_specs.time_units.as_deref().unwrap_or("time")).into_owned();
// Resolve the time unit through the units Context's alias map so that
// inference uses the same canonical time unit as `units_check::check`.
// Without this, a model that declares some units with an aliased time
// name (e.g. `yr`) while `time_units` names the primary (`year`)
// produces a spurious `year` vs `yr` mismatch on every stock/flow
// constraint.
let time_units: UnitMap = self
.ctx
.lookup(&time_units_name)
.cloned()
.unwrap_or_else(|| [(time_units_name.clone(), 1)].iter().cloned().collect());

for (id, var) in model.variables.iter() {
let current_var = format!("{prefix}{id}");
Expand All @@ -640,13 +651,15 @@ impl UnitInferer<'_> {
{
let stock_ident = ident;
let stock_var = format!("{prefix}{stock_ident}");
let expected = [
(format!("@{prefix}{stock_ident}"), 1),
(time_units.clone(), -1),
]
.iter()
.cloned()
.collect::<UnitMap>();
// expected = @stock / time_units (the units a flow must carry).
let expected = combine(
UnitOp::Div,
[(format!("@{prefix}{stock_ident}"), 1)]
.iter()
.cloned()
.collect::<UnitMap>(),
time_units.clone(),
);
let mut check_flows = |flows: &Vec<Ident<Canonical>>| {
for flow_ident in flows.iter() {
let flow_var = format!("{prefix}{flow_ident}");
Expand Down Expand Up @@ -1136,8 +1149,15 @@ pub(crate) fn infer(
units_ctx: &Context,
model: &ModelStage1,
) -> UnitResult<HashMap<Ident<Canonical>, UnitMap>> {
let time_units =
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
// match what `units_check::check` uses (see the same resolution in
// `gen_all_constraints`).
let time_units: UnitMap = units_ctx
.lookup(&time_units_name)
.cloned()
.unwrap_or_else(|| [(time_units_name.clone(), 1)].iter().cloned().collect());

let units = UnitInferer {
ctx: units_ctx,
Expand All @@ -1147,7 +1167,7 @@ pub(crate) fn infer(
ast: None,
init_ast: None,
eqn: None,
units: Some([(time_units, 1)].iter().cloned().collect()),
units: Some(time_units),
tables: vec![],
non_negative: false,
is_flow: false,
Expand Down
Loading
Loading