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 all 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
16 changes: 2 additions & 14 deletions compiler/plc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1057,20 +1057,8 @@ pub fn pre_process(unit: &mut CompilationUnit, id_provider: IdProvider) {
pre_processor::pre_process(unit, id_provider)
}
impl Operator {
/// returns true, if this operator results in a bool value
pub fn is_bool_type(&self) -> bool {
matches!(
self,
Operator::Equal
| Operator::NotEqual
| Operator::Less
| Operator::Greater
| Operator::LessOrEqual
| Operator::GreaterOrEqual
)
}

/// returns true, if this operator is a comparison operator
/// returns true, if this operator is a comparison operator,
/// resulting in a bool value
/// (=, <>, >, <, >=, <=)
pub fn is_comparison_operator(&self) -> bool {
matches!(
Expand Down
12 changes: 12 additions & 0 deletions compiler/plc_ast/src/literals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,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
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ lazy_static! {
Error,
include_str!("./error_codes/E066.md"),
E067,
Info,
Warning,
include_str!("./error_codes/E067.md"), //Implicit typecast
E068,
Error,
Expand Down
2 changes: 1 addition & 1 deletion src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1327,7 +1327,7 @@ impl<'i> TypeAnnotator<'i> {
)
};

let target_name = if data.operator.is_bool_type() {
let target_name = if data.operator.is_comparison_operator() {
BOOL_TYPE.to_string()
} else {
bigger_type.get_name().to_string()
Expand Down
12 changes: 11 additions & 1 deletion src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#[cfg(test)]
mod tests;

#[derive(Clone)]
pub struct ValidationContext<'s, T: AnnotationMap> {
annotations: &'s T,
index: &'s Index,
Expand Down Expand Up @@ -86,6 +85,17 @@
}
}

impl<'s, T: AnnotationMap> Clone for ValidationContext<'s, T> {
fn clone(&self) -> Self {
ValidationContext {
annotations: self.annotations,
index: self.index,
qualifier: self.qualifier,
is_call: self.is_call,
}
}

Check warning on line 96 in src/validation.rs

View check run for this annotation

Codecov / codecov/patch

src/validation.rs#L89-L96

Added lines #L89 - L96 were not covered by tests
}

/// This trait should be implemented by any validator used by `validation::Validator`
pub trait Validators {
fn push_diagnostic(&mut self, diagnostic: Diagnostic);
Expand Down
119 changes: 99 additions & 20 deletions src/validation/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use std::{collections::HashSet, mem::discriminant};
use plc_ast::control_statements::ForLoopStatement;
use plc_ast::{
ast::{
flatten_expression_list, AstNode, AstStatement, DirectAccess, DirectAccessType, JumpStatement,
Operator, ReferenceAccess,
flatten_expression_list, AstNode, AstStatement, BinaryExpression, CallStatement, DirectAccess,
DirectAccessType, JumpStatement, Operator, ReferenceAccess, UnaryExpression,
},
control_statements::{AstControlStatement, ConditionalBlock},
literals::{Array, AstLiteral, StringValue},
Expand Down Expand Up @@ -833,9 +833,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 @@ -1343,26 +1342,106 @@ 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::new(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,
is_builtin_call: 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, .. })
if !operator.is_comparison_operator() =>
{
get_expression_types_and_locations(left, context, lhs_is_signed_int, false)
.into_iter()
.for_each(|(k, v)| map.entry(k).or_default().extend(v));
// the RHS type in a MOD expression has no impact on the resulting value type
if matches!(operator, Operator::Modulo) {
return map
};
get_expression_types_and_locations(right, context, lhs_is_signed_int, false)
.into_iter()
.for_each(|(k, v)| map.entry(k).or_default().extend(v));
}
AstStatement::UnaryExpression(UnaryExpression { operator, value })
if !operator.is_comparison_operator() =>
{
get_expression_types_and_locations(value, context, lhs_is_signed_int, false)
.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() {
return map
}
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());
}
}
AstStatement::CallStatement(CallStatement { operator, parameters })
// special handling for builtin selector functions MUX and SEL
if matches!(operator.get_flat_reference_name().unwrap_or_default(), "MUX" | "SEL") =>
{
let Some(args) = parameters else {
return map
};
if let AstStatement::ExpressionList(list) = args.get_stmt_peeled() {
// skip the selector argument since it will never be assigned to the target type
list.iter().skip(1).flat_map(|arg| {
get_expression_types_and_locations(arg, context, lhs_is_signed_int, true)
})
.for_each(|(k, v)| map.entry(k).or_default().extend(v));
};
}
_ => {
if !(context.annotations.get_generic_nature(expression).is_none() || is_builtin_call) {
return map
};
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(), false)
.into_iter()
.filter(|(dt, _)| !dt.is_aggregate_type() && results_in_truncation(dt))
.for_each(|(dt, location)| {
location.into_iter().for_each(|loc| {
validator.push_diagnostic(
Diagnostic::new(format!(
"Implicit downcast from '{}' to '{}'.",
get_datatype_name_or_slice(validator.context, dt),
get_datatype_name_or_slice(validator.context, left)
))
.with_error_code("E067")
.with_location(loc),
);
})
});
}

mod helper {
Expand Down
Loading
Loading