fix(semantics): one unit-name resolution rule for every evaluator, plus prompt scope, %calc arguments and quantity rendering

[devin-ai-integration]

Summary

Four quantity/name defects, all about where a unit name is looked up and how a quantity is printed.

Defect 1 — adjudication: (a), the unit position is an ordinary feature reference

x [u] is the invocation Quantities::'['(num: Number, mRef: ScalarMeasurementReference), so u is an ordinary operand expression. KerML gives resolution one rule for such a name and no type-directed variant of it:

• 8.2.3.5.1 Name Resolution Overview — resolution produces the element a name denotes; the expected type of the position is a separate, later question (conformance of what resolved).
• 8.2.3.5.3 Local and Visible Resolution — a simple name is resolved first against the names locally visible in the namespace it is written in, which includes the namespace's own members.
• 8.2.3.5.4 Full Resolution — only if local resolution finds nothing does resolution continue to the outer namespace. Nothing filters candidates by type, so a nearer non-unit is not skipped in favour of an imported unit.

So a sibling attribute m legitimately shadows an imported SI::m, and the constraint path was the wrong one: it reached past the nearer declaration and silently converted in metres. The four paths now share one routine, semantics.(*Model).unitTermOfName, which resolves once and then checks conformance:

sym := resolve(scope, qn)            // nearest declaration (8.2.3.5.3/.5.4)
if !m.IsMeasurementUnit(sym) {       // conformance of what resolved (8.2.3.5.1)
    return m.shadowedUnit(qn, sym)   // typed *ShadowedUnitError, unwraps to ErrNotAUnit
}

The condition path stopped diverging because runtime.(*Context).chainMembers now gives a body member the scope of the body that declares it (bodyScope), which is what the slot/action/calc paths already did — not because three call sites were patched.

Since option (a) makes this a diagnostic a user will hit while writing a perfectly reasonable dynamics model (m is mass), the message has to explain itself. It names the declaration, its kind, the namespace declaring it, the unit it hid, and the spelling that reaches that unit:

not a measurement unit: m resolves to the attributeUsage m declared in SH3::K1,
shadowing the measurement unit SI::metre — write SI::m to name the unit

All four paths now produce exactly that, and 1000.0 [kg] next to the shadowing m is unaffected. A sibling that is a unit still works; a name nothing declares still fails as unresolved unit furlong.

Defect 2 — the prompt evaluated in a synthetic root scope

%eval built a document-root scope, which reaches the loaded document's members but is not the namespace that declares the imports, so every unqualified unit failed while SI::m worked. Session.promptScope now returns the scope of the namespace the session is working in — the namespace a member typed at the prompt would be written in — and %eval/%calc evaluate there. Nothing about units is hard-coded; mass * 2 over a package member resolves for the same reason, which closes ROADMAP B2 as well. lookupSymbol falls back to that scope, so %eval m reports that a unit holds no value instead of that it does not exist.

Defect 3 — %calc split its arguments on whitespace

The argument tail is now parsed as a list of expressions with the REPL's own parser, so an argument containing spaces is one argument:

%calc Fall -15.0 [m/s] 8.5 [s]              → Fall(-15.0 [m/s], 8.5 [s]) = 372.50 [m]
%calc Fall(-15.0 [m/s], 8.5 [s])            → same (invocation form accepted)
%calc Fall (-5.0 [m/s] - 10.0 [m/s]) 8.5 [s] → same (parenthesized subexpression)

Arguments may be separated by a comma or by whitespace, and a whole invocation's parentheses are unwrapped. Because whitespace is no terminator in the notation, the argument list is cut into argument texts before the expression parser runs (splitArgs): it splits at top-level commas and top-level whitespace, then rejoins a fragment onto the one before it only where it continues that expression — it opens a unit or index bracket, or either side is not a complete expression on its own. So 5 -3 is two arguments while 5 - 3 is one subtraction. This needed one parser accessor: Parser.Offset(), the offset the parser stopped at — an expression's own span is not that offset for a parenthesized expression, which is why the first attempt left ") 8.5 [s]" behind.

