fix(validation): reject incomplete hardware addresses in FUNCTION/METHOD

[ghaith]

Variables with an incomplete hardware address (e.g. flag AT %I* : BOOL;) are placeholders that a VAR_CONFIG block later binds to a concrete address. They require a persistent state to bind to, so they may only be declared in PROGRAM, FUNCTION_BLOCK, or VAR_GLOBAL. FUNCTION and METHOD bodies have no such state.

Until now, declaring one inside a FUNCTION was silently accepted, which led to confusing downstream behaviour. Add a new check in visit_variable_block that mirrors the validate_vla POU-kind-aware shape and emits a fresh-code diagnostic at the variable's location.

Adds error code E134 (Error severity) with a descriptive markdown body. The diagnostic message names the allowed POU kinds explicitly so the user knows where to move the declaration.

Closes #1323

[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.5 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

Fixes #1323. Variables with incomplete hardware addresses (AT %I*) require persistent state to bind to via a VAR_CONFIG block, so they can only be declared in PROGRAM, FUNCTION_BLOCK, or VAR_GLOBAL. Previously, declaring one in a FUNCTION or METHOD was silently accepted and produced confusing downstream behavior. The fix adds a validate_template_address check inside visit_variable_block that emits a new diagnostic.

Critical: E134 collision with PR #1710

This PR and #1710 both register a new error code E134 for entirely different diagnostics. Master currently ends at E133, and both PRs add an E134 line directly after it:

• #1710 — E134: := used for an output parameter
• #1712 (this PR) — E134: Incomplete hardware address in FUNCTION/METHOD

Whichever PR lands first will force the second to renumber. Please coordinate with #1710's author and pick a single owner of E134; the loser should bump to E135 (and #1710's E135 to E136), update its .md filename and registry entry, and refresh affected snapshots. Worth catching before merge to avoid a noisy second-PR rebase.

What I like (the rest of the PR)

• The check is small, focused, and lives in the right place. Slipping into visit_variable_block next to the VLA POU-kind check follows an established pattern. The new helper has a clear comment explaining the IEC rationale (template addresses need persistent state).
• Predicate is exactly right. address.is_template() + matches!(pou.kind, PouType::Function | PouType::Method { .. }). The early Some(pou) = pou else { return } correctly allows VAR_GLOBAL (no enclosing POU). Confirmed is_template exists in compiler/plc_ast/src/ast.rs:1542.
• Test coverage is symmetric. Two failure cases (FUNCTION, METHOD) and three regression cases (FUNCTION_BLOCK, PROGRAM with VAR_CONFIG, VAR_GLOBAL with VAR_CONFIG). The clean cases include the actual VAR_CONFIG binding, which is exactly what protects against an accidental over-broad fix.
• Diagnostic message is excellent. Explicitly names the disallowed kinds and the allowed alternatives, so a user reading just the error knows where to move the declaration.
• E134.md doc is concise and actionable — example + remediation in one short page.

Suggestions

• Consider CLASS and other POU kinds. PouType::Function | PouType::Method { .. } is a positive list of disallowed kinds. If the enum grows (CLASS, INTERFACE, custom variants), the check would silently allow them. Inverting to a positive list of allowed kinds — matches!(pou.kind, Program | FunctionBlock | Class) — would be more defensible going forward, if Class should also be allowed. If not, a comment listing every variant and why each is or isn't covered would future-proof. Optional.
• Diagnostic message length. The full sentence is long enough that it wraps in narrower terminals. Splitting into a short headline + body via Diagnostic::with_secondary_location or similar (if the API supports it) would render better. Cosmetic.
• The check fires per-variable, but the underline is on flag (the name only). Pointing at flag AT %I* (the full declaration) would make the cause more visible. The variable's location may not contain the address span — worth checking whether variable.address.location (or similar) gives a tighter target.
• No test for VAR_INPUT / VAR_OUTPUT in a FUNCTION_BLOCK — the check fires for all variable blocks regardless of block.kind. Probably fine because parameters can't carry template addresses anyway, but a quick test that confirms the check doesn't fire incorrectly on a parameter (where address is None) would lock that in.

Risks

• Aside from the E134 collision: this is a new error that will fail any project currently declaring template-address variables in FUNCTION/METHOD bodies. The PR body notes this used to "silently accept" with confusing downstream behavior, so projects in this state were already broken; making the failure visible is a strict improvement. Worth a release-notes line.
• The early-return logic on address.as_ref() correctly skips variables without addresses, so 99% of variables incur a pointer check and bail. No measurable cost.

Verdict

The fix itself is LGTM — small, well-tested, and addresses a real silent-failure mode. The E134 collision with #1710 is a hard blocker, though, and needs to be resolved before either PR merges. Once that's settled, this is good to land.