Skip to content

Commit

Permalink
fix: false-positive downcast warnings
Browse files Browse the repository at this point in the history
Fixes false-positive downcast warnings on integrally promoted expressions. This is achieved by checking
each element's type in the expression individually and comparing it to the resulting type rather than checking
the expression's type as a whole.

TODO: literals that are out of range of the target type (i.e. x + 300 when assigning to USINT)
  • Loading branch information
mhasel committed Mar 1, 2024
1 parent b0f0073 commit dae46d0
Show file tree
Hide file tree
Showing 17 changed files with 439 additions and 35 deletions.
59 changes: 47 additions & 12 deletions src/validation/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ use std::{collections::HashSet, mem::discriminant};

use plc_ast::{
ast::{
flatten_expression_list, AstNode, AstStatement, DirectAccess, DirectAccessType, JumpStatement,
Operator, ReferenceAccess,
flatten_expression_list, AstNode, AstStatement, BinaryExpression, DirectAccess,
DirectAccessType, JumpStatement, Operator, ReferenceAccess,
},
control_statements::{AstControlStatement, ConditionalBlock},
literals::{Array, AstLiteral, StringValue},
Expand Down Expand Up @@ -815,9 +815,9 @@ fn validate_assignment<T: AnnotationMap>(
location.clone(),
));
}
} else if right.is_literal() {
// TODO: See https://github.com/PLC-lang/rusty/issues/857
// validate_assignment_type_sizes(validator, left_type, right_type, location, context)
} else {
// todo: literals
validate_assignment_type_sizes(validator, left_type, right, context)
}
}
}
Expand Down Expand Up @@ -1272,21 +1272,56 @@ fn validate_type_nature<T: AnnotationMap>(
}
}

