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

fix: false-positive downcast warnings #1114

Merged
merged 26 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
417e23a
fix: false-positive downcast warnings
mhasel Mar 1, 2024
f41bb6b
Merge branch 'master' into wrong-downcast
mhasel Mar 4, 2024
6635ad0
remove todo comment
mhasel Mar 4, 2024
1419a30
change info to warning, add warning for float=>int
mhasel Mar 5, 2024
4ee84f3
Merge branch 'master' into wrong-downcast
mhasel Mar 5, 2024
eb49360
fix failing test
mhasel Mar 5, 2024
ecb567c
adde two failing tests
riederm Mar 6, 2024
5dbe860
fix unary expressions
mhasel Mar 7, 2024
899b4d2
fix: modulo operation now only validates the lhs-operand for downcast
mhasel Mar 11, 2024
87b822f
fix edge cases for MUL and SEL
mhasel Mar 11, 2024
907efe9
Merge branch 'master' into wrong-downcast
mhasel Mar 11, 2024
65f0141
change downcast severity to warning
mhasel Mar 11, 2024
8b058c1
exclude aggregate types from downcast validation
mhasel Mar 11, 2024
0db1184
add explicit cast to integration test file so it doesn't warn when ru…
mhasel Mar 11, 2024
668d7ef
fmt
mhasel Mar 11, 2024
df6170c
remove unnecessary collect()
mhasel Mar 11, 2024
b75bafd
reduce nesting/branching
mhasel Mar 11, 2024
1c8a733
swap boolean operands to benefit from short-circuit evaluation
mhasel Mar 11, 2024
f8299b2
Merge branch 'master' into wrong-downcast
mhasel Mar 12, 2024
d687971
impl Clone
volsa Mar 14, 2024
d6cd8d2
Merge branch 'master' into wrong-downcast
mhasel Mar 14, 2024
32eecfe
Merge branch 'master' into wrong-downcast
mhasel Mar 15, 2024
0102554
Merge branch 'master' into wrong-downcast
mhasel Mar 16, 2024
d763042
use slice for dt names
mhasel Mar 18, 2024
fbca00b
review feedback
mhasel Mar 18, 2024
1b4cb0e
fmt
mhasel Mar 18, 2024
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
12 changes: 12 additions & 0 deletions compiler/plc_ast/src/literals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,18 @@ impl AstLiteral {
| AstLiteral::DateAndTime { .. }
)
}

pub fn is_numerical(&self) -> bool {
matches!(
self,
AstLiteral::Integer { .. }
| AstLiteral::Real { .. }
| AstLiteral::Time { .. }
| AstLiteral::Date { .. }
| AstLiteral::TimeOfDay { .. }
| AstLiteral::DateAndTime { .. }
)
}
}

impl Debug for AstLiteral {
Expand Down
90 changes: 70 additions & 20 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,8 @@ 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 {
validate_assignment_type_sizes(validator, left_type, right, context)
}
}
}
Expand Down Expand Up @@ -1272,26 +1271,77 @@ 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>,
) {
if left.get_type_information().get_size(context.index)
< right.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()
))
.with_error_code("E067")
.with_location(location.clone()),
)
use std::collections::HashMap;
fn get_expression_types_and_locations<'b, T: AnnotationMap>(
expression: &AstNode,
context: &'b ValidationContext<T>,
lhs_is_signed_int: bool,
) -> HashMap<&'b DataType, Vec<SourceLocation>> {
let mut map: HashMap<&DataType, Vec<SourceLocation>> = HashMap::new();
match expression.get_stmt_peeled() {
AstStatement::BinaryExpression(BinaryExpression { operator, left, right, .. }) => {
mhasel marked this conversation as resolved.
Show resolved Hide resolved
if !operator.is_bool_type() {
get_expression_types_and_locations(left, context, lhs_is_signed_int)
.into_iter()
.for_each(|(k, v)| map.entry(k).or_default().extend(v));
get_expression_types_and_locations(right, context, lhs_is_signed_int)
.into_iter()
.for_each(|(k, v)| map.entry(k).or_default().extend(v));
};
}
// `get_literal_actual_signed_type_name` will always return `LREAL` for FP literals, so they will be handled by the fall-through case according to their annotated type
AstStatement::Literal(lit) if !matches!(lit, &AstLiteral::Real(_)) => {
if lit.is_numerical() {
if let Some(dt) = get_literal_actual_signed_type_name(lit, lhs_is_signed_int)
.map(|name| context.index.get_type(name).unwrap_or(context.index.get_void_type()))
{
map.entry(dt).or_default().push(expression.get_location());
}
}
}
_ => {
if context.annotations.get_generic_nature(expression).is_none() {
if let Some(dt) = context.annotations.get_type(expression, context.index) {
map.entry(dt).or_default().push(expression.get_location());
}
}
}
};
map
}

