Skip to content

engine: variable-vs-dimension name collision is never refused at definition; each spelling resolves it differently (plain read accepted, PREVIOUS/INIT refused, bare read silently yields the element index) #1050

Description

@bpowers

Summary

A variable whose canonical name equals a dimension's canonical name (other vs Other) is never refused where it is defined. The XMILE spec forbids the collision outright (v1.0 section 3.7.1 Arrays: dimension names "must be distinct from model variables names within the whole-model", docs/reference/xmile-v1.0.html), and several lowering sites assume the rule holds (the comment at Expr2::from's SIZE arm in src/simlin-engine/src/ast/expr2.rs says so explicitly: "we don't need to disambiguate between a dimension and variable with the same name - that's an invalid model per the spec. We check dimension names first"). Nothing enforces it, so every consumer that meets the ambiguous name resolves it on its own, and the same read is accepted or refused -- with a different code -- depending purely on the spelling that reaches it.

This breaks the invariant PR #1040's design doc states for Phase 7.5 (docs/design-plans/2026-08-25-compiler-unification.md, "helper == plain spelling"): a PREVIOUS/INIT helper reads what the plain spelling reads and is refused the way the plain spelling is refused. Here the plain spelling is accepted and the helper is refused, or the two are refused with different codes.

Reproduction

Minimal XMILE (files at the bottom); simlin simulate <file>. Dimension Other = {a, b}.

Arrayed collision -- <aux name="other"><dimensions><dim name="Other"/></dimensions><eqn>1</eqn></aux>, read by y:

y's equation main d04593e6 branch compiler-unification-v2 d9de586a (PR #1040)
define other[Other] only accepted, no diagnostic same
y[Other] = other (bare, array ctx) compiles; y = [1, 2] -- the element index of dimension Other, not the variable's value [1, 1] same
y[Other] = other[Other] compiles; y = [1, 1] (reads the variable) same
y[Other] = PREVIOUS(other) NotSimulatable: "PREVIOUS requires a variable reference after helper rewriting" (codegen) does_not_exist
y[Other] = INIT(other) NotSimulatable: "INIT requires a variable reference argument" (codegen) does_not_exist
y[Other] = PREVIOUS(other[Other]) NotSimulatable: "PREVIOUS requires a variable reference after helper rewriting" same
y[Other] = INIT(other[Other]) NotSimulatable: "INIT requires a variable reference argument" same
y = other[a], y = PREVIOUS(other[a]) compile compile
y = SUM(other[*]), y = PREVIOUS(SUM(other[*])) compile compile

Scalar collision -- <aux name="other"><eqn>1</eqn></aux> (no dimensions):

y's equation main branch
define scalar other only accepted, no diagnostic same
y = other dimension_in_scalar_context ("Dimension 'other' cannot be used in a scalar equation") same
y = PREVIOUS(other) / y = INIT(other) dimension_in_scalar_context same

So for one model shape the outcomes range over: silent wrong numbers (bare read yields the element index), accepted (subscripted plain read), NotSimulatable from codegen with an internal-sounding message, DoesNotExist (branch), and DimensionInScalarContext -- and none of them points at the definition that is actually wrong. A release binary built from main on 2026-08-07 accepted PREVIOUS(other[Other]) and INIT(other[Other]), so main's own answer has also drifted over time; nothing pins it.

Why it matters

  • Correctness: y[Other] = other with an arrayed variable other silently produces the dimension's element indices. Per the spec the model is invalid; the one acceptable outcome is a loud refusal, and this is the silent-wrong-number outcome.
  • Diagnostics consistency: the plain read and the PREVIOUS/INIT helper of the same read disagree, contradicting the Phase 7.5 invariant; the branch also changes one of the codes (NotSimulatable -> DoesNotExist for the bare captured spelling) without any test noticing, because no test enumerates the collision.
  • Developer/user experience: "PREVIOUS requires a variable reference after helper rewriting" describes compiler internals and blames the reference; dimension_in_scalar_context blames a reference to a variable the user defined. The actionable fact -- "other is both a variable and a dimension" -- is never said.

