[Linter] Redundant ref deref (#16491)

## Description This lint rule detects and reports unnecessary temporary borrow operations followed by a dereference and a local borrow in Move code. It aims to improve code efficiency by suggesting direct usage of expressions without redundant operations. Implementation The lint rule is implemented as follows: A temporary borrow (TempBorrow) Followed by a dereference (Dereference) Where the dereferenced expression is either a BorrowLocal, Borrow, or another TempBorrow ## Test plan Added more use case including false positive, false negative case ## Release notes - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: --------- Co-authored-by: jamedzung <[email protected]> Co-authored-by: Lu Zhang <[email protected]> Co-authored-by: Cameron Swords <[email protected]>
MystenLabs · Oct 15, 2024 · aa2ca1d · aa2ca1d
1 parent 6b23159
commit aa2ca1d
Show file tree

Hide file tree

Showing 22 changed files with 1,297 additions and 0 deletions.
diff --git a/external-crates/move/crates/move-compiler/src/linters/mod.rs b/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 redundant_ref_deref;
 pub mod self_assignment;
 pub mod unnecessary_conditional;
 pub mod unnecessary_while_loop;
@@ -146,6 +147,12 @@ lints!(
         "self_assignment",
         "assignment preserves the same value"
     ),
+    (
+        RedundantRefDeref,
+        LinterDiagnosticCategory::Complexity,
+        "redundant_ref_deref",
+        "redundant reference/dereference"
+    )
 );
 
 pub const ALLOW_ATTR_CATEGORY: &str = "lint";
@@ -181,6 +188,7 @@ pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
                 loop_without_exit::LoopWithoutExit.visitor(),
                 unnecessary_conditional::UnnecessaryConditional.visitor(),
                 self_assignment::SelfAssignmentVisitor.visitor(),
+                redundant_ref_deref::RedundantRefDerefVisitor.visitor(),
             ]
         }
     }

