refactor and add testcase

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

@@ -3,84 +3,44 @@
 
 //! Implements a lint to detect and warn about binary operations with equal operands in Move code.
 //! Targets comparison, logical, bitwise, subtraction, and division operations where such usage might indicate errors or redundancies.
+
+use super::StyleCodes;
+use crate::parser::ast::BinOp_;
 use crate::{
     diag,
-    diagnostics::{
-        codes::{custom, DiagnosticInfo, Severity},
-        WarningFilters,
-    },
-    parser::ast::BinOp_,
-    shared::CompilationEnv,
     typing::{
         ast::{self as T, UnannotatedExp_},
-        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+        visitor::simple_visitor,
     },
 };
-use move_ir_types::location::Loc;
-
-use super::{LinterDiagnosticCategory, EQUAL_OPERANDS_DIAG_CODE, LINT_WARNING_PREFIX};
-
-const EQUAL_OPERANDS_DIAG: DiagnosticInfo = custom(
-    LINT_WARNING_PREFIX,
-    Severity::Warning,
-    LinterDiagnosticCategory::Suspicious as u8,
-    EQUAL_OPERANDS_DIAG_CODE,
-    "Equal operands detected in binary operation, which might indicate a logical error or redundancy.",
-);
-
-pub struct EqualOperandsCheck;
-
-pub struct Context<'a> {
-    env: &'a mut CompilationEnv,
-}
-
-impl TypingVisitorConstructor for EqualOperandsCheck {
-    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 {
+simple_visitor!(
+    EqualOperandsCheck,
+    fn visit_exp_custom(&mut self, exp: &T::Exp) -> bool {
         if let UnannotatedExp_::BinopExp(lhs, sp!(_, op), _, rhs) = &exp.exp.value {
-            if lhs.exp.value == rhs.exp.value && is_relevant_op(op) {
-                report_equal_operands(self.env, exp.exp.loc);
+            if should_check_operands(lhs, rhs, op) {
+                let diag = diag!(
+                    StyleCodes::EqualOperands.diag_info(),
+                    (
+                        exp.exp.loc,
+                        "Equal operands detected in binary operation, which might indicate a logical error or redundancy."
+                    )
+                );
+                self.add_diag(diag);
             }
         }
         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 should_check_operands(lhs: &T::Exp, rhs: &T::Exp, op: &BinOp_) -> bool {
+    lhs.exp.value == rhs.exp.value && is_relevant_op(op)
 }
 
 fn is_relevant_op(op: &BinOp_) -> bool {
+    use BinOp_::*;
     matches!(
         op,
-        BinOp_::Eq
-            | BinOp_::Neq
-            | BinOp_::Gt
-            | BinOp_::Ge
-            | BinOp_::Lt
-            | BinOp_::Le
-            | BinOp_::And
-            | BinOp_::Or
-            | BinOp_::BitAnd
-            | BinOp_::BitOr
-            | BinOp_::Xor
-            | BinOp_::Sub
-            | BinOp_::Div
+        Eq | Neq | Gt | Ge | Lt | Le | And | Or | BitAnd | BitOr | Xor | Sub | Div
     )
 }
-
-fn report_equal_operands(env: &mut CompilationEnv, loc: Loc) {
-    let msg = "Equal operands detected in binary operation, which might indicate a logical error or redundancy.";
-    let diag = diag!(EQUAL_OPERANDS_DIAG, (loc, msg));
-    env.add_diag(diag);
-}

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

@@ -15,6 +15,7 @@ use crate::{
 
 pub mod abort_constant;
 pub mod constant_naming;
+pub mod eq_op;
 pub mod loop_without_exit;
 pub mod meaningless_math_operation;
 pub mod redundant_ref_deref;
@@ -23,7 +24,6 @@ pub mod unnecessary_conditional;
 pub mod unnecessary_unit;
 pub mod unnecessary_while_loop;
 pub mod unneeded_return;
-
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum LintLevel {
     // No linters
@@ -162,6 +162,12 @@ lints!(
         "unnecessary_unit",
         "unit `()` expression can be removed or simplified"
     ),
+    (
+        EqualOperands,
+        LinterDiagnosticCategory::Suspicious,
+        "equal_operands",
+        "Equal operands detected in binary operation, which might indicate a logical error or redundancy."
+    ),
 );
 
 pub const ALLOW_ATTR_CATEGORY: &str = "lint";
@@ -199,6 +205,7 @@ pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
                 self_assignment::SelfAssignmentVisitor.visitor(),
                 redundant_ref_deref::RedundantRefDerefVisitor.visitor(),
                 unnecessary_unit::UnnecessaryUnit.visitor(),
+                eq_op::EqualOperandsCheck.visitor(),
             ]
         }
     }

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

@@ -0,0 +1,38 @@
+warning[Lint W01003]: math operator can be simplified
+  ┌─ tests/linter/false_negative_equal_operand.move:5:18
+  │
+5 │         let _ = (x + 0) == x;  // Semantically same operands but syntactically different
+  │                  ^^^^^
+  │                  │   │
+  │                  │   Because of this operand
+  │                  This operation has no effect and can be removed
+  │
+  = This warning can be suppressed with '#[allow(lint(unnecessary_math))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01003]: math operator can be simplified
+  ┌─ tests/linter/false_negative_equal_operand.move:6:18
+  │
+6 │         let _ = (x * 1) == x;  // Semantically same operands but syntactically different
+  │                  ^^^^^
+  │                  │   │
+  │                  │   Because of this operand
+  │                  This operation has no effect and can be removed
+  │
+  = This warning can be suppressed with '#[allow(lint(unnecessary_math))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W02011]: Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+  ┌─ tests/linter/false_negative_equal_operand.move:9:17
+  │
+9 │         let _ = get_constant() == get_constant();  // Same value but lint won't catch it
+  │                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+  │
+  = This warning can be suppressed with '#[allow(lint(equal_operands))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W02011]: Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+   ┌─ tests/linter/false_negative_equal_operand.move:19:17
+   │
+19 │         let _ = s == s;  // Should trigger lint
+   │                 ^^^^^^ Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+   │
+   = This warning can be suppressed with '#[allow(lint(equal_operands))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

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

@@ -0,0 +1,24 @@
+module 0x42::false_negative_equal_operand {
+   fun test_false_negatives() {
+        // Complex expressions that are effectively the same but syntactically different
+        let x = 10;
+        let _ = (x + 0) == x;  // Semantically same operands but syntactically different
+        let _ = (x * 1) == x;  // Semantically same operands but syntactically different
+
+        // Function calls that return the same value
+        let _ = get_constant() == get_constant();  // Same value but lint won't catch it
+    }
+
+    fun get_constant(): u64 { 42 }
+
+    // Additional test for struct equality
+    struct TestStruct has copy, drop { value: u64 }
+
+    fun test_struct_operations() {
+        let s = TestStruct { value: 10 };
+        let _ = s == s;  // Should trigger lint
+
+        let s2 = TestStruct { value: 10 };
+        let _ = s == s2; // Should not trigger lint (different instances)
+    }
+}

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

@@ -0,0 +1,8 @@
+warning[Lint W02011]: Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+  ┌─ tests/linter/false_negative_meaningless_math_operation.move:3:14
+  │
+3 │         x * (y - y) // This is effectively * 0, but is not currently caught
+  │              ^^^^^ Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+  │
+  = This warning can be suppressed with '#[allow(lint(equal_operands))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

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

@@ -0,0 +1,8 @@
+warning[Lint W02011]: Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+  ┌─ tests/linter/false_negative_unnecessary_while_loop.move:7:16
+  │
+7 │         while (true && true) {};
+  │                ^^^^^^^^^^^^ Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+  │
+  = This warning can be suppressed with '#[allow(lint(equal_operands))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

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

@@ -0,0 +1,78 @@
+warning[W09002]: unused variable
+  ┌─ tests/linter/false_positive_equal_operand.move:4:13
+  │
+4 │         let mut_ref = &mut 0;
+  │             ^^^^^^^ Unused local variable 'mut_ref'. Consider removing or prefixing with an underscore: '_mut_ref'
+  │
+  = This warning can be suppressed with '#[allow(unused_variable)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+error[E01002]: unexpected token
+  ┌─ tests/linter/false_positive_equal_operand.move:5:15
+  │
+5 │         while *mut_ref != *mut_ref {  // Legitimate use: checking for changes in mutable reference
+  │               ^
+  │               │
+  │               Unexpected '*'
+  │               Expected '('
+
+warning[W09002]: unused variable
+   ┌─ tests/linter/false_positive_equal_operand.move:10:13
+   │
+10 │         let nan_check = is_nan(1.0);  // Simulated floating point check
+   │             ^^^^^^^^^ Unused local variable 'nan_check'. Consider removing or prefixing with an underscore: '_nan_check'
+   │
+   = This warning can be suppressed with '#[allow(unused_variable)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+error[E01002]: unexpected token
+   ┌─ tests/linter/false_positive_equal_operand.move:10:34
+   │
+10 │         let nan_check = is_nan(1.0);  // Simulated floating point check
+   │                                  ^
+   │                                  │
+   │                                  Unexpected '0'
+   │                                  Expected an identifier or a decimal number
+
+error[E13001]: feature is not supported in specified edition
+   ┌─ tests/linter/false_positive_equal_operand.move:10:34
+   │
+10 │         let nan_check = is_nan(1.0);  // Simulated floating point check
+   │                                  ^ Positional fields are not supported by current edition 'legacy', only '2024.alpha' and '2024.beta' support this feature
+   │
+   = You can update the edition in the 'Move.toml', or via command line flag if invoking the compiler directly.
+
+warning[Lint W02011]: Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+   ┌─ tests/linter/false_positive_equal_operand.move:19:17
+   │
+19 │         assert!(x <= x && x <= y, 0); // Legitimate use in monotonicity checks
+   │                 ^^^^^^ Equal operands detected in binary operation, which might indicate a logical error or redundancy.
+   │
+   = This warning can be suppressed with '#[allow(lint(equal_operands))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+error[E04007]: incompatible types
+   ┌─ tests/linter/false_positive_equal_operand.move:33:9
+   │
+31 │     fun reference_equals<T>(a: &T, b: &T): bool {
+   │                                -- Given: '&T'
+32 │         // Simulated reference equality check
+33 │         std::hash::sha2_256(a) == std::hash::sha2_256(b)
+   │         ^^^^^^^^^^^^^^^^^^^^^^ Invalid call of 'std::hash::sha2_256'. Invalid argument for parameter 'data'
+   │
+   ┌─ /Users/dmr/Projects/rust/sui-network/external-crates/move/crates/move-stdlib/sources/hash.move:9:38
+   │
+ 9 │     native public fun sha2_256(data: vector<u8>): vector<u8>;
+   │                                      ---------- Expected: 'vector<u8>'
+
+error[E04007]: incompatible types
+   ┌─ tests/linter/false_positive_equal_operand.move:33:35
+   │
+31 │     fun reference_equals<T>(a: &T, b: &T): bool {
+   │                                       -- Given: '&T'
+32 │         // Simulated reference equality check
+33 │         std::hash::sha2_256(a) == std::hash::sha2_256(b)
+   │                                   ^^^^^^^^^^^^^^^^^^^^^^ Invalid call of 'std::hash::sha2_256'. Invalid argument for parameter 'data'
+   │
+   ┌─ /Users/dmr/Projects/rust/sui-network/external-crates/move/crates/move-stdlib/sources/hash.move:9:38
+   │
+ 9 │     native public fun sha2_256(data: vector<u8>): vector<u8>;
+   │                                      ---------- Expected: 'vector<u8>'
+

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

@@ -0,0 +1,35 @@
+module 0x42::false_positive_equal_operand {
+    fun test_false_positives() {
+        // Iterator-like patterns where comparing same variable is intentional
+        let mut_ref = &mut 0;
+        while *mut_ref != *mut_ref {  // Legitimate use: checking for changes in mutable reference
+            break
+        };
+
+        // Checking for NaN (Not a Number) in floating point implementations
+        let nan_check = is_nan(1.0);  // Simulated floating point check
+
+        // Checking for pointer/reference equality (if Move had raw pointers)
+        let obj = object();
+        let _ = reference_equals(obj, obj);  // Legitimate use: checking if references point to same object
+
+        // Checking for monotonicity
+        let x = 10;
+        let y = 20;
+        assert!(x <= x && x <= y, 0); // Legitimate use in monotonicity checks
+    }
+
+    // Helper functions for false positive cases
+    fun is_nan(_x: u64): bool { 
+        false  // Simulated NaN check
+    }
+
+    fun object(): &vector<u8> {
+        &vector[1, 2, 3]
+    }
+
+    fun reference_equals<T>(a: &T, b: &T): bool {
+        // Simulated reference equality check
+        std::hash::sha2_256(a) == std::hash::sha2_256(b)
+    }
+}