[move][move-linter] implement unnecessary while loop rules

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

@@ -8,6 +8,7 @@ use crate::{
     linters::constant_naming::ConstantNamingVisitor, typing::visitor::TypingVisitor,
 };
 pub mod constant_naming;
+mod unnecessary_while_loop;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum LintLevel {
@@ -34,26 +35,39 @@ pub const ALLOW_ATTR_CATEGORY: &str = "lint";
 pub const LINT_WARNING_PREFIX: &str = "Lint ";
 pub const CONSTANT_NAMING_FILTER_NAME: &str = "constant_naming";
 pub const CONSTANT_NAMING_DIAG_CODE: u8 = 1;
+pub const WHILE_TRUE_TO_LOOP_FILTER_NAME: &str = "while_true_to_loop";
+pub const WHILE_TRUE_TO_LOOP_DIAG_CODE: u8 = 4;
 
 pub fn known_filters() -> (Option<Symbol>, Vec<WarningFilter>) {
     (
         Some(ALLOW_ATTR_CATEGORY.into()),
-        vec![WarningFilter::code(
-            Some(LINT_WARNING_PREFIX),
-            LinterDiagnosticCategory::Style as u8,
-            CONSTANT_NAMING_DIAG_CODE,
-            Some(CONSTANT_NAMING_FILTER_NAME),
-        )],
+        vec![
+            WarningFilter::code(
+                Some(LINT_WARNING_PREFIX),
+                LinterDiagnosticCategory::Style as u8,
+                CONSTANT_NAMING_DIAG_CODE,
+                Some(CONSTANT_NAMING_FILTER_NAME),
+            ),
+            WarningFilter::code(
+                Some(LINT_WARNING_PREFIX),
+                LinterDiagnosticCategory::Complexity as u8,
+                WHILE_TRUE_TO_LOOP_DIAG_CODE,
+                Some(WHILE_TRUE_TO_LOOP_FILTER_NAME),
+            ),
+        ],
     )
 }
 
 pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
     match level {
         LintLevel::None | LintLevel::Default => vec![],
         LintLevel::All => {
-            vec![constant_naming::ConstantNamingVisitor::visitor(
-                ConstantNamingVisitor,
-            )]
+            vec![
+                constant_naming::ConstantNamingVisitor::visitor(ConstantNamingVisitor),
+                unnecessary_while_loop::WhileTrueToLoop::visitor(
+                    unnecessary_while_loop::WhileTrueToLoop,
+                ),
+            ]
         }
     }
 }

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

@@ -0,0 +1,72 @@
+//! Encourages replacing `while(true)` with `loop` for infinite loops in Move for clarity and conciseness.
+//! Identifies `while(true)` patterns, suggesting a more idiomatic approach using `loop`.
+//! Aims to enhance code readability and adherence to Rust idioms.
+use crate::{
+    diag,
+    diagnostics::{
+        codes::{custom, DiagnosticInfo, Severity},
+        WarningFilters,
+    },
+    expansion::ast::Value_,
+    shared::CompilationEnv,
+    typing::{
+        ast::{self as T, UnannotatedExp_},
+        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+    },
+};
+use move_ir_types::location::Loc;
+
+use super::{LinterDiagnosticCategory, LINT_WARNING_PREFIX, WHILE_TRUE_TO_LOOP_DIAG_CODE};
+
+const WHILE_TRUE_TO_LOOP_DIAG: DiagnosticInfo = custom(
+    LINT_WARNING_PREFIX,
+    Severity::Warning,
+    LinterDiagnosticCategory::Complexity as u8,
+    WHILE_TRUE_TO_LOOP_DIAG_CODE,
+    "Detected `while(true) {}` loop. Consider replacing with `loop {}`",
+);
+
+pub struct WhileTrueToLoop;
+
+pub struct Context<'a> {
+    env: &'a mut CompilationEnv,
+}
+
+impl TypingVisitorConstructor for WhileTrueToLoop {
+    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: &mut T::Exp) -> bool {
+        if let UnannotatedExp_::While(_, cond, _) = &exp.exp.value {
+            if is_condition_always_true(&cond.exp.value) {
+                let diag = diag!(
+                    WHILE_TRUE_TO_LOOP_DIAG,
+                    (exp.exp.loc, "`while (true)` can be replaced with `loop`")
+                );
+                self.env.add_diag(diag);
+            }
+        }
+        false
+    }
+}
+
+fn is_condition_always_true(condition: &UnannotatedExp_) -> bool {
+    if let UnannotatedExp_::Value(val) = condition {
+        if let Value_::Bool(b) = &val.value {
+            return *b;
+        }
+    }
+    false
+}

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