Named arguments are not supported (%calc Fall v0=-15.0 [m/s]). The notation writes those inside an invocation's parentheses as a different production; the prompt reports named arguments are not supported here; pass arguments positionally rather than misreading v0=-15.0 as an expression.

Defect 4 — rendering

• conditionText had no *ast.IndexExpr case, so a violated assertion said index > index. It now renders the bracket form as a quantity (1.0 [m] > 500.0 [m]) and #(…) as a sequence index — a printer fix, not a message fix.
• A quantity's magnitude went through %v, printing -15.200531548598184 [m/s] beside a bare -15.20. Quantity.TextWithMagnitude(magnitude) takes the already-formatted magnitude, so the REPL and the trace formatter each pass their existing Real formatting. Stored values are untouched; this is display only.

Tests

• Conformance, internal/core/runtime/testdata/conformance/: unit_shadowed_by_sibling_{slot,action,calc,constraint} assert the same diagnostic from all four evaluators with a same-named non-unit sibling present; unit_shadowed_by_local_unit a sibling that is a unit; unit_undeclared a name declared nowhere. The harness gained an expected-error field on a value/result (documented in the fixtures' README).
• Conformance: unit_shadowed_by_package_member — the shadow declared in the namespace that declares the import, whose diagnostic must still name the hidden unit (that lookup is a re-resolution with the shadowing declaration hidden, Resolver.LookupNameExcluding, not a parent-scope search).
• Robustness: quantity_unit_shadowed_by_sibling — typed *semantics.ShadowedUnitError (via errors.As), naming the declaration and the hidden unit, never a panic and never a magnitude in the wrong unit.
• REPL: TestEvalResolvesImportedUnitsUnqualified, TestCalcParsesExpressionArguments (quantity, invocation form, parenthesized subexpression, nested call, named-argument limitation), TestCalcSeparatesSignedArguments (a negative second argument with whitespace and with a comma, plus the merged 5 - 3 case), TestPromptScopeIsTheLastNamespaceDeclared, TestFormatValueQuantityUsesRealFormatting.
• Rendering: TestViolationRendersQuantityOperands (both a simple and a composed unit in a failed assertion).

Docs: docs/SPEC_COMPLIANCE.md gains the unit-position resolution rule with the KerML citations, the prompt-scope rule, the %calc argument rule (⚠️ approximate: whitespace separation and named arguments), the prompt-scope rule (⚠️ approximate: the last namespace declared wins), and the two rendering rules. docs/ROADMAP.md A3a's two residual items are closed and B2 is closed.

Gates

gofmt -l .                                                      (empty)
go build ./...                                                   ok
go vet ./...                                                     ok
staticcheck ./...                                                ok
go test -race -count=1 ./...                                     ok (all packages)
go test -run TestStdlibConformance ./internal/core/libs           ok
./scripts/download-training-examples.sh                          100 files
go test -count=1 ./internal/core/model -run TestTrainingExamples  ok — 98/100 clean

Reproduction fixtures — honest note

This VM was not the Systemica snapshot: no Go toolchain, no clone, and none of /home/ubuntu/checks/*.sysml or /home/ubuntu/repos/lunar-lander-simulation existed. I recreated sh1.sysml, sh3.sysml, cq.sysml and q4.sysml from the descriptions in the task and reproduced every case on main @ e5ebc7c with them before changing anything, then re-ran them on this branch.

Not verified, because the files do not exist here: descent_q_full2.sysml / descent_q_full.sysml (the 165-step descent numbers), touchdown_bound.sysml, and the lunar lander model's zero-diagnostic load. The equivalent in-repo cases are green: action_body_quantity_descent (the unit-carrying Euler descent), requirement_quantity_converted_unit (the km/h→m/s conversion canary) and the rest of the *_quantity_* conformance set, plus the whole race suite. Those four external checks still need running by someone who has the files.

Verified through the binary

The REPL was driven interactively against bin/sysml, A/B against a binary built from e5ebc7c, over all four evaluator paths, the prompt cases, the %calc accepted/malformed matrix and both rendering probes — screenshots in a comment below. Two findings from that run are fixed here: the signed %calc argument above, and the dropped remedy clause for a package-level shadow.

Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/07010afb905443ca9b6fc9f657882eba
Requested by: @HuiJun

[devin-ai-integration]

Original prompt from Devin Bot

Repo: https://github.com/JPL-Devin/Systemica (local clone at /home/ubuntu/repos/Systemica, base branch main).

Read AGENTS.md first and obey it: correctness over expedience, root cause not symptom, immutable AST,
semantics in side tables, runtime consumes lowered IR, error timing is part of the contract, no dead code,
conventional commits, and the four-layer test contracts in §5. Do not weaken or delete tests. Never
regenerate internal/core/model/testdata/training_examples_expected.txt (baseline is 98/100 clean, the two
remaining files hold four pinned OMG source errors).

Context: PR #104 fixed action/state body expressions to evaluate in their declaring scope. Quantities now
work end to end inside action bodies (a full unit-carrying Euler descent runs and matches its bare-Real
twin). What is left is a cluster of quantity/name defects that #104 did not touch. They are all about where
a unit name is looked up and how a quantity is rendered. Reproduce every case below on current main
before changing anything; do not trust this description over the binary.

Build with make build (or go build -o /tmp/sysml ./cmd/sysml) and drive the REPL as described in
.agents/skills/testing-sysml-repl/SKILL.md.

============================================================
DEFECT 1 (the important one) — a sibling member named like a unit breaks the unit, inconsistently

/home/ubuntu/checks/sh1.sysml and sh3.sysml reproduce it. Given public import SI::*; and a body that also
declares a member named m, the same quantity expression behaves differently per evaluator:

part probe {
attribute m : ISQBase::MassValue = 1000.0 [kg];
attribute h : ISQBase::LengthValue = 500.0 [m];
}
%instantiate probe; %slots probe
m = 1000 [kg]
h: <error: slot probe.h: not a quantity expression: not a measurement unit: m>

action act { attribute m : ISQBase::MassValue = 1000.0 [kg];
attribu... (9474 chars truncated...)

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR that start with 'DevinAI' or '@devin'.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[devin-ai-integration]

End-to-end verification through the REPL binary

Driven interactively in a terminal against bin/sysml, with a contrast binary built from the parent
commit e5ebc7c run on identical input. Nothing panicked or hung.

All four evaluator paths now answer with the same diagnostic

%action / %calc / %constraint on a body that declares attribute m next to 500.0 [m]:

The slot path agrees, and 1000.0 [kg] next to the shadowing m is unaffected (m = 1000.00 [kg]):

On the parent commit the constraint path does not error — it reaches past the nearer m and
compares in metres, printing index > index:

Prompt scope, %calc arguments, rendering

• %eval 1.0 [m/s] = 1.00 [m/s], 2.0 [km] + 500.0 [m] = 2.50 [km], 1.0 [SI::m] = 1.00 [SI::m],
  %eval m = "m" has no value to evaluate, %eval nosuch = symbol "nosuch" not found,
  %eval mass * 2 = 6.00 over a typed-in package. Parent: unresolved unit m.
  
• %calc — comma, invocation, whitespace-only, parenthesized subexpression, trailing comma and a
  nested invocation all give = 530.00 [m]; named arguments, unbalanced parens, a lone ,, empty
  arguments and wrong arity are each diagnosed and the session stays usable.
  
• Rendering: Assertion evaluated to false: 1.0 [m] > 500.0 [m], and the result table prints
  t = 17.20 [s] / h = -0.42 [m] / v = -42.86 [m/s] where the parent printed
  17.19999999999997 [s] / -0.41680000000065043 [m].
  
• A sibling that is a unit still evaluates; [furlong] still fails as unresolved unit furlong;
  1.0 [m] + 1.0 [s] → incommensurable units: cannot express s (SI::second) in m (SI::metre).

Two findings came out of the run; both are addressed:

1. A signed %calc argument merged into the one before it (5 -3 parsed as 5 - 3) — the same
   defect Devin Review raised. Fixed in c3c8d8d; see that thread for the rule and the tests.
2. The remedy clause was dropped for a package-level shadow. attribute m declared directly in
   the package that holds import SI::* printed only … m resolves to the attributeUsage m declared in ADV, because the hidden unit was looked for in the parent scope, where the import is not
   visible. unitOutside now resolves the name again with the shadowing declaration hidden
   (Resolver.LookupNameExcluding), so the most likely real-world spelling of the mistake gets the
   hint too. Pinned by the conformance case unit_shadowed_by_package_member.

One behavior worth stating rather than changing: the prompt evaluates in the last namespace the
session declared, so typing a scratch package mid-session moves that scope and the earlier package's
imports are then reached by qualified name only. Documented in docs/SPEC_COMPLIANCE.md (marked
approximate) and in ROADMAP B2, and pinned by TestPromptScopeIsTheLastNamespaceDeclared.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 2 additional findings in Devin Review.

[devin-ai-integration]

🟡 Error message can tell the user to write the exact name that already failed

The suggested spelling for a hidden measurement unit is built by re-qualifying the unit's full name (qualifyAs(m.fqnOf(outer), err.Name) at internal/core/semantics/units.go:508), which yields the bare, already-shadowed name whenever the hidden unit was declared outside any package, so the advice tells the user to write the very spelling that just failed.
Impact: A user hitting this diagnostic is told "write m to name the unit" when m is exactly what did not work, leaving no way to act on the message.

How a namespace-less unit produces a useless suggestion

qualifyAs(fqn, name) (internal/core/semantics/units.go:549-554) replaces the last segment of fqn with the written name, but falls back to returning name unchanged when fqn contains no ::. Index.GetFQN (internal/core/symbols/index.go:1017-1032) builds the qualified name only from owning scopes, so a unit declared directly at a document root (which a REPL session makes easy: a top-level attribute u : ISQBase::LengthUnit; followed by a package that declares a same-named member) has an FQN equal to its bare name. ShadowedUnitError.Error then renders … shadowing the measurement unit m — write m to name the unit.

A safer shape is to omit the remedy clause (leave Suggestion empty and print only the shadowing part) when no qualifier can be produced.

Prompt for agents

In internal/core/semantics/units.go, shadowedUnit() sets err.Suggestion = qualifyAs(m.fqnOf(outer), err.Name). qualifyAs falls back to returning the bare name when the unit's fully-qualified name has no '::' separator — which happens for a measurement unit declared at a document root rather than inside a package (Index.GetFQN builds the FQN purely from owning scopes). In that case the rendered diagnostic advises the user to write exactly the name that was just rejected ('write m to name the unit'). Make the remedy clause conditional: when no distinct qualified spelling can be produced, either leave Suggestion empty and have ShadowedUnitError.Error print only the 'shadowing the measurement unit X' part, or name the unit by some other unambiguous reference.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Fixed in 75a62ce along the lines you suggest: shadowedUnit sets Suggestion only when qualifyAs produces a spelling different from the written name, and Error() prints just …, shadowing the measurement unit u when it does not. So a unit declared at a document root no longer yields "write u to name the unit".

New robustness case quantity_shadowed_unit_without_a_qualifier builds exactly that model (a root-level attribute u : ISQBase::LengthUnit shadowed by test::u) and asserts the hidden unit is still named while Suggestion stays empty.