[Linter] Meaningless Math Operation

external-crates/move/crates/move-compiler/src/cfgir/visitor.rs

@@ -319,6 +319,18 @@ pub trait CFGIRVisitorContext {
     }
 }
 
+impl<V: CFGIRVisitor + 'static> From<V> for CFGIRVisitorObj {
+    fn from(value: V) -> Self {
+        Box::new(value)
+    }
+}
+
+impl<V: CFGIRVisitorConstructor> CFGIRVisitor for V {
+    fn visit(&mut self, env: &mut CompilationEnv, program: &mut G::Program) {
+        self.visit(env, program)
+    }
+}
+
 //**************************************************************************************************
 // simple absint visitor
 //**************************************************************************************************

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

@@ -0,0 +1,152 @@
+// 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`
+//! Aims to reduce code redundancy and improve clarity by flagging operations with no effect.
+use crate::{
+    cfgir::ast as G,
+    cfgir::visitor::{CFGIRVisitorConstructor, CFGIRVisitorContext},
+    diag,
+    diagnostics::{
+        codes::{custom, DiagnosticInfo, Severity},
+        WarningFilters,
+    },
+    hlir::ast::{self as H, Value_},
+    parser::ast::BinOp_,
+    shared::CompilationEnv,
+};
+use move_core_types::u256::U256;
+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,
+    "math operation can be simplified",
+);
+
+pub struct MeaninglessMathOperation;
+
+pub struct Context<'a> {
+    env: &'a mut CompilationEnv,
+}
+
+impl CFGIRVisitorConstructor for MeaninglessMathOperation {
+    type Context<'a> = Context<'a>;
+
+    fn context<'a>(env: &'a mut CompilationEnv, _program: &G::Program) -> Self::Context<'a> {
+        Context { env }
+    }
+}
+
+impl CFGIRVisitorContext for Context<'_> {
+    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 visit_exp_custom(&mut self, exp: &mut H::Exp) -> bool {
+        let H::UnannotatedExp_::BinopExp(lhs, op, rhs) = &exp.exp.value else {
+            return false;
+        };
+
+        // unchanged operations
+        let is_unchanged = match op.value {
+            BinOp_::Mul => is_one(lhs).or(is_one(rhs)),
+            BinOp_::Div => is_one(rhs),
+            BinOp_::Add => is_zero(lhs).or(is_zero(rhs)),
+            BinOp_::Sub => is_zero(rhs),
+            BinOp_::Shl | BinOp_::Shr => is_zero(rhs),
+            _ => None,
+        };
+        if let Some(meaningless_operand) = is_unchanged {
+            let msg = "This operation has no effect and can be removed";
+            self.env.add_diag(diag!(
+                MEANINGLESS_MATH_OP_DIAG,
+                (exp.exp.loc, msg),
+                (meaningless_operand, "Because of this operand"),
+            ));
+        }
+
+        // always zero
+        let is_always_zero = match op.value {
+            BinOp_::Mul => is_zero(lhs).or(is_zero(rhs)),
+            BinOp_::Div => is_zero(lhs),
+            BinOp_::Mod => is_zero(lhs).or(is_one(rhs)),
+            _ => None,
+        };
+        if let Some(zero_operand) = is_always_zero {
+            let msg = "This operation is always zero and can be replaced with '0'";
+            self.env.add_diag(diag!(
+                MEANINGLESS_MATH_OP_DIAG,
+                (exp.exp.loc, msg),
+                (zero_operand, "Because of this operand"),
+            ));
+        }
+
+        // always one
+        let is_always_one = match op.value {
+            BinOp_::Mod => is_one(lhs),
+            _ => None,
+        };
+        if let Some(one_operand) = is_always_one {
+            let msg = "This operation is always one and can be replaced with '1'";
+            self.env.add_diag(diag!(
+                MEANINGLESS_MATH_OP_DIAG,
+                (exp.exp.loc, msg),
+                (one_operand, "Because of this operand"),
+            ));
+        }
+
+        // for aborts, e.g. x / 0, we will let the optimizer give a warning
+
+        false
+    }
+}
+
+fn is_zero(exp: &H::Exp) -> Option<Loc> {
+    let H::UnannotatedExp_::Value(sp!(loc, value_)) = &exp.exp.value else {
+        return None;
+    };
+    match value_ {
+        Value_::U8(0) | Value_::U16(0) | Value_::U32(0) | Value_::U64(0) | Value_::U128(0) => {
+            Some(*loc)
+        }
+        Value_::U256(u) if u == &U256::zero() => Some(*loc),
+        Value_::U8(_)
+        | Value_::U16(_)
+        | Value_::U32(_)
+        | Value_::U64(_)
+        | Value_::U128(_)
+        | Value_::U256(_)
+        | Value_::Address(_)
+        | Value_::Bool(_)
+        | Value_::Vector(_, _) => None,
+    }
+}
+
+fn is_one(exp: &H::Exp) -> Option<Loc> {
+    let H::UnannotatedExp_::Value(sp!(loc, value_)) = &exp.exp.value else {
+        return None;
+    };
+    match value_ {
+        Value_::U8(1) | Value_::U16(1) | Value_::U32(1) | Value_::U64(1) | Value_::U128(1) => {
+            Some(*loc)
+        }
+        Value_::U256(u) if u == &U256::one() => Some(*loc),
+        Value_::U8(_)
+        | Value_::U16(_)
+        | Value_::U32(_)
+        | Value_::U64(_)
+        | Value_::U128(_)
+        | Value_::U256(_)
+        | Value_::Address(_)
+        | Value_::Bool(_)
+        | Value_::Vector(_, _) => None,
+    }
+}

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

