Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Diagnostic> {
std::mem::take(&mut self.diagnostics)
Expand Down
8 changes: 8 additions & 0 deletions src/validation/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,14 @@ fn validate_reference_expression<T: AnnotationMap>(
.with_location(m.get_location())
.with_error_code("E119"),
);
} else if base.is_this() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if listing all these special cases here is the correct way to go?

Aren't (almost all of) these cases caught by one generic type-check? like see if the member could be resolved under this base? Or is the resolver actually able to find anything for Member in these cases?

What I mean is, should these two lines maybe give the same error-message?

VAR_GLOBAL
    p_fb : POINTER TO FB ...;
END_VAR

FUNCTION_BLOCK fb
...
METHOD m : DINT
    THIS.a;    // cannot access a on type pointer to fb
    p_fb.a;     // cannot access a on type pointer to fb
END_METHOD

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the logic for both this and super (and normal pointers) to get a clearer message, now we report that you should dereference the pointer before accessing a member.

// THIS is `POINTER TO <enclosing FB>`; 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"),
);
}
}

Expand Down
31 changes: 31 additions & 0 deletions src/validation/tests/this_keyword_validation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.<member>` (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.
┌─ <internal>:12:22
12 │ THIS.a.legs();
│ ^ `THIS` must be dereferenced to access its members.
");
}
24 changes: 24 additions & 0 deletions tests/lit/single/oop/this_member_access_without_deref_errors.st
Original file line number Diff line number Diff line change
@@ -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.<member>` 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
Loading