From c4343a297240fe8c684efd815ffe01d953d6a2d2 Mon Sep 17 00:00:00 2001 From: Tom Cat <48447545+tx-tomcat@users.noreply.github.com> Date: Thu, 15 Aug 2024 06:01:33 +0700 Subject: [PATCH] [Linter] Unnecessary while loop (#16876) # Description This linter encourages replacing `while(true)` loops with the more idiomatic loop construct. Here's a breakdown of how it works: It checks each expression in the AST. If the expression is a While loop, it examines the condition. If the condition is always true (using the `is_condition_always_true` function), it reports a diagnostic suggesting to use loop instead. The `is_condition_always_true` function checks if the condition is a boolean literal with the value true. ## Test plan Added more use case including false positive, false negative case ## Release notes - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [X] CLI: Move will now lint against `while (true)`, which should be replaced by `loop` - [ ] Rust SDK: --------- Co-authored-by: jamedzung Co-authored-by: Todd Nowacki --- .../crates/move-compiler/src/linters/mod.rs | 32 ++++++--- .../src/linters/unnecessary_while_loop.rs | 68 +++++++++++++++++++ ...false_negative_unnecessary_while_loop.move | 11 +++ .../suppress_unnecessary_while_loop.move | 7 ++ .../true_negative_unnecessary_while_loop.move | 11 +++ .../true_positive_unnecessary_while_loop.exp | 18 +++++ .../true_positive_unnecessary_while_loop.move | 6 ++ 7 files changed, 144 insertions(+), 9 deletions(-) create mode 100644 external-crates/move/crates/move-compiler/src/linters/unnecessary_while_loop.rs create mode 100644 external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_while_loop.move create mode 100644 external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_while_loop.move create mode 100644 external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_while_loop.move create mode 100644 external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.exp create mode 100644 external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.move diff --git a/external-crates/move/crates/move-compiler/src/linters/mod.rs b/external-crates/move/crates/move-compiler/src/linters/mod.rs index 01ab4d3c2902f..0457c5d511f51 100644 --- a/external-crates/move/crates/move-compiler/src/linters/mod.rs +++ b/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,16 +35,26 @@ 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"; +pub const WHILE_TRUE_TO_LOOP_DIAG_CODE: u8 = 4; pub fn known_filters() -> (Option, Vec) { ( 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), + ), + ], ) } @@ -51,9 +62,12 @@ pub fn linter_visitors(level: LintLevel) -> Vec { 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, + ), + ] } } } diff --git a/external-crates/move/crates/move-compiler/src/linters/unnecessary_while_loop.rs b/external-crates/move/crates/move-compiler/src/linters/unnecessary_while_loop.rs new file mode 100644 index 0000000000000..0985a02e7b01c --- /dev/null +++ b/external-crates/move/crates/move-compiler/src/linters/unnecessary_while_loop.rs @@ -0,0 +1,68 @@ +//! 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 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, + "unnecessary 'while (true)', replace 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 { + let UnannotatedExp_::While(_, cond, _) = &exp.exp.value else { + return false; + }; + let UnannotatedExp_::Value(sp!(_, Value_::Bool(true))) = &cond.exp.value else { + return false; + }; + + let msg = "'while (true)' can be always replaced with 'loop'"; + let mut diag = diag!(WHILE_TRUE_TO_LOOP_DIAG, (exp.exp.loc, msg)); + diag.add_note( + "A 'loop' is more useful in these cases. Unlike 'while', 'loop' can have a \ + 'break' with a value, e.g. 'let x = loop { break 42 };'", + ); + self.env.add_diag(diag); + + false + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_while_loop.move b/external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_while_loop.move new file mode 100644 index 0000000000000..0f06f05bc8e51 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_while_loop.move @@ -0,0 +1,11 @@ +module 0x42::loop_test { + + // These should trigger but currently dont + public fun false_negative_obfuscated_true() { + let always_true = true; + while (always_true) {}; + while (true && true) {}; + while (true || false) {}; + while (1 > 0) {}; + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_while_loop.move b/external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_while_loop.move new file mode 100644 index 0000000000000..1477d966a74c3 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_while_loop.move @@ -0,0 +1,7 @@ +module 0x42::loop_test { + + #[allow(lint(while_true))] + public fun suppressed_while_true() { + while (true) {}; + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_while_loop.move b/external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_while_loop.move new file mode 100644 index 0000000000000..d5ad1e0cfb6f6 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_while_loop.move @@ -0,0 +1,11 @@ +module 0x42::loop_test { + + public fun true_negative_while_with_condition() { + let b = false; + while (false) {}; + while (b) {}; + while (false && true) {}; + while (false || false) {}; + while (0 > 1) {}; + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.exp b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.exp new file mode 100644 index 0000000000000..2ba9789c925b3 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.exp @@ -0,0 +1,18 @@ +warning[Lint W01004]: unnecessary 'while (true)', replace with 'loop' + ┌─ tests/linter/true_positive_unnecessary_while_loop.move:3:9 + │ +3 │ while (true) {}; + │ ^^^^^^^^^^^^^^^ 'while (true)' can be always replaced with 'loop' + │ + = A 'loop' is more useful in these cases. Unlike 'while', 'loop' can have a 'break' with a value, e.g. 'let x = loop { break 42 };' + = This warning can be suppressed with '#[allow(lint(while_true))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W01004]: unnecessary 'while (true)', replace with 'loop' + ┌─ tests/linter/true_positive_unnecessary_while_loop.move:4:9 + │ +4 │ while (true) { break } + │ ^^^^^^^^^^^^^^^^^^^^^^ 'while (true)' can be always replaced with 'loop' + │ + = A 'loop' is more useful in these cases. Unlike 'while', 'loop' can have a 'break' with a value, e.g. 'let x = loop { break 42 };' + = This warning can be suppressed with '#[allow(lint(while_true))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + diff --git a/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.move b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.move new file mode 100644 index 0000000000000..eaf03b83ba5e8 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_while_loop.move @@ -0,0 +1,6 @@ +module 0x42::loop_test { + public fun true_positive_infinite_loop() { + while (true) {}; + while (true) { break } + } +}