Skip to content

engine: macro-attributable Diagnostic classifier is duplicated across a crate compilation boundary (lockstep enforced only by a comment) #563

@bpowers

Description

@bpowers

Summary

The "is this Diagnostic macro-attributable?" classifier predicate is copy-pasted across a crate compilation boundary, byte-for-byte logic-identical, with only a hand-written "kept in lockstep" comment guaranteeing the two copies stay synchronized:

  • src/simlin-engine/src/macro_expansion_tests.rs:1015-1037 -- an in-crate #[cfg(test)] lib unit-test copy (comment: "kept in lockstep with tests/metasd_macros.rs")
  • src/simlin-engine/tests/metasd_macros.rs:357-384 -- a separate integration-test binary copy (the AC6.4 classifier), a different compilation unit

Both copies implement the identical filter: a project-level macro-registry build error (is_project_level && DiagnosticError::Model && code ∈ {CircularDependency, DuplicateMacroName}) OR a macro/model name collision (code ∈ {BadModelName, DuplicateMacroName} && (in_macro_model || is_project_level)), returning registry_build_error || name_collision.

This is currently correct -- both copies are logic-identical and each is independently verified. This is preventive DRY hardening, not a present defect.

Why it matters

There is no mechanical guarantee the two copies stay in sync. They live in different compilation units (lib #[cfg(test)] vs. the tests/ integration binary), so the compiler cannot catch divergence. A future change to one copy -- e.g. adding project-level BadModelName handling, or refining the macro-attributable definition -- could silently diverge from the other, weakening the thyroid / C-LEARN macro-attributable regression guards (macros.AC6.2 / macros.AC6.4) without any test failing. That is a silent erosion of the macro-validation safety net.

The classifier is the load-bearing predicate behind the Phase 7 metasd expansion tier and the C-LEARN no-macro-error assertion of the Vensim macro epic, so a silent drift here directly undermines the epic's regression coverage.

Components affected

  • src/simlin-engine/src/macro_expansion_tests.rs
  • src/simlin-engine/tests/metasd_macros.rs

(simlin-engine; post-implementation test-infrastructure hardening for the Vensim macros epic, docs/implementation-plans/2026-05-13-macros/.)

Suggested remediation

Extract the macro-attributable-diagnostic classifier into a single shared crate-public helper (e.g. a #[cfg(any(test, feature = "..."))]-gated crate::<module>::is_macro_attributable(&Diagnostic, &macro_model_names) -> bool) callable from BOTH the in-crate #[cfg(test)] lib test AND the tests/ integration binary. This eliminates the duplication and enforces the lockstep at the type/compilation level -- mirroring the single-source-of-truth principle the delayn fix itself applied to is_renamed_builtin_macro_collision.

Design consideration: exposing a test-only helper across the lib -> integration-test boundary needs an appropriate #[cfg]/visibility approach (a plain #[cfg(test)] item is not visible to a separate tests/ integration binary; the helper must be reachable from the normal crate surface under a test/feature gate, or moved into a shared non-test module).

Discovery context

Surfaced in the code review of the Phase 7 delayn (#554-follow-up) fix (commit 029cf97f) for the Vensim macros epic. Reviewer verdict was APPROVED with this as a non-blocking Minor, explicitly: "Suggest tracking via track-issue rather than blocking this merge ... File a tracked tech-debt item for Minor #1 (cross-compilation-unit classifier duplication) so the lockstep is enforced mechanically, consistent with this repo's track-issue convention."

Out of scope for the macros epic implementation itself; tracked as post-implementation test-infra hardening.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions