[Linter] Checks for explicit self-assignments. (#17124)

## Description 
This code implements a linter check to detect self-assignments in Move
code.
The check focuses on two types of expressions:
   1. Mutate expressions (`*a = b`)
   2. Assign expressions (`a = b`)
 
In each case, the compiler tracks for the same "memory locations" of the
assignment, attempting to track the same local variable. It does not
have any sort of aliasing notion nor does it track references in any
meaningful way.


## Test plan
Added more use case

## Release notes

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [X] CLI: Move lint now warns against unnecessary self-assignments. 
- [ ] Rust SDK:

---------

Co-authored-by: jamedzung <[email protected]>
Co-authored-by: Todd Nowacki <[email protected]>

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

@@ -15,6 +15,7 @@ pub mod abort_constant;
 pub mod constant_naming;
 pub mod loop_without_exit;
 pub mod meaningless_math_operation;
+pub mod self_assignment;
 pub mod unnecessary_conditional;
 pub mod unnecessary_while_loop;
 pub mod unneeded_return;
@@ -138,7 +139,13 @@ lints!(
         LinterDiagnosticCategory::Complexity,
         "unnecessary_conditional",
         "'if' expression can be removed"
-    )
+    ),
+    (
+        SelfAssignment,
+        LinterDiagnosticCategory::Suspicious,
+        "self_assignment",
+        "assignment preserves the same value"
+    ),
 );
 
 pub const ALLOW_ATTR_CATEGORY: &str = "lint";
@@ -173,6 +180,7 @@ pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
                 abort_constant::AssertAbortNamedConstants.visitor(),
                 loop_without_exit::LoopWithoutExit.visitor(),
                 unnecessary_conditional::UnnecessaryConditional.visitor(),
+                self_assignment::SelfAssignmentVisitor.visitor(),
             ]
         }
     }

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

