From 4098c56e38faa5d52ebd9bdb36e59fbb7331c486 Mon Sep 17 00:00:00 2001 From: tx-tomcat Date: Fri, 31 May 2024 22:05:49 +0700 Subject: [PATCH] [move][move-linter] implemen meaningless math operator rules --- .../src/linters/constant_naming.rs | 4 +- .../src/linters/meaningless_math_operation.rs | 102 ++++++++++++++++ .../crates/move-compiler/src/linters/mod.rs | 44 ++++--- .../src/linters/shift_overflow.rs | 109 ------------------ .../move-compiler/src/sui_mode/linters/mod.rs | 5 +- .../tests/custom_rules/shift_overflow.exp | 24 ---- .../tests/custom_rules/shift_overflow.move | 9 -- .../correct_meaningless_math_operation.move | 10 ++ .../linter/incorrect_constant_naming.exp | 4 +- .../incorrect_meaningless_math_operation.exp | 32 +++++ .../incorrect_meaningless_math_operation.move | 27 +++++ .../tests/linter/suppressed_lints.move | 5 + 12 files changed, 210 insertions(+), 165 deletions(-) create mode 100644 external-crates/move/crates/move-compiler/src/linters/meaningless_math_operation.rs delete mode 100644 external-crates/move/crates/move-compiler/src/linters/shift_overflow.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/correct_meaningless_math_operation.move create mode 100644 external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.exp create mode 100644 external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.move diff --git a/external-crates/move/crates/move-compiler/src/linters/constant_naming.rs b/external-crates/move/crates/move-compiler/src/linters/constant_naming.rs index 99525fd2757602..7c8e3e6407e90f 100644 --- a/external-crates/move/crates/move-compiler/src/linters/constant_naming.rs +++ b/external-crates/move/crates/move-compiler/src/linters/constant_naming.rs @@ -19,13 +19,13 @@ use crate::{ }, }; -use super::{LinterDiagCategory, CONSTANT_NAMING_DIAG_CODE, LINT_WARNING_PREFIX}; +use super::{LinterDiagnosticCategory, CONSTANT_NAMING_DIAG_CODE, LINT_WARNING_PREFIX}; /// Diagnostic information for constant naming violations. const CONSTANT_NAMING_DIAG: DiagnosticInfo = custom( LINT_WARNING_PREFIX, Severity::Warning, - LinterDiagCategory::Style as u8, + LinterDiagnosticCategory::Style as u8, CONSTANT_NAMING_DIAG_CODE, "constant should follow naming convention", ); diff --git a/external-crates/move/crates/move-compiler/src/linters/meaningless_math_operation.rs b/external-crates/move/crates/move-compiler/src/linters/meaningless_math_operation.rs new file mode 100644 index 00000000000000..c334b43bfd12a2 --- /dev/null +++ b/external-crates/move/crates/move-compiler/src/linters/meaningless_math_operation.rs @@ -0,0 +1,102 @@ +//! Detects meaningless math operations like `x * 0`, `x << 0`, `x >> 0`, `x * 1`, `x + 0`, `x - 0`, and `x / 0`. +//! Aims to reduce code redundancy and improve clarity by flagging operations with no effect. +use crate::{ + diag, + diagnostics::{ + codes::{custom, DiagnosticInfo, Severity}, + WarningFilters, + }, + expansion::ast::Value_, + parser::ast::BinOp_, + shared::CompilationEnv, + typing::{ + ast::{self as T, Exp, UnannotatedExp_}, + visitor::{TypingVisitorConstructor, TypingVisitorContext}, + }, +}; +use move_ir_types::location::Loc; + +use super::{LinterDiagnosticCategory, LINT_WARNING_PREFIX, MEANINGLESS_MATH_OP_DIAG_CODE}; + +const MEANINGLESS_MATH_OP_DIAG: DiagnosticInfo = custom( + LINT_WARNING_PREFIX, + Severity::Warning, + LinterDiagnosticCategory::Complexity as u8, + MEANINGLESS_MATH_OP_DIAG_CODE, + "Detected a meaningless math operation that has no effect.", +); + +pub struct MeaninglessMathOperation; + +pub struct Context<'a> { + env: &'a mut CompilationEnv, +} + +impl TypingVisitorConstructor for MeaninglessMathOperation { + 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 visit_exp_custom(&mut self, exp: &mut T::Exp) -> bool { + if let UnannotatedExp_::BinopExp(_, op, _, rhs) = &exp.exp.value { + match op.value { + BinOp_::Mul | BinOp_::Div => { + if is_zero(&rhs) || (matches!(op.value, BinOp_::Mul) && is_one(&rhs)) { + report_meaningless_math_op(self.env, op.loc); + } + } + BinOp_::Add | BinOp_::Sub => { + if is_zero(&rhs) { + report_meaningless_math_op(self.env, op.loc); + } + } + BinOp_::Shl | BinOp_::Shr => { + if is_zero(&rhs) { + report_meaningless_math_op(self.env, 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 is_zero(exp: &Exp) -> bool { + if let UnannotatedExp_::Value(spanned) = &exp.exp.value { + matches!(spanned.value, Value_::U64(0)) + } else { + false + } +} + +fn is_one(exp: &Exp) -> bool { + if let UnannotatedExp_::Value(spanned) = &exp.exp.value { + matches!(spanned.value, Value_::U64(1)) + } else { + false + } +} + +fn report_meaningless_math_op(env: &mut CompilationEnv, loc: Loc) { + let diag = diag!( + MEANINGLESS_MATH_OP_DIAG, + ( + loc, + "Detected a meaningless math operation that has no effect.", + ) + ); + env.add_diag(diag); +} 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 0baa1323936c1c..dd85fbce5659b2 100644 --- a/external-crates/move/crates/move-compiler/src/linters/mod.rs +++ b/external-crates/move/crates/move-compiler/src/linters/mod.rs @@ -1,13 +1,16 @@ // Copyright (c) The Move Contributors // SPDX-License-Identifier: Apache-2.0 -use move_symbol_pool::Symbol; - use crate::{ command_line::compiler::Visitor, diagnostics::codes::WarningFilter, - linters::constant_naming::ConstantNamingVisitor, typing::visitor::TypingVisitor, + linters::constant_naming::ConstantNamingVisitor, + linters::meaningless_math_operation::MeaninglessMathOperation, typing::visitor::TypingVisitor, }; +use move_symbol_pool::Symbol; + pub mod constant_naming; +pub mod meaningless_math_operation; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum LintLevel { // No linters @@ -32,22 +35,28 @@ pub enum LinterDiagnosticCategory { 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 enum LinterDiagCategory { - Style, -} +pub const MEANINGLESS_MATH_OP_FILTER_NAME: &str = "meaningless_math_op"; +pub const MEANINGLESS_MATH_OP_DIAG_CODE: u8 = 6; pub fn known_filters() -> (Option, Vec) { ( Some(ALLOW_ATTR_CATEGORY.into()), - vec![WarningFilter::code( - Some(LINT_WARNING_PREFIX), - LinterDiagCategory::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, + MEANINGLESS_MATH_OP_DIAG_CODE, + Some(MEANINGLESS_MATH_OP_FILTER_NAME), + ), + ], ) } @@ -56,9 +65,12 @@ pub fn linter_visitors(level: LintLevel) -> Vec { LintLevel::None => vec![], LintLevel::Default => vec![], LintLevel::All => { - vec![constant_naming::ConstantNamingVisitor::visitor( - ConstantNamingVisitor, - )] + vec![ + constant_naming::ConstantNamingVisitor::visitor(ConstantNamingVisitor), + meaningless_math_operation::MeaninglessMathOperation::visitor( + MeaninglessMathOperation, + ), + ] } } } 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 a66d97525fb8e1..00000000000000 --- 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/sui_mode/linters/mod.rs b/external-crates/move/crates/move-compiler/src/sui_mode/linters/mod.rs index 012d78fc852fb2..53bc614a56dc5a 100644 --- a/external-crates/move/crates/move-compiler/src/sui_mode/linters/mod.rs +++ b/external-crates/move/crates/move-compiler/src/sui_mode/linters/mod.rs @@ -13,7 +13,6 @@ use crate::{ }; use move_ir_types::location::Loc; use move_symbol_pool::Symbol; - pub mod coin_field; pub mod collection_equality; pub mod custom_state_change; @@ -21,8 +20,8 @@ pub mod freeze_wrapped; pub mod public_random; pub mod self_transfer; pub mod share_owned; - pub const SUI_PKG_NAME: &str = "sui"; +pub const INCLUDE_NEW_RULES: bool = true; pub const TRANSFER_MOD_NAME: &str = "transfer"; pub const TRANSFER_FUN: &str = "transfer"; @@ -87,7 +86,7 @@ pub enum LinterDiagnosticCode { } pub fn known_filters() -> (Option, Vec) { - let filters = vec![ + let mut filters = vec![ WarningFilter::All(Some(LINT_WARNING_PREFIX)), WarningFilter::code( Some(LINT_WARNING_PREFIX), 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 ed6efd6a2ea675..00000000000000 --- 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 e5dc71e6f6563e..00000000000000 --- 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/correct_meaningless_math_operation.move b/external-crates/move/crates/move-compiler/tests/linter/correct_meaningless_math_operation.move new file mode 100644 index 00000000000000..6da16413756080 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/correct_meaningless_math_operation.move @@ -0,0 +1,10 @@ +module 0x42::M { + // Intentionally meaningful operations for control cases + public fun multiply(x: u64, y: u64): u64 { + x * y // This should not trigger the linter + } + + public fun divide_by_nonzero(x: u64, y: u64): u64 { + x / y // Assuming y is not zero, this should not trigger the linter + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/incorrect_constant_naming.exp b/external-crates/move/crates/move-compiler/tests/linter/incorrect_constant_naming.exp index 763e36f065ba38..944dd5bdad57fe 100644 --- a/external-crates/move/crates/move-compiler/tests/linter/incorrect_constant_naming.exp +++ b/external-crates/move/crates/move-compiler/tests/linter/incorrect_constant_naming.exp @@ -1,4 +1,4 @@ -warning[Lint W00001]: constant should follow naming convention +warning[Lint W04001]: constant should follow naming convention ┌─ tests/linter/incorrect_constant_naming.move:3:5 │ 3 │ const Another_BadName: u64 = 42; // Should trigger a warning @@ -6,7 +6,7 @@ warning[Lint W00001]: constant should follow naming convention │ = This warning can be suppressed with '#[allow(lint(constant_naming))]' applied to the 'module' or module member ('const', 'fun', or 'struct') -warning[Lint W00001]: constant should follow naming convention +warning[Lint W04001]: constant should follow naming convention ┌─ tests/linter/incorrect_constant_naming.move:4:5 │ 4 │ const JSON_Max_Size: u64 = 1048576; diff --git a/external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.exp b/external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.exp new file mode 100644 index 00000000000000..394ae41f00199d --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.exp @@ -0,0 +1,32 @@ +warning[Lint W01006]: Detected a meaningless math operation that has no effect. + ┌─ tests/linter/incorrect_meaningless_math_operation.move:4:11 + │ +4 │ x * 0 // This should trigger the linter + │ ^ Detected a meaningless math operation that has no effect. + │ + = This warning can be suppressed with '#[allow(lint(meaningless_math_op))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W01006]: Detected a meaningless math operation that has no effect. + ┌─ tests/linter/incorrect_meaningless_math_operation.move:16:11 + │ +16 │ x * 1 // This should trigger the linter + │ ^ Detected a meaningless math operation that has no effect. + │ + = This warning can be suppressed with '#[allow(lint(meaningless_math_op))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W01006]: Detected a meaningless math operation that has no effect. + ┌─ tests/linter/incorrect_meaningless_math_operation.move:20:11 + │ +20 │ x + 0 // This should trigger the linter + │ ^ Detected a meaningless math operation that has no effect. + │ + = This warning can be suppressed with '#[allow(lint(meaningless_math_op))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W01006]: Detected a meaningless math operation that has no effect. + ┌─ tests/linter/incorrect_meaningless_math_operation.move:24:11 + │ +24 │ x - 0 // This should trigger the linter + │ ^ Detected a meaningless math operation that has no effect. + │ + = This warning can be suppressed with '#[allow(lint(meaningless_math_op))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + diff --git a/external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.move b/external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.move new file mode 100644 index 00000000000000..6f275ab9eed1ba --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/incorrect_meaningless_math_operation.move @@ -0,0 +1,27 @@ +module 0x42::M { + + public fun multiply_by_zero(x: u64): u64 { + x * 0 // This should trigger the linter + } + + public fun left_shift_by_zero(x: u64): u64 { + x << 0 // This should trigger the linter + } + + public fun right_shift_by_zero(x: u64): u64 { + x >> 0 // This should trigger the linter + } + + public fun multiply_by_one(x: u64): u64 { + x * 1 // This should trigger the linter + } + + public fun add_zero(x: u64): u64 { + x + 0 // This should trigger the linter + } + + public fun subtract_zero(x: u64): u64 { + x - 0 // This should trigger the linter + } + +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/suppressed_lints.move b/external-crates/move/crates/move-compiler/tests/linter/suppressed_lints.move index a75210a0364a70..b24d8b6f0b899d 100644 --- a/external-crates/move/crates/move-compiler/tests/linter/suppressed_lints.move +++ b/external-crates/move/crates/move-compiler/tests/linter/suppressed_lints.move @@ -2,4 +2,9 @@ module 0x42::M { #[allow(lint(constant_naming))] const Another_BadName: u64 = 42; // Should trigger a warning + + #[allow(lint(meaningless_math_op))] + public fun subtract_zero(x: u64): u64 { + x - 0 // This should trigger the linter + } }