[lint] cyclomatic_complexity (#17170)

* lint: cyclomatic_complexity

* add more tests, fix bugs with while and for

* mv cyclomatic_complexity into experimental

* Update third_party/move/tools/move-linter/src/model_ast_lints/cyclomatic_complexity.rs

Co-authored-by: Vineeth Kashyap <[email protected]>

* suppress complexity warning in test

* addressing comments

* UPBL

* fixing docs

* UPBL

---------

Co-authored-by: Vineeth Kashyap <[email protected]>

Author: GotenJBZ

third_party/move/tools/move-linter/src/model_ast_lints.rs

@@ -7,6 +7,7 @@ mod aborting_overflow_checks;
 mod almost_swapped;
 mod assert_const;
 mod blocks_in_conditions;
+mod cyclomatic_complexity;
 mod equal_operands_in_bin_op;
 mod known_to_abort;
 mod needless_bool;
@@ -27,7 +28,7 @@ use std::collections::BTreeMap;
 /// Returns a default pipeline of "expression linters" to run.
 pub fn get_default_linter_pipeline(config: &BTreeMap<String, String>) -> Vec<Box<dyn ExpChecker>> {
     // Start with the default set of checks.
-    let checks: Vec<Box<dyn ExpChecker>> = vec![
+    let mut checks: Vec<Box<dyn ExpChecker>> = vec![
         Box::<aborting_overflow_checks::AbortingOverflowChecks>::default(),
         Box::<almost_swapped::AlmostSwapped>::default(),
         Box::<assert_const::AssertConst>::default(),
@@ -52,6 +53,7 @@ pub fn get_default_linter_pipeline(config: &BTreeMap<String, String>) -> Vec<Box
     }
     if checks_category == "experimental" {
         // Push experimental checks to `checks`.
+        checks.push(Box::<cyclomatic_complexity::CyclomaticComplexity>::default());
     }
     checks
 }

third_party/move/tools/move-linter/src/model_ast_lints/cyclomatic_complexity.rs

@@ -0,0 +1,150 @@
+// Copyright (c) Aptos Foundation
+// SPDX-License-Identifier: Apache-2.0
+
+//! Cyclomatic complexity measures the number of linearly independent execution paths
+//! through a function.  A high value generally correlates with code that is
+//! harder to test and maintain.
+//!
+//! This linter performs an approximation while traversing the Move
+//! expression tree:
+//!   1. The complexity score starts at **1**.
+//!   2. The score is incremented for each control-flow decision point found:
+//!         * +1 for each `if`
+//!         * +1 for each `else if`
+//!         * +1 for each `loop`, `while`, or `for`
+//!         * +1 for each `break` or `continue`
+//!         * +1 for each `return` statement that is not the final expression in the function
+//!         * +n where n = (number of match arms - 1)
+//!
+//! When the accumulated score exceeds `DEFAULT_THRESHOLD` (currently **10**),
+//! the linter emits a diagnostic suggesting that the function be simplified or
+//! decomposed.
+//!
+//! NOTE: The threshold is intentionally conservative.
+
+use crate::utils::{detect_for_loop, detect_while_loop};
+use move_compiler_v2::external_checks::ExpChecker;
+use move_model::{
+    ast::ExpData,
+    model::{FunctionEnv, Loc, NodeId},
+};
+
+const DEFAULT_THRESHOLD: i32 = 10;
+
+pub struct CyclomaticComplexity {
+    complexity: i32,
+    reported: bool,
+    root_node: Option<NodeId>,
+    final_return_node: Option<NodeId>,
+}
+
+impl CyclomaticComplexity {
+    fn bump(&mut self, delta: i32) {
+        self.complexity = self.complexity.saturating_add(delta);
+    }
+
+    /// Returns the NodeId of the final return statement in the function, if any.
+    fn get_final_return(function: &FunctionEnv) -> Option<NodeId> {
+        if let Some(def) = function.get_def() {
+            return Self::get_final_return_from_exp(def);
+        }
+        None
+    }
+
+    /// Helper method to recursively find the final return statement in an expression.
+    fn get_final_return_from_exp(expr: &move_model::ast::Exp) -> Option<NodeId> {
+        use move_model::ast::ExpData::*;
+        match expr.as_ref() {
+            Return(id, _) => Some(*id),
+            Sequence(_, seq) => seq.last().and_then(Self::get_final_return_from_exp),
+            Block(_, _, _, body) => Self::get_final_return_from_exp(body),
+            _ => None,
+        }
+    }
+
+    fn maybe_report(&mut self, function: &FunctionEnv) {
+        if self.reported || self.complexity <= DEFAULT_THRESHOLD {
+            return;
+        }
+        let env = function.env();
+        let loc: Loc = function.get_loc();
+        self.reported = true;
+        self.report(
+            env,
+            &loc,
+            &format!(
+                "Function `{}` has cyclomatic complexity {}, which exceeds the allowed threshold of {}",
+                function.get_full_name_str(),
+                self.complexity,
+                DEFAULT_THRESHOLD,
+            ),
+        );
+    }
+}
+
+impl Default for CyclomaticComplexity {
+    fn default() -> Self {
+        Self {
+            complexity: 1,
+            reported: false,
+            root_node: None,
+            final_return_node: None,
+        }
+    }
+}
+
+impl ExpChecker for CyclomaticComplexity {
+    fn get_name(&self) -> String {
+        "cyclomatic_complexity".to_string()
+    }
+
+    fn visit_expr_pre(&mut self, function: &FunctionEnv, expr: &ExpData) {
+        if self.root_node.is_none() {
+            self.root_node = function.get_def().map(|def| def.node_id());
+            self.final_return_node = Self::get_final_return(function);
+        }
+
+        use ExpData::*;
+
+        match expr {
+            // loop, while, for
+            Loop(_, _) => {
+                let delta = if detect_for_loop(expr, function) {
+                    // For loop expansion generates: Loop(+1) + IfElse(+1) + IfElse(+1) + IfElse(+1) + LoopCont(+1) + LoopCont(+1) = +6 extra
+                    // But we want for to count as +1 total, so we subtract 4 here
+                    -4
+                } else if detect_while_loop(expr) {
+                    // While loop expansion generates: Loop(+1) + IfElse(+1) + LoopCont(+1) = +3 extra
+                    // But we want while to count as +1 total, so we subtract 1 here
+                    -1
+                } else {
+                    1
+                };
+                self.bump(delta);
+            },
+            // if and else if (+1)
+            IfElse(..) => self.bump(1),
+            // break and continue (+1)
+            LoopCont(..) => self.bump(1),
+
+            // return, if is not the last statement (+1)
+            Return(_, _) => {
+                if self.final_return_node != Some(expr.node_id()) {
+                    self.bump(1);
+                }
+            },
+            // match (+n-1)
+            Match(_, _, arms) if !arms.is_empty() => self.bump(arms.len() as i32 - 1),
+
+            _ => {},
+        }
+    }
+
+    fn visit_expr_post(&mut self, function: &FunctionEnv, expr: &ExpData) {
+        if let Some(root) = self.root_node {
+            if expr.node_id() == root {
+                self.maybe_report(function);
+            }
+        }
+    }
+}

third_party/move/tools/move-linter/src/model_ast_lints/while_true.rs

@@ -4,10 +4,10 @@
 //! This module implements an expression linter that checks code of the form:
 //! `while (true) { ... }` and suggests to use `loop { ... }` instead.
 
-use legacy_move_compiler::parser::syntax::FOR_LOOP_UPDATE_ITER_FLAG;
+use crate::utils::detect_for_loop;
 use move_compiler_v2::external_checks::ExpChecker;
 use move_model::{
-    ast::{Exp, ExpData, Value},
+    ast::{ExpData, Value},
     model::FunctionEnv,
 };
 
@@ -23,7 +23,7 @@ impl ExpChecker for WhileTrue {
         use ExpData::{IfElse, Loop};
         // Check if `expr` is of the form: Loop(IfElse(true, then, _)).
         let Loop(id, body) = expr else { return };
-        let IfElse(_, cond, then, _) = body.as_ref() else {
+        let IfElse(_, cond, _, _) = body.as_ref() else {
             return;
         };
         let ExpData::Value(_, Value::Bool(b)) = cond.as_ref() else {
@@ -33,7 +33,7 @@ impl ExpChecker for WhileTrue {
             return;
         }
         // Check if it is the `for` loop.
-        if detect_for_loop(then, function) {
+        if detect_for_loop(expr, function) {
             return;
         }
         let env = function.env();
@@ -45,23 +45,3 @@ impl ExpChecker for WhileTrue {
         );
     }
 }
-
-fn detect_for_loop(then: &Exp, function: &FunctionEnv) -> bool {
-    use ExpData::{IfElse, LocalVar, Sequence};
-    // Check if `then` is of the form:
-    //   Sequence([IfElse(LocalVar(FOR_LOOP_UPDATE_ITER_FLAG), ...)])
-    // If so, it is the `for` loop.
-    let Sequence(_, stmts) = then.as_ref() else {
-        return false;
-    };
-    let Some(stmt) = stmts.first() else {
-        return false;
-    };
-    let IfElse(_, cond, _, _) = stmt.as_ref() else {
-        return false;
-    };
-    let LocalVar(_, name) = cond.as_ref() else {
-        return false;
-    };
-    name.display(function.symbol_pool()).to_string() == FOR_LOOP_UPDATE_ITER_FLAG
-}

third_party/move/tools/move-linter/src/utils.rs

@@ -2,8 +2,15 @@
 // SPDX-License-Identifier: Apache-2.0
 
 //! This module holds utility functions for the Move linter.
-
-use move_model::ast::{ExpData, Operation};
+use legacy_move_compiler::parser::syntax::FOR_LOOP_UPDATE_ITER_FLAG;
+use move_model::{
+    ast::{
+        ExpData,
+        ExpData::{IfElse, LocalVar, Loop, Sequence},
+        Operation,
+    },
+    model::FunctionEnv,
+};
 
 /// Returns `true` if two expressions represent the same simple access pattern.
 /// This compares nested `Select`, `Borrow`, and local variable references for structural equality.
@@ -33,3 +40,69 @@ pub(crate) fn is_simple_access_equal(expr1: &ExpData, expr2: &ExpData) -> bool {
         _ => false,
     }
 }
+
+/// Detects if the given Loop expression matches the pattern of a while loop.
+/// ```ignore
+/// loop {
+///     if (condition) {
+///         // loop body
+///     } else {
+///         break;
+///     }
+/// }
+/// ```
+///
+pub(crate) fn detect_while_loop(expr: &ExpData) -> bool {
+    let Loop(_, loop_body) = expr else {
+        return false;
+    };
+    match loop_body.as_ref() {
+        ExpData::IfElse(_, _, _, else_expr) => {
+            matches!(else_expr.as_ref(), ExpData::LoopCont(_, nest, is_continue) if *nest == 0 && !*is_continue)
+        },
+        _ => false,
+    }
+}
+
+/// Detects if the Loop expression matches the pattern of an expanded for loop.
+///
+/// The expanded for loop has this structure:
+/// ```ignore
+/// loop {
+///     if (true) {
+///         if (flag) {
+///             increment;
+///         } else {
+///             flag = true;
+///         }
+///         if (i < limit) {
+///             body;
+///         } else {
+///             break;
+///         }
+///     } else {
+///         break;
+///     }
+/// }
+/// ```
+pub(crate) fn detect_for_loop(expr: &ExpData, function: &FunctionEnv) -> bool {
+    let Loop(_, body) = expr else { return false };
+
+    let IfElse(_, _, then, _) = body.as_ref() else {
+        return false;
+    };
+
+    let Sequence(_, stmts) = then.as_ref() else {
+        return false;
+    };
+    let Some(stmt) = stmts.first() else {
+        return false;
+    };
+    let IfElse(_, cond, _, _) = stmt.as_ref() else {
+        return false;
+    };
+    let LocalVar(_, name) = cond.as_ref() else {
+        return false;
+    };
+    name.display(function.symbol_pool()).to_string() == FOR_LOOP_UPDATE_ITER_FLAG
+}