@@ -0,0 +1,211 @@
+// Copyright (c) The Move Contributors
+// SPDX-License-Identifier: Apache-2.0
+
+//! Detects and reports explicit self-assignments in code, such as `x = x;`, which are generally unnecessary
+//! and could indicate potential errors or misunderstandings in the code logic.
+use super::StyleCodes;
+use crate::{
+    diag,
+    diagnostics::WarningFilters,
+    naming::ast::Var,
+    shared::CompilationEnv,
+    typing::{
+        ast::{self as T},
+        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+    },
+};
+use move_ir_types::location::Loc;
+use move_proc_macros::growing_stack;
+
+pub struct SelfAssignmentVisitor;
+
+pub struct Context<'a> {
+    env: &'a mut CompilationEnv,
+}
+
+impl TypingVisitorConstructor for SelfAssignmentVisitor {
+    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 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, e: &T::Exp) -> bool {
+        use T::UnannotatedExp_ as E;
+        match &e.exp.value {
+            E::Mutate(lhs, rhs) => check_mutate(self, e.exp.loc, lhs, rhs),
+            E::Assign(lvalues, _, rhs) => check_assign(self, lvalues, rhs),
+            _ => (),
+        }
+        false
+    }
+}
+
+fn check_mutate(context: &mut Context, loc: Loc, lhs: &T::Exp, rhs: &T::Exp) {
+    #[growing_stack]
+    fn same_memory_location(lhs: &T::Exp, rhs: &T::Exp) -> Option<(Loc, Loc)> {
+        use T::UnannotatedExp_ as E;
+        let lhs = inner_exp(lhs);
+        let rhs = inner_exp(rhs);
+        match &lhs.exp.value {
+            E::Unit { .. }
+            | E::Value(_)
+            | E::Constant(_, _)
+            | E::ModuleCall(_)
+            | E::Vector(_, _, _, _)
+            | E::IfElse(_, _, _)
+            | E::Match(_, _)
+            | E::VariantMatch(_, _, _)
+            | E::While(_, _, _)
+            | E::Loop { .. }
+            | E::Assign(_, _, _)
+            | E::Mutate(_, _)
+            | E::Return(_)
+            | E::Abort(_)
+            | E::Continue(_)
+            | E::Give(_, _)
+            | E::Dereference(_)
+            | E::UnaryExp(_, _)
+            | E::BinopExp(_, _, _, _)
+            | E::Pack(_, _, _, _)
+            | E::PackVariant(_, _, _, _, _)
+            | E::ExpList(_)
+            | E::TempBorrow(_, _)
+            | E::Cast(_, _)
+            | E::ErrorConstant { .. }
+            | E::UnresolvedError => None,
+            E::Block(s) | E::NamedBlock(_, s) => {
+                debug_assert!(s.1.len() > 1);
+                None
+            }
+
+            E::Move { var: l, .. } | E::Copy { var: l, .. } | E::Use(l) | E::BorrowLocal(_, l) => {
+                same_local(l, rhs)
+            }
+            E::Builtin(b1, l) => {
+                if !gives_memory_location(b1) {
+                    return None;
+                }
+                match &rhs.exp.value {
+                    E::Builtin(b2, r) if b1 == b2 => same_memory_location(l, r),
+                    _ => None,
+                }
+            }
+            E::Borrow(_, l, lfield) => match &rhs.exp.value {
+                E::Borrow(_, r, rfield) if lfield == rfield => {
+                    same_memory_location(l, r)?;
+                    Some((lhs.exp.loc, rhs.exp.loc))
+                }
+                _ => None,
+            },
+
+            E::Annotate(_, _) => unreachable!(),
+        }
+    }
+
+    let rhs = inner_exp(rhs);
+    let rhs = match &rhs.exp.value {
+        T::UnannotatedExp_::Dereference(inner) => inner,
+        _ => rhs,
+    };
+    let Some((lhs_loc, rhs_loc)) = same_memory_location(lhs, rhs) else {
+        return;
+    };
+    report_self_assignment(context, "mutation", loc, lhs_loc, rhs_loc);
+}
+
+fn check_assign(context: &mut Context, sp!(_, lvalues_): &T::LValueList, rhs: &T::Exp) {
+    let vars = lvalues_.iter().map(lvalue_var).collect::<Vec<_>>();
+    let rhs_items = exp_list_items(rhs);
+    for (lhs_opt, rhs) in vars.into_iter().zip(rhs_items) {
+        let Some((loc, lhs)) = lhs_opt else {
+            continue;
+        };
+        if let Some((lhs_loc, rhs_loc)) = same_local(lhs, rhs) {
+            report_self_assignment(context, "assignment", loc, lhs_loc, rhs_loc);
+        }
+    }
+}
+
+fn same_local(lhs: &Var, rhs: &T::Exp) -> Option<(Loc, Loc)> {
+    use T::UnannotatedExp_ as E;
+    match &rhs.exp.value {
+        E::Copy { var: r, .. } | E::Move { var: r, .. } | E::BorrowLocal(_, r) => {
+            if lhs == r {
+                Some((lhs.loc, r.loc))
+            } else {
+                None
+            }
+        }
+        _ => None,
+    }
+}
+
+fn gives_memory_location(sp!(_, b_): &T::BuiltinFunction) -> bool {
+    match b_ {
+        T::BuiltinFunction_::Freeze(_) => true,
+        T::BuiltinFunction_::Assert(_) => false,
+    }
+}
+
+fn inner_exp(mut e: &T::Exp) -> &T::Exp {
+    use T::UnannotatedExp_ as E;
+    loop {
+        match &e.exp.value {
+            E::Annotate(inner, _) => e = inner,
+            E::Block((_, seq)) | E::NamedBlock(_, (_, seq)) if seq.len() == 1 => {
+                match &seq[0].value {
+                    T::SequenceItem_::Seq(inner) => e = inner,
+                    T::SequenceItem_::Declare(_) | T::SequenceItem_::Bind(_, _, _) => break e,
+                }
+            }
+            _ => break e,
+        }
+    }
+}
+
+fn lvalue_var(sp!(loc, lvalue_): &T::LValue) -> Option<(Loc, &Var)> {
+    use T::LValue_ as L;
+    match &lvalue_ {
+        L::Var { var, .. } => Some((*loc, var)),
+        L::Ignore
+        | L::Unpack(_, _, _, _)
+        | L::BorrowUnpack(_, _, _, _, _)
+        | L::UnpackVariant(_, _, _, _, _)
+        | L::BorrowUnpackVariant(_, _, _, _, _, _) => None,
+    }
+}
+
+fn exp_list_items(e: &T::Exp) -> Vec<&T::Exp> {
+    match &inner_exp(e).exp.value {
+        T::UnannotatedExp_::ExpList(items) => items
+            .iter()
+            .flat_map(|item| match item {
+                T::ExpListItem::Single(e, _) => vec![e],
+                T::ExpListItem::Splat(_, e, _) => exp_list_items(e),
+            })
+            .collect::<Vec<_>>(),
+        _ => vec![e],
+    }
+}
+
+fn report_self_assignment(context: &mut Context, case: &str, eloc: Loc, lloc: Loc, rloc: Loc) {
+    let msg =
+        format!("Unnecessary self-{case}. The {case} is redundant and will not change the value");
+    context.env.add_diag(diag!(
+        StyleCodes::SelfAssignment.diag_info(),
+        (eloc, msg),
+        (lloc, "This location"),
+        (rloc, "Is the same as this location"),
+    ));
+}

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