@@ -4,10 +4,11 @@
 use move_symbol_pool::Symbol;
 
 use crate::{
-    command_line::compiler::Visitor, diagnostics::codes::WarningFilter,
-    linters::constant_naming::ConstantNamingVisitor, typing::visitor::TypingVisitor,
+    cfgir::visitor::CFGIRVisitor, command_line::compiler::Visitor,
+    diagnostics::codes::WarningFilter, typing::visitor::TypingVisitor,
 };
 pub mod constant_naming;
+pub mod meaningless_math_operation;
 mod unnecessary_while_loop;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -37,6 +38,8 @@ pub const CONSTANT_NAMING_FILTER_NAME: &str = "constant_naming";
 pub const CONSTANT_NAMING_DIAG_CODE: u8 = 1;
 pub const WHILE_TRUE_TO_LOOP_FILTER_NAME: &str = "while_true";
 pub const WHILE_TRUE_TO_LOOP_DIAG_CODE: u8 = 4;
+pub const MEANINGLESS_MATH_FILTER_NAME: &str = "unnecessary_math";
+pub const MEANINGLESS_MATH_DIAG_CODE: u8 = 8;
 
 pub fn known_filters() -> (Option<Symbol>, Vec<WarningFilter>) {
     (
@@ -54,6 +57,12 @@ pub fn known_filters() -> (Option<Symbol>, Vec<WarningFilter>) {
                 WHILE_TRUE_TO_LOOP_DIAG_CODE,
                 Some(WHILE_TRUE_TO_LOOP_FILTER_NAME),
             ),
+            WarningFilter::code(
+                Some(LINT_WARNING_PREFIX),
+                LinterDiagnosticCategory::Complexity as u8,
+                MEANINGLESS_MATH_DIAG_CODE,
+                Some(MEANINGLESS_MATH_FILTER_NAME),
+            ),
         ],
     )
 }
@@ -63,10 +72,9 @@ pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
         LintLevel::None | LintLevel::Default => vec![],
         LintLevel::All => {
             vec![
-                constant_naming::ConstantNamingVisitor::visitor(ConstantNamingVisitor),
-                unnecessary_while_loop::WhileTrueToLoop::visitor(
-                    unnecessary_while_loop::WhileTrueToLoop,
-                ),
+                constant_naming::ConstantNamingVisitor.visitor(),
+                unnecessary_while_loop::WhileTrueToLoop.visitor(),
+                meaningless_math_operation::MeaninglessMathOperation.visitor(),
             ]
         }
     }

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

@@ -0,0 +1,12 @@
+module 0x42::M {
+    public fun zero_shift_complex(x: u64, y: u64): u64 {
+        x * (y - y) // This is effectively * 0, but is not currently caught
+    }
+
+    public fun ast_fold() {
+        // we do not lint on these because they are folded to a single value
+        let x = 0;
+        x * 1;
+        1 * 0;
+    }
+}

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

@@ -0,0 +1,8 @@
+// tests suppression of unnecessary math operations
+module 0x42::M {
+
+    #[allow(lint(unnecessary_math))]
+    public fun add_zero(x: u64): u64 {
+        x + 0
+    }
+}

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
+    }
+}