[Linter] Checks for equal operands to comparison, logical and bitwise…

…, difference and division binary operators (==, >, etc., &&, ||, &, |, ^, - and /). (#17115) ## Description Implements a new lint rule that detects binary operations with identical operands, which often indicate programming errors or redundant code. This lint helps maintain code quality by catching potentially problematic patterns early in development. ## Release notes - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [X] CLI: Move will now lint against certain binary operations where the operands are identical, non-value expressions. - [ ] Rust SDK: --------- Co-authored-by: jamedzung <[email protected]> Co-authored-by: Todd Nowacki <[email protected]>
MystenLabs · Dec 4, 2024 · e1a12ab · e1a12ab
1 parent 9d6cf6a
commit e1a12ab
Show file tree

Hide file tree

Showing 34 changed files with 544 additions and 94 deletions.
diff --git a/external-crates/move/crates/move-compiler/src/cfgir/optimize/constant_fold.rs b/external-crates/move/crates/move-compiler/src/cfgir/optimize/constant_fold.rs
@@ -4,6 +4,7 @@
 
 use crate::{
     cfgir::cfg::MutForwardCFG,
+    diagnostics::DiagnosticReporter,
     expansion::ast::Mutability,
     hlir::ast::{
         BaseType, BaseType_, Command, Command_, Exp, FunctionSignature, SingleType, TypeName,
@@ -19,17 +20,22 @@ use std::convert::TryFrom;
 
 /// returns true if anything changed
 pub fn optimize(
+    reporter: &DiagnosticReporter,
     _signature: &FunctionSignature,
     _locals: &UniqueMap<Var, (Mutability, SingleType)>,
     constants: &UniqueMap<ConstantName, Value>,
     cfg: &mut MutForwardCFG,
 ) -> bool {
+    let context = Context {
+        reporter,
+        constants,
+    };
     let mut changed = false;
     for block_ref in cfg.blocks_mut().values_mut() {
         let block = std::mem::take(block_ref);
         *block_ref = block
             .into_iter()
-            .filter_map(|mut cmd| match optimize_cmd(constants, &mut cmd) {
+            .filter_map(|mut cmd| match optimize_cmd(&context, &mut cmd) {
                 None => {
                     changed = true;
                     None
@@ -44,31 +50,34 @@ pub fn optimize(
     changed
 }
 
+struct Context<'a> {
+    #[allow(dead_code)]
+    reporter: &'a DiagnosticReporter<'a>,
+    constants: &'a UniqueMap<ConstantName, Value>,
+}
+
 //**************************************************************************************************
 // Scaffolding
 //**************************************************************************************************
 
 // Some(changed) to keep
 // None to remove the cmd
 #[growing_stack]
-fn optimize_cmd(
-    consts: &UniqueMap<ConstantName, Value>,
-    sp!(_, cmd_): &mut Command,
-) -> Option<bool> {
+fn optimize_cmd(context: &Context, sp!(_, cmd_): &mut Command) -> Option<bool> {
     use Command_ as C;
     Some(match cmd_ {
-        C::Assign(_, _ls, e) => optimize_exp(consts, e),
+        C::Assign(_, _ls, e) => optimize_exp(context, e),
         C::Mutate(el, er) => {
-            let c1 = optimize_exp(consts, er);
-            let c2 = optimize_exp(consts, el);
+            let c1 = optimize_exp(context, er);
+            let c2 = optimize_exp(context, el);
             c1 || c2
         }
         C::Return { exp: e, .. }
         | C::Abort(_, e)
         | C::JumpIf { cond: e, .. }
-        | C::VariantSwitch { subject: e, .. } => optimize_exp(consts, e),
+        | C::VariantSwitch { subject: e, .. } => optimize_exp(context, e),
         C::IgnoreAndPop { exp: e, .. } => {
-            let c = optimize_exp(consts, e);
+            let c = optimize_exp(context, e);
             if ignorable_exp(e) {
                 // value(s), so the command can be removed
                 return None;
@@ -83,9 +92,9 @@ fn optimize_cmd(
 }
 
 #[growing_stack]
-fn optimize_exp(consts: &UniqueMap<ConstantName, Value>, e: &mut Exp) -> bool {
+fn optimize_exp(context: &Context, e: &mut Exp) -> bool {
     use UnannotatedExp_ as E;
-    let optimize_exp = |e| optimize_exp(consts, e);
+    let optimize_exp = |e| optimize_exp(context, e);
     match &mut e.exp.value {
         //************************************
         // Pass through cases
@@ -103,7 +112,7 @@ fn optimize_exp(consts: &UniqueMap<ConstantName, Value>, e: &mut Exp) -> bool {
             let E::Constant(name) = e_ else {
                 unreachable!()
             };
-            if let Some(value) = consts.get(name) {
+            if let Some(value) = context.constants.get(name) {
                 *e_ = E::Value(value.clone());
                 true
             } else {
@@ -152,7 +161,10 @@ fn optimize_exp(consts: &UniqueMap<ConstantName, Value>, e: &mut Exp) -> bool {
             let changed1 = optimize_exp(e1);
             let changed2 = optimize_exp(e2);
             let changed = changed1 || changed2;
-            if let (Some(v1), Some(v2)) = (foldable_exp(e1), foldable_exp(e2)) {
+            let v1_opt = foldable_exp(e1);
+            let v2_opt = foldable_exp(e2);
+            // TODO warn on operations that always fail
+            if let (Some(v1), Some(v2)) = (v1_opt, v2_opt) {
                 if let Some(folded) = fold_binary_op(e.exp.loc, op, v1, v2) {
                     *e_ = folded;
                     true
@@ -170,6 +182,7 @@ fn optimize_exp(consts: &UniqueMap<ConstantName, Value>, e: &mut Exp) -> bool {
                 _ => unreachable!(),
             };
             let changed = optimize_exp(e);
+            // TODO warn on operations that always fail
             let v = match foldable_exp(e) {
                 Some(v) => v,
                 None => return changed,

diff --git a/external-crates/move/crates/move-compiler/src/cfgir/optimize/eliminate_locals.rs b/external-crates/move/crates/move-compiler/src/cfgir/optimize/eliminate_locals.rs
@@ -4,6 +4,7 @@
 
 use crate::{
     cfgir::{cfg::MutForwardCFG, remove_no_ops},
+    diagnostics::DiagnosticReporter,
     expansion::ast::Mutability,
     hlir::ast::{FunctionSignature, SingleType, Value, Var},
     parser,
@@ -13,6 +14,7 @@ use std::collections::BTreeSet;
 
 /// returns true if anything changed
 pub fn optimize(
+    _reporter: &DiagnosticReporter,
     signature: &FunctionSignature,
     _locals: &UniqueMap<Var, (Mutability, SingleType)>,
     _constants: &UniqueMap<parser::ast::ConstantName, Value>,

diff --git a/external-crates/move/crates/move-compiler/src/cfgir/optimize/forwarding_jumps.rs b/external-crates/move/crates/move-compiler/src/cfgir/optimize/forwarding_jumps.rs
@@ -33,6 +33,7 @@ use crate::{
         ast::remap_labels,
         cfg::{MutForwardCFG, CFG},
     },
+    diagnostics::DiagnosticReporter,
     expansion::ast::Mutability,
     hlir::ast::{BasicBlocks, Command, Command_, FunctionSignature, Label, SingleType, Value, Var},
     parser::ast::ConstantName,
@@ -43,6 +44,7 @@ use std::collections::{BTreeMap, BTreeSet};
 
 /// returns true if anything changed
 pub fn optimize(
+    _reporter: &DiagnosticReporter,
     _signature: &FunctionSignature,
     _locals: &UniqueMap<Var, (Mutability, SingleType)>,
     _constants: &UniqueMap<ConstantName, Value>,

diff --git a/external-crates/move/crates/move-compiler/src/cfgir/optimize/inline_blocks.rs b/external-crates/move/crates/move-compiler/src/cfgir/optimize/inline_blocks.rs
@@ -9,6 +9,7 @@ use crate::{
         ast::remap_labels,
         cfg::{MutForwardCFG, CFG},
     },
+    diagnostics::DiagnosticReporter,
     expansion::ast::Mutability,
     hlir::ast::{BasicBlocks, Command_, FunctionSignature, Label, SingleType, Value, Var},
     parser::ast::ConstantName,
@@ -18,6 +19,7 @@ use std::collections::{BTreeMap, BTreeSet};
 
 /// returns true if anything changed
 pub fn optimize(
+    _reporter: &DiagnosticReporter,
     _signature: &FunctionSignature,
     _locals: &UniqueMap<Var, (Mutability, SingleType)>,
     _constants: &UniqueMap<ConstantName, Value>,

diff --git a/external-crates/move/crates/move-compiler/src/cfgir/optimize/mod.rs b/external-crates/move/crates/move-compiler/src/cfgir/optimize/mod.rs
@@ -12,6 +12,7 @@ use move_symbol_pool::Symbol;
 
 use crate::{
     cfgir::cfg::MutForwardCFG,
+    diagnostics::DiagnosticReporter,
     editions::FeatureGate,
     expansion::ast::Mutability,
     hlir::ast::*,
@@ -20,6 +21,7 @@ use crate::{
 };
 
 pub type Optimization = fn(
+    &DiagnosticReporter,
     &FunctionSignature,
     &UniqueMap<Var, (Mutability, SingleType)>,
     &UniqueMap<ConstantName, Value>,
@@ -44,6 +46,7 @@ const MOVE_2024_OPTIMIZATIONS: &[Optimization] = &[
 #[growing_stack]
 pub fn optimize(
     env: &CompilationEnv,
+    reporter: &DiagnosticReporter,
     package: Option<Symbol>,
     signature: &FunctionSignature,
     locals: &UniqueMap<Var, (Mutability, SingleType)>,
@@ -66,7 +69,7 @@ pub fn optimize(
         }
 
         // reset the count if something has changed
-        if optimization(signature, locals, constants, cfg) {
+        if optimization(reporter, signature, locals, constants, cfg) {
             count = 0
         } else {
             count += 1

diff --git a/external-crates/move/crates/move-compiler/src/cfgir/optimize/simplify_jumps.rs b/external-crates/move/crates/move-compiler/src/cfgir/optimize/simplify_jumps.rs
@@ -6,6 +6,7 @@ use move_proc_macros::growing_stack;
 
 use crate::{
     cfgir::cfg::MutForwardCFG,
+    diagnostics::DiagnosticReporter,
     expansion::ast::Mutability,
     hlir::ast::{
         Command, Command_, Exp, FunctionSignature, SingleType, UnannotatedExp_, Value, Value_, Var,
@@ -16,6 +17,7 @@ use crate::{
 
 /// returns true if anything changed
 pub fn optimize(
+    _reporter: &DiagnosticReporter,
     _signature: &FunctionSignature,
     _locals: &UniqueMap<Var, (Mutability, SingleType)>,
     _constants: &UniqueMap<ConstantName, Value>,

diff --git a/external-crates/move/crates/move-compiler/src/cfgir/translate.rs b/external-crates/move/crates/move-compiler/src/cfgir/translate.rs
@@ -249,7 +249,7 @@ fn constants(
                 .collect::<Vec<_>>()
                 .join(", ");
             let mut diag = diag!(
-                BytecodeGeneration::UnfoldableConstant,
+                CodeGeneration::UnfoldableConstant,
                 (
                     *consts.get_loc(&scc[0]).unwrap(),
                     format!("Constant definitions form a circular dependency: {}", names),
@@ -275,7 +275,7 @@ fn constants(
             .collect();
         for node in neighbors {
             context.add_diag(diag!(
-                BytecodeGeneration::UnfoldableConstant,
+                CodeGeneration::UnfoldableConstant,
                 (
                     *consts.get_loc(&node).unwrap(),
                     format!(
@@ -517,6 +517,7 @@ fn constant_(
     );
     cfgir::optimize(
         context.env,
+        &context.reporter,
         context.current_package,
         &fake_signature,
         &locals,
@@ -526,7 +527,7 @@ fn constant_(
 
     if blocks.len() != 1 {
         context.add_diag(diag!(
-            BytecodeGeneration::UnfoldableConstant,
+            CodeGeneration::UnfoldableConstant,
             (full_loc, CANNOT_FOLD)
         ));
         return None;
@@ -538,7 +539,7 @@ fn constant_(
             C::IgnoreAndPop { exp, .. } => exp,
             _ => {
                 context.add_diag(diag!(
-                    BytecodeGeneration::UnfoldableConstant,
+                    CodeGeneration::UnfoldableConstant,
                     (*cloc, CANNOT_FOLD)
                 ));
                 continue;
@@ -560,7 +561,7 @@ fn check_constant_value(context: &mut Context, e: &H::Exp) {
     match &e.exp.value {
         E::Value(_) => (),
         _ => context.add_diag(diag!(
-            BytecodeGeneration::UnfoldableConstant,
+            CodeGeneration::UnfoldableConstant,
             (e.exp.loc, CANNOT_FOLD)
         )),
     }
@@ -677,6 +678,7 @@ fn function_body(
             if !context.env.has_errors() {
                 cfgir::optimize(
                     context.env,
+                    &context.reporter,
                     context.current_package,
                     signature,
                     &locals,

diff --git a/external-crates/move/crates/move-compiler/src/cfgir/visitor.rs b/external-crates/move/crates/move-compiler/src/cfgir/visitor.rs
@@ -14,7 +14,7 @@ use crate::{
     diagnostics::{warning_filters::WarningFilters, Diagnostic, Diagnostics},
     expansion::ast::ModuleIdent,
     hlir::ast::{self as H, Command, Exp, LValue, LValue_, Label, ModuleCall, Type, Type_, Var},
-    parser::ast::{ConstantName, DatatypeName, FunctionName},
+    parser::ast::{ConstantName, DatatypeName, Field, FunctionName},
     shared::CompilationEnv,
 };
 use move_core_types::account_address::AccountAddress;
@@ -938,3 +938,76 @@ fn exp_satisfies_(e: &Exp, p: &mut impl FnMut(&Exp) -> bool) -> bool {
         }
     }
 }
+
+/// Assumes equal types and as such will not check type arguments for equality.
+/// Assumes function calls, assignments, and similar expressions are effectful and thus not equal.
+pub fn same_value_exp(e1: &H::Exp, e2: &H::Exp) -> bool {
+    same_value_exp_(&e1.exp.value, &e2.exp.value)
+}
+
+#[growing_stack]
+pub fn same_value_exp_(e1: &H::UnannotatedExp_, e2: &H::UnannotatedExp_) -> bool {
+    use H::UnannotatedExp_ as E;
+    match (e1, e2) {
+        (E::Value(v1), E::Value(v2)) => v1 == v2,
+        (E::Unit { .. }, E::Unit { .. }) => true,
+        (E::Constant(c1), E::Constant(c2)) => c1 == c2,
+        (E::Move { var, .. } | E::Copy { var, .. } | E::BorrowLocal(_, var), other)
+        | (other, E::Move { var, .. } | E::Copy { var, .. } | E::BorrowLocal(_, var)) => {
+            same_local_(var, other)
+        }
+
+        (E::Vector(_, _, _, e1), E::Vector(_, _, _, e2)) => {
+            e1.len() == e2.len()
+                && e1
+                    .iter()
+                    .zip(e2.iter())
+                    .all(|(e1, e2)| same_value_exp(e1, e2))
+        }
+
+        (E::Dereference(e) | E::Freeze(e), other) | (other, E::Dereference(e) | E::Freeze(e)) => {
+            same_value_exp_(&e.exp.value, other)
+        }
+        (E::UnaryExp(op1, e1), E::UnaryExp(op2, e2)) => op1 == op2 && same_value_exp(e1, e2),
+        (E::BinopExp(l1, op1, r1), E::BinopExp(l2, op2, r2)) => {
+            op1 == op2 && same_value_exp(l1, l2) && same_value_exp(r1, r2)
+        }
+
+        (E::Pack(n1, _, fields1), E::Pack(n2, _, fields2)) => {
+            n1 == n2 && same_value_fields(fields1, fields2)
+        }
+        (E::PackVariant(n1, v1, _, fields1), E::PackVariant(n2, v2, _, fields2)) => {
+            n1 == n2 && v1 == v2 && same_value_fields(fields1, fields2)
+        }
+
+        (E::Borrow(_, e1, f1, _), E::Borrow(_, e2, f2, _)) => f1 == f2 && same_value_exp(e1, e2),
+
+        // false for anything effectful
+        (E::ModuleCall(_), _) => false,
+
+        // TODO there is some potential for equality here, but a bit too brittle now
+        (E::Cast(_, _), _) | (E::ErrorConstant { .. }, _) => false,
+
+        // catch all
+        _ => false,
+    }
+}
+
+fn same_value_fields(
+    fields1: &[(Field, H::BaseType, Exp)],
+    fields2: &[(Field, H::BaseType, Exp)],
+) -> bool {
+    fields1.len() == fields2.len()
+        && fields1
+            .iter()
+            .zip(fields2.iter())
+            .all(|((_, _, e1), (_, _, e2))| same_value_exp(e1, e2))
+}
+
+fn same_local_(lhs: &Var, rhs: &H::UnannotatedExp_) -> bool {
+    use H::UnannotatedExp_ as E;
+    match &rhs {
+        E::Copy { var: r, .. } | E::Move { var: r, .. } | E::BorrowLocal(_, r) => lhs == r,
+        _ => false,
+    }
+}
diff --git a/external-crates/move/crates/move-compiler/src/diagnostics/codes.rs b/external-crates/move/crates/move-compiler/src/diagnostics/codes.rs
@@ -290,7 +290,7 @@ codes!(
         InvalidTransfer: { msg: "invalid transfer of references", severity: NonblockingError },
         AmbiguousVariableUsage: { msg: "ambiguous usage of variable", severity: NonblockingError },
     ],
-    BytecodeGeneration: [
+    CodeGeneration: [
         UnfoldableConstant: { msg: "cannot compute constant value", severity: NonblockingError },
     ],
     // errors for any unused code or items

diff --git a/external-crates/move/crates/move-compiler/src/diagnostics/warning_filters.rs b/external-crates/move/crates/move-compiler/src/diagnostics/warning_filters.rs
@@ -1,3 +1,6 @@
+// Copyright (c) The Move Contributors
+// SPDX-License-Identifier: Apache-2.0
+
 use crate::{
     diagnostics::{
         codes::{Category, DiagnosticInfo, ExternalPrefix, Severity, UnusedItem},