From c3927c32f0e339d17eaa9d17b89a05f372d25224 Mon Sep 17 00:00:00 2001 From: Ghaith Hachem Date: Wed, 6 May 2026 16:12:02 +0200 Subject: [PATCH 1/3] fix(validation): reject THIS. without dereference Fixes #1728. THIS is POINTER TO , so member access on it requires an explicit `^` deref. Without this fix, THIS. slipped through the validator -- `--check` was silent, `build` was silent -- and reached codegen, which panicked with "Builder error: GEP pointee is not a struct (pointee: \"ptr\", index: 1)" when attempting to GEP through the unindirected pointer operand. Mirror the existing SUPER pattern in `validate_reference_expression`: when the base is THIS (and not THIS^), emit E120 -- which already documents this rule in its markdown but had no validator site emitting it. Also dedupes exact-duplicate diagnostics in `Validator::push_diagnostic`. Lowering occasionally clones a receiver AST into multiple positions in the post-lowered tree (operator chain + synthesised `self` argument), which caused both the new THIS check and the existing SUPER check to render twice for the same source location. The dedup is keyed on `Diagnostic`'s existing PartialEq (code + message + locations + sub-diagnostics), so distinct diagnostics at the same span are unaffected. Adds: - `tests/lit/single/oop/this_member_access_without_deref_errors.st` -- runs `plc -c` on the minimal repro, asserts E120 fires and no binary is produced. - `this_member_access_without_deref_is_an_error` unit test in `src/validation/tests/this_keyword_validation_tests.rs` with an inline snapshot pinning the diagnostic shape. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/validation.rs | 10 +++++- src/validation/statement.rs | 8 +++++ .../tests/this_keyword_validation_tests.rs | 31 +++++++++++++++++++ ...this_member_access_without_deref_errors.st | 24 ++++++++++++++ 4 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 tests/lit/single/oop/this_member_access_without_deref_errors.st diff --git a/src/validation.rs b/src/validation.rs index 6429701757a..4bef9ded5f1 100644 --- a/src/validation.rs +++ b/src/validation.rs @@ -110,7 +110,15 @@ pub struct Validator<'a> { impl Validators for Validator<'_> { fn push_diagnostic(&mut self, diagnostic: Diagnostic) { - self.diagnostics.push(diagnostic); + // Skip exact duplicates: lowering passes occasionally clone an AST + // node into multiple positions in the post-lowered tree (e.g. the + // receiver of a method call ends up both in the call's operator + // chain and in its synthesised `self` argument), which causes the + // same statement-level diagnostic to be raised more than once for + // the same source location. + if !self.diagnostics.contains(&diagnostic) { + self.diagnostics.push(diagnostic); + } } fn take_diagnostics(&mut self) -> Vec { std::mem::take(&mut self.diagnostics) diff --git a/src/validation/statement.rs b/src/validation/statement.rs index 6c9c6edc9bf..8d52b5669a9 100644 --- a/src/validation/statement.rs +++ b/src/validation/statement.rs @@ -226,6 +226,14 @@ fn validate_reference_expression( .with_location(m.get_location()) .with_error_code("E119"), ); + } else if base.is_this() { + // THIS is `POINTER TO `; a member access on it + // requires an explicit dereference (`THIS^.member`). + validator.push_diagnostic( + Diagnostic::new("`THIS` must be dereferenced to access its members.") + .with_location(m.get_location()) + .with_error_code("E120"), + ); } } diff --git a/src/validation/tests/this_keyword_validation_tests.rs b/src/validation/tests/this_keyword_validation_tests.rs index da47ecab363..f20bf585de1 100644 --- a/src/validation/tests/this_keyword_validation_tests.rs +++ b/src/validation/tests/this_keyword_validation_tests.rs @@ -448,3 +448,34 @@ fn this_calling_functionblock_body_from_method_is_ok() { ); assert!(diagnostics.is_empty()); } + +#[test] +fn this_member_access_without_deref_is_an_error() { + // Regression test for #1728: `THIS.` (without `^`) used to slip + // through validation and panic in codegen with + // `Builder error: GEP pointee is not a struct`. It must surface as E120. + let diagnostics = parse_and_validate_buffered( + r#" + INTERFACE IAnimal + METHOD legs : DINT + END_METHOD + END_INTERFACE + + FUNCTION_BLOCK fb + VAR + a : IAnimal; + END_VAR + METHOD m : DINT + THIS.a.legs(); + END_METHOD + END_FUNCTION_BLOCK + "#, + ); + assert_snapshot!(diagnostics, @r" + error[E120]: `THIS` must be dereferenced to access its members. + ┌─ :12:22 + │ + 12 │ THIS.a.legs(); + │ ^ `THIS` must be dereferenced to access its members. + "); +} diff --git a/tests/lit/single/oop/this_member_access_without_deref_errors.st b/tests/lit/single/oop/this_member_access_without_deref_errors.st new file mode 100644 index 00000000000..57588aaf350 --- /dev/null +++ b/tests/lit/single/oop/this_member_access_without_deref_errors.st @@ -0,0 +1,24 @@ +// RUN: rm -f %T/%basename_t.out +// RUN: not %COMPILE --error-format clang %s 2>&1 | %CHECK %s +// RUN: test ! -e %T/%basename_t.out + +// Regression test for #1728: `THIS.` without an explicit `^` deref +// must be rejected by validation, not silently passed through to codegen +// (where it used to crash with "GEP pointee is not a struct"). + +// CHECK: {{.*}}this_member_access_without_deref_errors.st:{{[0-9]+}}:{{[0-9]+}}:{{.*}}error[E120]: `THIS` must be dereferenced to access its members. +// CHECK: Compilation aborted due to critical errors + +INTERFACE IAnimal + METHOD legs : DINT + END_METHOD +END_INTERFACE + +FUNCTION_BLOCK fb +VAR + a : IAnimal; +END_VAR +METHOD m : DINT + THIS.a.legs(); +END_METHOD +END_FUNCTION_BLOCK From 61a8f438616219b52a3a37e7e10313dd9daf473c Mon Sep 17 00:00:00 2001 From: Ghaith Hachem Date: Mon, 11 May 2026 10:04:38 +0200 Subject: [PATCH 2/3] refactor(validation): unify pointer-base member-access check (E137) Per maintainer review on #1729: THIS, SUPER, and user-declared `POINTER TO ...` bases all share the same shape -- the value of the base is a pointer, not the pointee -- so member access on any of them without dereference should produce the same diagnostic. Today `THIS.a` emits E120, `SUPER.a` emits E119, and `p_fb.a` emits a misleading E048 ("Could not resolve reference to a" -- but `a` is a valid field on `fb`, just not on `POINTER TO fb`). Replace the SUPER- and THIS-specific deref branches in `validate_reference_expression` with a single type-driven check: if the base resolves to a non-auto-deref `Pointer` data type, emit a new E137 -- "Cannot access `` on `POINTER TO `; dereference with `^` first" -- and return early to suppress the follow-on E048. The member name comes from the source slice so the diagnostic shows the user's text (`prop`) even when the resolver has rewritten the AST to a synthetic name (`__get_prop`). E119 and E120 stay -- they still cover the THIS/SUPER-as-member case (`x.THIS`, `x.SUPER`) and the invalid-context checks (SUPER outside an extending POU, THIS outside an FB). Only the deref-required sub-rule moves to E137. Also add `return` after the `m.is_super()` branch when the enclosing POU actually extends something, so users don't get "SUPER not allowed in member-access position" followed by a redundant deref-required cascade. The gate keeps the "SUPER outside extending POU" diagnostic when the qualifier doesn't extend -- that's orthogonal signal. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/diagnostics/diagnostics_registry.rs | 1 + .../src/diagnostics/error_codes/E137.md | 44 ++++++++++++++ src/validation/statement.rs | 59 ++++++++++++++----- .../tests/super_keyword_validation_tests.rs | 38 ++++-------- .../tests/this_keyword_validation_tests.rs | 6 +- ...this_member_access_without_deref_errors.st | 2 +- 6 files changed, 106 insertions(+), 44 deletions(-) create mode 100644 compiler/plc_diagnostics/src/diagnostics/error_codes/E137.md diff --git a/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs b/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs index ba79f71f3ab..8525054bb06 100644 --- a/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs +++ b/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs @@ -236,6 +236,7 @@ lazy_static! { E132, Ignore, include_str!("./error_codes/E132.md"), // Mixing implicit and explicit call parameters E133, Error, include_str!("./error_codes/E133.md"), // AND_THEN/OR_ELSE with non-boolean operands E136, Error, include_str!("./error_codes/E136.md"), // Incomplete hardware address in FUNCTION/METHOD + E137, Error, include_str!("./error_codes/E137.md"), // Member access on a non-auto-deref pointer base ); } diff --git a/compiler/plc_diagnostics/src/diagnostics/error_codes/E137.md b/compiler/plc_diagnostics/src/diagnostics/error_codes/E137.md new file mode 100644 index 00000000000..6f1edabfcc9 --- /dev/null +++ b/compiler/plc_diagnostics/src/diagnostics/error_codes/E137.md @@ -0,0 +1,44 @@ +# Member access on a non-auto-deref pointer base + +Member access (`base.member`) requires `base` to be a struct, function-block, or class instance — not a pointer to one. The compiler will not implicitly dereference pointers when they appear on the left of a `.` (auto-dereferencing applies only to `REFERENCE TO` / alias pointers). + +This rule covers three cases that share the same underlying shape — the value of `base` is a pointer, not the pointee: + +1. `THIS` is `POINTER TO `. Use `THIS^.member`. +2. `SUPER` is `POINTER TO ` (before dereferencing). Use `SUPER^.member`. +3. A user-declared `POINTER TO ...` variable. Use `^.member`. + +## Example of invalid use + +```iecst +FUNCTION_BLOCK fb + VAR + a : DINT; + END_VAR +END_FUNCTION_BLOCK + +FUNCTION_BLOCK other + VAR + p_fb : POINTER TO fb; + END_VAR + METHOD m : DINT + p_fb.a := 1; // Error: Cannot access `a` on `POINTER TO fb` + THIS.b := 2; // Error: Cannot access `b` on `POINTER TO other` (THIS is a pointer) + END_METHOD +END_FUNCTION_BLOCK +``` + +## Example of valid use + +```iecst +FUNCTION_BLOCK other + VAR + p_fb : POINTER TO fb; + b : DINT; + END_VAR + METHOD m : DINT + p_fb^.a := 1; // Dereference the pointer first + THIS^.b := 2; // Dereference THIS first + END_METHOD +END_FUNCTION_BLOCK +``` diff --git a/src/validation/statement.rs b/src/validation/statement.rs index 8d52b5669a9..c35144e20d5 100644 --- a/src/validation/statement.rs +++ b/src/validation/statement.rs @@ -218,22 +218,51 @@ fn validate_reference_expression( .with_location(m.get_location()) .with_error_code("E119"), ); - } else if (base.is_super() || base.has_super_metadata()) - && !(base.is_super_deref() || base.has_super_metadata_deref()) - { - validator.push_diagnostic( - Diagnostic::new("`SUPER` must be dereferenced to access its members.") - .with_location(m.get_location()) - .with_error_code("E119"), - ); - } else if base.is_this() { - // THIS is `POINTER TO `; a member access on it - // requires an explicit dereference (`THIS^.member`). - validator.push_diagnostic( - Diagnostic::new("`THIS` must be dereferenced to access its members.") + // Only suppress follow-on cascades when the enclosing POU + // actually extends something. Otherwise let + // `validate_member_access` recurse so the SUPER-leaf + // check still emits "Invalid use of `SUPER`. Usage is + // only allowed within a POU that directly extends + // another POU" -- that's orthogonal signal worth + // keeping. + let qualifier_extends = context + .qualifier + .and_then(|q| context.index.find_pou(q)) + .and_then(|pou| match pou { + PouIndexEntry::Method { parent_name, .. } + | PouIndexEntry::Action { parent_name, .. } => { + context.index.find_pou(parent_name) + } + other => Some(other), + }) + .and_then(|pou| pou.get_super_class()) + .is_some(); + if qualifier_extends { + return; + } + } else if let Some(base_type) = context.annotations.get_type(base, context.index) { + // Member access through a non-auto-deref pointer needs an + // explicit `^`. This subsumes THIS, SUPER, and + // user-declared `POINTER TO ...` bases, which all share + // the same underlying shape (the value of the base is a + // pointer, not the pointee). + let info = base_type.get_type_information(); + if info.is_pointer() && !info.is_auto_deref() { + let inner = info.get_inner_name(); + // Use the source slice for the member name so the + // diagnostic shows what the user wrote (e.g. `prop`) + // rather than any synthetic name introduced by an + // earlier lowering pass (e.g. `__get_prop`). + let m_name = validator.context.slice(&m.get_location()); + validator.push_diagnostic( + Diagnostic::new(format!( + "Cannot access `{m_name}` on `POINTER TO {inner}`; dereference with `^` first" + )) .with_location(m.get_location()) - .with_error_code("E120"), - ); + .with_error_code("E137"), + ); + return; + } } } diff --git a/src/validation/tests/super_keyword_validation_tests.rs b/src/validation/tests/super_keyword_validation_tests.rs index 6ad65afb6e4..3ca8a3ea2e7 100644 --- a/src/validation/tests/super_keyword_validation_tests.rs +++ b/src/validation/tests/super_keyword_validation_tests.rs @@ -314,12 +314,6 @@ fn super_accessor_cannot_be_accessed_from_outside_of_its_pou() { 16 │ fb.SUPER.x := 2; │ ^^^^^ Invalid use of `SUPER`. Usage is only allowed within a POU that directly extends another POU. - error[E119]: `SUPER` must be dereferenced to access its members. - ┌─ :16:22 - │ - 16 │ fb.SUPER.x := 2; - │ ^ `SUPER` must be dereferenced to access its members. - error[E119]: `SUPER` is not allowed in member-access position. ┌─ :17:16 │ @@ -975,12 +969,6 @@ fn invalid_super_dereferencing_patterns() { 16 │ SUPER^.SUPER.x := 40; │ ^^^^^ `SUPER` is not allowed in member-access position. - error[E119]: `SUPER` must be dereferenced to access its members. - ┌─ :16:26 - │ - 16 │ SUPER^.SUPER.x := 40; - │ ^ `SUPER` must be dereferenced to access its members. - warning[E049]: Illegal access to private member parent.x ┌─ :16:26 │ @@ -1268,12 +1256,12 @@ fn super_dereferencing_with_method_calls() { "#, ); - assert_snapshot!(diagnostics, @r" - error[E119]: `SUPER` must be dereferenced to access its members. + assert_snapshot!(diagnostics, @" + error[E137]: Cannot access `get_value` on `POINTER TO parent`; dereference with `^` first ┌─ :19:28 │ 19 │ x := SUPER.get_value(); // Trying to call method on pointer - │ ^^^^^^^^^ `SUPER` must be dereferenced to access its members. + │ ^^^^^^^^^ Cannot access `get_value` on `POINTER TO parent`; dereference with `^` first error[E037]: Invalid assignment: cannot assign 'get_value' to 'parent' ┌─ :20:17 @@ -1307,18 +1295,18 @@ fn super_without_deref_accessing_members() { "#, ); - assert_snapshot!(diagnostics, @r" - error[E119]: `SUPER` must be dereferenced to access its members. + assert_snapshot!(diagnostics, @" + error[E137]: Cannot access `x` on `POINTER TO parent`; dereference with `^` first ┌─ :11:19 │ 11 │ SUPER.x := 20; // Should be SUPER^.x - │ ^ `SUPER` must be dereferenced to access its members. + │ ^ Cannot access `x` on `POINTER TO parent`; dereference with `^` first - error[E119]: `SUPER` must be dereferenced to access its members. + error[E137]: Cannot access `ptr` on `POINTER TO parent`; dereference with `^` first ┌─ :14:19 │ 14 │ SUPER.ptr^ := 30; // Should be SUPER^.ptr^ - │ ^^^ `SUPER` must be dereferenced to access its members. + │ ^^^ Cannot access `ptr` on `POINTER TO parent`; dereference with `^` first warning[E049]: Illegal access to private member parent.ptr ┌─ :17:20 @@ -1366,18 +1354,18 @@ fn super_with_property_access_errors() { "#, ); - assert_snapshot!(diagnostics, @r" - error[E119]: `SUPER` must be dereferenced to access its members. + assert_snapshot!(diagnostics, @" + error[E137]: Cannot access `prop` on `POINTER TO parent`; dereference with `^` first ┌─ :21:19 │ 21 │ SUPER.prop := 10; // Should be SUPER^.prop - │ ^^^^ `SUPER` must be dereferenced to access its members. + │ ^^^^ Cannot access `prop` on `POINTER TO parent`; dereference with `^` first - error[E119]: `SUPER` must be dereferenced to access its members. + error[E137]: Cannot access `prop` on `POINTER TO parent`; dereference with `^` first ┌─ :22:24 │ 22 │ x := SUPER.prop; // Should be SUPER^.prop - │ ^^^^ `SUPER` must be dereferenced to access its members. + │ ^^^^ Cannot access `prop` on `POINTER TO parent`; dereference with `^` first error[E007]: Properties cannot be called like functions. Remove `()` ┌─ :25:13 diff --git a/src/validation/tests/this_keyword_validation_tests.rs b/src/validation/tests/this_keyword_validation_tests.rs index f20bf585de1..e0ed26c8283 100644 --- a/src/validation/tests/this_keyword_validation_tests.rs +++ b/src/validation/tests/this_keyword_validation_tests.rs @@ -471,11 +471,11 @@ fn this_member_access_without_deref_is_an_error() { END_FUNCTION_BLOCK "#, ); - assert_snapshot!(diagnostics, @r" - error[E120]: `THIS` must be dereferenced to access its members. + assert_snapshot!(diagnostics, @" + error[E137]: Cannot access `a` on `POINTER TO fb`; dereference with `^` first ┌─ :12:22 │ 12 │ THIS.a.legs(); - │ ^ `THIS` must be dereferenced to access its members. + │ ^ Cannot access `a` on `POINTER TO fb`; dereference with `^` first "); } diff --git a/tests/lit/single/oop/this_member_access_without_deref_errors.st b/tests/lit/single/oop/this_member_access_without_deref_errors.st index 57588aaf350..b4931fbd4ec 100644 --- a/tests/lit/single/oop/this_member_access_without_deref_errors.st +++ b/tests/lit/single/oop/this_member_access_without_deref_errors.st @@ -6,7 +6,7 @@ // must be rejected by validation, not silently passed through to codegen // (where it used to crash with "GEP pointee is not a struct"). -// CHECK: {{.*}}this_member_access_without_deref_errors.st:{{[0-9]+}}:{{[0-9]+}}:{{.*}}error[E120]: `THIS` must be dereferenced to access its members. +// CHECK: {{.*}}this_member_access_without_deref_errors.st:{{[0-9]+}}:{{[0-9]+}}:{{.*}}error[E137]: Cannot access `a` on `POINTER TO fb`; dereference with `^` first // CHECK: Compilation aborted due to critical errors INTERFACE IAnimal From d0ebccbd6323e9193d7a150c3c698b2d2d6dd98d Mon Sep 17 00:00:00 2001 From: Ghaith Hachem Date: Fri, 29 May 2026 09:42:45 +0200 Subject: [PATCH 3/3] fix: test missing an error: --- tests/lit/single/oop/this_member_access_without_deref_errors.st | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lit/single/oop/this_member_access_without_deref_errors.st b/tests/lit/single/oop/this_member_access_without_deref_errors.st index 82cf6d52e72..5224e6da20b 100644 --- a/tests/lit/single/oop/this_member_access_without_deref_errors.st +++ b/tests/lit/single/oop/this_member_access_without_deref_errors.st @@ -7,7 +7,7 @@ // (where it used to crash with "GEP pointee is not a struct"). // CHECK: {{.*}}this_member_access_without_deref_errors.st:{{[0-9]+}}:{{[0-9]+}}:{{.*}}error[E141]: Cannot access `a` on `POINTER TO fb`; dereference with `^` first -// CHECK: Compilation aborted due to critical errors +// CHECK: error: Compilation aborted due to critical errors INTERFACE IAnimal METHOD legs : DINT