[move-compiler] Add macro for "simple" visitors (#20062)

## Description - Add a macro for the simple visitor declarations ## Test plan - Updated one test --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
MystenLabs · Oct 28, 2024 · cb80140 · cb80140
1 parent 495dbb7
commit cb80140
Show file tree

Hide file tree

Showing 20 changed files with 260 additions and 504 deletions.
diff --git a/external-crates/move/crates/move-compiler/src/cfgir/visitor.rs b/external-crates/move/crates/move-compiler/src/cfgir/visitor.rs
@@ -66,7 +66,7 @@ pub trait CFGIRVisitorConstructor: Send {
 }
 
 pub trait CFGIRVisitorContext {
-    fn add_warning_filter_scope(&mut self, filter: WarningFilters);
+    fn add_warning_filter_scope(&mut self, filters: WarningFilters);
     fn pop_warning_filter_scope(&mut self);
 
     fn visit_module_custom(&mut self, _ident: ModuleIdent, _mdef: &G::ModuleDefinition) -> bool {
@@ -327,6 +327,51 @@ impl<V: CFGIRVisitorConstructor + Send + Sync> CFGIRVisitor for V {
     }
 }
 
+macro_rules! simple_visitor {
+    ($visitor:ident, $($overrides:item),*) => {
+        pub struct $visitor;
+
+        pub struct Context<'a> {
+            env: &'a mut crate::shared::CompilationEnv,
+        }
+
+        impl crate::cfgir::visitor::CFGIRVisitorConstructor for $visitor {
+            type Context<'a> = Context<'a>;
+
+            fn context<'a>(env: &'a mut crate::shared::CompilationEnv, _program: &crate::cfgir::ast::Program) -> Self::Context<'a> {
+                Context {
+                    env,
+                }
+            }
+        }
+
+        impl Context<'_> {
+            #[allow(unused)]
+            fn add_diag(&mut self, diag: crate::diagnostics::Diagnostic) {
+                self.env.add_diag(diag);
+            }
+
+            #[allow(unused)]
+            fn add_diags(&mut self, diags: crate::diagnostics::Diagnostics) {
+                self.env.add_diags(diags);
+            }
+        }
+
+        impl crate::cfgir::visitor::CFGIRVisitorContext for Context<'_> {
+            fn add_warning_filter_scope(&mut self, filters: crate::diagnostics::WarningFilters) {
+                self.env.add_warning_filter_scope(filters)
+            }
+
+            fn pop_warning_filter_scope(&mut self) {
+                self.env.pop_warning_filter_scope()
+            }
+
+            $($overrides)*
+        }
+    }
+}
+pub(crate) use simple_visitor;
+
 //**************************************************************************************************
 // simple absint visitor
 //**************************************************************************************************

diff --git a/external-crates/move/crates/move-compiler/src/linters/abort_constant.rs b/external-crates/move/crates/move-compiler/src/linters/abort_constant.rs
@@ -39,9 +39,20 @@ impl CFGIRVisitorConstructor for AssertAbortNamedConstants {
     }
 }
 
+impl Context<'_> {
+    fn add_diag(&mut self, diag: crate::diagnostics::Diagnostic) {
+        self.env.add_diag(diag);
+    }
+
+    #[allow(unused)]
+    fn add_diags(&mut self, diags: crate::diagnostics::Diagnostics) {
+        self.env.add_diags(diags);
+    }
+}
+
 impl CFGIRVisitorContext for Context<'_> {
-    fn add_warning_filter_scope(&mut self, filter: WarningFilters) {
-        self.env.add_warning_filter_scope(filter)
+    fn add_warning_filter_scope(&mut self, filters: WarningFilters) {
+        self.env.add_warning_filter_scope(filters)
     }
 
     fn pop_warning_filter_scope(&mut self) {
@@ -76,7 +87,7 @@ impl Context<'_> {
                 diag.add_note("Consider using an error constant with the '#[error]' to allow for a more descriptive error.");
             }
 
-            self.env.add_diag(diag);
+            self.add_diag(diag);
         }
     }
 }
diff --git a/external-crates/move/crates/move-compiler/src/linters/constant_naming.rs b/external-crates/move/crates/move-compiler/src/linters/constant_naming.rs
@@ -6,30 +6,14 @@
 //! within a module against this convention.
 use crate::{
     diag,
-    diagnostics::WarningFilters,
     expansion::ast::ModuleIdent,
     linters::StyleCodes,
     parser::ast::ConstantName,
-    shared::CompilationEnv,
-    typing::{
-        ast as T,
-        visitor::{TypingVisitorConstructor, TypingVisitorContext},
-    },
+    typing::{ast as T, visitor::simple_visitor},
 };
 
