[Linter] 'loop' without exit (MystenLabs#16875)

# Description

- Gives a suspicious lint for loop without break or return.
- Does not currently consider if that break or return is reachable 

## Test plan

- New tests

## Release notes

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [X] CLI: Move lint will now warn against `loop`s without `break` or
`return`
- [ ] Rust SDK:

---------

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

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

@@ -0,0 +1,67 @@
+//! Detects empty loop expressions, including `while(true) {}` and `loop {}` without exit mechanisms, highlighting potential infinite loops.
+//! Aims to identify and warn against loops that may lead to hangs or excessive resource consumption due to lack of content.
+//! Encourages adding meaningful logic within loops or ensuring proper exit conditions to improve code reliability and maintainability.
+use super::StyleCodes;
+use crate::{
+    diag,
+    diagnostics::WarningFilters,
+    shared::CompilationEnv,
+    typing::{
+        ast::{self as T, UnannotatedExp_},
+        visitor::{has_special_exp, TypingVisitorConstructor, TypingVisitorContext},
+    },
+};
+
+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()
+    }
+
+    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`
+        let UnannotatedExp_::Loop {
+            name: _,
+            has_break: false,
+            body,
+        } = &exp.exp.value
+        else {
+            return false;
+        };
+        // TODO maybe move this to Loop? Bit of an n^2 problem here in the worst case
+        if has_return(body) {
+            return false;
+        }
+        let diag = diag!(
+            StyleCodes::LoopWithoutExit.diag_info(),
+            (
+                exp.exp.loc,
+                "'loop' without 'break' or 'return'. \
+                This code will until it errors, e.g. reaching an 'abort' or running out of gas"
+            )
+        );
+        self.env.add_diag(diag);
+        false
+    }
+}
+
+fn has_return(e: &T::Exp) -> bool {
+    has_special_exp(e, |e| matches!(e.exp.value, UnannotatedExp_::Return(_)))
+}

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

@@ -13,6 +13,7 @@ use crate::{
 
 pub mod abort_constant;
 pub mod constant_naming;
+pub mod loop_without_exit;
 pub mod meaningless_math_operation;
 pub mod unnecessary_while_loop;
 pub mod unneeded_return;
@@ -103,7 +104,7 @@ lints!(
     ),
     (
         WhileTrueToLoop,
-        LinterDiagnosticCategory::Complexity,
+        LinterDiagnosticCategory::Style,
         "while_true",
         "unnecessary 'while (true)', replace with 'loop'"
     ),
@@ -125,6 +126,12 @@ lints!(
         "abort_without_constant",
         "'abort' or 'assert' without named constant"
     ),
+    (
+        LoopWithoutExit,
+        LinterDiagnosticCategory::Suspicious,
+        "loop_without_exit",
+        "'loop' without 'break' or 'return'"
+    )
 );
 
 pub const ALLOW_ATTR_CATEGORY: &str = "lint";
@@ -157,6 +164,7 @@ pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
                 meaningless_math_operation::MeaninglessMathOperation.visitor(),
                 unneeded_return::UnneededReturnVisitor.visitor(),
                 abort_constant::AssertAbortNamedConstants.visitor(),
+                loop_without_exit::LoopWithoutExit.visitor(),
             ]
         }
     }

external-crates/move/crates/move-compiler/src/typing/visitor.rs

@@ -1096,3 +1096,107 @@ impl<V: TypingMutVisitorConstructor> TypingMutVisitor for V {
         self.visit(env, program)
     }
 }
+
+//**************************************************************************************************
+// util
+//**************************************************************************************************
+
+pub fn has_special_exp<F>(e: &T::Exp, mut p: F) -> bool
+where
+    F: FnMut(&T::Exp) -> bool,
+{
+    has_special_exp_(e, &mut p)
+}
+
+pub fn has_special_seq<F>(seq: &T::Sequence, mut p: F) -> bool
+where
+    F: FnMut(&T::Exp) -> bool,
+{
+    has_special_seq_(seq, &mut p)
+}
+
+pub fn has_special_exp_list<F>(list: &[T::ExpListItem], mut p: F) -> bool
+where
+    F: FnMut(&T::Exp) -> bool,
+{
+    has_special_exp_list_(list, &mut p)
+}
+
+#[growing_stack]
+fn has_special_exp_<F>(e: &T::Exp, p: &mut F) -> bool
+where
+    F: FnMut(&T::Exp) -> bool,
+{
+    use T::UnannotatedExp_ as E;
+    if p(e) {
+        return true;
+    }
+    match &e.exp.value {
+        E::Unit { .. }
+        | E::Value(_)
+        | E::Move { .. }
+        | E::Copy { .. }
+        | E::Use(_)
+        | E::Constant(..)
+        | E::Continue(_)
+        | E::BorrowLocal(..)
+        | E::ErrorConstant { .. }
+        | E::UnresolvedError => false,
+        E::Builtin(_, e)
+        | E::Vector(_, _, _, e)
+        | E::Loop { body: e, .. }
+        | E::Assign(_, _, e)
+        | E::Return(e)
+        | E::Abort(e)
+        | E::Give(_, e)
+        | E::Dereference(e)
+        | E::UnaryExp(_, e)
+        | E::Borrow(_, e, _)
+        | E::TempBorrow(_, e)
+        | E::Cast(e, _)
+        | E::Annotate(e, _) => has_special_exp_(e, p),
+        E::While(_, e1, e2) | E::Mutate(e1, e2) | E::BinopExp(e1, _, _, e2) => {
+            has_special_exp_(e1, p) || has_special_exp_(e2, p)
+        }
+        E::IfElse(e1, e2, e3) => {
+            has_special_exp_(e1, p) || has_special_exp_(e2, p) || has_special_exp_(e3, p)
+        }
+        E::ModuleCall(c) => has_special_exp_(&c.arguments, p),
+        E::Match(esubject, arms) => {
+            has_special_exp_(esubject, p)
+                || arms
+                    .value
+                    .iter()
+                    .any(|sp!(_, arm)| has_special_exp_(&arm.rhs, p))
+        }
+        E::VariantMatch(esubject, _, arms) => {
+            has_special_exp_(esubject, p) || arms.iter().any(|(_, arm)| has_special_exp_(arm, p))
+        }
+
+        E::NamedBlock(_, seq) | E::Block(seq) => has_special_seq_(seq, p),
+
+        E::Pack(_, _, _, fields) | E::PackVariant(_, _, _, _, fields) => fields
+            .iter()
+            .any(|(_, _, (_, (_, e)))| has_special_exp_(e, p)),
+        E::ExpList(list) => has_special_exp_list_(list, p),
+    }
+}
+
+fn has_special_seq_<F>(seq: &T::Sequence, p: &mut F) -> bool
+where
+    F: FnMut(&T::Exp) -> bool,
+{
+    seq.1.iter().any(|item| match &item.value {
+        T::SequenceItem_::Declare(_) => false,
+        T::SequenceItem_::Seq(e) | T::SequenceItem_::Bind(_, _, e) => has_special_exp_(e, p),
+    })
+}
+
+fn has_special_exp_list_<F>(list: &[T::ExpListItem], p: &mut F) -> bool
+where
+    F: FnMut(&T::Exp) -> bool,
+{
+    list.iter().any(|item| match item {
+        T::ExpListItem::Single(e, _) | T::ExpListItem::Splat(_, e, _) => has_special_exp_(e, p),
+    })
+}

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

@@ -0,0 +1,15 @@
+// tests for false negatives in the loop without exit lint
+
+module a::m {
+   public fun t1() {
+        loop {
+            if (false) break
+        }
+    }
+
+    public fun t2() {
+        loop {
+            if (false) return
+        }
+    }
+}

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

@@ -0,0 +1,7 @@
+module a::m {
+    // Suppress Case
+    #[allow(lint(loop_without_exit))]
+    public fun suppressed_empty_loop() {
+        loop {}
+    }
+}

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

@@ -1,7 +1,7 @@
-module 0x42::loop_test {
+module a::loop_test {
 
     #[allow(lint(while_true))]
     public fun suppressed_while_true() {
-        while (true) {};
+        while(true) {}
     }
 }

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

@@ -0,0 +1,19 @@
+// tests for negatives in the loop without exit lint
+
+module a::m {
+    public fun t1() {
+        let i = 0;
+        loop {
+            if (i >= 10) break;
+            i = i + 1;
+        }
+    }
+
+    public fun t2() {
+        let i = 0;
+        loop {
+            if (i >= 10) return;
+            i = i + 1;
+        }
+    }
+}

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

@@ -0,0 +1,27 @@
+warning[Lint W02006]: 'loop' without 'break' or 'return'
+  ┌─ tests/linter/true_positive_loop_without_exit.move:5:9
+  │
+5 │         loop {}
+  │         ^^^^^^^ 'loop' without 'break' or 'return'. This code will until it errors, e.g. reaching an 'abort' or running out of gas
+  │
+  = This warning can be suppressed with '#[allow(lint(loop_without_exit))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W02006]: 'loop' without 'break' or 'return'
+   ┌─ tests/linter/true_positive_loop_without_exit.move:10:9
+   │
+10 │         loop { abort ZERO }
+   │         ^^^^^^^^^^^^^^^^^^^ 'loop' without 'break' or 'return'. This code will until it errors, e.g. reaching an 'abort' or running out of gas
+   │
+   = This warning can be suppressed with '#[allow(lint(loop_without_exit))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W02006]: 'loop' without 'break' or 'return'
+   ┌─ tests/linter/true_positive_loop_without_exit.move:14:9
+   │  
+14 │ ╭         loop {
+15 │ │             t2();
+16 │ │             t2();
+17 │ │         }
+   │ ╰─────────^ 'loop' without 'break' or 'return'. This code will until it errors, e.g. reaching an 'abort' or running out of gas
+   │  
+   = This warning can be suppressed with '#[allow(lint(loop_without_exit))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

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

@@ -0,0 +1,19 @@
+// tests for positives in the loop without exit lint
+
+module a::m {
+    public fun t1() {
+        loop {}
+    }
+
+    const ZERO: u64 = 0;
+    public fun t2() {
+        loop { abort ZERO }
+    }
+
+    public fun t3() {
+        loop {
+            t2();
+            t2();
+        }
+    }
+}