Skip to content

engine: infer and check macro unit polymorphism (#619)#621

Merged
bpowers merged 1 commit into
mainfrom
fix-macro-unit-polymorphism
May 22, 2026
Merged

engine: infer and check macro unit polymorphism (#619)#621
bpowers merged 1 commit into
mainfrom
fix-macro-unit-polymorphism

Conversation

@bpowers
Copy link
Copy Markdown
Owner

@bpowers bpowers commented May 22, 2026

Summary

Closes #619.

A Vensim macro annotates its body variables' units with the formal-parameter NAMES (a polymorphic idiom, e.g. ~ xfrom, or a ratio of parameters, inside C-LEARN's RAMP FROM TO). PR #618 contained the resulting xfrom/xto error storm by skipping declared-units constraints for macro bodies entirely -- but that contained the polymorphism without resolving it: parameter-named units were never resolved to the actual arguments, and a macro body's declared signature was never checked.

This PR replaces the skip with a precise treatment that delivers both halves of #619.

What changed

Resolution + checking (units_infer::gen_all_constraints): each parameter-named unit identifier in a macro body is lowered to that parameter's per-instantiation metavariable (lower_macro_unit_to_metavars); genuine base units (dmnl) are kept as-is and still checked. Because each instantiation already walks the macro body under a unique prefix, the parameter metavariable is the same one the module's input binding ties to the actual argument. So:

  • a parameter-named unit resolves to the actual argument units at each instantiation (issue point 1), and
  • a declared-vs-equation inconsistency in the body is caught (issue point 2),
  • while the storm engine: robust (partial-result) unit inference and checking #618 contained cannot return -- the parameter name is a metavariable already tied to the argument, never a literal base unit.

The canonical formal-parameter names are threaded onto ModelStage0/ModelStage1 as macro_params (from MacroSpec, via model::macro_param_idents) at all construction sites.

Clear, user-facing errors: end users are modelers, not software developers, so a conflict that touches a synthetic instantiation name ($⁚{var}⁚{n}⁚{func}·…) is rewritten by clarify_macro_conflict / synthetic_owner_and_func into a plain-language message naming the function and the variable using it, instead of synthetic-name / @-metavariable text. This also de-gibberishes stdlib-module instantiation conflict messages, which had the same problem.

Testing (TDD)

Three new units_infer.rs tests, each written first and watched fail for the right reason before implementing:

  1. a parameter-named macro-body unit (~ amount on a constant) resolves to the actual argument's units;
  2. an internally-inconsistent macro produces a conflict whose rendered message names the macro and the using variable, with no $⁚ synthetic names or @ metavariables;
  3. regression: a RAMP-FROM-TO-shaped macro mixing parameter-named units and genuine base units infers cleanly (storm guard).

Full pre-commit suite green: all engine tests, the metasd macro corpus, every macro_clearn simulation (including the RAMP FROM TO fixture), plus TypeScript and Python. Unit conflicts remain warnings, so simulation/numeric paths are unaffected.

Deferred follow-up

Checking is at instantiation granularity. Checking each macro body once at its definition (with free metavariables, attributing a signature error to the macro itself rather than to each call site) is left as a follow-up; it would additionally catch a latent signature error that no instantiation's actual arguments happen to trigger.

A Vensim macro annotates its body variables' units with the formal
parameter NAMES (a polymorphic idiom, e.g. `~ xfrom`, `~ xfrom/tstart`
inside C-LEARN's RAMP FROM TO). GH #618 contained the resulting
`xfrom`/`xto` error storm by skipping declared-units constraints for
macro bodies entirely -- but that neither resolved the parameter-named
units to the actual arguments nor checked the body's declared signature.

Replace the skip with a precise treatment: `gen_all_constraints` lowers
each parameter-named unit identifier to that parameter's per-instantiation
metavariable (`lower_macro_unit_to_metavars`), keeping genuine base units
(`dmnl`) as-is. Because each instantiation already walks the macro body
under a unique prefix, the parameter metavar is the same one the module's
input binding ties to the actual argument -- so the unit resolves to the
argument per instantiation and the storm cannot return, while a genuine
declared-vs-equation inconsistency now surfaces as a conflict. The
canonical formal-parameter names are threaded onto ModelStage0/ModelStage1
as `macro_params` (from `MacroSpec`, via `model::macro_param_idents`).

End users are modelers, not software developers, so a conflict that
touches a synthetic instantiation name (`$⁚{var}⁚{n}⁚{func}·…`) is
rewritten by `clarify_macro_conflict`/`synthetic_owner_and_func` into a
plain-language message naming the function and the variable using it,
instead of synthetic-name/`@`-metavariable text. This also de-gibberishes
stdlib-module instantiation conflicts.

Checking is at instantiation granularity; checking a macro body once at
its definition (free metavars, attributing errors to the macro itself)
is left as a follow-up.
@bpowers bpowers force-pushed the fix-macro-unit-polymorphism branch from e99c956 to 85ed61d Compare May 22, 2026 14:59
@bpowers bpowers merged commit 61c1cb8 into main May 22, 2026
10 of 13 checks passed
@bpowers bpowers deleted the fix-macro-unit-polymorphism branch May 22, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

engine: macro unit polymorphism is contained, not resolved

1 participant