fix: false-positive downcast warnings (PLC-lang#1114)

* fix: false-positive downcast warnings

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.
---------

Co-authored-by: Mathias Rieder <[email protected]>
Co-authored-by: Volkan Sagcan <[email protected]>

Author: 3 people

compiler/plc_ast/src/ast.rs

@@ -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!(

compiler/plc_ast/src/literals.rs

@@ -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 {

compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs

@@ -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,

src/resolver.rs

@@ -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()

src/validation.rs

@@ -30,7 +30,6 @@ mod variable;
 #[cfg(test)]
 mod tests;
 
-#[derive(Clone)]
 pub struct ValidationContext<'s, T: AnnotationMap> {
     annotations: &'s T,
     index: &'s Index,
@@ -86,6 +85,17 @@ impl<'s, T: AnnotationMap> ValidationContext<'s, T> {
     }
 }
 
+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,
+        }
+    }
+}
+
 /// This trait should be implemented by any validator used by `validation::Validator`
 pub trait Validators {
     fn push_diagnostic(&mut self, diagnostic: Diagnostic);

src/validation/statement.rs

@@ -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},
@@ -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)
         }
     }
 }
@@ -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 {