Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add additional pointer and type validations #1341

Merged
merged 4 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
2 changes: 2 additions & 0 deletions compiler/plc_diagnostics/src/diagnostics/error_codes/E052.md
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
# Unknown type

Use of undeclared type-identifier.
3 changes: 3 additions & 0 deletions src/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1628,6 +1628,9 @@ impl Index {
if let DataTypeInformation::Pointer { inner_type_name, .. } = initial_type {
if let Some(ty) = self.find_effective_type_info(inner_type_name) {
return self.find_elementary_pointer_type(self.find_intrinsic_type(ty));
} else {
// the inner type can't be found, return VOID as placeholder
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a stack-overflow in typesystem::is_same_type_class when dealing with pointers of undefined type

return self.get_void_type().get_type_information();
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/index/const_expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ impl ConstExpression {
#[derive(Debug, PartialEq, Clone)]
pub struct InitData {
pub initializer: Option<AstNode>,
pub target_type_name: String,
pub target_type_name: Option<String>,
pub scope: Option<String>,
pub lhs: Option<String>,
}
Expand All @@ -102,7 +102,7 @@ impl InitData {
) -> Self {
InitData {
initializer: initializer.cloned(),
target_type_name: target_type.unwrap_or_default().to_string(),
target_type_name: target_type.map(|it| it.into()),
scope: scope.map(|it| it.into()),
lhs: target.map(|it| it.into()),
}
Expand Down
2 changes: 1 addition & 1 deletion src/lowering/initializers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl<'lwr> Init<'lwr> for Initializers {
.for_each(|(scope, data)| {
assignments.maybe_insert_initializer(
&scope,
Some(data.lhs.as_ref().unwrap_or(&data.target_type_name)),
data.lhs.as_ref().or(data.target_type_name.as_ref()).map(|it| it.as_str()),
&data.initializer,
);
});
Expand Down
6 changes: 4 additions & 2 deletions src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,7 @@ fn parse_pointer_definition(
(
DataTypeDeclaration::DataTypeDefinition {
data_type: DataType::PointerType { name, referenced_type: Box::new(decl), auto_deref },
// FIXME: this currently includes the initializer in the sourcelocation, resulting in 'REF_TO A := B' when creating a slice
Copy link
Member

Choose a reason for hiding this comment

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

Should we track this on GitHub instead in the code itself? I feel like these FIXMEs are easily forgotten

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I've already set up a draft for this issue - just need to add a reproducible example.

location: lexer.source_range_factory.create_range(start_pos..lexer.last_range.end),
scope: lexer.scope.clone(),
},
Expand All @@ -772,14 +773,15 @@ fn parse_type_reference_type_definition(
None
};

let initial_value =
let end = lexer.last_range.end;

let initial_value: Option<AstNode> =
if lexer.try_consume(&KeywordAssignment) || lexer.try_consume(&KeywordReferenceAssignment) {
Some(parse_expression(lexer))
} else {
None
};

let end = lexer.last_range.end;
if name.is_some() || bounds.is_some() {
let data_type = match bounds {
Some(AstNode { stmt: AstStatement::ExpressionList(expressions), id, location }) => {
Expand Down
10 changes: 5 additions & 5 deletions src/resolver/tests/const_resolver_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1311,7 +1311,7 @@ fn ref_initializer_is_marked_as_resolve_later() {

assert_eq!(init.scope, Some("foo".into()));
assert_eq!(init.lhs, Some("ps".into()));
assert_eq!(init.target_type_name, "__foo_ps".to_string());
assert_eq!(init.target_type_name, Some("__foo_ps".to_string()));
}

#[test]
Expand All @@ -1338,7 +1338,7 @@ fn adr_initializer_is_marked_as_resolve_later() {

assert_eq!(init.scope, Some("foo".into()));
assert_eq!(init.lhs, Some("ps".into()));
assert_eq!(init.target_type_name, "__foo_ps".to_string());
assert_eq!(init.target_type_name, Some("__foo_ps".to_string()));
}

#[test]
Expand Down Expand Up @@ -1368,13 +1368,13 @@ fn alias_initializer_is_marked_as_resolve_later() {

assert_eq!(init.scope, Some("foo".into()));
assert_eq!(init.lhs, Some("ps1".into()));
assert_eq!(init.target_type_name, "__foo_ps1".to_string());
assert_eq!(init.target_type_name, Some("__foo_ps1".to_string()));

let Some(UnresolvableKind::Address(ref init)) = unresolvable[1].kind else { panic!() };

assert_eq!(init.scope, Some("foo".into()));
assert_eq!(init.lhs, Some("ps2".into()));
assert_eq!(init.target_type_name, "__foo_ps2".to_string());
assert_eq!(init.target_type_name, Some("__foo_ps2".to_string()));
}

#[test]
Expand All @@ -1401,5 +1401,5 @@ fn reference_to_initializer_is_marked_as_resolve_later() {

assert_eq!(init.scope, Some("foo".into()));
assert_eq!(init.lhs, Some("ps".into()));
assert_eq!(init.target_type_name, "__foo_ps".to_string());
assert_eq!(init.target_type_name, Some("__foo_ps".to_string()));
}
18 changes: 18 additions & 0 deletions src/validation/tests/assignment_validation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1355,6 +1355,24 @@ fn ref_assignment_with_global_local_variables_and_aliased_types() {
18 │ invalidB : REFERENCE TO fooGlobal;
│ ^^^^^^^^^^^^^^^^^^^^^^ REFERENCE TO variables can not reference other variables

error[E052]: Unknown type: AliasedDINT
┌─ <internal>:11:64
11 │ referenceToAlias : REFERENCE TO AliasedDINT;
│ ^^^^^^^^^^^ Unknown type: AliasedDINT

error[E052]: Unknown type: fooLocal
┌─ <internal>:17:41
17 │ invalidA : REFERENCE TO fooLocal;
│ ^^^^^^^^ Unknown type: fooLocal

error[E052]: Unknown type: fooGlobal
┌─ <internal>:18:41
18 │ invalidB : REFERENCE TO fooGlobal;
│ ^^^^^^^^^ Unknown type: fooGlobal

error[E037]: Invalid assignment: cannot assign 'INT' to 'DINT'
┌─ <internal>:28:13
Expand Down
3 changes: 0 additions & 3 deletions src/validation/tests/reference_resolve_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,6 @@ fn resolve_function_members_via_qualifier() {
let diagnostics = parse_and_validate_buffered(
"
PROGRAM prg
VAR
s : MyStruct;
END_VAR
foo(a := 1, b := 2, c := 3); (* ok *)
foo.a; (* not ok *)
foo.b; (* not ok *)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,19 @@ source: src/validation/tests/reference_resolve_tests.rs
expression: "&diagnostics"
---
error[E048]: Could not resolve reference to a
┌─ <internal>:7:21
┌─ <internal>:4:21
7 │ foo.a; (* not ok *)
4 │ foo.a; (* not ok *)
│ ^ Could not resolve reference to a

error[E048]: Could not resolve reference to b
┌─ <internal>:8:21
┌─ <internal>:5:21
8 │ foo.b; (* not ok *)
5 │ foo.b; (* not ok *)
│ ^ Could not resolve reference to b

error[E048]: Could not resolve reference to c
┌─ <internal>:9:21
┌─ <internal>:6:21
9 │ foo.c; (* not ok *)
6 │ foo.c; (* not ok *)
│ ^ Could not resolve reference to c


Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,37 @@ source: src/validation/tests/statement_validation_tests.rs
expression: "&diagnostics"
---
warning[E067]: Implicit downcast from 'LINT' to 'INT'.
┌─ <internal>:11:17
┌─ <internal>:10:17
11 │ var1_lint, // downcast
10 │ var1_lint, // downcast
│ ^^^^^^^^^ Implicit downcast from 'LINT' to 'INT'.

warning[E067]: Implicit downcast from 'LWORD' to 'DWORD'.
┌─ <internal>:12:17
┌─ <internal>:11:17
12 │ var_lword, // downcast
11 │ var_lword, // downcast
│ ^^^^^^^^^ Implicit downcast from 'LWORD' to 'DWORD'.

warning[E067]: Implicit downcast from 'INT' to 'SINT'.
┌─ <internal>:14:17
┌─ <internal>:13:17
14 │ INT#var1_lint, // downcast
13 │ INT#var1_lint, // downcast
│ ^^^^^^^^^^^^^ Implicit downcast from 'INT' to 'SINT'.

warning[E067]: Implicit downcast from 'LINT' to 'INT'.
┌─ <internal>:15:17
┌─ <internal>:14:17
15 │ var2_lint, // downcast
14 │ var2_lint, // downcast
│ ^^^^^^^^^ Implicit downcast from 'LINT' to 'INT'.

error[E037]: Invalid assignment: cannot assign 'WSTRING' to 'STRING'
┌─ <internal>:16:17
┌─ <internal>:15:17
16 │ var_wstr, // invalid
15 │ var_wstr, // invalid
│ ^^^^^^^^ Invalid assignment: cannot assign 'WSTRING' to 'STRING'

warning[E067]: Implicit downcast from 'LINT' to 'DINT'.
┌─ <internal>:17:17
┌─ <internal>:16:17
17 │ var1_lint // downcast
16 │ var1_lint // downcast
│ ^^^^^^^^^ Implicit downcast from 'LINT' to 'DINT'.
1 change: 0 additions & 1 deletion src/validation/tests/statement_validation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,6 @@ fn program_implicit_downcast() {
r#"
PROGRAM main
VAR
fb: fb_t;
var1_lint, var2_lint : LINT;
var_real : REAL;
var_lword : LWORD;
Expand Down
111 changes: 111 additions & 0 deletions src/validation/tests/variable_validation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,117 @@ fn unresolved_references_to_const_builtins_in_initializer_are_reported() {
"###);
}

#[test]
fn unknown_types_are_reported() {
let diagnostics = parse_and_validate_buffered(
r#"
VAR_GLOBAL
a: undefined;
END_VAR
"#,
);

assert_snapshot!(diagnostics, @r###"
error[E052]: Unknown type: undefined
┌─ <internal>:3:16
3 │ a: undefined;
│ ^^^^^^^^^ Unknown type: undefined

"###);
}

#[test]
fn aliases_are_not_reported_as_unknown_types() {
let diagnostics = parse_and_validate_buffered(
r#"
TYPE myDeclaredType : DINT; END_TYPE
VAR_GLOBAL
a: myDeclaredType;
END_VAR
"#,
);

assert!(diagnostics.is_empty())
}

#[test]
fn aliasing_to_undeclared_type_is_an_error() {
let diagnostics = parse_and_validate_buffered(
r#"
TYPE myDeclaredType : undeclaredType; END_TYPE
VAR_GLOBAL
a: myDeclaredType;
END_VAR
"#,
);

assert_snapshot!(diagnostics, @r###"
error[E052]: Unknown type: myDeclaredType
┌─ <internal>:4:16
4 │ a: myDeclaredType;
│ ^^^^^^^^^^^^^^ Unknown type: myDeclaredType
Comment on lines +771 to +772
Copy link
Member

Choose a reason for hiding this comment

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

We already talked about this yesterday offline, but the UX isn't good here; maybe we should also track this in a issue seperately and see if there's a solution to this (minor however)


"###);
}

#[test]
fn trying_to_initialize_a_pointer_of_unknown_type_is_reported() {
let diagnostics = parse_and_validate_buffered(
r#"
VAR_GLOBAL
a: undefined;
END_VAR
FUNCTION_BLOCK foo
VAR
bar : REF_TO undefined := REF(a);
END_VAR
END_FUNCTION_BLOCK
"#,
);

assert_snapshot!(diagnostics, @r###"
error[E052]: Unknown type: undefined
┌─ <internal>:7:26
7 │ bar : REF_TO undefined := REF(a);
│ ^^^^^^^^^ Unknown type: undefined

error[E052]: Unknown type: undefined
┌─ <internal>:3:16
3 │ a: undefined;
│ ^^^^^^^^^ Unknown type: undefined

"###);
}

#[test]
fn trying_to_initialize_a_pointer_with_builtin_ref_with_type_mismatch_leads_to_error() {
let diagnostics = parse_and_validate_buffered(
r#"
VAR_GLOBAL
a: DINT;
END_VAR
FUNCTION_BLOCK foo
VAR
bar : REF_TO STRING := REF(a);
END_VAR
END_FUNCTION_BLOCK
"#,
);

assert_snapshot!(diagnostics, @r###"
error[E037]: Invalid assignment: cannot assign 'DINT' to 'REF_TO STRING := REF(a)'
┌─ <internal>:7:36
7 │ bar : REF_TO STRING := REF(a);
│ ^^^^^^ Invalid assignment: cannot assign 'DINT' to 'REF_TO STRING := REF(a)'

"###);
}

#[test]
fn unconfigured_template_variables_are_validated() {
let diagnostics = parse_and_validate_buffered(
Expand Down
14 changes: 12 additions & 2 deletions src/validation/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,15 @@ pub fn visit_data_type_declaration<T: AnnotationMap>(
declaration: &DataTypeDeclaration,
context: &ValidationContext<T>,
) {
if let DataTypeDeclaration::DataTypeDefinition { data_type, location, .. } = declaration {
visit_data_type(validator, data_type, location, context);
match declaration {
DataTypeDeclaration::DataTypeReference { referenced_type, location } => {
if context.index.find_effective_type_by_name(referenced_type).is_none() {
validator.push_diagnostic(Diagnostic::unknown_type(referenced_type, location.into()));
};
}
DataTypeDeclaration::DataTypeDefinition { data_type, location, .. } => {
visit_data_type(validator, data_type, location, context)
}
}
}

Expand All @@ -39,6 +46,9 @@ pub fn visit_data_type<T: AnnotationMap>(
DataType::VarArgs { referenced_type: Some(referenced_type), .. } => {
visit_data_type_declaration(validator, referenced_type.as_ref(), context);
}
DataType::PointerType { referenced_type, .. } => {
visit_data_type_declaration(validator, referenced_type.as_ref(), context);
}
_ => {}
}
}
Expand Down
Loading
Loading