@@ -0,0 +1,26 @@
+// tests for cases that self-assignment could warn, but currently dont
+
+module a::m {
+    fun t(cond: bool, other: u64) {
+        let x = 0;
+        x = if (cond) x else x;
+        x;
+
+        x = if (cond) x else other;
+        x;
+
+        x = { 0; x };
+        x;
+
+        x = { let y = 0; y; x };
+        x;
+
+        // TODO move most lints to 2024
+        // x = match (cond) { true => x, false => x };
+        // x;
+
+        let x = &other;
+        other = *x;
+        other;
+    }
+}

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

@@ -0,0 +1,15 @@
+#[allow(lint(self_assignment))]
+module a::m {
+    fun foo(x: u64): u64 {
+        x = x;
+        x
+    }
+}
+
+module a::m2 {
+    #[allow(lint(self_assignment))]
+    fun foo(x: u64): u64 {
+        x = x;
+        x
+    }
+}

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

@@ -0,0 +1,63 @@
+// tests for cases that self-assignment should not warn
+
+module a::m {
+    const C: u64 = 112;
+
+    fun t() {
+        let x1 = 5;
+        x1;
+        x1 = 5; // we don't track values
+        x1;
+
+        let c1 = C;
+        c1;
+        c1 = C; // we don't track values
+        c1;
+
+        let x2 = 5;
+        x2;
+        let x2 = x2; // shadowing is not self-assignment
+        x2;
+
+        let (x3, x4) = (5, 5);
+        x3;
+        x4;
+        (x4, x3) = (x3, x4); // swap is not self-assignment
+        x3;
+        x4;
+
+        let r1 = &mut 0;
+        let r2 = &mut 0;
+        *r1;
+        *r2;
+        *r1 = *r2; // different references
+        *r1;
+
+        let r = &mut 0;
+        *id(r) = *id(r);
+
+        let x5 = 0;
+        x5;
+        x5 = { let x5 = 0; x5 }; // different x's
+        x5;
+    }
+
+
+    struct S has copy, drop { f1: u64, f2: u64 }
+    struct P has copy, drop { s1: S, s2: S }
+    fun fields(m1: &mut S, m2: &mut S, s1: S, s2: S) {
+        s1.f1 = s1.f2; // different fields
+        m1.f1 = m1.f2; // different fields
+        s1.f1 = s2.f1; // different locals
+        m1.f1 = m2.f1; // different references
+    }
+
+    fun nested_fields(p1: &mut P, p2: &mut P) {
+        p1.s1.f1 = p1.s1.f2; // different fields
+        p1.s1.f1 = p2.s1.f1; // different references
+    }
+
+    fun id<T>(x: &mut T): &mut T {
+        x
+    }
+}