-pub struct ConstantNamingVisitor;
-pub struct Context<'a> {
-    env: &'a mut CompilationEnv,
-}
-impl TypingVisitorConstructor for ConstantNamingVisitor {
-    type Context<'a> = Context<'a>;
-
-    fn context<'a>(env: &'a mut CompilationEnv, _program: &T::Program) -> Self::Context<'a> {
-        Context { env }
-    }
-}
-
-impl TypingVisitorContext for Context<'_> {
+simple_visitor!(
+    ConstantNamingVisitor,
     fn visit_constant_custom(
         &mut self,
         _module: ModuleIdent,
@@ -41,19 +25,11 @@ impl TypingVisitorContext for Context<'_> {
             let uid_msg =
                 format!("'{name}' should be ALL_CAPS. Or for error constants, use PascalCase",);
             let diagnostic = diag!(StyleCodes::ConstantNaming.diag_info(), (cdef.loc, uid_msg));
-            self.env.add_diag(diagnostic);
+            self.add_diag(diagnostic);
         }
         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()
-    }
-}
+);
 
 /// Returns `true` if the string is in all caps snake case, including numeric characters.
 fn is_valid_name(name: &str) -> bool {

diff --git a/external-crates/move/crates/move-compiler/src/linters/loop_without_exit.rs b/external-crates/move/crates/move-compiler/src/linters/loop_without_exit.rs
@@ -4,36 +4,14 @@
 use super::StyleCodes;
 use crate::{
     diag,
-    diagnostics::WarningFilters,
-    shared::CompilationEnv,
     typing::{
         ast::{self as T, UnannotatedExp_},
-        visitor::{exp_satisfies, TypingVisitorConstructor, TypingVisitorContext},
+        visitor::{exp_satisfies, simple_visitor},
     },
 };
 
-pub struct LoopWithoutExit;
-
-pub struct Context<'a> {
-    env: &'a mut CompilationEnv,
-}
-
-impl TypingVisitorConstructor for LoopWithoutExit {
-    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()
-    }
-
+simple_visitor!(
+    LoopWithoutExit,
     fn visit_exp_custom(&mut self, exp: &T::Exp) -> bool {
         // we do not care about `while` since there is another lint that handles reporting
         // that `while (true)` should be `loop`
@@ -57,10 +35,10 @@ impl TypingVisitorContext for Context<'_> {
                 This code will until it errors, e.g. reaching an 'abort' or running out of gas"
             )
         );
-        self.env.add_diag(diag);
+        self.add_diag(diag);
         false
     }
-}
+);
 
 fn has_return(e: &T::Exp) -> bool {
     exp_satisfies(e, |e| matches!(e.exp.value, UnannotatedExp_::Return(_)))

diff --git a/external-crates/move/crates/move-compiler/src/linters/meaningless_math_operation.rs b/external-crates/move/crates/move-compiler/src/linters/meaningless_math_operation.rs
@@ -4,40 +4,17 @@
 //! 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},
+    cfgir::visitor::simple_visitor,
     diag,
-    diagnostics::WarningFilters,
     hlir::ast::{self as H, Value_},
     linters::StyleCodes,
     parser::ast::BinOp_,
-    shared::CompilationEnv,
 };
 use move_core_types::u256::U256;
 use move_ir_types::location::Loc;
 
-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()
-    }
-
+simple_visitor!(
+    MeaninglessMathOperation,
     fn visit_exp_custom(&mut self, exp: &H::Exp) -> bool {
         let H::UnannotatedExp_::BinopExp(lhs, op, rhs) = &exp.exp.value else {
             return false;
@@ -54,7 +31,7 @@ impl CFGIRVisitorContext for Context<'_> {
         };
         if let Some(meaningless_operand) = is_unchanged {
             let msg = "This operation has no effect and can be removed";
-            self.env.add_diag(diag!(
+            self.add_diag(diag!(
                 StyleCodes::MeaninglessMath.diag_info(),
                 (exp.exp.loc, msg),
                 (meaningless_operand, "Because of this operand"),
@@ -70,7 +47,7 @@ impl CFGIRVisitorContext for Context<'_> {
         };
         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!(
+            self.add_diag(diag!(
                 StyleCodes::MeaninglessMath.diag_info(),
                 (exp.exp.loc, msg),
                 (zero_operand, "Because of this operand"),
@@ -84,7 +61,7 @@ impl CFGIRVisitorContext for Context<'_> {
         };
         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!(
+            self.add_diag(diag!(
                 StyleCodes::MeaninglessMath.diag_info(),
                 (exp.exp.loc, msg),
                 (one_operand, "Because of this operand"),
@@ -95,7 +72,7 @@ impl CFGIRVisitorContext for Context<'_> {
 
         false
     }
-}
+);
 
 fn is_zero(exp: &H::Exp) -> Option<Loc> {
     let H::UnannotatedExp_::Value(sp!(loc, value_)) = &exp.exp.value else {

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
@@ -8,41 +8,19 @@
 use crate::linters::StyleCodes;
 use crate::{
     diag,
-    diagnostics::WarningFilters,
-    shared::CompilationEnv,
     typing::{
-        ast::{self as T, Exp, UnannotatedExp_ as TE},
-        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+        ast::{Exp, UnannotatedExp_ as TE},
+        visitor::simple_visitor,
     },
 };
 
-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()
-    }
-
+simple_visitor!(
+    RedundantRefDerefVisitor,
     fn visit_exp_custom(&mut self, exp: &Exp) -> bool {
         self.check_redundant_ref_deref(exp);
         false
     }
-}
+);
 
 impl Context<'_> {
     // Check for &* pattern
@@ -59,33 +37,31 @@ impl Context<'_> {
             return;
         }
         match &deref_exp.exp.value {
-            TE::TempBorrow(_, inner) if is_simple_deref_ref_exp(inner) => self.env.add_diag(diag!(
+            TE::TempBorrow(_, inner) if is_simple_deref_ref_exp(inner) => self.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!(
+            TE::TempBorrow(_, inner) if all_deref_borrow(inner) => self.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. \
+            TE::Borrow(false, _, _) if exp.exp.loc != deref_exp.exp.loc => self.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!(
+                )
+            )),
+            TE::Borrow(_, _, _) | TE::BorrowLocal(_, _) => self.add_diag(diag!(
                 StyleCodes::RedundantRefDeref.diag_info(),
                 (
                     exp.exp.loc,