Conversation
The design phase for Vensim macro support: the validated design plan, a first-party reference document, and a 14-model macro test corpus from the MetaSD model library. The design reuses the engine's existing module machinery rather than adding a new primitive. Simlin already implements its stock-and-flow builtins (SMTH1, DELAY3, and friends) as small models instantiated as modules on function-call syntax; this generalizes that mechanism from a fixed build-time stdlib registry to a per-project, data-driven one. A macro definition becomes a Model carrying a new optional MacroSpec marker; a single-output invocation stays as function-call equation text expanded at compile time; only the multi-output ':' form is materialized in the datamodel. The compiler and VM are unchanged. Key decisions: - One additive protobuf field on Model; no new datamodel primitive - XMILE supported symmetrically via the simlin: extension namespace - Macros shadow builtins, eliminating a current silent-wrong-result bug - 7 implementation phases, C-LEARN as the hero validation Includes docs/reference/vensim-macros.md (implementation reference) and test/metasd/ (14 macro-using models, CC BY 3.0, trimmed to .mdl/.vdf sources with per-model provenance). Two of the bundled .vdf fixtures trip the Rust VDF slot-table scanner's known under-count; they are added to the vdf_structural_invariants exemption list and tracked in #549.
Introduce a MacroSpec struct and an Option<MacroSpec> field on datamodel::Model. Some marks the model as a callable macro template whose ordinary variables are the macro body; MacroSpec records the calling convention (formal parameters, primary output, and the :-list additional outputs from Vensim multi-output macros). This is the in-memory representation only -- no protobuf, JSON, or import/export wiring yet (those follow in later tasks). Adding the field requires macro_spec: None on every existing datamodel::Model struct literal across the workspace; the stdlib.gen.rs generator (gen_stdlib.rs) emits it too, since no stdlib model is a macro. The xmile reader's From<datamodel::Model> destructure ignores macro_spec with an explicit `_` binding: XMILE has no macro concept, so it is dropped on export until the later XMILE phase.
Add a MacroSpec protobuf message and Model.macro_spec (field 7) to project_io.proto, regenerate project_io.gen.rs, and wire the datamodel::MacroSpec <-> project_io::MacroSpec conversions into serde.rs alongside the existing LoopMetadata impls. A singular proto3 message field is presence-tracked, so prost generates Model.macro_spec as Option<MacroSpec>; the two Model conversion impls map over that Option. test_model_with_macro_spec_ roundtrip covers macros.AC1.4 (protobuf half): a macro-bearing model with non-empty parameters, a primary output, additional outputs, and a body variable round-trips losslessly, and an ordinary model still round-trips with macro_spec None. Regenerating the prost bindings also adds the field to project_io::Model, so the libsimlin protobuf-fixture tests that build project_io::Model literals get macro_spec: None.
Mirror the MacroSpec datamodel type into json::Model exactly as the LoopMetadata nested-struct precedent does, using the optional-singular serde idiom from json::Model.sim_specs (skip_serializing_if = Option::is_none, default) since macro_spec is Option<MacroSpec> rather than a Vec. additionalOutputs uses the is_empty_vec skip helper so an ordinary single-output macro serializes compactly. The json_proptest model_strategy gains a macro_spec_strategy wired through prop::option::of, so the existing json/datamodel/protobuf roundtrip property tests and generated_json_validates_against_schema now exercise random MacroSpec values against the regenerated schema. docs/simlin-project.schema.json is regenerated by the generate_and_write_schema test and now describes MacroSpec under $defs plus a macroSpec property on Model. simlin-mcp-core's build_empty_project_with_specs constructs a json::Model literal; the compiler flagged it for the new field, so it gets macro_spec: None (an empty project is never a macro template).
json-types.ts is the hand-maintained TypeScript mirror of json.rs; add JsonMacroSpec with the same camelCase keys and optionality as json::MacroSpec (parameters and primaryOutput always present, additionalOutputs skip-serialized when empty) and a macroSpec? field on JsonModel. datamodel.ts gains the readonly MacroSpec interface plus macroSpecFromJson/macroSpecToJson converters mirroring the LoopMetadata precedent, and macroSpec is wired through modelFromJson/modelToJson. This proves the TypeScript half of macros.AC1.4: a MacroSpec round-trips losslessly, and an empty additionalOutputs is omitted from JSON and restored as [].
json.rs is the source of truth for the Python json mirror. Add a MacroSpec dataclass to json_types.py (all fields defaulted, mirroring LoopMetadata) and a macro_spec field to the Model dataclass. cattrs centralizes (de)serialization in json_converter.py: a structure_macro_spec hook reads the camelCase keys (primaryOutput, additionalOutputs), structure_model wires the optional macroSpec key, and MacroSpec is added to additional_type_required_fields so parameters and primary_output always serialize at their defaults while additional_outputs is omitted when empty -- matching json::MacroSpec's serde attributes exactly. This proves the Python half of macros.AC1.4: a macro-bearing Model round-trips losslessly through the json_converter using camelCase keys.
The SDAI JSON bridge constructs datamodel::Model with macro_spec: None in both conversion directions (SdaiModel -> Project import and the Project -> SdaiModel fallback empty model). A bare macro_spec: None in a conversion is non-obvious -- a reader cannot tell if it is a deliberate drop or an oversight. Mirror the rationale style already used in xmile/model.rs: SDAI is a lossy AI-augmentation bridge format with no macro concept, so macro_spec is intentionally not carried. Correct for Phase 1 of Vensim macro support; revisit if SDAI ever needs macro awareness. Comment-only; no behavior change.
The MDL ConversionContext pipeline already turns a list of MdlItems into a datamodel::Model, but it was hardwired to (a) drain its items from an EquationReader and (b) emit exactly one model named "main". Phase 2's macro import needs to run the same passes over a macro body with a scoped symbol table, so this factors out both hardwirings without changing any observable behavior. new_with_data now delegates struct assembly to a new new_from_items constructor that takes a caller-supplied item list plus the parent's already-built dimensions/data_provider/formatter (XmileFormatter gains a cheap Clone so a sub-context can reuse the parent's subrange-name state). build_project is split into build_model(name) (the model-building core, producing a Model with sim_specs/macro_spec None) plus the Project assembly; build_groups loses its unused &mut self. The "main" conversion is byte-for-byte unchanged and the public convert_mdl/convert_mdl_with_data signatures are untouched.
Each parsed MacroDef now becomes a macro-marked datamodel::Model alongside the "main" model, instead of being silently discarded. A new convert/macros pass (run after stock/flow linking, before build_project consumes self) builds each macro body via a scoped ConversionContext over the body equations -- reusing Task 4's new_from_items with the parent's shared dimensions/data_provider/formatter -- so single-aux, multi-equation, and stock INTEG bodies all convert through the existing passes for free. The format-agnostic half -- synthesizing a port Variable per formal parameter and attaching the MacroSpec -- lives in a new pub(crate) Model::new_macro on datamodel::Model so the XMILE reader can reuse it verbatim in a later phase. A synthetic placeholder equation is prepended per parameter before the scoped conversion so link_stocks_and_flows assigns each port the correct stock/flow/aux kind itself: the INTEG-rate parameter becomes a Flow port and the INTEG-initial parameter an Aux port, mirroring stdlib delay1/smth1. new_macro then flips can_be_module_input on those pipeline-built ports without disturbing their kind, which collect_module_idents requires to treat a macro model (an ordinary non-stdlib sub-model) as having input slots. Macro name, parameters, and additional outputs are canonicalized to the variable-ident form (lowercase then space-to-underbar) so MacroSpec entries are byte-identical to the body variables they name and to the synthesized port idents. Additional colon-list outputs are body-computed and so are not synthesized as ports. Single-output macro invocations are left as ordinary equation text (materialized in a later phase).
A `$`-suffixed time reference in a macro body (`Time$`, `TIME STEP$`, `INITIAL TIME$`, `FINAL TIME$`, `DT$`) is Vensim's escape, valid only inside a macro body, for reaching the caller's global time variables; after lexing it is an Expr::Var whose name still includes the trailing `$`. The engine already resolves the bare canonical time idents inside a module body at any nesting depth (they are zero-arity builtins), so the only work is a front-end name translation. A new pub(super) rewrite_dollar_time helper in convert/helpers.rs recursively rewrites every Expr::Var whose name -- lowercased, space-normalized, trailing `$` stripped -- matches a Vensim time variable to the canonical engine ident (time / time_step / initial_time / final_time / dt), recursing through Op1/Op2/Paren and the args and output_bindings of App. The macro-conversion routine applies it to each body equation's expression before the scoped conversion formats it; it is scoped to macro bodies only (the global formatter and non-macro equations are untouched), so a `$`-time reference outside a macro body -- which is meaningless -- is out of scope, as is a non-time `$`-suffixed reference. The shared XMILE formatter then renders the canonical idents in its uniform time-name form (TIME / DT / ...), exactly as it does for an ordinary non-macro equation that references Time / TIME STEP; the import-level test cross-checks byte-equality against that non-macro baseline to prove the escape was translated to the same canonical idents, not merely stripped.
The debug_assert! guarding empty output_bindings in the XMILE formatter's App arm used a Unicode em-dash in its message string. Project guidelines favor ASCII in code and comments, so replace it with " -- ". Message-string-only change: the assertion condition and guard placement are unchanged.
Generalize the model.rs stdlib pre-classification to also recognize project-macro calls, and make the per-project macro registry available at the compile entry points so a macro-using model classifies its caller variables correctly and an invalid macro set fails the compile with a clear error. The registry is a salsa-tracked query (db_macro_registry:: project_macro_registry) keyed on the project's SourceModels: macro_spec is now carried on the SourceModel salsa input (mirroring Compat), so editing a non-macro variable does not invalidate it. equation_is_stdlib_call becomes equation_is_module_call, takes the registry, and returns true for a macro call too; the change is threaded through collect_module_idents and module_ident_context_for_model (which gains a SourceProject parameter, since the registry is a project-level concept) and on to the one simlin-cli caller. Registry-build validation (recursion cycle / duplicate macro name / macro-model name collision) must run on the datamodel Vec<Model>: the SourceProject.models map is name-keyed, so two same-named macros -- or a macro colliding with a model -- collapse and become indistinguishable post-sync. The build is therefore run at sync time on the datamodel and its error message stored on a new SourceProject field; the salsa query surfaces it both as a project-level diagnostic (via collect_all_ diagnostics) and through compile_project_incremental as sim_err!( NotSimulatable, msg). For a valid project every model name is unique, so the resolution registry is rebuilt exactly from the deduplicated models. The registry query plus the sync-time error helper live in a new db_macro_registry top-level module (a sibling of db, like db_ltm_ir) to keep db.rs under the per-file line cap. A macro call still hits UnknownBuiltin in BuiltinVisitor after this commit -- expansion is Task 3. No-macro projects are byte-for-byte unchanged (full lib suite and the simulate integration tests green). A small TestProject::from_datamodel constructor wraps an open_vensim / convert_mdl project so the compile/run/diagnostic helpers apply to a multi-model (macro-bearing) project unchanged.
Generalize the stdlib-hardcoded module rewrite in BuiltinVisitor into a
descriptor-driven helper and consult the macro registry, so a macro
invocation expands into a synthetic Variable::Module exactly the way a
stdlib call does -- the compiler, module-instantiation machinery, and
VM are unchanged.
expand_module_function takes a &ModuleFunctionDescriptor: model_name,
the ModuleReference dst ports, and the replacement Var("<module>·
<primary_output>") all come from the descriptor (was format!("stdlib⁚
{func}"), stdlib_args, and a hardcoded ·output). stdlib primary_output
is "output" and the synthetic-name / arg-hoisting / A2A-subscript
logic is reused verbatim, so stdlib expansion is byte-for-byte
identical (builtins_visitor + simulate suites unchanged). A macro is
strict-arity (args.len() must equal parameter_ports.len(), else
BadBuiltinArgs over the call span); stdlib stays lenient.
walk()'s App arm now resolves the registry at the very top -- before
alias normalization / modulo / previous / init / is_builtin_fn / the
stdlib lookup -- so a project macro shadows an identically named
builtin or stdlib func (Vensim's rule; fixes the latent SSHAPE /
RAMP FROM TO mis-resolution). The stdlib branch swaps stdlib_args-else-
UnknownBuiltin for stdlib_descriptor-else-UnknownBuiltin, so a genuinely
unknown name still fails UnknownBuiltin (AC5.6).
contains_stdlib_call becomes contains_module_call, threaded with the
registry and macro-aware, so an arrayed macro invocation enters the
per-element apply-to-all path (Phase 4 exercises it end-to-end).
The registry is threaded from the Task 2 salsa parse query
(parse_source_variable_with_module_context, which has SourceProject)
down through parse_source_variable_impl / parse_var_with_module_context
to instantiate_implicit_modules. parse_var and the ~40 macro-free /
test callers pass None -> a shared 'static empty registry (no extra
allocation, no Option handling at resolve_macro sites); LTM synthetic
equations are engine-generated and never carry macro calls so they pass
None too. The one simlin-cli model_module_ident_context caller is
updated for the Task 2 SourceProject parameter.
module_functions.rs's module-wide #![allow(dead_code)] is removed
entirely -- after Tasks 2-3 every pub(crate) item has a real production
caller and clippy is clean without it in all feature configs.
Verified: macros.AC2.1 (smoke), AC5.1 (arity), AC5.4 (SSHAPE +
RAMP FROM TO shadow), AC5.6 (unknown name), plus a structural synthetic-
module test and a contains_module_call macro-awareness test; the Task 2
AC5.2/AC5.3 end-to-end tests still pass; builtins_visitor (22) and the
simulate integration suite (55) confirm stdlib is unchanged.
`project_macro_registry` previously recovered the diagnostic's `ErrorCode`
by inspecting the build-error message shape
(`message.starts_with("recursive macro")` -> `CircularDependency`, else
`DuplicateMacroName`). The code is already known at build time --
`MacroRegistry::build` returns an `Error` carrying the correct typed
`code` -- so re-deriving it from prose downstream was fragile internal
string-coupling: a reword of either build error message in
`module_functions.rs` would silently mis-tag the diagnostic with no test
catching it.
Thread the typed code end-to-end instead. `macro_registry_build_error`
now returns `Option<(ErrorCode, String)>` taken directly from the `Err`;
`SourceProject::macro_registry_build_error` stores the pair; and
`project_macro_registry` tags the accumulated diagnostic with that exact
code. The user-facing message text and the project-level `NotSimulatable`
compile failure are unchanged. A new `db_macro_registry::tests` module
locks the propagation in with an oracle assertion (stored code ==
`MacroRegistry::build`'s own `Err.code`), verified red-green to fail if
the code is mis-tagged.
A Vensim multi-output macro invocation (total = add3(a, b, c : minv,
maxv)) has no plain-text equivalent -- a call returns several named
values at once -- so it cannot ride the single-output text-then-expand
path. It is now materialized at MDL import, before build_equation / the
XMILE formatter ever sees the multi-output Expr::App (the formatter
asserts output_bindings.is_empty()).
A new convert/multi_output.rs pass runs after the macro-marked models
are built (so their MacroSpecs are available) and before build_project's
per-symbol loop. For each main-model symbol whose top-level RHS is an
Expr::App with non-empty output_bindings it emits: one input-only
Variable::Module named {lhs}_macro (collision-safe and
serialization-stable -- deliberately not the compile-time-synthetic
prefix, since this is round-tripped), a primary-output binding Aux
replacing the LHS (reads {module}.{primary_output}), and one
additional-output binding Aux per :-list entry (the call-site name
becomes the ident, reading {module}.{macro_internal_output}). The
ASCII-period reference text canonicalizes to U+00B7 at compile-time
parse and resolves through the existing general get_submodel_offset
machinery. Unknown-macro and arity mismatches are ConvertError::Other
naming the macro.
Real Vensim multi-output models do not separately declare the :-list
names (the call creates them); the materializer additionally adds those
idents to skip_symbols defensively so a stray same-named declaration
cannot shadow the binding reference.
Phase 3 made instantiate_implicit_modules's apply-to-all path
macro-aware (contains_module_call), so an arrayed macro invocation rides
the existing per-element module-expansion machinery with no new
mechanism. This task is verification: a stockless arrayed fixture
(macro_arrayed: out[Region] = SCALE(inp[Region], factor)), an
expansion-level assertion that the invocation expands into one
independent synthetic Variable::Module per Region element (asserted
through the full compile pipeline by inspecting the per-element
$⁚out⁚n⁚scale⁚{elem}·scale body slots in Results.offsets, not a shared
instance), and an inline arrayed-stock test
(ACCUM(rate, init) = INTEG(rate, init), total[Region] =
ACCUM(rate[Region], 0), rate = [1, 3]) asserting each element integrates
its OWN rate independently (total[r1] = 0,1,2,3,4 vs
total[r2] = 0,3,6,9,12) -- proving per-element persistent state.
No arrayed-macro-specific gap was found: the existing path handles both
the stockless and stock-bearing cases, so no builtins_visitor.rs change
was needed. SCALE/ACCUM each take two parameters because a 1-arg MDL
call NAME(arg) is rewritten to LOOKUP before macro resolution (GH #553).
… models Tests-only early gate (the full tiered corpus harness + Vensim-reference comparison is Phase 7). THEIL (test/metasd/theil-statistics/Theil_2011.mdl): asserts the 2-input/13-output THEIL multi-output invocation materialized (one Variable::Module + 1 primary + 13 additional binding auxes), compiles, and runs to the end; ~40ms so it is a regular (non-ignored) test. SSTATS (the COVID model, #[ignore]d): asserts BOTH 2-input/10-output SSTATS invocations materialized (2 modules + 22 binding auxes) and that no macro-specific compile diagnostic fires -- the model's only blockers are its unrelated unresolved *_data GET-DIRECT references (no DataProvider supplied), so the assertion is narrowed per the phase plan and the GET-DIRECT-data blocker is reported for Phase-7 scope. C-LEARN (~53k lines, #[ignore]d): asserts all four macros (SAMPLE UNTIL, SSHAPE, RAMP FROM TO, INIT) import as macro-marked models with the correct MacroSpecs. C-LEARN surfaced a real, separately-tracked macro bug: its uninvoked INIT(x) = INITIAL(x) macro trips a FALSE recursive macro: init -> init in MacroRegistry::build. The MDL importer renames the INITIAL builtin to INIT (the engine's canonical initial-value intrinsic name), so the stored body becomes INIT = INIT(x); collect_called_macros then treats the renamed-builtin call as a recursive call to the same-named macro and fails the whole registry build, which cascades (no registry => SSHAPE / SAMPLE UNTIL / RAMP FROM TO stop shadowing builtins => BadBuiltinArgs / UnknownBuiltin at their call sites). In Vensim the body wrote INITIAL (a distinct name), so there is no recursion and C-LEARN runs; the clash is manufactured by the necessary rename. This contradicts the design's macros.AC6.2 / AC1.7 and needs a fix at the macro-recursion detector + shadowing precedence, which is out of this tests-only task's scope. The C-LEARN test asserts only the verifiable import facts plus the current buggy behavior (so a regression / the eventual fix surfaces here) and documents the blocker for follow-up.
Addresses three Phase 4 code-review findings on the MDL multi-output convert path. Important: the converter only detected a multi-output macro call when the `output_bindings`-bearing `Expr::App` was the *entire* RHS. The parser, however, syntactically accepts the invalid nested form (`y = 1 + ADD3(p, q, r : lo, hi)`) -- the inner App keeps its output_bindings. Nothing converted that to an error, so it reached the XMILE formatter: a debug/test build PANICKED on the Phase-2 `debug_assert!(output_bindings.is_empty())`, and a release build SILENTLY formatted `ADD3(p, q, r)` and dropped the `:`-list outputs with no diagnostic -- a debug-panic / release-data-loss split. multi_output.rs's comment asserted "a multi-output call cannot legally be a sub-expression" as an UNENFORCED invariant. `find_nested_multi_output_call` now walks each selected equation's expression tree (variant coverage mirrors the Phase-2 `rewrite_dollar_time` recursion) and rejects any output_bindings-bearing App in a non-top-level position with a `ConvertError` naming the macro and conveying the whole-RHS-only rule, run as a guard before `detect_multi_output_call`. The legitimate whole-RHS App is not flagged (only its args/`:`-outputs are descended for *nested* calls). This enforces the invariant the comment only asserted; the comment is rewritten to describe the guard rather than state a false unconditional invariant. Minor: cross-reference the now-filed GitHub issue #554 in the C-LEARN false `init -> init` recursion pin (comment-only; assertions unchanged) so a future engineer who fixes #554 finds and upgrades the test. Tighten `count_materialized_macro` to match the EXACT `{module}.{output}` binding form (first-period split, exact module-ident prefix, single non-empty output segment) instead of a `starts_with("{module}.")` prefix that could over-count an unrelated aux merely referencing a module output; the exact THEIL (==14) and SSTATS (==22) count assertions still pass unchanged.
…t predicate
Two Phase-4 cycle-2 re-review Minors, both completeness/accuracy fixes
to code introduced earlier in Phase 4.
The nested-multi-output guard in materialize_multi_output_invocations
scanned only select_equation's single representative pick (PurgeAFOEq
returns non_empty[0]). An arrayed variable carries one FullEquation per
element override and build_variable_with_elements formats every valid
per-element equation, so a nested multi-output call (`y = c + ADD3(...:
lo,hi)`) in a *later* per-element equation bypassed the guard, reached
the XMILE formatter, and tripped the Phase-2 debug_assert! (debug/test
panic, release silent :-output drop) -- the exact failure mode the
cycle-1 fix was meant to eliminate. The guard now scans every non-empty
equation (same is_empty_rhs predicate the build pass filters on); the
legitimate whole-RHS detection still keys off select_equation so the
cycle-1 scalar materialization behavior is preserved exactly.
The is_module_output_binding test helper's rustdoc claimed it rejects an
aux that merely references a module output inside a larger expression
(`mod.out + 1`), but the suffix-has-no-period check accepted it. Tighten
the predicate so the suffix must be a single bare identifier token
(ASCII alphanumeric or `_`) -- the verbatim form materialization emits
(`format!("{module}.{output}")`, output a bare macro-output ident) --
making the rustdoc accurate. THEIL (14) and SSTATS (22) binding counts
are unchanged, confirming the materialized form is genuinely bare.
Flesh out the previously-empty xmile::Macro stub into a real serde type mirroring the xmile::Model/xmile::Var patterns: a name attribute, ordered <parm> children (each an optional default), an expression-form <eqn>, an optional <variables> body, an optional per-macro <sim_specs>, <doc>, and a namespace attribute. The Eq derive is dropped (the new variables/sim_specs fields transitively contain f64). A <views> field is deliberately omitted -- a macro body's diagram is inert and quick_xml silently drops the unknown element on read. convert_file_to_project becomes fallible and, after building models from file.models, bridges each file.macros entry to a macro-marked datamodel::Model. The body is the <variables> if present, else the <eqn> normalized into a macro-named Aux (the AC1.3 expression-form requirement). Port synthesis and MacroSpec construction reuse the shared Model::new_macro helper -- identical to the MDL path, not re-implemented. Body variable idents are canonicalized to the engine ident form so they stay byte-identical to the canonical MacroSpec names and Model::new_macro's port matching, mirroring the MDL path's invariant. A non-empty per-macro <sim_specs> is the documented unsupported limitation and is rejected with a clear BadSimSpecs error.
Add a hand-written ToXml impl for xmile::Macro mirroring the Model::write_xml style (<macro name>, <eqn> with the primary-output name, <parm>s with optional default attrs, <variables> looping Var::write_xml, <doc>), and a From<datamodel::Model> for Macro bridge that excludes the synthesized parameter ports from <variables> (they are reconstructed from the <parm>s on re-import, so re-emitting them would break round-trip stability). File::write_xml now iterates the macros after the models. From<datamodel::Project> for File partitions models by macro_spec.is_some(): macro-marked models become top-level <macro> siblings, the rest stay <model>s. Whenever the project contains a macro, the <uses_macros recursive_macros="false" option_filters="false"/> header option is emitted -- this required adding the previously-absent <options>/feature writing to Header::write_xml plus a write_tag_empty_with_attrs helper for the self-closing form. The deterministic fixed "false"/"false" attributes keep the byte-stable round-trip stable. Fix the latent Feature::UsesMacros serde bug: the recursive_macros / option_filters fields lacked #[serde(rename="@...")] so they parsed as child elements instead of the spec-required attributes -- the reader and the new writer would have disagreed. A single-output macro emits a plain standard <macro> with no simlin: extension.
Standard XMILE has no multi-output-port or multi-output-call concept.
A multi-output macro definition's additional output ports already
round-trip via the <simlin:additional-outputs names="..."/> child on
<macro> (wired in Tasks 1-2 through MacroSpec.additional_outputs).
This adds the second extension: a multi-output *invocation*. Phase 4
materializes total = add3(a,b,c : minv, maxv) as an input-only
Variable::Module plus the LHS primary-output binding Aux and one Aux
per additional :-output. Standard <module> references a <model>, not a
<macro>, so the cluster round-trips through a single
<simlin:macro-invocation> element (module/macro names, input wirings,
and primary + additional output bindings) recorded on the <model>.
The project-level From<datamodel::Project> for File builds a macro
registry and, before the per-model bridge runs, extracts each
materialized cluster (a Variable::Module whose model_name resolves to
a macro-marked model, plus the binding auxes whose scalar equation is
exactly {module}.{output}) into the extension and removes the cluster
variables. The reader reconstructs the cluster exactly (ASCII-period
module-output separator, the Phase-4 datamodel convention), so an
XMILE->datamodel->XMILE round-trip is byte-stable. The extension is
emitted only for multi-output macros; single-output projects stay
standards-clean. Verified end-to-end including a cross-format
.mdl->XMILE->datamodel test on the macro_multi_output fixture.
Wires the four .xmile macro fixtures (macro_expression, macro_multi_expression, macro_multi_macros [two <macro> elements], macro_stock [stock-bearing macro body]) into simulate.rs's TEST_MODELS, so each runs through import -> simulate-vs-output.tab -> protobuf round-trip -> byte-stable XMILE round-trip. Adds a single-output cross-format test (.mdl -> datamodel -> .xmile -> datamodel) asserting the macro-marked Model + MacroSpec and the invocation equation survive the conversion (macros.AC4.4). Wiring the fixtures surfaced a real reader gap: the XMILE <macro> import path (macro_to_datamodel / Model::new_macro) did not order the macro-marked model's variables by canonical ident, whereas the <model> import path (From<xmile::Model> for datamodel::Model) does, and the protobuf serialization (From<Model> for project_io::Model) re-sorts every model's variables by that same key for deterministic encoding. A protobuf serialize -> deserialize round-trip is therefore identity only for projects already in canonical order; Model::new_macro appends the synthesized parameter ports after the body variables, so the macro model's round-trip was lossy and tripped simulate_path_with's assert_eq!(datamodel_project, datamodel_project2). Fixed at the root by sorting the macro model's variables by canonical ident in the reader, mirroring the <model> path exactly. Also corrected the Task 3 multi-output byte-stable test, which compared to_xmile of a hand-built (non-canonically-ordered) project against its reader-normalized round-trip; it now asserts the genuine fixed-point property a reader-produced project must satisfy (the same invariant simulate_path_with enforces).
…guard)
Three Minor code-review follow-ups from Phase 5; no behavior change.
convert_file_to_project deep-cloned the entire parsed File only to move
file.macros out afterward, copying every non-macro model on every XMILE
import. From<File> for datamodel::Project never reads file.macros, so we
std::mem::take the macros first and pass the rest of the File by move.
extract_macro_invocations builds a first-wins (or_insert_with) scalar-
equation index, so when two auxes share a "{module}.{primary_output}"
binding equation only the first becomes the extracted binding and any
aliases remain plain auxes in the residual model. That still round-trips
(the module output is reconstructed on re-import), but the nuance was
undocumented; added a note to the rustdoc.
reconstruct_macro_invocation hard-codes default documentation/units/
Compat for the reconstructed module and additional-output binding auxes.
That is correct because the Phase-4 materializer (multi_output.rs) is the
only producer and emits those defaults, but a future materializer change
that added module-level doc/units/compat would silently lose them through
the XMILE round-trip with no test catching it. Added cross-reference
comments at both sites and debug_assert!s at the producer so a lockstep
violation is caught in debug/test builds.
Remove the now-stale `macros (:MACRO:) -- the writer rejects them` exclusion line and add all 8 macro fixtures to TEST_MDL_MODELS: the 6 bundled macro_* fixtures plus the Phase-4-authored macro_multi_output and macro_arrayed. Each now flows through mdl_to_mdl_roundtrip's parse_mdl -> project_to_mdl -> parse_mdl -> assert_semantic_equivalence loop. Extend assert_model_equivalence to also compare Model.macro_spec (PartialEq) and Model.name. Without this a malformed-but-present MacroSpec would slip through: the body variables live in the macro model, so a missing macro model only changes the model count (already checked) but a wrong MacroSpec (bad parameters / outputs) would not. The comparison is a no-op for the non-macro fixtures (the main model is "main" with macro_spec: None on both passes) and was confirmed non-vacuous: dropping :MACRO: blocks fails all 8 fixtures, and corrupting only MacroSpec.parameters trips `macro_spec differs` with the model count unchanged. All 8 fixtures round-trip: macro_cross_reference / macro_multi_macros exercise multiple :MACRO: blocks, macro_stock a macro-body stock, macro_trailing_definition a block emitted before its invocation, macro_multi_output the `:` multi-output reconstruction (the materialized Variable::Module cluster survives), macro_arrayed an arrayed single-output invocation that stays as equation text.
…rationale
main_model is a pub(crate) "safe lookup" helper, but its fallback was
`.unwrap_or(&project.models[0])` -- a panicking index. It is unreachable
on every real path (both callers run after project_to_mdl's reject gate,
which rejects when the non-macro model count != 1, including the empty
0 != 1 case), but a safe-lookup helper should not carry a panicking-index
fallback even when unreachable. Return `Option<&Model>` and have the two
post-gate callers `.expect(MAIN_MODEL_EXPECT)`, turning the unreachable
panic into a loud, non-indexing assertion that names the invariant.
Observable behavior on every real (post-gate) path is unchanged.
Also expand the scalar_auxes first-wins comment in
build_multi_output_reconstructions to record the reviewer's
documented-intentional finding: the `or_insert` first-wins is unreachable
for binding-aux detection (a module's distinct outputs yield distinct
`{module_ident}.{output}` keys, so two bindings of one module never
collide), and it is retained only to mirror the documented first-wins of
xmile::model::extract_macro_invocations. Comment-only; zero logic change.
A Vensim macro whose body invokes an opcode-backed engine intrinsic (init, previous) whose canonical name equals the macro's own name triggered a false self-recursion cycle in MacroRegistry::build, which failed the entire macro registry and -- because an empty registry un-shadows every other macro -- blocked expansion of ALL of that model's macros (GH #554). Concretely: the MDL importer NECESSARILY renames the Vensim INITIAL builtin to INIT (xmile_compat.rs; Expr1 lowering recognizes only the short opcode name `init`, not `initial`), so C-LEARN's uninvoked `:MACRO: INIT(x) ... INIT = INITIAL(x)` is stored as the datamodel body `init = init(x)`. collect_called_macros canonicalized that renamed-builtin call to `init`, collided it with the registered `init` macro, and recorded a spurious `init -> init` self-edge -> a false CircularDependency that cascaded SSHAPE/SAMPLE UNTIL/RAMP FROM TO into BadBuiltinArgs/UnknownBuiltin. The fix is two coordinated halves sharing one predicate (module_functions::is_renamed_opcode_intrinsic, the set {init, previous} -- the only opcode-backed intrinsics with dedicated temp-arg routing in BuiltinVisitor::walk, and the rename targets of xmile_compat.rs's INITIAL->INIT / SAMPLE IF TRUE->PREVIOUS): Part A (collect_called_macros): do not record the macro self-edge when the called name canonicalizes to the enclosing macro's own canonical name AND that name is a renamed opcode intrinsic. Scoped strictly to the self-edge case: a genuinely self-recursive non-intrinsic macro (`foo = foo(x)`) and mutual recursion by non-intrinsic names are still rejected (macros.AC5.2 unweakened); a different macro merely named after an intrinsic still produces a real edge (`init -> previous -> init` still a rejected cycle). Part B (BuiltinVisitor::walk): when expanding a macro body, a call matching that same predicate resolves to the intrinsic instead of the macro-shadows-everything precedence -- without this an INVOKED such macro would infinite-loop (the body's `init(x)` would re-resolve to the macro forever). The enclosing macro's name is threaded via a new salsa-tracked macro_body_owner map (SourceVariable.model_name is a Module variable's referenced target, empty for non-Module vars, so it could not answer "which macro owns this body variable"). The Phase 4 C-LEARN pin (corpus_clearn_macros_import) is upgraded from pinning the buggy false-recursion to asserting the corrected behavior (four macros import + no macro-registry CircularDependency cascade); it stays #[ignore]d as the Phase 7 C-LEARN macro-expansion regression guard.
…macro Two code-review follow-ups to the #554 fix (b0ef57a), both zero-risk to the core fix logic. Minor #1 (doc-only): the defensive `continue` in `MacroRegistry::check_for_recursion` (skip a macro-marked model absent from `self.macros` -- only possible for an already-rejected duplicate) predates #554. Now that `from` doubles as the #554 `enclosing` self-edge carve-out fed to `collect_called_macros`, the comment was incomplete. Added one clause noting the skip fires BEFORE `collect_called_macros` is reached, so the (impossible) dropped model contributes no edges and `from` is never misused as the carve-out for it -- correct behavior, the comment just now says why. No logic change (the guard is byte-identical). Minor #2 (test-only): the existing end-to-end #554 coverage (`issue_554_invoked_macro_wrapping_own_init_intrinsic_compiles_and_runs`) only exercised Part B for `init`; `previous` was covered only at the registry-build level (Part A). Added the faithful `previous` mirror, `issue_554_invoked_macro_wrapping_own_previous_intrinsic_compiles_and_runs`: a macro canonically named `previous` whose body wraps its own `PREVIOUS` intrinsic (the importer's `SAMPLE IF TRUE` -> `PREVIOUS(SELF, init)` rename target), invoked alongside a sibling macro. PREVIOUS's verified signature is `PREVIOUS(input, initial)` (first step returns `initial` since `prev_values` is not yet valid, then `input`'s previous-timestep value -- vm.rs `LoadPrev`/`use_prev_fallback`, cross-checked against test/previous/output.tab); with constant port `x=9` and `k=4` over the t=0,1,2 run the series is `[4, 9, 9]`, hand-verifiable. The test passes with the committed #554 fix as-is: Part B's shared `is_renamed_opcode_intrinsic` predicate ({init, previous}) already covers `previous` end-to-end, so no production gap was surfaced and no core #554 logic was touched. The test's teeth were confirmed by temporarily restricting the Part-B predicate to `init` only -- the new test then failed with the expected infinite-recursion symptom (a salsa model_module_map dependency cycle) -- then restored. macros.AC5.2 stays unweakened (the adjacent genuine/mutual-recursion guards still pass).
Phase 7 Task 1 (macros.AC6.2, macros.AC6.3). Extends the #554-upgraded corpus_clearn_macros_import regression guard with the AC6.2 compile + diagnostics step: C-LEARN is open_vensim'd, synced and compiled via the salsa path, every diagnostic is collected, and a shared macro_attributable_diagnostics classifier asserts NONE is macro-attributable. The classifier (also used by the metasd corpus harness in Task 2) flags exactly three shapes -- a project-level macro-registry build error (the #554 cascade class), an Error-severity diagnostic inside a macro template body, and a macro-resolution-failure code on a macro model / project-level / co-occurring with a registry error -- and deliberately does NOT flag C-LEARN's allowed non-macro blockers (model-logic circular deps, dimension mismatches, unit warnings, and Phase 3's non-time $ reference, which surfaces as an ordinary unresolved-reference diagnostic). A focused classifier unit test pins this separation so neither harness can degrade into 'flags everything' or 'flags nothing'. Adds three focused C-LEARN-macro isolation fixtures (test/test-models/tests/macro_clearn_{sample_until,sshape,ramp_from_to}/) whose :MACRO: blocks are copied verbatim from C-LEARN and invoked with known constant inputs; each output.tab is hand-computed from the macro body formula, grounded in the engine's STEP/RAMP/INTEG semantics and documented in a README. INIT is not invoked anywhere in C-LEARN so it needs no focused model (AC6.2 covers the defined-but-never-invoked case). simulates_clearn stays #[ignore]d, but its comment no longer blames macro expansion: the macros now expand with zero macro-attributable diagnostics. The remaining blockers are C-LEARN's non-macro issues (compile_vm fails with NotSimulatable before the VM is built -- a model-logic CircularDependency on main.previous_emissions_intensity_vs_refyr, dimension mismatches, an unknown dependency, a non-time $ reference, plus unit warnings). These are out of scope for the macro work and tracked separately per the design.
Phase 7 Task 2 (macros.AC6.1, macros.AC6.4). AC6.1 (confirmation): verified all eight test/test-models/tests/macro_* fixtures are already wired and green -- the .mdl variants in simulate.rs (Phase 3) and mdl_roundtrip.rs (Phase 6), the four .xmile variants in simulate.rs (Phase 5). No earlier-phase wiring gap was found, so no change to simulate.rs / mdl_roundtrip.rs was needed. AC6.4: adds src/simlin-engine/tests/metasd_macros.rs, a tiered corpus harness over all 17 macro-using metasd .mdl files (14 directories, annotated TEST_SDEVERYWHERE_MODELS-style with per-model tier status). The expansion tier (open_vensim -> sync -> compile -> collect_all_diagnostics) asserts NO macro-attributable diagnostic; it is split into a fast default test (light models) plus #[ignore]d opt-in tests for the heavy real-world models and the full 17 (sum of compiles ~10s exceeds the per-test budget per docs/dev/rust.md). All 16 models that do not hit the known bug below produce zero macro-attributable diagnostics even though most have unrelated non-macro blockers (engine-unsupported RANDOM NORMAL in pink_noise bodies, model-logic issues, MDL-parse errors) -- those are surfaced as per-model annotations, not flagged, because the macro still expands (correct MacroSpec) and the body just hits the same engine gap a main equation would. The corpus uses a narrower AC6.4 macro-attributable classifier than Task 1's C-LEARN one (registry-build error or macro/model name collision only, NOT 'any Error in a macro body'), because metasd macro bodies legitimately use engine-unsupported builtins; the divergence and its rationale are documented at length in the module header. KNOWN MACRO BUG, ESCALATED: thyroid-2008-d.mdl hits a genuine macro-attributable failure -- :MACRO: DELAYN with body DELAYN = DELAY N(...) produces a false-positive 'recursive macro: delayn -> delayn' macro-registry CircularDependency. This is structurally the #554 false positive, but #554's is_renamed_opcode_ intrinsic fix is deliberately scoped to the opcode intrinsics init/previous and explicitly excludes stdlib-module-backed builtins like DELAY N (the #554 termination argument does not extend to them without a follow-up change to both #554 halves). It is NOT masked: excluded from the expansion-tier assertion explicitly (the other 16 are asserted) and pinned by a dedicated thyroid_known_macro_bug_is_macro_attributable test that flips when the engine fix lands. This needs a focused #554-follow-up engine fix and is escalated for issue tracking. Simulation tier: no metasd macro model is simulation-tier-eligible as of this phase (Theil_2011 compiles+simulates with zero errors but has no checked-in reference output -- a documented prerequisite; the models that carry a sibling .vdf, FREE/free 6.mdl and the groupon models, have heavy unrelated non-macro blockers). Every non-eligible model carries a documented, asserted-non-empty reason so a tracked blocker can never be silently dropped. A corpus-integrity test pins the list at exactly 17 files / 14 directories.
Phase 7 Task 3 (macros.AC6.5, macros.AC6.6). AC6.6: adds isMacroModel(model: Model): boolean to module-navigation.ts next to isStdlibModel (the model-level gating precedent) -- it returns model.macroSpec !== undefined -- and uses it in getAvailableModels' loop over project.models so a macro-marked model is never offered as a selectable module-reference target (it is also removed from the stdlib group in the unlikely case a user macro shadows a stdlib name). A macro-marked model is an ordinary project.models entry after import (the diagram receives it via projectFromJson automatically), but the engine materializes macro invocations directly -- inlined for single-output macros, a Variable::Module + binding auxes for multi-output ones -- so the diagram must not let a user point a module at a macro template. getAvailableModels is confirmed the only model-LIST surface (its sole consumer is ModuleDetails.tsx's <select data-testid=model-ref-select>; navigation is otherwise a drill-in stack with no flat list), so the one filter is sufficient. AC6.5: confirmed a macro-bearing project opens without crashing with NO production change. A new editor-open-project test drives openEngineProject with a project whose models include a macro-marked model (macroSpec + synthesized port variables) alongside main and asserts the open resolves, state.activeProject is defined, no error is surfaced, and the macro model round-trips through projectFromJson with its macroSpec intact. (TDD surfaced an initial fixture bug -- JsonAuxiliary uses , not -- not a real crash; the open path handles macro-marked models transparently as the architecture predicted.) Tests: module-details-utils.test.ts gains an isMacroModel describe block and getAvailableModels macro-filter cases (makeModel extended with an optional macroSpec); editor-open-project.test.ts gains the macro-bearing-project open test. All 871 @simlin/diagram tests pass; pnpm build and pnpm tsc are green.
Extends the #554 renamed-builtin self-call mechanism to the stdlib-module-backed builtins. #554 fixed the false-positive macro recursion where a macro whose canonical name equals an importer-renamed OPCODE-backed intrinsic (init/previous) and whose body calls that name was wrongly flagged as recursive; it was deliberately scoped to opcode-backed only because for init/previous the skipped self-call falls through to a terminal LoadInitial/LoadPrev opcode. The metasd thyroid-2008-d.mdl harness surfaced the same class for a stdlib-module-backed builtin: :MACRO: DELAYN(...) ... DELAYN = DELAY N(...). The MDL importer rewrites the body Vensim DELAY N(...) to the single-token XMILE DELAYN(...), colliding with the enclosing macro's canonical name; collect_called_macros recorded a false delayn -> delayn self-edge that failed the whole MacroRegistry::build and un-shadowed every other project macro (the #554 cascade), blocking AC6.4. The corrective behavior is unambiguous: a macro DELAYN whose body is DELAY N(...) is not recursive (Vensim macros cannot recurse and the source wrote the distinct builtin name). The fix adds is_renamed_stdlib_module_builtin (delegating to the authoritative builtins::is_stdlib_module_function so the suppression set cannot drift from the names that actually resolve to a stdlib module) and a combined is_renamed_builtin_macro_collision used by BOTH the Part A recursion check (collect_called_macros self-edge suppression) and the Part B builtins_visitor macro-shadows-everything precedence, keeping a single shared source of truth so the two sites agree by construction (the #554 design property, preserved and extended). Termination (verified against builtins_visitor::walk): the skipped self-call falls through to rewrite_alias_module_call/stdlib_descriptor and resolves to a Variable::Module whose model_name is stdlib<U+205A>... (delay1/delay3/...). The U+205A separator is not a legal Vensim identifier character and the importer never mints that prefix for a user model, so the stdlib model is necessarily distinct from the user macro's model; its fixed body never references the user macro, so compiling it does not re-enter the macro and the expansion terminates. An invoked such-macro compiles, runs, and computes DELAY N's defined behavior. AC5.2 is unweakened and #554 (init/previous, registry-level and invoked e2e) is not regressed: a genuinely self-recursive macro whose name is not a renamed builtin still records its self-edge, and a different macro merely named after a builtin still produces a real edge (A->B->A cycles stay rejected). thyroid-2008-d.mdl now passes the expansion tier with zero macro-attributable diagnostics (17/17), unblocking AC6.4; its residual in-body UnknownBuiltin (DELAY N with a macro-port order is the orthogonal pre-existing stdlib constant-order limitation, not a macro-handling failure) is the same gap a main equation hits.
Addresses three Phase 7 Tasks 1-3 code-review findings (1 Critical, 2 Minor) for the Vensim macro epic. CRITICAL -- metasd_macros missing required-features=["file_io"]. Cargo auto-discovers tests/metasd_macros.rs as a test target, but its simulation tier calls simlin_engine::load_dat / load_csv, both #[cfg(feature = "file_io")]-gated. Unlike every sibling file_io test (simulate, simulate_ltm, simulate_systems, systems_roundtrip, vdf_alias_decoder -- each carrying an explicit [[test]] required-features=["file_io"] block) it had none, so `cargo build -p simlin-engine --all-targets` and `cargo test -p simlin-engine --no-run` WITHOUT file_io failed with error[E0425] (load_dat/load_csv configured out). CI passed only by the fragile cross-crate feature-unification accident that simlin-cli requests simlin-engine/file_io. Adds the mirroring [[test]] entry so Cargo correctly skips the target when file_io is off; verified the single-package build/test now succeed without the feature and are unchanged with it. MINOR -- SAMPLE UNTIL fixture did not exercise the freeze. With a constant input=7 the INTEG stock reached 7 after step 1 and held, so the (1-STEP(1,lastTime)) gate force-zeroed an already-zero flow; sampled=[2,7,7,7,7,7] would be byte-identical with or without the lastTime gate, so the fixture only pinned the init->7 jump, not SAMPLE UNTIL's defining behavior (sample a CHANGING signal, then FREEZE it). The :MACRO: block is kept byte-verbatim from C-LEARN (verified with a diff against C-LEARN v77); only the invocation changed: the input is now the time-varying 5+RAMP(1,0,10) and lastTime=4 (init 99, a distinct sentinel), so the sample tracks the rising input through t=3 then freezes at 8 while the input keeps rising to 13 -- the frozen value is now distinct from both the init and every later input. output.tab and README were recomputed by hand from the verbatim macro body and the engine's actual STEP/RAMP/forward-Euler-INTEG semantics (vm.rs::step is time + dt/2 > step_time; vm.rs::ramp is 0 at time<=start; macro_stock pins the INTEG Euler convention), then confirmed by the existing simulate.rs test against the engine's VM output. A scratch check confirmed a model without the gate would now produce a different series (differs at t=5..8), so the fixture genuinely discriminates the freeze. MINOR -- the narrower metasd AC6.4 classifier had no non-vacuity pin. Unlike simulate.rs's classifier (pinned by macro_attributable_classifier_separates_macro_from_nonmacro) the intentionally narrower metasd copy (project-level registry-build error + name collision only -- narrower because metasd macro bodies legitimately hit unrelated engine gaps like RANDOM NORMAL) had no independent pin; thyroid_produces_no_macro_attributable_diagnostics only exercises the registry-error-ABSENT path, so the classifier could silently degrade with no test failing. Extracted the inline closure into a named narrower_macro_attributable_diagnostics (behavior byte-identical; mirrors simulate.rs's named-function + pin structure) and added narrower_classifier_flags_registry_error_but_not_in_body_unknown_builtin, which drives REAL diagnostics through the same salsa path the corpus harness uses: (a) a directly-recursive macro must be flagged (project-level Model CircularDependency, the #554 cascade class); (b) the literal metasd RANDOM-NORMAL-in-a-macro-body UnknownBuiltin must NOT be flagged. Verified genuinely non-vacuous: mutating the classifier to flag-everything fails (b) and to flag-nothing fails (a) (both observed, then reverted). The de-duplication of the two classifiers across compilation units is out of scope (tracked as GH #563).
Aligns the direction-(b) rustdoc fragment with the actual RANDOM NORMAL test construct per the comment-accuracy standard; the old NOTAFUNCTION/B example was a leftover draft never updated when the body became the real RANDOM NORMAL builtin. Comment-only, zero behavior change.
The macro work (Phases 1-7) added MacroSpec/Model.macro_spec as a first-class datamodel concept threaded through every serialization layer, two new top-level engine modules (module_functions.rs, db_macro_registry.rs), MDL/XMILE macro read+write, and a diagram-side gate. Update only the CLAUDE.md files whose documented module map, contracts, or known-gaps statements were made stale by this: - src/simlin-engine/CLAUDE.md: its own maintenance note mandates the module map stay current; added the two new sibling-of-db modules, the new mdl/convert submodules, the SourceProject/SourceModel salsa inputs, datamodel MacroSpec/Model::new_macro, the renamed module-call pre-scan predicates, XMILE/MDL macro round-trip, the new test harness/unit-test module, and the file_io gating. - src/simlin-engine/src/mdl/CLAUDE.md: removed the now-false 'macro conversion not implemented' Known Gap and listed convert/macros.rs and convert/multi_output.rs. - src/core/CLAUDE.md: documented the new Model.macroSpec contract and its round-trip helpers. - src/diagram/CLAUDE.md: documented isMacroModel and the macro-marked model exclusion from module-reference targets. Root CLAUDE.md, docs/README.md, and the engine/pysimlin CLAUDE.md files were left unchanged: they document no contract the macro work invalidated, and the macro design/reference docs they already link predate this branch.
The macro epic renamed equation_is_stdlib_call -> equation_is_module_call and contains_stdlib_call -> contains_module_call, but the shared rustdoc on is_stdlib_module_function still named the old functions. Caught by the project-context verification pass; the project comment standard requires nearby comments to be updated in lockstep with renames.
![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: 80530bae03
ℹ️ 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".
Comment on lines
+1502
to
+1505
| let Some(&(primary_ident, primary_doc, primary_units)) = | ||
| scalar_auxes.get(primary_key.as_str()) | ||
| else { | ||
| continue; |
There was a problem hiding this comment.
When MDL export sees a macro-backed Module without a matching primary-output binding aux, this branch continues and leaves the module unreconstructed. Later, write_variable_entry always skips Variable::Module, so the invocation is silently omitted from the exported .mdl instead of failing fast. This can happen if a user edits or partially patches a materialized multi-output macro cluster (e.g., deletes/renames one binding aux), and it causes silent model corruption in exported files.
Useful? React with 👍 / 👎.
Code review: PR #564 (Vensim/XMILE macro support)I reviewed the core logic of this PR (the unified [P2] Macro recursion check misses calls in multi-element arrayed macro bodies
Each body variable's call edges are collected by parsing Overall correctness: correct, with one non-blocking robustness gap. Single-output and scalar multi-output paths, serialization, and round-trip are sound and well-tested for valid models. The recursion-check blind spot above is the only substantive issue and is reachable only via invalid (recursive) macro input; it does not affect results for valid models. |
This was referenced May 15, 2026
Records four process/CI observations surfaced while reviewing the macro epic: the missing CI feature matrix (which let a no-file_io build break propagate), CLAUDE.md changelog decay, back-loaded corpus validation, and the implementation plan being untracked during execution. Filed per the project's track-discovered-issues convention; the separator-fragility hazard went to GH #565 and the DSS-oracle policy gap to a #561 comment.
Review: Add Vensim macro (
|
project_to_mdl's reject gate admits any Variable::Module whose model_name resolves to a macro-marked model, on the assumption the writer will reconstruct it into the Vensim ':' call syntax. But that assumption only holds for a pristine Phase-4 materialized cluster. The datamodel is mutable post-import (MCP ProjectPatch delete/rename/upsert; JSON/protobuf round-trip then edit), and a single realistic edit -- deleting or re-equationing one binding aux, dropping a ModuleReference -- leaves the cluster unreconstructable. build_multi_output_reconstructions then hit one of three continue paths AFTER confirming the module is macro-backed, the module was not added to the suppress set, and write_variable_entry's Variable::Module(_) => return silently emitted nothing: the invocation vanished and the surviving binding auxes dangled on an unwritten module -- a corrupt .mdl with no error. Per Simlin's mission, AI agents patch models programmatically via MCP, so this is a reachable path for the primary user, not a theoretical one. The asymmetry was the defect: the same gate already hard-rejects an ordinary Variable::Module with a clear error, and the design rejects recursive macros explicitly rather than mishandling them. A malformed macro cluster being silently dropped is the same failure mode the design everywhere else refuses to allow; the phase_06 'harmless fallback' note was written for the well-formed case and never contemplated post-import mutation. build_multi_output_reconstructions now records each unreconstructable macro-backed cluster (with an actionable message naming the module, the macro, and the missing argument/primary/additional binding) instead of silently skipping it; write_equations_section returns Result and rejects before emitting any output, so write_project (already Result-returning) propagates a clear error. Detection stays co-located with reconstruction -- single source of truth, no gate/writer drift. Addresses the PR #564 codex review P1.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b200eab8a
ℹ️ 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".
Comment on lines
+1294
to
+1296
| // Only the Symbol branch handles this: a `:` in a | ||
| // `CallKind::Builtin` call is not valid Vensim, so the | ||
| // Function branch deliberately does not parse it. |
There was a problem hiding this comment.
This parser now accepts macro definitions whose names shadow builtins, but call-site : output bindings are only parsed in the TokenKind::Symbol branch. If a multi-output macro is named like a builtin (for example SMTH1), its invocation tokenizes as Function, and SMTH1(a, b : lo, hi) fails because the Function path never consumes : bindings before expecting ). That makes valid multi-output macro calls unimportable whenever the macro name collides with a builtin, despite the rest of this change explicitly supporting builtin-name macro shadowing.
Useful? React with 👍 / 👎.
Code review — Vensim macro supportReviewed the substantive surface (engine MDL/macro conversion, the unified [P2] Whole-RHS multi-output call on a subscripted LHS is silently materialized as a scalar
[P3] Output-binding aux idents are not canonicalized, inconsistent with the rest of the cluster
The Overall correctness verdict: essentially correct. No P0/P1 issues; the macro registry, recursion fix, protobuf/JSON/XMILE round-trip, and UI filtering are sound. The P2 above is a real silent-data-loss path for an uncommon arrayed-caller input and an unenforced documented invariant the author would likely want closed (with a |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #564 +/- ##
==========================================
+ Coverage 82.69% 82.82% +0.13%
==========================================
Files 247 252 +5
Lines 65692 67464 +1772
==========================================
+ Hits 54322 55877 +1555
- Misses 11370 11587 +217 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Four genuine findings from the PR #564 automated reviews, each fixed test-first. The Codex P1 on writer.rs ("reject malformed macro clusters") was already closed by 5b200ea and is left untouched (verified by its three project_to_mdl_rejects_macro_cluster_* tests); a distinct adjacent gap -- a Variable::Module inside a macro-body model is silently dropped on MDL export because the project_to_mdl gate scans only the main model -- is out of scope here and tracked as GitHub #566. - parser.rs: the Function branch now parses an optional ':' output list. A multi-output macro whose name shadows a builtin tokenizes as Token::Function (the normalizer has no macro-registry awareness), so such a macro was definable but its multi-output call site was a hard parse error. A ':' in a call is unambiguously a macro invocation (no Vensim builtin uses ':'), so the call re-tags as CallKind::Symbol, exactly what the Symbol branch produces. - module_functions.rs check_for_recursion: scan each arrayed element formula (and the EXCEPT default) individually instead of the '\n'-joined Equation::source_text(), which does not reparse as one expression and silently dropped every macro-call edge of an arrayed body variable -- a recursion cycle closed in a per-element equation passed registry build and then hung/overflowed the depth-limit-free expander instead of reporting CircularDependency. - multi_output.rs: a whole-RHS multi-output call on a subscripted (arrayed) LHS now yields a ConvertError instead of being silently materialized as a scalar binding (which discarded the subscript and every sibling per-element equation). Arrayed multi-output invocation is a documented scoping limitation, so it must be an actionable diagnostic like the nested-call guard, not silent corruption. - multi_output.rs: ':'-output binding idents use variable_ident (the lowercased canonical form) like every other ident in a materialized cluster, instead of a case-preserving quoted_space_to_underbar, restoring byte-stable MDL/XMILE export.
|
Review: PR #564 — Vensim/XMILE macro support Reviewed the ~6,400-line core change (49 source files) across the engine resolver/inlining core, MDL import/convert, MDL+XMILE serialization round-trips, and the datamodel/serde/JSON/TS/Python surface. The implementation is unusually defensive and well-tested: the protobuf change is wire-compatible (new tag [P3] Multi-output guard only inspects
Overall correctness: correct. No P0–P2 issues found. Existing code/tests are not broken and the protobuf change is backward-compatible. The single P3 is a defensive-hardening gap on a contrived, non-valid-Vensim input path, not a blocking defect. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
:MACRO:/ XMILE<macro>support. Macro definitions import as macro-markeddatamodel::Models, persisted via aMacroSpecmarker onModelthrough protobuf, Rust/TS/Python JSON, and the JSON schema. Single-output macros inline through the compiler'sBuiltinVisitor; multi-output (:-list) invocations materialize at import. Round-trips through MDL and XMILE (the latter viasimlin:vendor extensions for multi-output, plus a<uses_macros>header option).ModuleFunctionDescriptor/MacroRegistryresolver (module_functions.rsplus adb_macro_registry.rssalsa query) shared by stdlib functions and user macros, with a single shared renamed-builtin-collision predicate so the pre-scan and walk-time paths cannot drift. Diagram UI excludes macro-marked models from the model-reference dropdown and standalone navigation.macros.AC1.1-macros.AC6.6) are covered by non-vacuous automated tests, with classifiers non-vacuity-pinned in both directions. Includes two user-approved in-scope prerequisite bug fixes: GH engine: macro whose body wraps a same-canonical-name intrinsic (INIT = INITIAL(x)) causes false recursion cycle, blocking C-LEARN macro expansion #554 (falseinit -> initmacro-recursion blocking C-LEARN) and a engine: macro whose body wraps a same-canonical-name intrinsic (INIT = INITIAL(x)) causes false recursion cycle, blocking C-LEARN macro expansion #554-classDELAY N-> delayn bug blocking AC6.4.Design:
docs/design-plans/2026-05-13-macros.mdImplementation plan:
docs/implementation-plans/2026-05-13-macros/Test Plan
Automated coverage is exhaustive at the engine / datamodel / serialization / round-trip layer and is gated by the pre-commit hook (Rust fmt+clippy+test, TS lint+build+tsc+test, pysimlin) -- green on every commit on this branch.
Human verification (full plan:
docs/test-plans/2026-05-13-macros.md):maindiagrams render without crashsimlin:extensions present, optional Vensim DSS numeric comparison15 follow-up issues filed for out-of-scope / design-deferred items (#551-#563; #554 is fixed in this branch). The metasd simulation tier and DSS-grade
.vdfreferences are documented prerequisites (GH #561), not coverage gaps.