Where the decisions are made (all dimension-first, none guarded by a definition-time check)

  • src/simlin-engine/src/ast/expr2.rs, Expr2::from, Expr1::Var arm: is_dimension_name && !is_array_context -> DimensionInScalarContext.
  • src/simlin-engine/src/ast/expr3.rs, IndexExpr3::from IndexExpr2::Expr arm: a bare name that is a dimension name becomes IndexExpr3::Dimension (element precedence, but not variable precedence).
  • src/simlin-engine/src/compiler/context.rs, the Var lowering: a dimension name in scalar context -> DimensionInScalarContext; in array context -> the element index.
  • src/simlin-engine/src/builtins_visitor.rs: index_spans_a_dimension, classify_snapshot_index, arg_needs_element_scope all ask "is this a dimension name" before "is this a variable", so PREVIOUS(other[Other]) is classified as a direct slot read whose base the compiler cannot then address, and codegen refuses it at src/simlin-engine/src/compiler/codegen.rs (static_slot returning None -> the two NotSimulatable messages above).
  • src/simlin-engine/src/db/diagnostic.rs, model_duplicate_variables / emit_duplicate_variable_diagnostics: the definition-time collision check that exists for variable-vs-variable (DuplicateVariable, GH engine: legacy ModelStage0 construction still silently collapses duplicate canonical variable idents (last-wins) #891) but has no variable-vs-dimension counterpart.

Possible approaches

  1. One definition-time refusal (preferred): next to model_duplicate_variables, derive the set of variables whose canonical name is also a canonical dimension name and emit one Error-severity diagnostic at the variable (a new code such as VariableShadowsDimension, or DuplicateVariable with details naming the dimension), and have compile_project_incremental refuse the model the way it refuses duplicate variables. Every downstream dimension-first rule then becomes sound by construction, and the per-spelling codes above never fire for this shape. The Expr2::from comment's assumption becomes a checked invariant.
  2. If (1) is judged too strict for imported models (MDL corpora may carry the collision), at minimum route the capture path through the same refusal the plain spelling gets -- a parse/lowering-time code at the argument span -- instead of letting codegen refuse with NotSimulatable, and make the bare array-context read refuse rather than silently read the element index.

Whichever is chosen, the test should derive its rows from the spelling enumeration (definition only; bare in array context; subscripted; scalar context) x wrapper (plain; PREVIOUS; INIT; a module-function argument such as SMTH1) x variable shape (scalar; arrayed), asserting one code and one span for every row, on compile_project_incremental and on the CLI. Today no test covers any row of that table.

Discovery context

Out-of-scope discovery in the Phase 7.5c+d implementer report of PR #1040 (compiler-unification-v2); reproduced on origin/main d04593e6 and on the branch at d9de586a with fresh debug builds of simlin-cli. Pre-existing on main; the branch changes only the bare captured spelling's code (NotSimulatable -> DoesNotExist).

Files

Arrayed collision (plain, prev, init variants differ only in y's equation):

<?xml version="1.0" encoding="UTF-8"?>
<xmile version="1.0" xmlns="http://docs.oasis-open.org/xmile/ns/XMILE/v1.0">
  <header><vendor>test</vendor><product version="1.0">test</product></header>
  <sim_specs><start>0</start><stop>2</stop><dt>1</dt></sim_specs>
  <dimensions>
    <dim name="Other"><elem name="a"/><elem name="b"/></dim>
  </dimensions>
  <model>
    <variables>
      <aux name="other"><dimensions><dim name="Other"/></dimensions><eqn>1</eqn></aux>
      <aux name="y"><dimensions><dim name="Other"/></dimensions><eqn>PREVIOUS(other[Other])</eqn></aux>
    </variables>
  </model>
</xmile>

Scalar collision: drop the <dimensions> element from other and use <aux name="y"><eqn>PREVIOUS(other)</eqn></aux>.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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