[move][move-linter] implemen meaningless math operator rules

external-crates/move/crates/move-compiler/src/linters/meaningless_math_operation.rs

@@ -0,0 +1,105 @@
+// Copyright (c) The Move Contributors
+// SPDX-License-Identifier: Apache-2.0
+
+//! Detects meaningless math operations like `x * 0`, `x << 0`, `x >> 0`, `x * 1`, `x + 0`, `x - 0`, and `x / 0`.
+//! Aims to reduce code redundancy and improve clarity by flagging operations with no effect.
+use crate::{
+    diag,
+    diagnostics::{
+        codes::{custom, DiagnosticInfo, Severity},
+        WarningFilters,
+    },
+    expansion::ast::Value_,
+    parser::ast::BinOp_,
+    shared::CompilationEnv,
+    typing::{
+        ast::{self as T, Exp, UnannotatedExp_},
+        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+    },
+};
+use move_ir_types::location::Loc;
+
+use super::{LinterDiagnosticCategory, LINT_WARNING_PREFIX, MEANINGLESS_MATH_DIAG_CODE};
+
+const MEANINGLESS_MATH_OP_DIAG: DiagnosticInfo = custom(
+    LINT_WARNING_PREFIX,
+    Severity::Warning,
+    LinterDiagnosticCategory::Complexity as u8,
+    MEANINGLESS_MATH_DIAG_CODE,
+    "Detected a meaningless math operation that has no effect.",
+);
+
+pub struct MeaninglessMathOperation;
+
+pub struct Context<'a> {
+    env: &'a mut CompilationEnv,
+}
+
+impl TypingVisitorConstructor for MeaninglessMathOperation {
+    type Context<'a> = Context<'a>;
+
+    fn context<'a>(env: &'a mut CompilationEnv, _program: &T::Program) -> Self::Context<'a> {
+        Context { env }
+    }
+}
+
+impl TypingVisitorContext for Context<'_> {
+    fn visit_exp_custom(&mut self, exp: &mut T::Exp) -> bool {
+        if let UnannotatedExp_::BinopExp(_, op, _, rhs) = &exp.exp.value {
+            match op.value {
+                BinOp_::Mul | BinOp_::Div => {
+                    if is_zero(&rhs) || (matches!(op.value, BinOp_::Mul) && is_one(&rhs)) {
+                        report_meaningless_math_op(self.env, op.loc);
+                    }
+                }
+                BinOp_::Add | BinOp_::Sub => {
+                    if is_zero(&rhs) {
+                        report_meaningless_math_op(self.env, op.loc);
+                    }
+                }
+                BinOp_::Shl | BinOp_::Shr => {
+                    if is_zero(&rhs) {
+                        report_meaningless_math_op(self.env, op.loc);
+                    }
+                }
+                _ => {}
+            }
+        }
+
+        false
+    }
+    fn add_warning_filter_scope(&mut self, filter: WarningFilters) {
+        self.env.add_warning_filter_scope(filter)
+    }
+
+    fn pop_warning_filter_scope(&mut self) {
+        self.env.pop_warning_filter_scope()
+    }
+}
+
+fn is_zero(exp: &Exp) -> bool {
+    if let UnannotatedExp_::Value(spanned) = &exp.exp.value {
+        matches!(spanned.value, Value_::U64(0))
+    } else {
+        false
+    }
+}
+
+fn is_one(exp: &Exp) -> bool {
+    if let UnannotatedExp_::Value(spanned) = &exp.exp.value {
+        matches!(spanned.value, Value_::U64(1))
+    } else {
+        false
+    }
+}
+
+fn report_meaningless_math_op(env: &mut CompilationEnv, loc: Loc) {
+    let diag = diag!(
+        MEANINGLESS_MATH_OP_DIAG,
+        (
+            loc,
+            "Detected a meaningless math operation that has no effect.",
+        )
+    );
+    env.add_diag(diag);
+}

external-crates/move/crates/move-compiler/src/linters/mod.rs

@@ -8,6 +8,7 @@ use crate::{
     linters::constant_naming::ConstantNamingVisitor, typing::visitor::TypingVisitor,
 };
 pub mod constant_naming;