let lhs = left.get_type_information();
let lhs_size = lhs.get_size(context.index);
let results_in_truncation = |rhs: &DataType| {
let rhs = rhs.get_type_information();
let rhs_size = rhs.get_size(context.index);
lhs_size < rhs_size
|| (lhs_size == rhs_size
&& ((lhs.is_signed_int() && rhs.is_unsigned_int()) || (lhs.is_int() && rhs.is_float())))
};

get_expression_types_and_locations(right, context, lhs.is_signed_int())
.into_iter()
.filter(|(dt, _)| results_in_truncation(dt))
.for_each(|(dt, location)| {
location.into_iter().for_each(|loc| {
validator.push_diagnostic(
Diagnostic::warning(format!(
"Implicit downcast from '{}' to '{}'.",
get_datatype_name_or_slice(validator.context, dt),
left.get_name()
))
.with_error_code("E067")
.with_location(loc),
);
})
});
}

mod helper {
Expand Down
97 changes: 97 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,100 @@ 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_bigger_types_in_expression() {
let diagnostics = parse_and_validate_buffered(
"
FUNCTION main : INT
VAR
a, b, c : SINT;
d, e : DINT;
END_VAR
a := a + b + c + d + e;
END_FUNCTION
",
);

assert_snapshot!(diagnostics)
}

#[test]
fn literals_out_of_range_for_lhs_will_result_in_downcast_warning() {
let diagnostics = parse_and_validate_buffered(
"
FUNCTION main : INT
VAR
a : SINT;
b : USINT;
END_VAR

a := a + 255 + 1000; // literals out of range of i8 -> warning
b := b + 255; // 255 is in range of u8 -> no warning, but will silently overflow for anything other than b == 0. Not sure if there is a better way to guard against this
a := a + b; // B is same size as a, but unsigned -> warning
a := a + USINT#100; // will fit into a, but is cast to unsigned type -> warning
END_FUNCTION
",
);

assert_snapshot!(diagnostics)
}


#[test]
fn literals_out_of_range_inside_unary_expressions_will_cause_no_warning() {
// GIVEN literals behind unary expressions (which will be annotated as DINT)
// WHEN we validate
let diagnostics = parse_and_validate_buffered(
"
FUNCTION main : INT
VAR
a : INT;
END_VAR
a := a;
a := a + 254;
a := a + 254 - 20;
a := a + 254 + (-(10+10)); //rewrite this as a unary expression
END_FUNCTION
",
);
//WE EXPECT NO VALIDATION PROBLEMS
assert_snapshot!(diagnostics)
}

#[test]
fn literals_out_of_range_in_a_modulo_operation_cannot_exceed_the_left_operand() {
// GIVEN an expression INT MOD DINT
// WHEN we validate
let diagnostics = parse_and_validate_buffered(
"
FUNCTION main : INT
VAR
a : INT;
d : DINT := 9;
END_VAR
a := a + (a MOD d); // I know this is bullshit - lets ignore it
mhasel marked this conversation as resolved.
Show resolved Hide resolved
END_FUNCTION
",
);
//THEN we expect no validation problems, since a mod d should remain in a's datatype
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 `[]`

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

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

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

warning: 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,66 @@
source: src/validation/tests/assignment_validation_tests.rs
expression: diagnostics
---
warning: Implicit downcast from 'LREAL' to 'BYTE'.
┌─ <internal>:29:15
29 │ v_byte := v_lreal; // valid
│ ^^^^^^^ Implicit downcast from 'LREAL' to 'BYTE'.

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

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

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

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

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

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

warning: Implicit downcast from 'TIME' to 'BYTE'.
┌─ <internal>:36:15
36 │ v_byte := TIME#10h20m30s; // valid
│ ^^^^^^^^^^^^^^ Implicit downcast from 'TIME' to 'BYTE'.

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

warning: 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 +92,38 @@ error: Invalid assignment: cannot assign 'CHAR' to 'BYTE'
43 │ v_byte := CHAR#'c'; // INVALID
│ ^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'CHAR' to 'BYTE'

warning: 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'.

warning: Implicit downcast from 'TIME_OF_DAY' to 'BYTE'.
┌─ <internal>:45:15
45 │ v_byte := TOD#15:36:30; // valid
│ ^^^^^^^^^^^^ Implicit downcast from 'TIME_OF_DAY' to 'BYTE'.

warning: 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'

warning: 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'


Loading
Loading