fix(validation): flag wrong assignment direction at call sites

[ghaith]

Summary

Calls today silently accept a direction-mismatched assignment operator on a named argument: foo(out_val := x) where out_val is VAR_OUTPUT compiles and produces broken code, with no diagnostic at parse, validate, or --check time. Per IEC 61131-3 the operators are direction-keyed: := writes into INPUT/IN_OUT, => captures from OUTPUT.

validate_call's per-argument loop now checks the direction:

• E134 (Error) — := used for an OUTPUT parameter. Surfaces at --check and aborts compile. Names the parameter and points to =>.
• E135 (Ignore by default) — => used for an INPUT or IN_OUT parameter. Symmetrical wording. Some IEC implementations historically tolerate it, so it's not surfaced by default; users who want it can promote via --error-config '{"error":["E135"]}'.

Behaviour change for users

Before:

$ plc instance_call_with_typo.st --check
$ echo $?
0

After (E134 / OUTPUT direction):

$ plc instance_call_with_typo.st --check
error[E134]: 'out_val' is an output parameter; use '=>' instead of ':='
   ┌─ instance_call_with_typo.st:12:27
   │
12 │     instance(in_val := 5, out_val := local);
   │                           ^^^^^^^^^^^^^^^^ ...

Compilation aborted due to critical errors.
$ echo $?
1

After (E135 / INPUT direction, default):

$ plc mirror_typo.st --check
$ echo $?
0

After (E135 / INPUT direction, opt-in):

$ plc mirror_typo.st --check --error-config e135.json
error[E135]: 'in_val' is an input parameter; use ':=' instead of '=>'
   ┌─ mirror_typo.st:10:14
...
$ echo $?
1

Notes