+pub mod meaningless_math_operation;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum LintLevel {
@@ -34,26 +35,39 @@ pub const ALLOW_ATTR_CATEGORY: &str = "lint";
 pub const LINT_WARNING_PREFIX: &str = "Lint ";
 pub const CONSTANT_NAMING_FILTER_NAME: &str = "constant_naming";
 pub const CONSTANT_NAMING_DIAG_CODE: u8 = 1;
+pub const MEANINGLESS_MATH_FILTER_NAME: &str = "meaningless_math_operation";
+pub const MEANINGLESS_MATH_DIAG_CODE: u8 = 8;
 
 pub fn known_filters() -> (Option<Symbol>, Vec<WarningFilter>) {
     (
         Some(ALLOW_ATTR_CATEGORY.into()),
-        vec![WarningFilter::code(
-            Some(LINT_WARNING_PREFIX),
-            LinterDiagnosticCategory::Style as u8,
-            CONSTANT_NAMING_DIAG_CODE,
-            Some(CONSTANT_NAMING_FILTER_NAME),
-        )],
+        vec![
+            WarningFilter::code(
+                Some(LINT_WARNING_PREFIX),
+                LinterDiagnosticCategory::Style as u8,
+                CONSTANT_NAMING_DIAG_CODE,
+                Some(CONSTANT_NAMING_FILTER_NAME),
+            ),
+            WarningFilter::code(
+                Some(LINT_WARNING_PREFIX),
+                LinterDiagnosticCategory::Complexity as u8,
+                MEANINGLESS_MATH_DIAG_CODE,
+                Some(MEANINGLESS_MATH_FILTER_NAME),
+            ),
+        ],
     )
 }
 
 pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
     match level {
         LintLevel::None | LintLevel::Default => vec![],
         LintLevel::All => {
-            vec![constant_naming::ConstantNamingVisitor::visitor(
-                ConstantNamingVisitor,
-            )]
+            vec![
+                constant_naming::ConstantNamingVisitor::visitor(ConstantNamingVisitor),
+                meaningless_math_operation::MeaninglessMathOperation::visitor(
+                    meaningless_math_operation::MeaninglessMathOperation,
+                ),
+            ]
         }
     }
 }

external-crates/move/crates/move-compiler/tests/linter/false_negative_meaningless_math_operation.exp

@@ -0,0 +1,8 @@
+warning[Lint W01008]: Detected a meaningless math operation that has no effect.
+   ┌─ tests/linter/false_negative_meaningless_math_operation.move:16:11
+   │
+16 │         x / 0 // This is undefined behavior, should be caught by a different linter
+   │           ^ Detected a meaningless math operation that has no effect.
+   │
+   = This warning can be suppressed with '#[allow(lint(meaningless_math_operation))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

external-crates/move/crates/move-compiler/tests/linter/false_negative_meaningless_math_operation.move

@@ -0,0 +1,22 @@
+module 0x42::M {
+     public fun complex_zero_operation(x: u64): u64 {
+         x * (1 - 1) // This is effectively x * 0, but might not be caught
+     }
+
+     public fun complex_one_operation(x: u64): u64 {
+         x * (2 - 1) // This is effectively x * 1, but might not be caught
+     }
+
+     public fun zero_shift_complex(x: u64, y: u64): u64 {
+         x * (y - y) // This is effectively x << 0, but might not be caught
+     }
+
+    // Edge cases
+    public fun divide_by_zero(x: u64): u64 {
+        x / 0 // This is undefined behavior, should be caught by a different linter
+    }
+
+    public fun complex_zero_divide(x: u64): u64 {
+        x / (1 - 1) // This is also undefined, might not be caught by this linter
+    }
+}

external-crates/move/crates/move-compiler/tests/linter/false_positive_meaningless_math_operation.move

@@ -0,0 +1,9 @@
+module 0x42::M {
+   public fun multiply_by_zero_var(x: u64, y: u64): u64 {
+        x * y // Might trigger if y is always 0, but shouldn't if y is variable
+    }
+
+    public fun add_zero_var(x: u64, y: u64): u64 {
+        x + y // Might trigger if y is always 0, but shouldn't if y is variable
+    }
+}

external-crates/move/crates/move-compiler/tests/linter/true_negative_meaningless_math_operation.move

@@ -0,0 +1,17 @@
+module 0x42::M {
+    public fun multiply_by_two(x: u64): u64 {
+        x * 2 // Should not trigger the linter
+    }
+
+    public fun left_shift_by_one(x: u64): u64 {
+        x << 1 // Should not trigger the linter
+    }
+
+    public fun add_one(x: u64): u64 {
+        x + 1 // Should not trigger the linter
+    }
+
+    public fun divide_by_two(x: u64): u64 {
+        x / 2 // Should not trigger the linter
+    }
+}