diff --git a/external-crates/move/crates/move-analyzer/src/analysis/typing_analysis.rs b/external-crates/move/crates/move-analyzer/src/analysis/typing_analysis.rs index e22321ddf20ac..1c97aed29d7e8 100644 --- a/external-crates/move/crates/move-analyzer/src/analysis/typing_analysis.rs +++ b/external-crates/move/crates/move-analyzer/src/analysis/typing_analysis.rs @@ -907,7 +907,7 @@ impl<'a> TypingVisitorContext for TypingAnalysisContext<'a> { match &fdef.body.value { T::FunctionBody_::Defined(seq) => { - self.visit_seq(seq); + self.visit_seq(fdef.body.loc, seq); } T::FunctionBody_::Macro | T::FunctionBody_::Native => (), } @@ -985,7 +985,7 @@ impl<'a> TypingVisitorContext for TypingAnalysisContext<'a> { } } - fn visit_seq(&mut self, (use_funs, seq): &T::Sequence) { + fn visit_seq(&mut self, _loc: Loc, (use_funs, seq): &T::Sequence) { let old_traverse_mode = self.traverse_only; // start adding new use-defs etc. when processing arguments if use_funs.color == 0 { 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 1aaf95a668c27..85a21836fc7aa 100644 --- a/external-crates/move/crates/move-compiler/src/linters/mod.rs +++ b/external-crates/move/crates/move-compiler/src/linters/mod.rs @@ -18,6 +18,7 @@ pub mod meaningless_math_operation; pub mod redundant_ref_deref; pub mod self_assignment; pub mod unnecessary_conditional; +pub mod unnecessary_unit; pub mod unnecessary_while_loop; pub mod unneeded_return; @@ -152,7 +153,13 @@ lints!( LinterDiagnosticCategory::Complexity, "redundant_ref_deref", "redundant reference/dereference" - ) + ), + ( + UnnecessaryUnit, + LinterDiagnosticCategory::Style, + "unnecessary_unit", + "unit `()` expression can be removed or simplified" + ), ); pub const ALLOW_ATTR_CATEGORY: &str = "lint"; @@ -189,6 +196,7 @@ pub fn linter_visitors(level: LintLevel) -> Vec { unnecessary_conditional::UnnecessaryConditional.visitor(), self_assignment::SelfAssignmentVisitor.visitor(), redundant_ref_deref::RedundantRefDerefVisitor.visitor(), + unnecessary_unit::UnnecessaryUnit.visitor(), ] } } diff --git a/external-crates/move/crates/move-compiler/src/linters/unnecessary_unit.rs b/external-crates/move/crates/move-compiler/src/linters/unnecessary_unit.rs new file mode 100644 index 0000000000000..affda52627fca --- /dev/null +++ b/external-crates/move/crates/move-compiler/src/linters/unnecessary_unit.rs @@ -0,0 +1,128 @@ +//! Detects an unnecessary unit expression in a block, sequence, if, or else. + +use crate::{ + diag, + diagnostics::WarningFilters, + ice, + linters::StyleCodes, + shared::CompilationEnv, + typing::{ + ast::{self as T, SequenceItem_, UnannotatedExp_}, + visitor::{TypingVisitorConstructor, TypingVisitorContext}, + }, +}; +use move_ir_types::location::Loc; + +pub struct UnnecessaryUnit; + +pub struct Context<'a> { + env: &'a mut CompilationEnv, +} + +impl TypingVisitorConstructor for UnnecessaryUnit { + 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_seq_custom(&mut self, loc: Loc, (_, seq_): &T::Sequence) -> bool { + let n = seq_.len(); + match n { + 0 => { + self.env + .add_diag(ice!((loc, "Unexpected empty block without a value"))); + } + 1 => { + // TODO probably too noisy for now, we would need more information about + // blocks were added by the programmer + // self.env.add_diag(diag!( + // StyleCodes::UnnecessaryBlock.diag_info(), + // (e.exp.loc, "Unnecessary block expression '{}')" + // (e.exp.loc, if_msg), + // )); + } + n => { + let last = n - 1; + for (i, stmt) in seq_.iter().enumerate() { + if i != last && is_unit_seq(self, stmt) { + let msg = "Unnecessary unit in sequence '();'. Consider removing"; + self.env.add_diag(diag!( + StyleCodes::UnnecessaryUnit.diag_info(), + (stmt.loc, msg), + )); + } + } + } + } + false + } + + fn visit_exp_custom(&mut self, e: &T::Exp) -> bool { + use UnannotatedExp_ as TE; + let TE::IfElse(e_cond, e_true, e_false_opt) = &e.exp.value else { + return false; + }; + if is_unit(self, e_true) { + let u_msg = "Unnecessary unit '()'"; + let if_msg = "Consider negating the 'if' condition and simplifying"; + let mut diag = diag!( + StyleCodes::UnnecessaryUnit.diag_info(), + (e_true.exp.loc, u_msg), + (e_cond.exp.loc, if_msg), + ); + diag.add_note("For example 'if (cond) () else e' can be simplified to 'if (!cond) e'"); + self.env.add_diag(diag); + } + if let Some(e_false) = e_false_opt { + if is_unit(self, e_false) { + let u_msg = "Unnecessary 'else ()'."; + let if_msg = "An 'if' without an 'else' has an implicit 'else ()'. \ + Consider removing the 'else' branch"; + let mut diag = diag!( + StyleCodes::UnnecessaryUnit.diag_info(), + (e_false.exp.loc, u_msg), + (e.exp.loc, if_msg), + ); + diag.add_note( + "For example 'if (cond) e else ()' can be simplified to 'if (cond) e'", + ); + self.env.add_diag(diag); + } + } + false + } +} + +fn is_unit_seq(context: &mut Context, s: &T::SequenceItem) -> bool { + match &s.value { + SequenceItem_::Seq(e) => is_unit(context, e), + SequenceItem_::Declare(_) | SequenceItem_::Bind(_, _, _) => false, + } +} + +fn is_unit(context: &mut Context, e: &T::Exp) -> bool { + use UnannotatedExp_ as TE; + match &e.exp.value { + TE::Unit { .. } => true, + TE::Annotate(inner, _) => is_unit(context, inner), + TE::Block((_, seq)) if seq.is_empty() => { + context + .env + .add_diag(ice!((e.exp.loc, "Unexpected empty block without a value"))); + false + } + TE::Block((_, seq)) if seq.len() == 1 => is_unit_seq(context, &seq[0]), + _ => false, + } +} diff --git a/external-crates/move/crates/move-compiler/src/sui_mode/info.rs b/external-crates/move/crates/move-compiler/src/sui_mode/info.rs index 2bfaedeafef51..614443d2c2679 100644 --- a/external-crates/move/crates/move-compiler/src/sui_mode/info.rs +++ b/external-crates/move/crates/move-compiler/src/sui_mode/info.rs @@ -303,6 +303,6 @@ fn add_private_transfers( let mut visitor = TransferVisitor { transferred }; match &fdef.body.value { T::FunctionBody_::Native | &T::FunctionBody_::Macro => (), - T::FunctionBody_::Defined(seq) => visitor.visit_seq(seq), + T::FunctionBody_::Defined(seq) => visitor.visit_seq(fdef.body.loc, seq), } } diff --git a/external-crates/move/crates/move-compiler/src/sui_mode/typing.rs b/external-crates/move/crates/move-compiler/src/sui_mode/typing.rs index b895758f2602f..7ec26a176398c 100644 --- a/external-crates/move/crates/move-compiler/src/sui_mode/typing.rs +++ b/external-crates/move/crates/move-compiler/src/sui_mode/typing.rs @@ -293,7 +293,7 @@ fn function(context: &mut Context, name: FunctionName, fdef: &T::Function) { entry_signature(context, *entry_loc, name, signature); } if let sp!(_, T::FunctionBody_::Defined(seq)) = body { - context.visit_seq(seq) + context.visit_seq(body.loc, seq) } context.in_test = prev_in_test; } diff --git a/external-crates/move/crates/move-compiler/src/typing/visitor.rs b/external-crates/move/crates/move-compiler/src/typing/visitor.rs index d78ab689d0d57..4f3e874e6f11f 100644 --- a/external-crates/move/crates/move-compiler/src/typing/visitor.rs +++ b/external-crates/move/crates/move-compiler/src/typing/visitor.rs @@ -245,7 +245,7 @@ pub trait TypingVisitorContext { self.visit_type(None, &fdef.signature.return_type); } if let T::FunctionBody_::Defined(seq) = &fdef.body.value { - self.visit_seq(seq); + self.visit_seq(fdef.body.loc, seq); } self.pop_warning_filter_scope(); } @@ -291,11 +291,19 @@ pub trait TypingVisitorContext { // -- SEQUENCES AND EXPRESSIONS -- - fn visit_seq(&mut self, (use_funs, seq): &T::Sequence) { + /// Custom visit for a sequence. It will skip `visit_seq` if `visit_seq_custom` returns true. + fn visit_seq_custom(&mut self, _loc: Loc, _seq: &T::Sequence) -> bool { + false + } + + fn visit_seq(&mut self, loc: Loc, seq @ (use_funs, seq_): &T::Sequence) { + if self.visit_seq_custom(loc, seq) { + return; + } if Self::VISIT_USE_FUNS { self.visit_use_funs(use_funs); } - for s in seq { + for s in seq_ { self.visit_seq_item(s); } } @@ -458,8 +466,8 @@ pub trait TypingVisitorContext { self.visit_exp(e2); } E::Loop { body, .. } => self.visit_exp(body), - E::NamedBlock(_, seq) => self.visit_seq(seq), - E::Block(seq) => self.visit_seq(seq), + E::NamedBlock(_, seq) => self.visit_seq(exp.exp.loc, seq), + E::Block(seq) => self.visit_seq(exp.exp.loc, seq), E::Assign(lvalues, ty_ann, e) => { // visit the RHS first to better match control flow self.visit_exp(e); diff --git a/external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_conditional.move b/external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_conditional.move index 4e676fa7ca518..a70e993da5b7e 100644 --- a/external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_conditional.move +++ b/external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_conditional.move @@ -1,12 +1,14 @@ module a::m { // These very simply could be rewritten but we are overly conservative when it comes to blocks public fun t0(condition: bool) { - if (condition) { (); true } else false; - if (condition) b"" else { (); (); vector[] }; + if (condition) { foo(); true } else false; + if (condition) b"" else { foo(); foo(); vector[] }; } // we don't do this check after constant folding public fun t1(condition: bool) { if (condition) 1 + 1 else 2; } + + fun foo() {} } diff --git a/external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_unit.move b/external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_unit.move new file mode 100644 index 0000000000000..da9533c8d5890 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_unit.move @@ -0,0 +1,12 @@ +// suppress unnecessary_unit lint +module a::m { + + #[allow(lint(unnecessary_unit))] + public fun test_empty_else(x: bool): bool { + if (x) { x = true; } else {}; + if (!x) () else { test_empty_else(x); }; + { (); }; + (); + x + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_unit.move b/external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_unit.move new file mode 100644 index 0000000000000..dac10008319ab --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_unit.move @@ -0,0 +1,12 @@ +// tests unnecessary units. These caeses are not errors and should not be reported +module a::unnecessary_unit { + public fun t_if_without_else(cond: bool): u64 { + let x = 0; + if (cond) x = 1; + x + } + + public fun t() { + () // unit here is okay + } +} diff --git a/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_unit.exp b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_unit.exp new file mode 100644 index 0000000000000..3b81304356f04 --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_unit.exp @@ -0,0 +1,151 @@ +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:6:16 + │ +6 │ if (b) () else { x = 1 }; + │ - ^^ Unnecessary unit '()' + │ │ + │ Consider negating the 'if' condition and simplifying + │ + = For example 'if (cond) () else e' can be simplified to 'if (!cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:8:16 + │ +8 │ if (b) {} else { x = 1 }; + │ - ^^ Unnecessary unit '()' + │ │ + │ Consider negating the 'if' condition and simplifying + │ + = For example 'if (cond) () else e' can be simplified to 'if (!cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:10:16 + │ +10 │ if (b) { () } else { x = 1 }; + │ - ^^^^^^ Unnecessary unit '()' + │ │ + │ Consider negating the 'if' condition and simplifying + │ + = For example 'if (cond) () else e' can be simplified to 'if (!cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:12:16 + │ +12 │ if (b) { + │ - Consider negating the 'if' condition and simplifying + │ ╭────────────────^ +13 │ │ // new line and comment does not suppress it +14 │ │ } else { x = 1 }; + │ ╰─────────^ Unnecessary unit '()' + │ + = For example 'if (cond) () else e' can be simplified to 'if (!cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:21:31 + │ +21 │ if (b) { x = 1 } else (); + │ ----------------------^^ + │ │ │ + │ │ Unnecessary 'else ()'. + │ An 'if' without an 'else' has an implicit 'else ()'. Consider removing the 'else' branch + │ + = For example 'if (cond) e else ()' can be simplified to 'if (cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:23:31 + │ +23 │ if (b) { x = 1 } else {}; + │ ----------------------^^ + │ │ │ + │ │ Unnecessary 'else ()'. + │ An 'if' without an 'else' has an implicit 'else ()'. Consider removing the 'else' branch + │ + = For example 'if (cond) e else ()' can be simplified to 'if (cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:25:31 + │ +25 │ if (b) { x = 1 } else { () }; + │ ----------------------^^^^^^ + │ │ │ + │ │ Unnecessary 'else ()'. + │ An 'if' without an 'else' has an implicit 'else ()'. Consider removing the 'else' branch + │ + = For example 'if (cond) e else ()' can be simplified to 'if (cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:27:31 + │ +27 │ ╭ if (b) { x = 1 } else { + │ ╭─────────────────────────────────^ +28 │ │ │ // new line and comment does not suppress it +29 │ │ │ }; + │ ╰─│─────────^ Unnecessary 'else ()'. + │ ╰─────────' An 'if' without an 'else' has an implicit 'else ()'. Consider removing the 'else' branch + │ + = For example 'if (cond) e else ()' can be simplified to 'if (cond) e' + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:34:9 + │ +34 │ (); + │ ^^ Unnecessary unit in sequence '();'. Consider removing + │ + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:37:18 + │ +37 │ if (b) { (); () } else { x = 1 }; // doesn't trigger if/else case + │ ^^ Unnecessary unit in sequence '();'. Consider removing + │ + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:39:33 + │ +39 │ if (b) { x = 1 } else { (); (); () }; // doesn't trigger if/else case + │ ^^ Unnecessary unit in sequence '();'. Consider removing + │ + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:39:37 + │ +39 │ if (b) { x = 1 } else { (); (); () }; // doesn't trigger if/else case + │ ^^ Unnecessary unit in sequence '();'. Consider removing + │ + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:41:9 + │ +41 │ {}; + │ ^^ Unnecessary unit in sequence '();'. Consider removing + │ + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:42:9 + │ +42 │ { () }; // inner isn't an error but the outer is + │ ^^^^^^ Unnecessary unit in sequence '();'. Consider removing + │ + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W04010]: unit `()` expression can be removed or simplified + ┌─ tests/linter/true_positive_unnecessary_unit.move:43:11 + │ +43 │ { (); }; // inner is an error but outer isn't + │ ^^ Unnecessary unit in sequence '();'. Consider removing + │ + = This warning can be suppressed with '#[allow(lint(unnecessary_unit))]' 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_unit.move b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_unit.move new file mode 100644 index 0000000000000..3cd380ffbd1ce --- /dev/null +++ b/external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_unit.move @@ -0,0 +1,52 @@ +// tests unnecessary units in if, else, and block +module a::unnecessary_unit { + public fun t_if(b: bool) { + let x = 0; + x; + if (b) () else { x = 1 }; + x; + if (b) {} else { x = 1 }; + x; + if (b) { () } else { x = 1 }; + x; + if (b) { + // new line and comment does not suppress it + } else { x = 1 }; + x; + } + + public fun t_else(b: bool) { + let x = 0; + x; + if (b) { x = 1 } else (); + x; + if (b) { x = 1 } else {}; + x; + if (b) { x = 1 } else { () }; + x; + if (b) { x = 1 } else { + // new line and comment does not suppress it + }; + x; + } + + public fun t_block(b: bool) { + (); + let x = 0; + x; + if (b) { (); () } else { x = 1 }; // doesn't trigger if/else case + x; + if (b) { x = 1 } else { (); (); () }; // doesn't trigger if/else case + x; + {}; + { () }; // inner isn't an error but the outer is + { (); }; // inner is an error but outer isn't + () + } + + // public fun t_if_else_if(b: bool, c: bool) { + // let x = 0; + // x; + // if (b) { x = 1 } else if (c) {}; + // } +}