engine: fix unit-analysis false positives and diagnostic quality - #1011
Conversation
Sweeping the test corpus (metasd, test-models, xmutil; 153 models) surfaced 307 unit diagnostics, most of them false positives or noise. This lands the corpus-driven fixes, bringing the sweep to 44 diagnostics, all of which trace to genuine unit sloppiness in the models themselves. WORLD3, C-LEARN, Theil_2011, thyroid-2008-d, the teacup XMILE samples, and the arithmetics fixtures now check clean -- C-LEARN matches Vensim exactly (zero mismatches). Semantics fixes (mirrored in units_check and units_infer): - A bare numeric literal is unit-polymorphic through MAX/MIN/PREVIOUS and IF-THEN-ELSE (Units::first_explicit). Returning the first argument's verdict collapsed WORLD3's 'MAX(0, land) / time' to '1/time' and Covid19US's 'IF t<t0 THEN 0 ELSE flow' to dmnl. - SQRT halves unit exponents (Vensim fn_sqrt: SQRT(units*units) -> units); a non-perfect-square argument warns in checking, and inference degrades it to Constant (a squared metavariable is unsolvable, single_fv). - x^n with an integer-literal exponent multiplies unit exponents; x^0.5 roots a perfect square; other exponent shapes degrade to polymorphic instead of wrongly keeping x's units (Theil rmse's unit^2). Policy and UX fixes: - Unit checking is opt-in by declaring units: a model declaring units on no variable gets no consistency diagnostics either (the arrayed element-consistency pass used to fire off sim_specs time_units alone). - Identical (variable, message) rows dedup before emission: arrayed variables repeated one identical mismatch per element (ph 6x, scirev 50x). - Declared-vs-computed mismatches now name both sides. - Unit definition errors carry the offending units string; multi-word unit names (Stella's 'Degrees Fahrenheit') join to identifiers per XMILE 3.5.1. - new_with_builtins covers the XMILE 3.5.4 builtin table: time-unit abbreviations (s/min/hr/wk/mo/qtr), quarters, sub-second units, per_X derived units, and unitless as dimensionless. Vensim claims cited from vensim.com documentation (fn_sqrt, Units Synonyms); XMILE claims from docs/reference/xmile-v1.0.html sections 3.5.1/3.5.4.
The consistency-mismatch detail string changed to name both computed and specified units; update the doc comments that quote it as an example so docs and behavior stay aligned.
Fixes from an adversarial review of the unit-analysis branch:
- Dedup keyed on the UnitError Display form, which embeds the source
location, so it only collapsed byte-identical element equations; key on
(variable, code, details) instead, which is exactly what the user reads.
- Equation-bearing per_* builtins were prepended to the unit list, and
Context::new's second pass parses equations in order with no fixpoint --
so a model defining its referent by equation (week = day) left per_week
resolving against a phantom minted base unit. Model units now come first.
- UnitMap::exp neither pruned zero exponents (x^0 produced {meter: 0},
which prints as dmnl but compares unequal to the empty map -- a
self-contradictory diagnostic) nor guarded the multiply (a within-range
literal exponent times a base exponent overflows i32: panic in debug,
wraparound in release, on untrusted model input). Clear on zero,
saturate the multiply.
- The literal-exponent unit tree is now the shared units::power_units
(checker and inference cannot drift), and it handles all half-integer
exponents (x^-0.5, x^1.5) rather than exactly 0.5 -- silently degrading
those let genuine dimensional errors pass while 1/SQRT(x) warned.
- The literal-polymorphism rule now also covers SAFEDIV (an explicit
fallback's units carry past a literal quotient) and SSHAPE (bottom/top
carry the units, must agree, literal is polymorphic; the x argument is
now visited at all).
- join_multiword_unit_names is byte-length-preserving (each joining
space/tab maps 1:1 to underscore, nothing collapsed or trimmed): parse
error offsets are computed against the joined string but rendered
against the original, so a length change shifted every underline after
a whitespace run.
- Every XMILE citation said 3.5.x; the units material is section 3.3.6.
- Refreshed five more stale doc examples of the old mismatch wording;
removed a dead test fixture and pinned the phrase-unit join behavior
('widgets per year' is one base unit) it silently introduced; dropped a
duplicate test builder in favor of array_with_ranges_direct.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1011 +/- ##
==========================================
+ Coverage 91.85% 91.89% +0.04%
==========================================
Files 247 247
Lines 159097 159807 +710
==========================================
+ Hits 146143 146860 +717
+ Misses 12954 12947 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b70a453d92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| for unit in self.map.values_mut() { | ||
| *unit *= exp; | ||
| *unit = unit.saturating_mul(exp); |
There was a problem hiding this comment.
When a large power saturates here, the resulting i32::MAX/i32::MIN remains unsafe for ordinary UnitMap operations: for example, units meter^2 raised via m2^1073741824 saturate to i32::MAX, and composing that as m2^1073741824 * m2 overflows at UnitMap::mul's n + m; a large negative power can likewise make Display::fmt overflow on i32::MIN.abs(). Overflow-checked/debug builds therefore panic while release builds wrap and produce invalid unit maps, defeating the stated protection for untrusted model files. Return an unknown/error verdict on exponent overflow or make every downstream exponent operation checked/saturating as well.
Useful? React with 👍 / 👎.
| Expr2::Op1(UnaryOp::Negative, inner, _, _) => literal_exponent(inner).map(|n| -n), | ||
| _ => None, |
There was a problem hiding this comment.
When a valid exponent is written with an explicit plus sign, such as length^(+2), the parser produces Op1(UnaryOp::Positive, Const(2)), but this match returns None. Both checking and inference then take their non-literal exponent path and classify the power as Constant, so even an incorrect declaration such as length^(+2) ~ second produces no unit mismatch and no useful inference constraints. Handle UnaryOp::Positive by recursively returning the inner literal, alongside the existing negative arm.
AGENTS.md reference: AGENTS.md:L97-L97
Useful? React with 👍 / 👎.
| let mut combined_units: Vec<Unit> = units.to_vec(); | ||
| combined_units.extend( |
There was a problem hiding this comment.
Appending all built-ins after model definitions makes the newly added equation-bearing per_* units unavailable while model-defined unit equations are parsed, because Context::new resolves those equations sequentially without a fixpoint. For example, a valid model unit hazard = per_year is stored as the unrelated base unit {per_year: 1}; only afterward is the built-in per_year = 1/year installed, so variables declared in hazard and per_year spuriously mismatch. User-defined aliases of built-in units are explicitly supported by XMILE, so derived built-ins and model equations need dependency-aware/fixpoint resolution rather than this one-way ordering.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
Review summaryReviewed the unit-analysis changes across
No qualifying bugs found. Behavior changes for pre-existing models that used phrase-form unit strings (e.g. Overall correctness verdictCorrect. |
Three findings from the codex review of #1011, all confirmed: - Saturated exponents could still overflow-panic downstream of UnitMap::exp: Mul's exponent addition, reciprocal's negation of i32::MIN, and Display's .abs() on i32::MIN. The contract for absurd exponents is only 'no crash on untrusted input', so each is now saturating (unsigned_abs for Display) -- one-line changes apiece. - literal_exponent didn't see through unary plus, so 'length^(+2)' silently degraded to unit-polymorphic and a wrong declaration passed unchecked. Positive now recurses like Negative. - Ordering model units before the equation-bearing per_* builtins fixed 'week = day' but broke the converse: a model unit DEFINED VIA a builtin ('hazard = per_year') resolved before per_year's equation was parsed and bound to a phantom base unit. One-way ordering cannot satisfy both directions, so Context::new's equation pass is now dependency-aware: each pass resolves every unit whose equation references no still-unresolved equation-bearing unit, repeating until done. All aliases register before any equation resolves (an equation may reference an alias declared later). A no-progress pass means circular definitions (forbidden by XMILE 3.3.6); those degrade to the old in-order behavior -- unresolved references mint base units -- so a malformed model still yields a usable partial context rather than a hang, a panic, or an empty context.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d35565706b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ("per_minute", &[], Some("1/minute")), | ||
| ("per_hour", &[], Some("1/hour")), | ||
| ("per_day", &[], Some("1/day")), | ||
| ("per_week", &[], Some("1/week")), |
There was a problem hiding this comment.
When a conforming model explicitly declares the baseline primary weeks, the collision filter removes the week builtin because weeks is one of its aliases, but this remaining equation still references week. With no week -> weeks alias registered, dependency-aware resolution mints an independent {week} base, so per_week spuriously differs from 1/weeks; the same problem affects the other derived time units. The in-repo XMILE table defines the plural primaries and equations such as per_week = 1/weeks, so use those canonical referents or preserve the alias when filtering repeated definitions.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
| Units::Constant => Units::Constant, | ||
| Units::Explicit(units) => match crate::units::try_sqrt(&units) { | ||
| Some(root) => Units::Explicit(root), | ||
| None => Units::Constant, |
There was a problem hiding this comment.
When the SQRT result itself has no declared units, degrading its symbolic argument to Constant removes the only relationship between the result and its source. For example, with source ~ meter^2, root = SQRT(source) (undeclared), and consumer = root ~ second, inference can satisfy the remaining constraints by assigning root the consumer's second units, and concrete checking then reports no mismatch. Preserve a root constraint that can be resolved after source is bound, or otherwise evaluate the resolved source units before discarding this relationship.
Useful? React with 👍 / 👎.
| if id_str == "dmnl" | ||
| || id_str == "nil" | ||
| || id_str == "dimensionless" | ||
| || id_str == "unitless" |
There was a problem hiding this comment.
When sim_specs.time_units is Unitless, this new branch parses a variable declaration of Unitless as the empty dimensionless map, but model_time_units does not recognize the same special names and falls back to {unitless: 1}. Consequently a valid variable such as x = TIME ~ Unitless now emits a mismatch, and stock/flow expectations are likewise built from a fictitious unit. Normalize the simulation time name through the same dimensionless handling used here.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
Review — no blocking findingsReviewed all 19 files in the diff, focusing on the substantive engine changes ( Highlights of what I verified:
Overall correctness verdictCorrect. No blocking issues; no non-blocking bugs worth flagging either. The change is well-motivated (corpus-driven, with each fix pinned by a dedicated test) and semantically consistent across the checker/inferer boundary. |
|
@codex review |
Three more findings, all confirmed:
- A model explicitly declaring one spelling of a builtin unit group (the
XMILE baseline primary 'weeks') severed the group: the collision filter
dropped the whole builtin, leaving 'week'/'wk' unregistered, so the
surviving 'per_week = 1/week' minted a phantom base unit and no longer
equaled '1/weeks'. The group's non-colliding spellings are now re-tied
to the model's unit as a chained alias-by-equation ('week = weeks',
aliases 'wk') -- the mechanism XMILE 3.3.6 describes for user aliases
of built-in units, and the direction its collision rule points
('respect the unit definitions for the model').
- Inference degraded SQRT of a symbolic (metavariable-bearing) map to a
free Constant, severing the only relationship between an undeclared
SQRT result and its source -- a consumer could bind 'SQRT(meter^2)' to
'second' with no complaint from either inference or checking. The
result is now a fresh metavariable R with the residual constraint
R^2 == arg: single_fv refuses |exp| != 1 so R is never mis-bound by
the constraint itself, but once other constraints bind R, substitution
scales it in and a wrong binding surfaces as a concrete contradiction.
The '^' half-integer NonSquareRoot arm gets the same treatment
(x^n = sqrt(x^2n)).
- Adding 'unitless' to the dimensionless spellings regressed a
dimensionless clock: a variable's 'Unitless' parsed to the empty map
while model_time_units minted {unitless: 1}, so 'x = TIME ~ Unitless'
mismatched (and stock/flow expectations used the fictitious unit; the
same held for time_units='dmnl' before this branch). Single-name
resolution -- aliases, dimensionless spellings, unknown-name fallback
-- is now one shared Context::resolve_name used by unit-equation
references, model_time_units, and both of inference's time-unit
resolution sites; Context::lookup remains only as test introspection.
|
@codex review |
ReviewReviewed the semantic changes (SQRT/ No blocking issues found. The edge cases I traced all check out:
Non-blocking observations (informational only):
Overall correctness: correct |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8db359fa45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub(crate) fn power_units(base: &UnitMap, n: f64) -> PowerUnits { | ||
| let doubled = 2.0 * n; | ||
| if n.fract() == 0.0 && n.abs() <= i32::MAX as f64 { | ||
| PowerUnits::Explicit(base.clone().exp(n as i32)) |
There was a problem hiding this comment.
Fresh evidence after the saturation fixes: the inference solver still calls exp.abs()/exponent.abs() on symbolic exponents. When a unit expression first builds a squared metavariable and then raises it to a large negative integer, e.g. x = (y*y)^-1073741824 + z with declared units on y/z, this call can create an i32::MIN exponent for @y; during substitution the solver hits i32::MIN.abs(), which panics in debug builds and can abort panic=abort hosts. Saturating here is therefore not sufficient for untrusted models; overflow should degrade to unknown/error, or every solver exponent operation needs checked/unsigned/saturating handling as well.
Useful? React with 👍 / 👎.
| let next_is_word = j < chars.len() && is_word_char(chars[j]); | ||
| for run_char in chars.iter().take(j).skip(i) { | ||
| if prev_is_word && next_is_word { | ||
| out.push('_'); |
There was a problem hiding this comment.
When a multi-word unit name contains a run of spaces or tabs, this converts every byte in the run to _ before the normal identifier canonicalizer sees it. Model unit names and aliases still go directly through canonicalize, which collapses each whitespace run to a single underscore, so a unit declared as Widget Years = widget/year is stored as widget_years while a variable using the same presented units text resolves to a fresh base unit widget__years; equation-bearing aliases with extra spaces therefore stop applying and produce false unit mismatches. Preserve source offsets without changing the canonical identity of whitespace runs.
Useful? React with 👍 / 👎.
| if let Some(primary) = leftovers.next() { | ||
| combined_units.push(Unit { | ||
| name: primary.to_string(), | ||
| equation: Some(model_spelling), |
There was a problem hiding this comment.
When the collision is only on a builtin abbreviation/alias, this rewires the remaining full spellings to the model's unit. For example, a model that defines s as a stock unit and also uses the full builtin second now gets a synthetic second = s, so expressions like student_count + duration_in_seconds are accepted as compatible and per_second becomes 1/s. Preserve the baseline-unit override case without aliasing non-colliding builtin names to an arbitrary model-defined abbreviation.
Useful? React with 👍 / 👎.
Three more findings, all confirmed; fixed with a bias toward simple, explainable behavior: - The inference solver still took |exponent| with plain i32 abs() at four sites (single_fv, solve_for's assert, substitute's scale), so a literal like '(y*y)^-1073741824' -- whose symbolic map holds @y^(i32::MIN) -- panicked in debug builds. unsigned_abs/saturating_abs, same crash-only contract as the concrete-map operations. - The byte-length-preserving multiword join collapsed a whitespace RUN to one underscore PER CHARACTER ('Widget Years' -> widget__years) while canonicalize stores a declared unit name with the run collapsed to ONE underscore (widget_years) -- so a declaration with a doubled space silently detached from an identical units string. Joining runs now collapse to a single underscore, matching canonicalize's identity; offsets stay exact everywhere except after such a collapsed run (the pathological case, traded for correctness), and non-joining whitespace is still untouched. - Re-tying a builtin group to the model's unit was too eager when the collision was only a short abbreviation: a model using 's' for its own unit got a synthetic 'second = s', quietly equating seconds with whatever 's' means. The rule is now spelled by which name the model claimed: a FULL spelling (weeks, year) takes over the baseline group (chained alias, as before); a short abbreviation (s, min, hr, wk, mo, qtr, ns, us, ms, $s) takes only that name, and the builtin keeps its full spellings independently. yr/yrs deliberately count as full -- they are widespread genuine spellings of year in Vensim models with no plausible other meaning.
|
@codex review |
Review of PR 1011I reviewed the semantic changes (
FindingsNone. Overall correctnessCorrect. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f65bbb728
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| pub(crate) fn power_units(base: &UnitMap, n: f64) -> PowerUnits { | ||
| let doubled = 2.0 * n; | ||
| if n.fract() == 0.0 && n.abs() <= i32::MAX as f64 { |
There was a problem hiding this comment.
For the valid i32 exponent -2147483648, the absolute-value guard rejects the literal even though it can be represented and the hardened UnitMap operations now handle it. Consequently an expression such as length^-2147483648 ~ second is classified as Polymorphic/Constant, so both checking and inference silently accept the unrelated declared units. Check the lower and upper bounds independently rather than comparing abs() with i32::MAX.
Useful? React with 👍 / 👎.
| - **`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`, or a parameter ratio such as xfrom over tstart, inside RAMP FROM TO). `gen_all_constraints` LOWERS each parameter-named unit identifier to that parameter's per-instantiation metavariable (`lower_macro_unit_to_metavars`, gated on `ModelStage1::is_macro`/`macro_params`), so it resolves to the actual argument units at each instantiation instead of leaking the parameter name as a literal base unit -- while genuine base units (`dmnl`) are kept and still checked. This both *resolves* parameter-named units (GH #619 point 1) and *checks* a macro body's declared signature against its equations (point 2), superseding the GH #618 skip-entirely containment (which neither resolved nor checked them). A conflict that involves a synthetic module/macro instantiation is rewritten by `clarify_macro_conflict` into a plain-language diagnostic naming the function and the using variable (parsed from the `$⁚{var}⁚{n}⁚{func}` synthetic name by `synthetic_owner_and_func`) rather than synthetic-name/metavariable text -- end users are modelers, not software developers (this also cleans up stdlib-module conflict messages). The cross-module parameter bindings + per-instantiation prefix already monomorphize the macro body, so the RAMP FROM TO storm GH #618 contained does not return. `gen_all_constraints` recurses through every module instantiation, and the module graph is a graph, not a tree, so it threads an `InstantiationPath` -- the models being walked on the CURRENT path, as a cons list whose entries live exactly as long as their stack frames. An edge back to a model already on the path is DECLINED, body and input constraints together, so a module cycle degrades to a partial result instead of overflowing the stack -- which, unlike a panic, aborts the whole `panic=abort` host process. Dropping the input constraint has a KNOWN COST, deliberately paid: the callee-side metavariable is not necessarily dead, since a parent equation reading `{module}·{var}` emits that same metavariable, so a genuine cross-module dimensional conflict inside a cycle goes UNREPORTED. That is accepted because the project is already rejected as `CircularDependency` and a unit conflict on a model that cannot compile is noise -- and `back_edge_declines_a_real_cross_module_conflict` builds the shape and pins the silence, so the trade is documented rather than silent. The path is deliberately not a visited-ANYWHERE set: in a diamond (`a` instantiates `b` and `c`, both of which instantiate `d`) `d` really is instantiated twice under two prefixes and both instantiations must be constrained; the diamond tests, not the cycle tests, are what pin that. Depth is bounded by the model count but the number of instantiation prefixes is not (`k` models each instantiating the next twice reaches the last `2^k` times, all legal) -- the guard makes the walk finite, not cheap. The cycle gets no unit diagnostic of its own -- `project_module_graph` already reports it as a `CircularDependency`. | ||
| Unit checking is **opt-in by declaring units**: a model that declares units on NO variable gets no unit diagnostics at all (`check_model_units`'s `has_declared_units` gate covers inference conflicts AND the consistency pass -- sim_specs' `time_units` alone must not flood a purely numeric model with warnings about `cons - TIME`). | ||
|
|
||
| - **`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). `new_with_builtins`' table is the XMILE 3.3.6 built-in units (time units with abbreviation/singular aliases -- `s`/`min`/`hr`/`wk`/`mo`/`qtr`/`yr` -- plus the `per_X` derived units, each defined as 1 over X) merged with Vensim's default `22:` synonym groups ($/dollar, person/people, unit/units); a built-in is dropped whenever the model defines any of its names itself. `dmnl`/`dimensionless`/`unitless`/`fraction`/`nil` are all the empty map. `join_multiword_unit_names` joins whitespace-separated identifier runs in a unit equation (`Degrees Fahrenheit` per `Minute` becomes the single unit name `Degrees_Fahrenheit` over `minute`): XMILE unit names are identifiers "stored with underscores but generally presented to users with spaces", and Stella writes the presentation form (the canonical teacup model used to die with a bare `extra_token`). Every `DefinitionError` from `parse_units` carries the offending source string in its details. |
There was a problem hiding this comment.
Update this subsystem description because a builtin is no longer simply dropped whenever any spelling collides: new_with_builtins now either re-ties the remaining full spellings to the model-defined unit or removes only a claimed abbreviation. The current statement gives maintainers the opposite collision invariant from the implementation and can lead subsequent changes or tests to restore the previously broken behavior.
AGENTS.md reference: AGENTS.md:L118-L118
Useful? React with 👍 / 👎.
|
|
||
| - **`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). `new_with_builtins`' table is the XMILE 3.3.6 built-in units (time units with abbreviation/singular aliases -- `s`/`min`/`hr`/`wk`/`mo`/`qtr`/`yr` -- plus the `per_X` derived units, each defined as 1 over X) merged with Vensim's default `22:` synonym groups ($/dollar, person/people, unit/units); a built-in is dropped whenever the model defines any of its names itself. `dmnl`/`dimensionless`/`unitless`/`fraction`/`nil` are all the empty map. `join_multiword_unit_names` joins whitespace-separated identifier runs in a unit equation (`Degrees Fahrenheit` per `Minute` becomes the single unit name `Degrees_Fahrenheit` over `minute`): XMILE unit names are identifiers "stored with underscores but generally presented to users with spaces", and Stella writes the presentation form (the canonical teacup model used to die with a bare `extra_token`). Every `DefinitionError` from `parse_units` carries the offending source string in its details. | ||
| - **`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). A bare numeric literal is **unit-polymorphic**, and that propagates through the polymorphic positions of MAX/MIN/PREVIOUS and IF-THEN-ELSE via `Units::first_explicit`: `MAX(0, x)` and `IF c THEN 0 ELSE x` both have x's units (returning the first argument's verdict unconditionally collapsed WORLD3's `MAX(0, land)` over time to 1 over time and Homer's `effect * IF(t<t0, 0, flow)` to `dmnl`). SQRT halves unit exponents (Vensim fn_sqrt: "SQRT(units*units) --> units"); a non-perfect-square argument is a warning. `x^n` with an integer-literal exponent (seeing through unary negation via `literal_exponent`) multiplies unit exponents; `x^0.5` roots a perfect square; any other exponent shape degrades to unit-polymorphic rather than (wrongly) keeping x's units -- Vensim's non-literal-exponent behavior is unverified, so no warning is emitted there. Declared-vs-computed mismatches name BOTH sides ("the equation computes to units 'X', but the variable's specified units are 'Y'"). `check` dedups identical `(variable, message)` rows before returning -- an arrayed variable's per-element loops otherwise repeat one mismatch per element (C-LEARN's `ph` warned 6x, scirev 50x). | ||
| - **`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`, or a parameter ratio such as xfrom over tstart, inside RAMP FROM TO). `gen_all_constraints` LOWERS each parameter-named unit identifier to that parameter's per-instantiation metavariable (`lower_macro_unit_to_metavars`, gated on `ModelStage1::is_macro`/`macro_params`), so it resolves to the actual argument units at each instantiation instead of leaking the parameter name as a literal base unit -- while genuine base units (`dmnl`) are kept and still checked. This both *resolves* parameter-named units (GH #619 point 1) and *checks* a macro body's declared signature against its equations (point 2), superseding the GH #618 skip-entirely containment (which neither resolved nor checked them). A conflict that involves a synthetic module/macro instantiation is rewritten by `clarify_macro_conflict` into a plain-language diagnostic naming the function and the using variable (parsed from the `$⁚{var}⁚{n}⁚{func}` synthetic name by `synthetic_owner_and_func`) rather than synthetic-name/metavariable text -- end users are modelers, not software developers (this also cleans up stdlib-module conflict messages). The cross-module parameter bindings + per-instantiation prefix already monomorphize the macro body, so the RAMP FROM TO storm GH #618 contained does not return. `gen_all_constraints` recurses through every module instantiation, and the module graph is a graph, not a tree, so it threads an `InstantiationPath` -- the models being walked on the CURRENT path, as a cons list whose entries live exactly as long as their stack frames. An edge back to a model already on the path is DECLINED, body and input constraints together, so a module cycle degrades to a partial result instead of overflowing the stack -- which, unlike a panic, aborts the whole `panic=abort` host process. Dropping the input constraint has a KNOWN COST, deliberately paid: the callee-side metavariable is not necessarily dead, since a parent equation reading `{module}·{var}` emits that same metavariable, so a genuine cross-module dimensional conflict inside a cycle goes UNREPORTED. That is accepted because the project is already rejected as `CircularDependency` and a unit conflict on a model that cannot compile is noise -- and `back_edge_declines_a_real_cross_module_conflict` builds the shape and pins the silence, so the trade is documented rather than silent. The path is deliberately not a visited-ANYWHERE set: in a diamond (`a` instantiates `b` and `c`, both of which instantiate `d`) `d` really is instantiated twice under two prefixes and both instantiations must be constrained; the diamond tests, not the cycle tests, are what pin that. Depth is bounded by the model count but the number of instantiation prefixes is not (`k` models each instantiating the next twice reaches the last `2^k` times, all legal) -- the guard makes the walk finite, not cheap. The cycle gets no unit diagnostic of its own -- `project_module_graph` already reports it as a `CircularDependency`. `gen_constraints` mirrors `units_check`'s builtin semantics (literal-polymorphic MAX/MIN/PREVIOUS/IF via `first_explicit`; integer-literal `^` exponentiation), with one deliberate divergence: SQRT of a map with any odd exponent degrades to `Constant` (no constraint) instead of erroring -- the symbolic map is usually `@x^1`, which has no integer-exponent root and no solvable squared-metavar constraint (`single_fv` refuses `|exp| != 1`), so the concrete check in `units_check` remains the authority on SQRT. |
There was a problem hiding this comment.
Update this description because SQRT inference no longer degrades an odd symbolic map to Constant with no constraint. sqrt_result_metavar now returns a fresh metavariable and records R^2 == argument, specifically to preserve the relationship until other bindings resolve; documenting the old deliberate divergence obscures a correctness invariant that future solver work must retain.
AGENTS.md reference: AGENTS.md:L118-L118
Useful? React with 👍 / 👎.
1 participant
Fixes #613
Why
Sweeping the full test corpus (metasd, test-models, xmutil; 153 models) through
simlin simulatesurfaced 307 unit diagnostics, most of them our fault: false positives from wrong builtin unit semantics, identical messages repeated once per array element, and messages that named neither the expected units nor the offending source. After this branch the sweep produces 44 diagnostics, each verified to trace to genuine unit sloppiness in the model itself -- and C-LEARN drops from 14 warnings to 0, matching real Vensim exactly (which reports no mismatches on it). WORLD3, Theil_2011, thyroid-2008-d, the teacup XMILE samples, and the arithmetics fixtures also now check clean.This resolves #613's open question in an unexpected direction: the C-LEARN residual was neither Vensim leniency nor model error -- all three message classes traced to checker bugs (the
^-exponent handling behind thephlog-formula warnings and the permafrost coefficient non-cancellation, and IF-branch literal handling behind the rest).Semantics fixes (mirrored in
units_checkandunits_infer)Units::first_explicit). The old code returned the first argument's verdict, soMAX(0, land)/dev_timecomputed as1/yearinstead ofhectare/year(WORLD3's only warnings) andIF t<t0 THEN 0 ELSE flowcollapsed to dmnl (Homer's Covid19US).SQRT(units*units) --> units); a non-perfect-square argument warns in checking and degrades to unknown in inference (a squared metavariable is unsolvable).x^nwith a literal exponent multiplies unit exponents; half-integer exponents (x^0.5,x^-0.5,x^1.5) root the doubled map; other exponent shapes degrade to polymorphic rather than wrongly keepingx's units (Theil'srmseflagged a dimensionally correct model). One sharedunits::power_unitsdecision serves both files so they cannot drift.Policy and UX fixes
time_unitsalone, flooding purely numeric fixtures.ph6x, scirev 50x).extra_token). The join is byte-length-preserving so parse-error offsets stay aligned with the original string, and definition errors quote the offending units string.s/min/hr/wk/mo/qtr), quarters, sub-second units, theper_Xderived units, andunitlessas dimensionless -- merged with Vensim's default22:synonym groups as before. Model-defined units precede equation-bearing builtins soper_weekresolves against a model's ownweek = daydefinition.Non-obvious decisions
Hares/Hare, pendulum'sKilograms/Kilogram, workforce, SIR) are true positives Vensim would flag too -- deliberately left in place.^exponent is unverified, so those shapes degrade silently to polymorphic rather than warning.UnitMap::expnow clears onx^0(a{meter: 0}map printed as dmnl while comparing unequal to dmnl) and saturates the multiply (model files are untrusted input; an overflow panic would abortpanic=aborthosts).