diff --git a/external-crates/move/crates/move-compiler/src/linters/redundant_ref_deref.rs b/external-crates/move/crates/move-compiler/src/linters/redundant_ref_deref.rs
@@ -0,0 +1,155 @@
+// Copyright (c) The Move Contributors
+// SPDX-License-Identifier: Apache-2.0
+
+// Implements lint rule for Move code to detect redundant ref/deref patterns.
+// It identifies and reports unnecessary temporary borrow followed by a deref, suggesting either
+// removal or conversion to `copy`.
+
+use crate::linters::StyleCodes;
+use crate::{
+    diag,
+    diagnostics::WarningFilters,
+    shared::CompilationEnv,
+    typing::{
+        ast::{self as T, Exp, UnannotatedExp_ as TE},
+        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+    },
+};
+
+pub struct RedundantRefDerefVisitor;
+
+pub struct Context<'a> {
+    env: &'a mut CompilationEnv,
+}
+
+impl TypingVisitorConstructor for RedundantRefDerefVisitor {
+    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, exp: &Exp) -> bool {
+        self.check_redundant_ref_deref(exp);
+        false
+    }
+}
+
+impl Context<'_> {
+    // Check for &* pattern
+    fn check_redundant_ref_deref(&mut self, exp: &Exp) {
+        let TE::Dereference(deref_exp) = &exp.exp.value else {
+            return;
+        };
+        // This is a carve-out to handle cases like `&(s.value), which generate a field borrow and
+        // dereference to perform a  `copy`. In those cases, the location information is reused for
+        // both, meaning it was generated by typing, and thus not subject to warnings.
+        // TODO(cswords): In the future, we should mark which of these are generated during typing
+        // to handle paths.
+        if exp.exp.loc == deref_exp.exp.loc {
+            return;
+        }
+        match &deref_exp.exp.value {
+            TE::TempBorrow(_, inner) if is_simple_deref_ref_exp(inner) => self.env.add_diag(diag!(
+                StyleCodes::RedundantRefDeref.diag_info(),
+                (
+                    exp.exp.loc,
+                    "Redundant borrow-dereference detected. \
+                     Remove this borrow-deref and use the expression directly."
+                )
+            )),
+            TE::TempBorrow(_, inner) if all_deref_borrow(inner) => self.env.add_diag(diag!(
+                StyleCodes::RedundantRefDeref.diag_info(),
+                (
+                    exp.exp.loc,
+                    "Redundant borrow-dereference detected. \
+                     Use the inner expression directly."
+                )
+            )),
+            TE::Borrow(false, _, _) if exp.exp.loc != deref_exp.exp.loc => {
+                self.env.add_diag(diag!(
+                    StyleCodes::RedundantRefDeref.diag_info(),
+                    (
+                        exp.exp.loc,
+                        "Redundant borrow-dereference detected. \
+                         Use the field access directly."
+                    )
+                ))
+            }
+            TE::Borrow(_, _, _) | TE::BorrowLocal(_, _) => self.env.add_diag(diag!(
+                StyleCodes::RedundantRefDeref.diag_info(),
+                (
+                    exp.exp.loc,
+                    "Redundant borrow-dereference detected. \
+                     Replace this borrow-deref with 'copy'."
+                )
+            )),
+            _ => (),
+        }
+    }
+}
+
+/// Indicates if the expression is of the form `[&*]+....e`
+fn all_deref_borrow(exp: &Exp) -> bool {
+    let TE::Dereference(deref_exp) = &exp.exp.value else {
+        return false;
+    };
+    match &deref_exp.exp.value {
+        TE::TempBorrow(_, inner) => all_deref_borrow(inner),
+        TE::Borrow(_, _, _) | TE::BorrowLocal(_, _) => true,
+        _ => false,
+    }
+}
+
+/// Indicates if the expression at hand is of a form where `&*&` can always be reduces to `&`, such
+/// as function calls and constants.
+fn is_simple_deref_ref_exp(exp: &Exp) -> bool {
+    match &exp.exp.value {
+        TE::Value(_) => true,
+        TE::Constant(_, _) => true,
+        TE::ErrorConstant { .. } => true,
+        TE::ModuleCall(_) => true,
+        TE::Vector(_, _, _, _) => true,
+        TE::Copy { .. } => true,
+
+        TE::Cast(inner, _) | TE::Annotate(inner, _) => is_simple_deref_ref_exp(inner),
+
+        TE::Move { .. } => false, // Copy case
+        TE::Use(_) => todo!(),
+        TE::IfElse(_, _, _)
+        | TE::Match(_, _)
+        | TE::VariantMatch(_, _, _)
+        | TE::Loop { .. }
+        | TE::NamedBlock(_, _)
+        | TE::Block(_)
+        | TE::Dereference(_)
+        | TE::UnaryExp(_, _)
+        | TE::BinopExp(_, _, _, _)
+        | TE::Pack(_, _, _, _)
+        | TE::PackVariant(_, _, _, _, _)
+        | TE::ExpList(_)
+        | TE::Borrow(_, _, _)
+        | TE::TempBorrow(_, _)
+        | TE::BorrowLocal(_, _) => false,
+        // These are already errors
+        TE::Unit { .. }
+        | TE::Builtin(_, _)
+        | TE::While(_, _, _)
+        | TE::Assign(_, _, _)
+        | TE::Return(_)
+        | TE::Abort(_)
+        | TE::Mutate(_, _)
+        | TE::Give(_, _)
+        | TE::Continue(_)
+        | TE::UnresolvedError => false,
+    }
+}
diff --git a/external-crates/move/crates/move-compiler/tests/linter/correct_redundant_ref_deref.move b/external-crates/move/crates/move-compiler/tests/linter/correct_redundant_ref_deref.move
@@ -0,0 +1,10 @@
+module 0x42::M {
+    struct MyResource has copy, drop{
+        value: u64,
+    }
+
+    public fun test_borrow_deref_ref() {
+        let resource = MyResource { value: 10 };
+        let _ref1 = &resource;
+    }
+}
diff --git a/external-crates/move/crates/move-compiler/tests/linter/incorrect_redundant_ref_deref.exp b/external-crates/move/crates/move-compiler/tests/linter/incorrect_redundant_ref_deref.exp
@@ -0,0 +1,172 @@
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:10:21
+   │
+10 │         let _ref = &*(&resource);  // Redundant borrow-dereference
+   │                     ^^^^^^^^^^^^ Redundant borrow-dereference detected. Replace this borrow-deref with 'copy'.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:15:25
+   │
+15 │         let _ref = &mut *(&mut resource);  // Redundant mutable borrow-dereference
+   │                         ^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Replace this borrow-deref with 'copy'.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:20:22
+   │
+20 │         let _value = *(&resource.value);  // Redundant dereference of field borrow
+   │                      ^^^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the field access directly.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:57:21
+   │
+57 │         let _ref = &*(&*(&resource));  // Triple nested borrow-dereference, might be missed
+   │                     ^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the inner expression directly.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:57:24
+   │
+57 │         let _ref = &*(&*(&resource));  // Triple nested borrow-dereference, might be missed
+   │                        ^^^^^^^^^^^^ Redundant borrow-dereference detected. Replace this borrow-deref with 'copy'.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+error[E04010]: cannot infer type
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:68:13
+   │
+68 │         let _value = *((&resource).value);  // Complex expression, might be missed
+   │             ^^^^^^ Could not infer this type. Try adding an annotation
+
+error[E04007]: incompatible types
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:68:22
+   │
+ 3 │         value: u64,
+   │                --- Given: 'u64'
+   ·
+68 │         let _value = *((&resource).value);  // Complex expression, might be missed
+   │                      ^^^^^^^^^^^^^^^^^^^^
+   │                      │
+   │                      Invalid dereference.
+   │                      Expected: '&_'
+
+error[E04010]: cannot infer type
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:68:22
+   │
+68 │         let _value = *((&resource).value);  // Complex expression, might be missed
+   │                      ^^^^^^^^^^^^^^^^^^^^ Could not infer this type. Try adding an annotation
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:75:21
+   │
+75 │         let _ref = &*&resource.value;  // Redundant borrow-dereference on field
+   │                     ^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the field access directly.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:80:21
+   │
+80 │         let _ref = &*&(&resource).value;  // Nested redundant borrow-dereference on field
+   │                     ^^^^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the field access directly.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:86:21
+   │
+86 │         let _ref = &*&0;  // Redundant borrow-dereference on literal
+   │                     ^^^ Redundant borrow-dereference detected. Remove this borrow-deref and use the expression directly.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+   ┌─ tests/linter/incorrect_redundant_ref_deref.move:94:21
+   │
+94 │         let _ref = &*&get_resource();  // Redundant borrow-dereference on function call result
+   │                     ^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Remove this borrow-deref and use the expression directly.
+   │
+   = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:107:21
+    │
+107 │         let _ref = &*&(&*&resource.value);  // Multiple redundant borrows on field
+    │                     ^^^^^^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the inner expression directly.
+    │
+    = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+error[E04004]: expected a single non-reference type
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:107:22
+    │
+107 │         let _ref = &*&(&*&resource.value);  // Multiple redundant borrows on field
+    │                      ^^^^^^^^^^^^^^^^^^^^
+    │                      │ │
+    │                      │ Expected a single non-reference type, but found: '&u64'
+    │                      Invalid borrow
+
+warning[Lint W01009]: redundant reference/dereference
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:107:25
+    │
+107 │         let _ref = &*&(&*&resource.value);  // Multiple redundant borrows on field
+    │                         ^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the field access directly.
+    │
+    = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[W09002]: unused variable
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:111:13
+    │
+111 │         let mut resource = MyResource { value: 10 };
+    │             ^^^ Unused local variable 'mut'. Consider removing or prefixing with an underscore: '_mut'
+    │
+    = 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/incorrect_redundant_ref_deref.move:111:17
+    │
+111 │         let mut resource = MyResource { value: 10 };
+    │                 ^^^^^^^^
+    │                 │
+    │                 Unexpected 'resource'
+    │                 Expected ';'
+
+error[E04010]: cannot infer type
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:112:21
+    │
+112 │         let _ref = &*&mut *&resource;  // Mixed mutable and immutable redundant borrows
+    │                     ^^^^^^^^^^^^^^^^ Could not infer this type. Try adding an annotation
+
+error[E04010]: cannot infer type
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:112:27
+    │
+112 │         let _ref = &*&mut *&resource;  // Mixed mutable and immutable redundant borrows
+    │                           ^^^^^^^^^^ Could not infer this type. Try adding an annotation
+
+error[E03009]: unbound variable
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:112:29
+    │
+112 │         let _ref = &*&mut *&resource;  // Mixed mutable and immutable redundant borrows
+    │                             ^^^^^^^^ Unbound variable 'resource'
+
+warning[Lint W01009]: redundant reference/dereference
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:117:25
+    │
+117 │         let _value = *&(*&resource.value + 1);  // Redundant borrows in complex expression
+    │                         ^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the field access directly.
+    │
+    = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01009]: redundant reference/dereference
+    ┌─ tests/linter/incorrect_redundant_ref_deref.move:124:21
+    │
+124 │         let _ref = &*(&resource.value);  // Complex nested borrow on field, might be missed
+    │                     ^^^^^^^^^^^^^^^^^^ Redundant borrow-dereference detected. Use the field access directly.
+    │
+    = This warning can be suppressed with '#[allow(lint(redundant_ref_deref))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+