• The check uses existing helpers (AstNode::is_assignment / is_output_assignment, VariableIndexEntry::is_output / is_inout) and runs inside the existing parameter-resolution block in validate_call. Positional arguments naturally skip the check (they aren't Assignment / OutputAssignment nodes).
• One existing test fixture (validate_array_elements_passed_to_functions_by_ref) had a latent direction mismatch on byRefOutput := x[1] that this PR catches; the fixture is corrected to byRefOutput => x[1].
• E135 covers => on both INPUT and IN_OUT (the IEC rule that => is only valid for OUTPUT is symmetric across both). The diagnostic message names the actual direction (input vs in-out) for clarity.
• No new test infra for --error-config mid-test promotion. Tests use parse_and_validate (raw Vec<Diagnostic>) to assert E135 is emitted internally even when the buffered (Ignore-filtered) view is empty. The promotion mechanism itself is already covered by the registry's own unit tests.

Fixes #1228

Test plan

• New inline-snapshot tests in assignment_validation_tests.rs covering: E134 surfaces with parameter name; correct directions stay clean; E135 silent by default but emitted for opt-in (Input + IN_OUT cases); mixed named/positional calls don't double-report.
• Existing test fixture corrected.
• cargo test --workspace clean (2500/2500).
• cargo xtask lit clean (347/347).
• cargo fmt --all + cargo clippy --workspace -- -Dwarnings clean.
• Manual end-to-end: plc <file> --check exits 1 with E134 on the issue's case; plc <file> --check --error-config '{"error":["E135"]}' exits 1 with E135 on the mirror case; default config keeps E135 silent.

🤖 Generated with Claude Code

[github-actions]

Build Artifacts

🐧 Linux

Artifact	Link	Size
deb-x86_64	Download	38.5 MB
schema	Download	0.0 MB
stdlib	Download	32.4 MB
plc-x86_64	Download	43.6 MB
deb-aarch64	Download	30.9 MB
plc-aarch64	Download	43.5 MB

From workflow run

🪟 Windows

Artifact	Link	Size
stdlib.lib	Download	4.3 MB
stdlib.dll	Download	0.1 MB
plc.exe	Download	38.4 MB

From workflow run

[mhasel]

Note: this review was generated by Claude (via Claude Code) and posted on my behalf. Treat the suggestions as machine-generated input — verify before acting. — Michael

Overview

Adds two new validation diagnostics for direction-keyed assignment operator misuse at call sites, per IEC 61131-3:

• E134 (Error): := used for an OUTPUT parameter
• E135 (Ignore): => used for an INPUT or IN_OUT parameter

The check is a small addition to validate_call's parameter-resolution loop and is keyed off existing helpers. Resolves #1228.

What I like

• Asymmetric severity is well-justified. Making E134 an error (silently-broken code: captured variable never receives output) and E135 ignore-by-default (tolerated by some implementations historically) reflects real-world practice. Users who want strict mode can opt in to E135 via --error-config.
• The check is small and uses existing primitives. Four lines of meaningful condition logic, no new helpers. Sits inside the existing parameter-resolution Some(left) branch, so it can't fire on unresolved parameters or positional args.
• Documentation is solid. Both E134.md and E135.md include erroneous + corrected snippets, and E135.md spells out the --error-config opt-in JSON. That answers the "how do I turn this on?" question without forcing a user into the codebase.
• Test technique for E135 is clean. Using parse_and_validate (raw Vec<Diagnostic>) to assert the diagnostic is emitted internally, alongside parse_and_validate_buffered to assert it's filtered out by default — that's the right way to test severity independently of the registry pipeline. Worth replicating elsewhere.
• The E135 IN_OUT path also asserts the message string ("in-out" vs "input"), which is a nice anchor against future refactor drift.
• Existing-fixture correction (byRefOutput := x[1] → byRefOutput => x[1]) is exactly the kind of latent bug a new validator should turn up. The test still exercises array-by-ref semantics, just with the correct operator.

Suggestions

• Consider an E135 severity of Warning rather than Ignore. If the registry supports Warning, defaulting E135 to Warning would surface the issue without breaking builds — closer to "tolerated but not encouraged." Ignore-by-default means many users will never see it. If the registry only has Error/Ignore today, ignore the suggestion. (Worth a quick grep for Warning in the severity enum.)
• One additional test worth pinning: E134 for a method call. All current tests use FB instance calls (instance(...)). A instance.method(out_val := x) case (qualified-call-with-named-args path) would lock in that the new check fires through method-call lowering as well. Probably already works since the path converges, but worth one test.
• REFERENCE TO output parameters. If a parameter is VAR_OUTPUT of type REFERENCE TO X, does := still trigger E134? It should (the direction check is independent of the type), but a test would be cheap insurance. (#1701 is currently in the area, so it's worth being explicit.)
• is_output_assignment vs is_assignment are exhaustive but not stated. A short comment in validate_call saying "positional args reach this branch as neither, so they fall through cleanly" would future-proof the check against someone refactoring is_assignment/is_output_assignment and inadvertently broadening the predicate.
• The example in E134.md uses inline // ❌ and // ✅ emojis. Skim of the surrounding error_codes/*.md files (E131, E132, E133) would tell you whether this matches the house style. If not, plain // invalid / // correct would be safer for terminals/renderers that don't display emoji well.

Risks

• E134 is a new error. Any project that had output_param := var silently compiling will now fail. The PR body documents this clearly. Worth a release-notes call-out — and worth checking that the broken codegen the old path produced wasn't being relied on for some bizarre fallthrough behavior (almost certainly not, but worth thinking).
• The corrected internal fixture proves this PR is breaking a real-world pattern that was hiding in tests. That's a feature, not a bug — but it raises the prior probability that user code in the wild has the same shape. Consider adding the migration tip ("if you see E134 after upgrading, replace := with => for the named OUTPUT argument") to release notes.
• The check correctly lives inside the resolved-parameter branch and skips positional args; verified by the dedicated mixed-args test.

Verdict

LGTM. Clean addition with well-chosen severities, good test coverage, and clear user docs. The Warning-vs-Ignore question for E135 is worth a one-line answer from the team; the rest are nits.

[volsa]

Why is this marked as ignore while the other one isn't?

[volsa]

Missed this "This diagnostic defaults to severity Ignore because some IEC implementations historically tolerate the mistake; promote it via --error-config if you want it surfaced.". Are we talking about codesys here?

[ghaith]

yes, but maybe we warn?

[volsa]

Agreed, let's warn here.

[volsa]

Not sure this should be part of the markdown file?