From 0c7e7e2a6a79866689c3d2eed514aae92563329c Mon Sep 17 00:00:00 2001 From: jamedzung Date: Sat, 23 Mar 2024 01:43:33 +0700 Subject: [PATCH 1/3] implement rules --- .../src/linters/shift_overflow.rs | 109 ++++++++++++++++++ .../tests/custom_rules/shift_overflow.exp | 24 ++++ .../tests/custom_rules/shift_overflow.move | 9 ++ 3 files changed, 142 insertions(+) create mode 100644 external-crates/move/crates/move-compiler/src/linters/shift_overflow.rs create mode 100644 external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp create mode 100644 external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move diff --git a/external-crates/move/crates/move-compiler/src/linters/shift_overflow.rs b/external-crates/move/crates/move-compiler/src/linters/shift_overflow.rs new file mode 100644 index 0000000000000..a66d97525fb8e --- /dev/null +++ b/external-crates/move/crates/move-compiler/src/linters/shift_overflow.rs @@ -0,0 +1,109 @@ +//! Detect potential overflow scenarios where the number of bits being shifted exceeds the bit width of +//! the variable being shifted, which could lead to unintended behavior or loss of data. If such a +//! potential overflow is detected, a warning is generated to alert the developer. +use crate::{ + diag, + diagnostics::{ + codes::{custom, DiagnosticInfo, Severity}, + WarningFilters, + }, + expansion::ast::Value_, + naming::ast::{BuiltinTypeName_, TypeName_, Type_}, + parser::ast::BinOp_, + shared::{program_info::TypingProgramInfo, CompilationEnv}, + typing::{ + ast::{self as T, UnannotatedExp_}, + visitor::{TypingVisitorConstructor, TypingVisitorContext}, + }, +}; +use move_ir_types::location::Loc; +use std::str::FromStr; + +use super::{LinterDiagCategory, LINTER_DEFAULT_DIAG_CODE, LINT_WARNING_PREFIX}; + +const SHIFT_OPERATION_OVERFLOW_DIAG: DiagnosticInfo = custom( + LINT_WARNING_PREFIX, + Severity::Warning, + LinterDiagCategory::ShiftOperationOverflow as u8, + LINTER_DEFAULT_DIAG_CODE, + "Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted.", +); + +pub struct ShiftOperationOverflow; + +pub struct Context<'a> { + env: &'a mut CompilationEnv, +} + +impl TypingVisitorConstructor for ShiftOperationOverflow { + type Context<'a> = Context<'a>; + + fn context<'a>( + env: &'a mut CompilationEnv, + _program_info: &'a TypingProgramInfo, + _program: &T::Program_, + ) -> Self::Context<'a> { + Context { env } + } +} + +impl TypingVisitorContext for Context<'_> { + fn visit_exp_custom(&mut self, exp: &mut T::Exp) -> bool { + // Check if the expression is a binary operation and if it is a shift operation. + if let UnannotatedExp_::BinopExp(lhs, op, _, rhs) = &exp.exp.value { + // can't do let UnannotatedExp_::BinopExp(lhs, BinOp_::Shl | BinOp_::Shr, _, rhs) = &exp.exp.value else { return }; + // because the op is Spanned and not BinOp_ + if matches!(op.value, BinOp_::Shl | BinOp_::Shr) { + match ( + get_bit_width(&lhs.ty.value), + get_shift_amount(&rhs.exp.value), + ) { + (Some(bit_width), Some(shift_amount)) if shift_amount >= bit_width => { + report_overflow(self.env, shift_amount, bit_width, op.loc); + } + _ => (), + } + } + } + false + } + 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 get_bit_width(ty: &Type_) -> Option { + ty.builtin_name().and_then(|typ| match typ.value { + BuiltinTypeName_::U8 => Some(8), + BuiltinTypeName_::U16 => Some(16), + BuiltinTypeName_::U32 => Some(32), + BuiltinTypeName_::U64 => Some(64), + BuiltinTypeName_::U128 => Some(128), + BuiltinTypeName_::U256 => Some(256), + _ => None, + }) +} + +fn get_shift_amount(value: &UnannotatedExp_) -> Option { + if let UnannotatedExp_::Value(v) = value { + match &v.value { + Value_::U8(v) => Some(*v as u128), + _ => None, + } + } else { + None + } +} + +fn report_overflow(env: &mut CompilationEnv, shift_amount: u128, bit_width: u128, loc: Loc) { + let msg = format!( + "The {} of bits being shifted exceeds the {} bit width of the variable being shifted.", + shift_amount, bit_width + ); + let diag = diag!(SHIFT_OPERATION_OVERFLOW_DIAG, (loc, msg)); + env.add_diag(diag); +} diff --git a/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp b/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp new file mode 100644 index 0000000000000..ed6efd6a2ea67 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp @@ -0,0 +1,24 @@ +warning[Lint W00001]: Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted. + ┌─ tests/custom_rules/shift_overflow.move:5:20 + │ +5 │ let _b = x << 64; // Should not raise an issue + │ ^^ The 64 of bits being shifted exceeds the 64 bit width of the variable being shifted. + │ + = This warning can be suppressed with '#[allow(lint(shift_overflow))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W00001]: Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted. + ┌─ tests/custom_rules/shift_overflow.move:6:20 + │ +6 │ let _b = x << 65; // Should raise an issue + │ ^^ The 65 of bits being shifted exceeds the 64 bit width of the variable being shifted. + │ + = This warning can be suppressed with '#[allow(lint(shift_overflow))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W00001]: Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted. + ┌─ tests/custom_rules/shift_overflow.move:7:20 + │ +7 │ let _b = x >> 66; // Should raise an issue + │ ^^ The 66 of bits being shifted exceeds the 64 bit width of the variable being shifted. + │ + = This warning can be suppressed with '#[allow(lint(shift_overflow))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + diff --git a/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move b/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move new file mode 100644 index 0000000000000..e5dc71e6f6563 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move @@ -0,0 +1,9 @@ +module 0x42::M { + + fun func1(x: u64) { + let _b = x << 24; + let _b = x << 64; // Should raise an issue + let _b = x << 65; // Should raise an issue + let _b = x >> 66; // Should raise an issue + } +} \ No newline at end of file From c6308a696feac81b631b8fad78aa105181929ac2 Mon Sep 17 00:00:00 2001 From: tx-tomcat Date: Sat, 1 Jun 2024 00:18:19 +0700 Subject: [PATCH 2/3] [move][move-linter] implement unnecessary while loop rules --- .../crates/move-compiler/src/linters/mod.rs | 32 +++-- .../src/linters/shift_overflow.rs | 109 ------------------ .../src/linters/unnecessary_while_loop.rs | 72 ++++++++++++ .../tests/custom_rules/shift_overflow.exp | 24 ---- .../tests/custom_rules/shift_overflow.move | 9 -- ...false_negative_unnecessary_while_loop.move | 9 ++ ...false_positive_unnecessary_while_loop.move | 12 ++ .../suppress_unnecessary_while_loop.move | 12 ++ .../true_negative_unnecessary_while_loop.move | 17 +++ .../true_positive_unnecessary_while_loop.exp | 21 ++++ .../true_positive_unnecessary_while_loop.move | 16 +++ 11 files changed, 182 insertions(+), 151 deletions(-) delete mode 100644 external-crates/move/crates/move-compiler/src/linters/shift_overflow.rs create mode 100644 external-crates/move/crates/move-compiler/src/linters/unnecessary_while_loop.rs delete mode 100644 external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp delete mode 100644 external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move 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/false_positive_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..49a516fe8bff8 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_to_loop"; +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/shift_overflow.rs b/external-crates/move/crates/move-compiler/src/linters/shift_overflow.rs deleted file mode 100644 index a66d97525fb8e..0000000000000 --- a/external-crates/move/crates/move-compiler/src/linters/shift_overflow.rs +++ /dev/null @@ -1,109 +0,0 @@ -//! Detect potential overflow scenarios where the number of bits being shifted exceeds the bit width of -//! the variable being shifted, which could lead to unintended behavior or loss of data. If such a -//! potential overflow is detected, a warning is generated to alert the developer. -use crate::{ - diag, - diagnostics::{ - codes::{custom, DiagnosticInfo, Severity}, - WarningFilters, - }, - expansion::ast::Value_, - naming::ast::{BuiltinTypeName_, TypeName_, Type_}, - parser::ast::BinOp_, - shared::{program_info::TypingProgramInfo, CompilationEnv}, - typing::{ - ast::{self as T, UnannotatedExp_}, - visitor::{TypingVisitorConstructor, TypingVisitorContext}, - }, -}; -use move_ir_types::location::Loc; -use std::str::FromStr; - -use super::{LinterDiagCategory, LINTER_DEFAULT_DIAG_CODE, LINT_WARNING_PREFIX}; - -const SHIFT_OPERATION_OVERFLOW_DIAG: DiagnosticInfo = custom( - LINT_WARNING_PREFIX, - Severity::Warning, - LinterDiagCategory::ShiftOperationOverflow as u8, - LINTER_DEFAULT_DIAG_CODE, - "Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted.", -); - -pub struct ShiftOperationOverflow; - -pub struct Context<'a> { - env: &'a mut CompilationEnv, -} - -impl TypingVisitorConstructor for ShiftOperationOverflow { - type Context<'a> = Context<'a>; - - fn context<'a>( - env: &'a mut CompilationEnv, - _program_info: &'a TypingProgramInfo, - _program: &T::Program_, - ) -> Self::Context<'a> { - Context { env } - } -} - -impl TypingVisitorContext for Context<'_> { - fn visit_exp_custom(&mut self, exp: &mut T::Exp) -> bool { - // Check if the expression is a binary operation and if it is a shift operation. - if let UnannotatedExp_::BinopExp(lhs, op, _, rhs) = &exp.exp.value { - // can't do let UnannotatedExp_::BinopExp(lhs, BinOp_::Shl | BinOp_::Shr, _, rhs) = &exp.exp.value else { return }; - // because the op is Spanned and not BinOp_ - if matches!(op.value, BinOp_::Shl | BinOp_::Shr) { - match ( - get_bit_width(&lhs.ty.value), - get_shift_amount(&rhs.exp.value), - ) { - (Some(bit_width), Some(shift_amount)) if shift_amount >= bit_width => { - report_overflow(self.env, shift_amount, bit_width, op.loc); - } - _ => (), - } - } - } - false - } - 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 get_bit_width(ty: &Type_) -> Option { - ty.builtin_name().and_then(|typ| match typ.value { - BuiltinTypeName_::U8 => Some(8), - BuiltinTypeName_::U16 => Some(16), - BuiltinTypeName_::U32 => Some(32), - BuiltinTypeName_::U64 => Some(64), - BuiltinTypeName_::U128 => Some(128), - BuiltinTypeName_::U256 => Some(256), - _ => None, - }) -} - -fn get_shift_amount(value: &UnannotatedExp_) -> Option { - if let UnannotatedExp_::Value(v) = value { - match &v.value { - Value_::U8(v) => Some(*v as u128), - _ => None, - } - } else { - None - } -} - -fn report_overflow(env: &mut CompilationEnv, shift_amount: u128, bit_width: u128, loc: Loc) { - let msg = format!( - "The {} of bits being shifted exceeds the {} bit width of the variable being shifted.", - shift_amount, bit_width - ); - let diag = diag!(SHIFT_OPERATION_OVERFLOW_DIAG, (loc, msg)); - env.add_diag(diag); -} 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..4ddeeacce816d --- /dev/null +++ b/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 +} diff --git a/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp b/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp deleted file mode 100644 index ed6efd6a2ea67..0000000000000 --- a/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.exp +++ /dev/null @@ -1,24 +0,0 @@ -warning[Lint W00001]: Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted. - ┌─ tests/custom_rules/shift_overflow.move:5:20 - │ -5 │ let _b = x << 64; // Should not raise an issue - │ ^^ The 64 of bits being shifted exceeds the 64 bit width of the variable being shifted. - │ - = This warning can be suppressed with '#[allow(lint(shift_overflow))]' applied to the 'module' or module member ('const', 'fun', or 'struct') - -warning[Lint W00001]: Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted. - ┌─ tests/custom_rules/shift_overflow.move:6:20 - │ -6 │ let _b = x << 65; // Should raise an issue - │ ^^ The 65 of bits being shifted exceeds the 64 bit width of the variable being shifted. - │ - = This warning can be suppressed with '#[allow(lint(shift_overflow))]' applied to the 'module' or module member ('const', 'fun', or 'struct') - -warning[Lint W00001]: Potential overflow detected. The number of bits being shifted exceeds the bit width of the variable being shifted. - ┌─ tests/custom_rules/shift_overflow.move:7:20 - │ -7 │ let _b = x >> 66; // Should raise an issue - │ ^^ The 66 of bits being shifted exceeds the 64 bit width of the variable being shifted. - │ - = This warning can be suppressed with '#[allow(lint(shift_overflow))]' applied to the 'module' or module member ('const', 'fun', or 'struct') - diff --git a/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move b/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move deleted file mode 100644 index e5dc71e6f6563..0000000000000 --- a/external-crates/move/crates/move-compiler/tests/custom_rules/shift_overflow.move +++ /dev/null @@ -1,9 +0,0 @@ -module 0x42::M { - - fun func1(x: u64) { - let _b = x << 24; - let _b = x << 64; // Should raise an issue - let _b = x << 65; // Should raise an issue - let _b = x >> 66; // Should raise an issue - } -} \ No newline at end of file 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..d6a5f92dae79a --- /dev/null +++ b/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 + } + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/false_positive_unnecessary_while_loop.move b/external-crates/move/crates/move-compiler/tests/linter/false_positive_unnecessary_while_loop.move new file mode 100644 index 0000000000000..8ecd347a5bf67 --- /dev/null +++ b/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 + } +} 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..6215fd74360f6 --- /dev/null +++ b/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 + } + } + } +} 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..059214b8336da --- /dev/null +++ b/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; + } + } +} 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..bf636284b39e8 --- /dev/null +++ b/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') + 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..d306929ac8c81 --- /dev/null +++ b/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; + } + } +} From 3814b1f82712af796bca8ca590c37c9c218574af Mon Sep 17 00:00:00 2001 From: Todd Nowacki Date: Wed, 14 Aug 2024 10:42:46 -0700 Subject: [PATCH 3/3] small fixes --- .../crates/move-compiler/src/linters/mod.rs | 2 +- .../src/linters/unnecessary_while_loop.rs | 36 +++++++++---------- ...false_negative_unnecessary_while_loop.move | 8 +++-- ...false_positive_unnecessary_while_loop.move | 12 ------- .../suppress_unnecessary_while_loop.move | 9 ++--- .../true_negative_unnecessary_while_loop.move | 20 ++++------- .../true_positive_unnecessary_while_loop.exp | 35 +++++++++--------- .../true_positive_unnecessary_while_loop.move | 14 ++------ 8 files changed, 49 insertions(+), 87 deletions(-) delete mode 100644 external-crates/move/crates/move-compiler/tests/linter/false_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 49a516fe8bff8..0457c5d511f51 100644 --- a/external-crates/move/crates/move-compiler/src/linters/mod.rs +++ b/external-crates/move/crates/move-compiler/src/linters/mod.rs @@ -35,7 +35,7 @@ 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_FILTER_NAME: &str = "while_true"; pub const WHILE_TRUE_TO_LOOP_DIAG_CODE: u8 = 4; pub fn known_filters() -> (Option, Vec) { 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 index 4ddeeacce816d..0985a02e7b01c 100644 --- 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 @@ -14,7 +14,6 @@ use crate::{ visitor::{TypingVisitorConstructor, TypingVisitorContext}, }, }; -use move_ir_types::location::Loc; use super::{LinterDiagnosticCategory, LINT_WARNING_PREFIX, WHILE_TRUE_TO_LOOP_DIAG_CODE}; @@ -23,7 +22,7 @@ const WHILE_TRUE_TO_LOOP_DIAG: DiagnosticInfo = custom( Severity::Warning, LinterDiagnosticCategory::Complexity as u8, WHILE_TRUE_TO_LOOP_DIAG_CODE, - "Detected `while(true) {}` loop. Consider replacing with `loop {}`", + "unnecessary 'while (true)', replace with 'loop'", ); pub struct WhileTrueToLoop; @@ -49,24 +48,21 @@ impl TypingVisitorContext for Context<'_> { } 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 - } -} + let UnannotatedExp_::While(_, cond, _) = &exp.exp.value else { + return false; + }; + let UnannotatedExp_::Value(sp!(_, Value_::Bool(true))) = &cond.exp.value else { + return false; + }; -fn is_condition_always_true(condition: &UnannotatedExp_) -> bool { - if let UnannotatedExp_::Value(val) = condition { - if let Value_::Bool(b) = &val.value { - return *b; - } + 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 } - 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 index d6a5f92dae79a..0f06f05bc8e51 100644 --- 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 @@ -1,9 +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) { - // This should trigger the linter but might not due to indirection - } + while (always_true) {}; + while (true && true) {}; + while (true || false) {}; + while (1 > 0) {}; } } diff --git a/external-crates/move/crates/move-compiler/tests/linter/false_positive_unnecessary_while_loop.move b/external-crates/move/crates/move-compiler/tests/linter/false_positive_unnecessary_while_loop.move deleted file mode 100644 index 8ecd347a5bf67..0000000000000 --- a/external-crates/move/crates/move-compiler/tests/linter/false_positive_unnecessary_while_loop.move +++ /dev/null @@ -1,12 +0,0 @@ -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 - } -} 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 index 6215fd74360f6..1477d966a74c3 100644 --- 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 @@ -1,12 +1,7 @@ module 0x42::loop_test { - #[allow(lint(while_true_to_loop))] + #[allow(lint(while_true))] public fun suppressed_while_true() { - while (true) { - // This loop will run forever, but won't trigger the linter warning - if (false) { - break - } - } + 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 index 059214b8336da..d5ad1e0cfb6f6 100644 --- 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 @@ -1,17 +1,11 @@ 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; - } + 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 index bf636284b39e8..2ba9789c925b3 100644 --- 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 @@ -1,21 +1,18 @@ -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]: 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]: 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') +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 index d306929ac8c81..eaf03b83ba5e8 100644 --- 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 @@ -1,16 +1,6 @@ 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; - } + while (true) {}; + while (true) { break } } }