refactor and add testcase

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

@@ -1,64 +1,68 @@
 // Copyright (c) The Move Contributors
 // SPDX-License-Identifier: Apache-2.0
 
-//! Detects excessively nested blocks of code, warning when nesting exceeds a predefined threshold.
-//! Aims to improve code readability and maintainability by encouraging simpler, flatter code structures.
-//! Issues a single warning for each sequence of nested blocks that surpasses the limit, to avoid redundant alerts.
+//! Detects excessive nesting of control flow structures (if/while/loop)
+//! Aims to improve code readability by encouraging flatter code structure
 use crate::{
     diag,
-    diagnostics::WarningFilters,
     linters::StyleCodes,
-    shared::CompilationEnv,
     typing::{
-        ast::{self as T, UnannotatedExp_},
-        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+        ast::{self as H},
+        visitor::simple_visitor,
     },
 };
+const MAX_NESTING_LEVEL: u8 = 3;
 
-const NESTING_THRESHOLD: usize = 3;
-pub struct ExcessiveNesting;
+simple_visitor!(
+    NestingExceed,
+    fn visit_exp_custom(&mut self, exp: &H::Exp) -> bool {
+        let nesting_level = calculate_nesting_level(&exp.exp.value, 0);
 
-pub struct Context<'a> {
-    env: &'a mut CompilationEnv,
-    nesting_level: usize,
-    warning_issued: bool,
-}
-
-impl TypingVisitorConstructor for ExcessiveNesting {
-    type Context<'a> = Context<'a>;
-
-    fn context<'a>(env: &'a mut CompilationEnv, _program: &T::Program) -> Self::Context<'a> {
-        Context {
-            env,
-            nesting_level: 0,
-            warning_issued: false,
+        if nesting_level > MAX_NESTING_LEVEL {
+            let msg = format!(
+                "Nesting level of {} exceeds maximum allowed ({})",
+                nesting_level, MAX_NESTING_LEVEL
+            );
+            self.add_diag(diag!(
+                StyleCodes::ExcessiveNesting.diag_info(),
+                (exp.exp.loc, msg),
+                (exp.exp.loc, "Consider refactoring to reduce nesting"),
+            ));
         }
-    }
-}
-
-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()
+        false
     }
+);
 
-    fn visit_exp_custom(&mut self, exp: &T::Exp) -> bool {
-        if let UnannotatedExp_::Block(_) = &exp.exp.value {
-            self.nesting_level += 1;
-
-            if self.nesting_level > NESTING_THRESHOLD && !self.warning_issued {
-                self.env.add_diag(diag!(
-                    StyleCodes::ExcessiveNesting.diag_info(),
-                    (exp.exp.loc, "Detected excessive block nesting. Consider refactoring to simplify the code."),
-                ));
-                self.warning_issued = true;
-            }
-        } else if self.nesting_level <= NESTING_THRESHOLD {
-            self.warning_issued = false;
-        };
-        false
+fn calculate_nesting_level(exp: &H::UnannotatedExp_, current_level: u8) -> u8 {
+    match exp {
+        H::UnannotatedExp_::Block(seq) => seq
+            .1
+            .iter()
+            .map(|sp!(_, item)| {
+                if let H::SequenceItem_::Seq(e) = item {
+                    calculate_nesting_level(&e.exp.value, current_level)
+                } else {
+                    current_level
+                }
+            })
+            .max()
+            .unwrap_or(current_level),
+        H::UnannotatedExp_::IfElse(_, if_block, else_block) => {
+            let next_level = current_level + 1;
+            let if_level = calculate_nesting_level(&if_block.exp.value, next_level);
+            let else_level = else_block
+                .as_ref()
+                .map(|e| calculate_nesting_level(&e.exp.value, next_level))
+                .unwrap_or(next_level);
+            if_level.max(else_level)
+        }
+        H::UnannotatedExp_::While(_, _, block) => {
+            calculate_nesting_level(&block.exp.value, current_level + 1)
+        }
+        H::UnannotatedExp_::Loop { body, .. } => {
+            calculate_nesting_level(&body.exp.value, current_level + 1)
+        }
+        _ => current_level,
     }
 }

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 excessive_nesting;
 pub mod loop_without_exit;
 pub mod meaningless_math_operation;
 pub mod redundant_ref_deref;
@@ -162,6 +163,12 @@ lints!(
         "unnecessary_unit",
         "unit `()` expression can be removed or simplified"
     ),
+    (
+        ExcessiveNesting,
+        LinterDiagnosticCategory::Complexity,
+        "excessive_nesting",
+        "detected excessive block nesting. Consider refactoring to simplify the code."
+    ),
 );
 
 pub const ALLOW_ATTR_CATEGORY: &str = "lint";