fn _validate_assignment_type_sizes<T: AnnotationMap>(
fn validate_assignment_type_sizes<T: AnnotationMap>(
validator: &mut Validator,
left: &DataType,
right: &DataType,
location: &SourceLocation,
right: &AstNode,
context: &ValidationContext<T>,
) {
use std::collections::HashMap;
// todo: check literal sizes in expressions
let mut types_in_expr: HashMap<Option<&DataType>, SourceLocation> = HashMap::new();
fn helper<'b, T: AnnotationMap>(
node: &AstNode,
set: &mut HashMap<Option<&'b DataType>, SourceLocation>,
context: &'b ValidationContext<T>,
) {
match node.get_stmt_peeled() {
AstStatement::BinaryExpression(BinaryExpression { operator, left, right, .. }) => {
if operator.is_bool_type() {
return
};
helper(left, set, context);
helper(right, set, context);
}
AstStatement::Literal(_) => (),
_ => {
if context.annotations.get_generic_nature(node).is_some() {
return;
}
set.insert(context.annotations.get_type(node, context.index), node.get_location());
}
};
}

helper(right, &mut types_in_expr, context);

let mut dts = types_in_expr.keys().filter_map(|it| *it);
let Some(mut biggest) = dts.next() else { return };
for dt in dts {
biggest = typesystem::get_bigger_type(biggest, dt, context.index);
}

let location = types_in_expr.get(&Some(biggest)).unwrap();

if left.get_type_information().get_size(context.index)
< right.get_type_information().get_size(context.index)
< biggest.get_type_information().get_size(context.index)
{
validator.push_diagnostic(
Diagnostic::info(format!(
"Potential loss of information due to assigning '{}' to variable of type '{}'.",
left.get_name(),
right.get_name()
"Implicit downcast from '{}' to '{}'.",
biggest.get_name(),
left.get_name()
))
.with_error_code("E067")
.with_location(location.clone()),
Expand Down
34 changes: 34 additions & 0 deletions src/validation/tests/assignment_validation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -962,3 +962,37 @@ fn string_type_alias_assignment_can_be_validated() {

assert_snapshot!(diagnostics);
}

#[test]
fn integral_promotion_in_expression_does_not_cause_downcast_warning() {
let diagnostics = parse_and_validate_buffered(
"
FUNCTION main : INT
VAR
a, b, c : SINT;
END_VAR
a := a + b + c + 100;
END_FUNCTION
"
);

assert!(diagnostics.is_empty())
}

#[test]
fn downcast_will_report_biggest_type_in_expression() {
let diagnostics = parse_and_validate_buffered(
"
FUNCTION main : INT
VAR
a, b, c : SINT;
d : DINT;
END_VAR
a := a + b + c + d;
END_FUNCTION
"
);

assert_snapshot!(diagnostics)
}
3 changes: 1 addition & 2 deletions src/validation/tests/builtin_validation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ fn arithmetic_builtins_allow_mixing_of_fp_and_int_params() {
END_FUNCTION
",
);

assert!(diagnostics.is_empty());
assert_snapshot!(diagnostics);
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,30 @@ error: Array assignments must be surrounded with `[]`
39 │ v_arr_int_3 := (1, 2, 3, 4, 5, 6); // INVALID -> missing
│ ^^^^^^^^^^^^^^^^ Array assignments must be surrounded with `[]`

note: Implicit downcast from 'DINT' to 'INT'.
┌─ <internal>:40:23
40 │ v_arr_int_3[0] := v_dint; // valid
│ ^^^^^^ Implicit downcast from 'DINT' to 'INT'.

note: Implicit downcast from 'DINT' to 'INT'.
┌─ <internal>:41:23
41 │ v_arr_int_3[0] := DINT#10; // valid
│ ^^^^^^^ Implicit downcast from 'DINT' to 'INT'.

note: Implicit downcast from 'REAL' to 'INT'.
┌─ <internal>:42:23
42 │ v_arr_int_3[0] := v_real; // valid
│ ^^^^^^ Implicit downcast from 'REAL' to 'INT'.

note: Implicit downcast from 'REAL' to 'INT'.
┌─ <internal>:43:23
43 │ v_arr_int_3[0] := REAL#2.0; // valid
│ ^^^^^^^^ Implicit downcast from 'REAL' to 'INT'.

error: Invalid assignment: cannot assign 'STRING' to 'INT'
┌─ <internal>:44:5
Expand Down Expand Up @@ -103,5 +127,3 @@ error: Invalid assignment: cannot assign 'ARRAY[0..3] OF INT' to 'DINT'
52 │ v_dint := v_arr_int_3; // INVALID
│ ^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'ARRAY[0..3] OF INT' to 'DINT'


Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,60 @@
source: src/validation/tests/assignment_validation_tests.rs
expression: diagnostics
---
note: Implicit downcast from 'LREAL' to 'BYTE'.
┌─ <internal>:29:15
29 │ v_byte := v_lreal; // valid
│ ^^^^^^^ Implicit downcast from 'LREAL' to 'BYTE'.

note: Implicit downcast from 'REAL' to 'BYTE'.
┌─ <internal>:30:15
30 │ v_byte := REAL#2.0; // valid
│ ^^^^^^^^ Implicit downcast from 'REAL' to 'BYTE'.

note: Implicit downcast from 'UDINT' to 'BYTE'.
┌─ <internal>:31:15
31 │ v_byte := v_udint; // valid
│ ^^^^^^^ Implicit downcast from 'UDINT' to 'BYTE'.

note: Implicit downcast from 'UDINT' to 'BYTE'.
┌─ <internal>:32:15
32 │ v_byte := UDINT#10; // valid
│ ^^^^^^^^ Implicit downcast from 'UDINT' to 'BYTE'.

note: Implicit downcast from 'DINT' to 'BYTE'.
┌─ <internal>:33:15
33 │ v_byte := v_dint; // valid
│ ^^^^^^ Implicit downcast from 'DINT' to 'BYTE'.

note: Implicit downcast from 'DINT' to 'BYTE'.
┌─ <internal>:34:15
34 │ v_byte := DINT#20; // valid
│ ^^^^^^^ Implicit downcast from 'DINT' to 'BYTE'.

note: Implicit downcast from 'TIME' to 'BYTE'.
┌─ <internal>:35:15
35 │ v_byte := v_time; // valid
│ ^^^^^^ Implicit downcast from 'TIME' to 'BYTE'.

note: Implicit downcast from 'WORD' to 'BYTE'.
┌─ <internal>:37:15
37 │ v_byte := v_word; // valid
│ ^^^^^^ Implicit downcast from 'WORD' to 'BYTE'.

note: Implicit downcast from 'WORD' to 'BYTE'.
┌─ <internal>:38:15
38 │ v_byte := WORD#16#ffff; // valid
│ ^^^^^^^^^^^^ Implicit downcast from 'WORD' to 'BYTE'.

error: Invalid assignment: cannot assign 'STRING' to 'BYTE'
┌─ <internal>:39:5
Expand Down Expand Up @@ -32,16 +86,32 @@ error: Invalid assignment: cannot assign 'CHAR' to 'BYTE'
43 │ v_byte := CHAR#'c'; // INVALID
│ ^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'CHAR' to 'BYTE'

note: Implicit downcast from 'TIME_OF_DAY' to 'BYTE'.
┌─ <internal>:44:15
44 │ v_byte := v_tod; // valid
│ ^^^^^ Implicit downcast from 'TIME_OF_DAY' to 'BYTE'.

note: Implicit downcast from 'INT' to 'BYTE'.
┌─ <internal>:46:15
46 │ v_byte := v_ptr_int^; // valid
│ ^^^^^^^^^^ Implicit downcast from 'INT' to 'BYTE'.

error: Invalid assignment: cannot assign 'STRING' to 'BYTE'
┌─ <internal>:47:5
47 │ v_byte := v_ptr_string^; // INVALID
│ ^^^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'STRING' to 'BYTE'

note: Implicit downcast from 'INT' to 'BYTE'.
┌─ <internal>:48:15
48 │ v_byte := v_arr_int_3[0]; // valid
│ ^^^^^^^^^^^^^^ Implicit downcast from 'INT' to 'BYTE'.

error: Invalid assignment: cannot assign 'STRING' to 'BYTE'
┌─ <internal>:49:5
49 │ v_byte := v_arr_string_3[0]; // INVALID
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'STRING' to 'BYTE'


Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
source: src/validation/tests/assignment_validation_tests.rs
expression: diagnostics
---
note: Implicit downcast from 'DINT' to 'SINT'.
┌─ <internal>:7:30
7 │ a := a + b + c + d;
│ ^ Implicit downcast from 'DINT' to 'SINT'.
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,20 @@
source: src/validation/tests/assignment_validation_tests.rs
expression: "&diagnostics"
---
note: Implicit downcast from 'LINT' to 'DINT'.
┌─ <internal>:26:20
26 │ fb.foo(var1, var2, var3);
│ ^^^^ Implicit downcast from 'LINT' to 'DINT'.

note: Implicit downcast from 'LWORD' to 'DWORD'.
┌─ <internal>:26:26
26 │ fb.foo(var1, var2, var3);
│ ^^^^ Implicit downcast from 'LWORD' to 'DWORD'.

error: Invalid assignment: cannot assign 'STRING' to 'BYTE'
┌─ <internal>:26:32
26 │ fb.foo(var1, var2, var3);
│ ^^^^ Invalid assignment: cannot assign 'STRING' to 'BYTE'


Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,30 @@
source: src/validation/tests/assignment_validation_tests.rs
expression: diagnostics
---
note: Implicit downcast from 'LREAL' to 'UDINT'.
┌─ <internal>:31:16
31 │ v_udint := v_lreal; // valid
│ ^^^^^^^ Implicit downcast from 'LREAL' to 'UDINT'.

note: Implicit downcast from 'ULINT' to 'UDINT'.
┌─ <internal>:33:16
33 │ v_udint := v_ulint; // valid
│ ^^^^^^^ Implicit downcast from 'ULINT' to 'UDINT'.

note: Implicit downcast from 'ULINT' to 'UDINT'.
┌─ <internal>:34:16
34 │ v_udint := ULINT#10; // valid
│ ^^^^^^^^ Implicit downcast from 'ULINT' to 'UDINT'.

note: Implicit downcast from 'TIME' to 'UDINT'.
┌─ <internal>:37:16
37 │ v_udint := v_time; // valid
│ ^^^^^^ Implicit downcast from 'TIME' to 'UDINT'.

error: Invalid assignment: cannot assign 'STRING' to 'UDINT'
┌─ <internal>:41:5
Expand Down Expand Up @@ -32,6 +56,12 @@ error: Invalid assignment: cannot assign 'CHAR' to 'UDINT'
45 │ v_udint := CHAR#'c'; // INVALID
│ ^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'CHAR' to 'UDINT'

note: Implicit downcast from 'TIME_OF_DAY' to 'UDINT'.
┌─ <internal>:46:16
46 │ v_udint := v_tod; // valid
│ ^^^^^ Implicit downcast from 'TIME_OF_DAY' to 'UDINT'.

error: Invalid assignment: cannot assign 'STRING' to 'UDINT'
┌─ <internal>:49:5
Expand All @@ -44,6 +74,30 @@ error: Invalid assignment: cannot assign 'STRING' to 'UDINT'
51 │ v_udint := v_arr_string_3[0]; // INVALID
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'STRING' to 'UDINT'

note: Implicit downcast from 'LREAL' to 'DINT'.
┌─ <internal>:54:15
54 │ v_dint := v_lreal; // valid
│ ^^^^^^^ Implicit downcast from 'LREAL' to 'DINT'.

note: Implicit downcast from 'LINT' to 'DINT'.
┌─ <internal>:58:15
58 │ v_dint := v_lint; // valid
│ ^^^^^^ Implicit downcast from 'LINT' to 'DINT'.

note: Implicit downcast from 'LINT' to 'DINT'.
┌─ <internal>:59:15
59 │ v_dint := LINT#20; // valid
│ ^^^^^^^ Implicit downcast from 'LINT' to 'DINT'.

note: Implicit downcast from 'TIME' to 'DINT'.
┌─ <internal>:60:15
60 │ v_dint := v_time; // valid
│ ^^^^^^ Implicit downcast from 'TIME' to 'DINT'.

error: Invalid assignment: cannot assign 'STRING' to 'DINT'
┌─ <internal>:64:5
Expand Down Expand Up @@ -74,6 +128,12 @@ error: Invalid assignment: cannot assign 'CHAR' to 'DINT'
68 │ v_dint := CHAR#'c'; // INVALID
│ ^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'CHAR' to 'DINT'

note: Implicit downcast from 'TIME_OF_DAY' to 'DINT'.
┌─ <internal>:69:15
69 │ v_dint := v_tod; // valid
│ ^^^^^ Implicit downcast from 'TIME_OF_DAY' to 'DINT'.

error: Invalid assignment: cannot assign 'STRING' to 'DINT'
┌─ <internal>:72:5
Expand All @@ -85,5 +145,3 @@ error: Invalid assignment: cannot assign 'STRING' to 'DINT'
74 │ v_dint := v_arr_string_3[0]; // INVALID
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'STRING' to 'DINT'


Loading

0 comments on commit dae46d0

Please sign in to comment.