@@ -0,0 +1,9 @@
+module 0x42::loop_test {
+
+    public fun false_negative_obfuscated_true() {
+        let always_true = true;
+        while (always_true) {
+            // This should trigger the linter but might not due to indirection
+        }
+    }
+}

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

@@ -0,0 +1,12 @@
+module 0x42::loop_test {
+
+    public fun false_positive_complex_condition() {
+        while (complex_always_true_condition()) {
+            // This might trigger the linter if the condition is too complex to analyze
+        }
+    }
+
+   fun complex_always_true_condition(): bool {
+        true
+    }
+}

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

@@ -0,0 +1,12 @@
+module 0x42::loop_test {
+
+    #[allow(lint(while_true_to_loop))]
+    public fun suppressed_while_true() {
+        while (true) {
+            // This loop will run forever, but won't trigger the linter warning
+            if (false) {
+                break
+            }
+        }
+    }
+}

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

@@ -0,0 +1,17 @@
+module 0x42::loop_test {
+
+    // True Negative Cases
+    // These should not trigger the linter warning
+    public fun true_negative_correct_infinite_loop() {
+        loop {
+            // This is the correct way to write an infinite loop
+        }
+    }
+
+    public fun true_negative_while_with_condition(n: u64) {
+        let i = 0;
+        while (i < n) {
+            i = i + 1;
+        }
+    }
+}

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

@@ -0,0 +1,21 @@
+warning[Lint W01004]: Detected `while(true) {}` loop. Consider replacing with `loop {}`
+  ┌─ tests/linter/true_positive_unnecessary_while_loop.move:4:9
+  │  
+4 │ ╭         while (true) {
+5 │ │             // This should trigger the linter
+6 │ │         }
+  │ ╰─────────^ `while (true)` can be replaced with `loop`
+  │  
+  = This warning can be suppressed with '#[allow(lint(while_true_to_loop))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01004]: Detected `while(true) {}` loop. Consider replacing with `loop {}`
+   ┌─ tests/linter/true_positive_unnecessary_while_loop.move:11:9
+   │  
+11 │ ╭         while (true) {
+12 │ │             if (i == 10) break;
+13 │ │             i = i + 1;
+14 │ │         }
+   │ ╰─────────^ `while (true)` can be replaced with `loop`
+   │  
+   = This warning can be suppressed with '#[allow(lint(while_true_to_loop))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

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

@@ -0,0 +1,16 @@
+module 0x42::loop_test {
+    // This function should trigger the linter
+    public fun true_positive_infinite_loop() {
+        while (true) {
+            // This should trigger the linter
+        }
+    }
+
+    public fun true_positive_finite_loop() {
+        let i = 0;
+        while (true) {
+            if (i == 10) break;
+            i = i + 1;
+        }
+    }
+}

external-crates/move/crates/move-compiler/tests/move_check_testsuite.rs

@@ -302,4 +302,4 @@ pub fn run_test_inner(
     }
 }
 
-datatest_stable::harness!(move_check_testsuite, "tests/", r".*\.move$");
+datatest_stable::harness!(move_check_testsuite, "tests/linter", r".*\.move$");