@@ -199,6 +206,7 @@ pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
                 self_assignment::SelfAssignmentVisitor.visitor(),
                 redundant_ref_deref::RedundantRefDerefVisitor.visitor(),
                 unnecessary_unit::UnnecessaryUnit.visitor(),
+                excessive_nesting::NestingExceed.visitor(),
             ]
         }
     }

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

@@ -1,62 +1,17 @@
-warning[W09002]: unused variable
-  ┌─ tests/linter/false_negative_excessive_nesting.move:2:45
-  │
-2 │     public fun complex_logic_without_blocks(x: u64) {
-  │                                             ^ Unused parameter 'x'. Consider removing or prefixing with an underscore: '_x'
-  │
-  = 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_negative_excessive_nesting.move:3:12
-  │
-3 │         if x > 10 if x < 20 if x != 15 if x % 2 == 0 { x = x + 1 } else { x = x - 1 } else { x = x * 2 } else { x = x / 2 } else { x = 0 };
-  │            ^
-  │            │
-  │            Unexpected 'x'
-  │            Expected '('
-
-error[E03005]: unbound unscoped name
-   ┌─ tests/linter/false_negative_excessive_nesting.move:10:17
-   │
-10 │                 for (i in 0..x) {
-   │                 ^^^ Unbound function 'for' in current scope
-
-error[E03009]: unbound variable
-   ┌─ tests/linter/false_negative_excessive_nesting.move:10:22
-   │
-10 │                 for (i in 0..x) {
-   │                      ^ Unbound variable 'i'
-
-error[E01002]: unexpected token
-   ┌─ tests/linter/false_negative_excessive_nesting.move:10:24
-   │
-10 │                 for (i in 0..x) {
-   │                        ^^
-   │                        │
-   │                        Unexpected 'in'
-   │                        Expected ',' or ')'
-
-error[E01002]: unexpected token
-   ┌─ tests/linter/false_negative_excessive_nesting.move:10:33
-   │
-10 │                 for (i in 0..x) {
-   │                                 ^
-   │                                 │
-   │                                 Unexpected '{'
-   │                                 Expected ';'
-
-error[E01002]: unexpected token
-   ┌─ tests/linter/false_negative_excessive_nesting.move:17:13
-   │
-17 │             x = x - 1;
-   │             ^
-   │             │
-   │             Unexpected 'x'
-   │             Expected ';'
-
-error[E01002]: unexpected token
-   ┌─ tests/linter/false_negative_excessive_nesting.move:20:1
-   │
-20 │ }
-   │ ^ Invalid code unit. Expected 'address' or 'module'. Got '}'
+warning[Lint W01011]: detected excessive block nesting. Consider refactoring to simplify the code.
+   ┌─ tests/linter/false_negative_excessive_nesting.move:5:17
+   │    
+ 5 │             let y = if (x > 0) {
+   │ ╭───────────────────^
+   │ │ ╭─────────────────'
+ 6 │ │ │             if (x > 10) {
+ 7 │ │ │                 if (x > 20) {
+ 8 │ │ │                     if (x > 30) { // Nested in let binding
+   · │ │
+12 │ │ │             } else { x }
+13 │ │ │         } else { x };
+   │ ╰─│────────────────────^ Nesting level of 4 exceeds maximum allowed (3)
+   │   ╰────────────────────' Consider refactoring to reduce nesting
+   │    
+   = This warning can be suppressed with '#[allow(lint(excessive_nesting))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 

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

@@ -1,20 +1,15 @@
 module 0x42::excessive_nesting_false_negative {
-    public fun complex_logic_without_blocks(x: u64) {
-        if x > 10 if x < 20 if x != 15 if x % 2 == 0 { x = x + 1 } else { x = x - 1 } else { x = x * 2 } else { x = x / 2 } else { x = 0 };
-        // This is essentially 5 levels of nesting, but without explicit block structures
-    }
 
-    public fun mixed_control_structures(x: u64) {
-        while (x > 0) {
-            if (x % 2 == 0) {
-                for (i in 0..x) {
-                    if (i % 3 == 0) {
-                        // This is 4 levels deep, mixing different control structures
-                        // which might be missed by a lint focusing only on block structures
-                    }
-                }
-            }
-            x = x - 1;
-        }
+    #[allow(unused_variable)]
+    fun nested_in_let(x: u64) {
+        let y = if (x > 0) {
+            if (x > 10) {
+                if (x > 20) {
+                    if (x > 30) { // Nested in let binding
+                        x + 1
+                    } else { x }
+                } else { x }
+            } else { x }
+        } else { x };
     }
 }

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

@@ -1,44 +1,15 @@
-warning[W09002]: unused variable
-  ┌─ tests/linter/false_positive_excessive_nesting.move:2:40
-  │
-2 │     public fun nested_match_expression(x: u64) {
-  │                                        ^ Unused parameter 'x'. Consider removing or prefixing with an underscore: '_x'
-  │
-  = This warning can be suppressed with '#[allow(unused_variable)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
-
-error[E03009]: unbound variable
-  ┌─ tests/linter/false_positive_excessive_nesting.move:3:9
-  │
-3 │         match x {
-  │         ^^^^^ Unbound variable 'match'
-
-error[E01002]: unexpected token
-  ┌─ tests/linter/false_positive_excessive_nesting.move:3:15
-  │
-3 │         match x {
-  │               ^
-  │               │
-  │               Unexpected 'x'
-  │               Expected ';'
-
-warning[W09002]: unused variable
-   ┌─ tests/linter/false_positive_excessive_nesting.move:19:13
-   │
-19 │         let a = {
-   │             ^ Unused local variable 'a'. Consider removing or prefixing with an underscore: '_a'
-   │
-   = This warning can be suppressed with '#[allow(unused_variable)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
-
-warning[Lint W01008]: excessive nesting
-   ┌─ tests/linter/false_positive_excessive_nesting.move:22:29
-   │  
-22 │                       let d = {
-   │ ╭─────────────────────────────^
-23 │ │                         // This looks like 4 levels of nesting, but it's actually a series of let bindings
-24 │ │                         // which might be more readable in this format than flattened
-25 │ │                         10
-26 │ │                     };
-   │ ╰─────────────────────^ Detected excessive block nesting. Consider refactoring to simplify the code.
-   │  
+warning[Lint W01011]: detected excessive block nesting. Consider refactoring to simplify the code.
+   ┌─ tests/linter/false_positive_excessive_nesting.move:3:9
+   │    
+ 3 │ ╭ ╭         if (x > 0) {
+ 4 │ │ │             if (x > 10) {
+ 5 │ │ │                 if (x > 20) {
+ 6 │ │ │                     if (x > 30) { // Complex but necessary business logic
+   · │ │
+10 │ │ │             }
+11 │ │ │         };
+   │ ╰─│─────────^ Nesting level of 4 exceeds maximum allowed (3)
+   │   ╰─────────' Consider refactoring to reduce nesting
+   │    
    = This warning can be suppressed with '#[allow(lint(excessive_nesting))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 

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

@@ -1,34 +1,14 @@
-module 0x42::excessive_nesting_false_positive {
-    public fun nested_match_expression(x: u64) {
-        match x {
-            0 => {
-                {
-                    {
-                        {
-                            // This is technically 4 levels deep, but the outer level is a match expression
-                            // which might be necessary and not truly contributing to excessive nesting
-                        }
+module 0x42::reasonable_nesting {
+    fun match_nested(x: u64): u64 {
+        if (x > 0) {
+            if (x > 10) {
+                if (x > 20) {
+                    if (x > 30) { // Complex but necessary business logic
+                        return 1
                     }
                 }
-            },
-            _ => {}
-        }
-    }
-
-    public fun nested_let_bindings() {
-        let a = {
-            let b = {
-                let c = {
-                    let d = {
-                        // This looks like 4 levels of nesting, but it's actually a series of let bindings
-                        // which might be more readable in this format than flattened
-                        10
-                    };
-                    d + 1
-                };
-                c * 2
-            };
-            b - 5
+            }
         };
+        0
     }
 }

external-crates/move/crates/move-compiler/tests/linter/move_2024/unneeded_return_branch.exp

@@ -14,14 +14,6 @@ warning[Lint W04004]: unneeded return
   │
   = This warning can be suppressed with '#[allow(lint(unneeded_return))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 
-warning[Lint W01008]: excessive nesting
-  ┌─ tests/linter/move_2024/unneeded_return_branch.move:9:33
-  │
-9 │     if (cond) { return 5 } else { return 0 }
-  │                                 ^^^^^^^^^^^^ Detected excessive block nesting. Consider refactoring to simplify the code.
-  │
-  = This warning can be suppressed with '#[allow(lint(excessive_nesting))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
-
 warning[Lint W04004]: unneeded return
   ┌─ tests/linter/move_2024/unneeded_return_branch.move:9:35
   │