[Linter] Needless else (#16874)

# Description
Detects empty `else` branches in conditional structures, suggesting
their removal for cleaner code.
Aims to flag potentially unnecessary or unimplemented placeholders
within `if-else` statements.
Encourages code clarity and maintainability by eliminating redundant
branches.

# Run the linter
```
cargo test move_check_testsuit
```

# Testing
File testing: needless_else.move

## Release notes

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:

---------

Co-authored-by: jamedzung <[email protected]>
Co-authored-by: Todd Nowacki <[email protected]>

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 {

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<Visitor> {
                 unnecessary_conditional::UnnecessaryConditional.visitor(),
                 self_assignment::SelfAssignmentVisitor.visitor(),
                 redundant_ref_deref::RedundantRefDerefVisitor.visitor(),
+                unnecessary_unit::UnnecessaryUnit.visitor(),
             ]
         }
     }

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,
+    }
+}

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),
     }
 }

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;
 }

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);

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() {}
 }

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
+    }
+}

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
+    }
+}