[Linter] Unnecessary Conditional (MystenLabs#16856)

# Description

- Added a lint to check for unnecessary conditionals
- Triggered when
- Each branch is a single value and the values are equal (consider
removing the conditional)
- Each branch is a single bool, and the bools are not equal (use the
condition directly)

## Testing

- New tests

## Release notes

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [X] CLI: Move will now lint against unnecessary conditionals,
`if-else` expressions.
- [ ] Rust SDK:

---------

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

external-crates/move/crates/move-compiler/src/expansion/ast.rs

@@ -937,32 +937,6 @@ impl fmt::Display for Visibility {
     }
 }
 
-impl fmt::Display for Type_ {
-    fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result {
-        use Type_::*;
-        match self {
-            UnresolvedError => write!(f, "_"),
-            Apply(n, tys) => {
-                write!(f, "{}", n)?;
-                if !tys.is_empty() {
-                    write!(f, "<")?;
-                    write!(f, "{}", format_comma(tys))?;
-                    write!(f, ">")?;
-                }
-                Ok(())
-            }
-            Ref(mut_, ty) => write!(f, "&{}{}", if *mut_ { "mut " } else { "" }, ty),
-            Fun(args, result) => write!(f, "({}):{}", format_comma(args), result),
-            Unit => write!(f, "()"),
-            Multiple(tys) => {
-                write!(f, "(")?;
-                write!(f, "{}", format_comma(tys))?;
-                write!(f, ")")
-            }
-        }
-    }
-}
-
 impl std::fmt::Display for Value_ {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use Value_ as V;
@@ -976,7 +950,17 @@ impl std::fmt::Display for Value_ {
             V::U128(u) => write!(f, "{}", u),
             V::U256(u) => write!(f, "{}", u),
             V::Bool(b) => write!(f, "{}", b),
-            V::Bytearray(v) => write!(f, "{:?}", v), // Good enough?
+            // TODO preserve the user's original string
+            V::Bytearray(v) => {
+                write!(f, "vector[")?;
+                for (idx, byte) in v.iter().enumerate() {
+                    if idx != 0 {
+                        write!(f, ", ")?;
+                    }
+                    write!(f, "{}", byte)?;
+                }
+                write!(f, "]")
+            }
         }
     }
 }

external-crates/move/crates/move-compiler/src/linters/mod.rs

@@ -15,6 +15,7 @@ pub mod abort_constant;
 pub mod constant_naming;
 pub mod loop_without_exit;
 pub mod meaningless_math_operation;
+pub mod unnecessary_conditional;
 pub mod unnecessary_while_loop;
 pub mod unneeded_return;
 
@@ -131,6 +132,12 @@ lints!(
         LinterDiagnosticCategory::Suspicious,
         "loop_without_exit",
         "'loop' without 'break' or 'return'"
+    ),
+    (
+        UnnecessaryConditional,
+        LinterDiagnosticCategory::Complexity,
+        "unnecessary_conditional",
+        "'if' expression can be removed"
     )
 );
 
@@ -165,6 +172,7 @@ pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
                 unneeded_return::UnneededReturnVisitor.visitor(),
                 abort_constant::AssertAbortNamedConstants.visitor(),
                 loop_without_exit::LoopWithoutExit.visitor(),
+                unnecessary_conditional::UnnecessaryConditional.visitor(),
             ]
         }
     }

external-crates/move/crates/move-compiler/src/linters/unnecessary_conditional.rs

@@ -0,0 +1,118 @@
+// Copyright (c) The Move Contributors
+// SPDX-License-Identifier: Apache-2.0
+
+//! Detects and suggests simplification for `if (c) e1 else e2` can be removed
+use move_proc_macros::growing_stack;
+
+use crate::expansion::ast::Value;
+use crate::linters::StyleCodes;
+use crate::{
+    diag,
+    diagnostics::WarningFilters,
+    expansion::ast::Value_,
+    shared::CompilationEnv,
+    typing::{
+        ast::{self as T, SequenceItem_, UnannotatedExp_},
+        visitor::{TypingVisitorConstructor, TypingVisitorContext},
+    },
+};
+
+pub struct UnnecessaryConditional;
+
+pub struct Context<'a> {
+    env: &'a mut CompilationEnv,
+}
+
+impl TypingVisitorConstructor for UnnecessaryConditional {
+    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: &T::Exp) -> bool {
+        let UnannotatedExp_::IfElse(_, etrue, efalse) = &exp.exp.value else {
+            return false;
+        };
+        let Some(vtrue) = extract_value(etrue) else {
+            return false;
+        };
+        let Some(vfalse) = extract_value(efalse) else {
+            return false;
+        };
+
+        match (&vtrue.value, &vfalse.value) {
+            (Value_::Bool(v1 @ true), Value_::Bool(false))
+            | (Value_::Bool(v1 @ false), Value_::Bool(true)) => {
+                let negation = if *v1 { "" } else { "!" };
+                let msg = format!(
+                    "Detected an unnecessary conditional expression 'if (cond)'. Consider using \
+                    the condition directly, i.e. '{negation}cond'",
+                );
+                self.env.add_diag(diag!(
+                    StyleCodes::UnnecessaryConditional.diag_info(),
+                    (exp.exp.loc, msg)
+                ));
+            }
+            (v1, v2) if v1 == v2 => {
+                let msg =
+                    "Detected a redundant conditional expression 'if (..) v else v', where each \
+                    branch results in the same value 'v'. Consider using the value directly";
+                self.env.add_diag(diag!(
+                    StyleCodes::UnnecessaryConditional.diag_info(),
+                    (exp.exp.loc, msg),
+                    (vtrue.loc, "This value"),
+                    (vfalse.loc, "is the same as this value"),
+                ));
+            }
+            _ => (),
+        }
+
+        //     if let (Some(if_bool), Some(else_bool)) = (
+        //         extract_bool_literal_from_block(if_block),
+        //         extract_bool_literal_from_block(else_block),
+        //     ) {
+        //         if if_bool != else_bool {
+        //             let msg = format!(
+        //                 "Detected a redundant conditional expression `if (...) {} else {}`. Consider using the condition directly.",
+        //                 if_bool, else_bool
+        //             );
+        //             let diag = diag!(
+        //                 StyleCodes::UnnecessaryConditional.diag_info(),
+        //                 (exp.exp.loc, msg)
+        //             );
+
+        //             self.env.add_diag(diag);
+        //         }
+        //     }
+        // }
+        false
+    }
+}
+
+#[growing_stack]
+fn extract_value(block: &T::Exp) -> Option<&Value> {
+    match &block.exp.value {
+        UnannotatedExp_::Block((_, seq)) if seq.len() == 1 => extract_value_seq_item(&seq[0]),
+        UnannotatedExp_::Value(v) => Some(v),
+        UnannotatedExp_::Annotate(e, _) => extract_value(e),
+        _ => None,
+    }
+}
+
+#[growing_stack]
+fn extract_value_seq_item(sp!(_, item_): &T::SequenceItem) -> Option<&Value> {
+    match &item_ {
+        SequenceItem_::Declare(_) | SequenceItem_::Bind(_, _, _) => None,
+        SequenceItem_::Seq(e) => extract_value(e),
+    }
+}

external-crates/move/crates/move-compiler/tests/linter/false_negative_unnecessary_conditional.move

@@ -0,0 +1,12 @@
+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[] };
+    }
+
+    // we don't do this check after constant folding
+    public fun t1(condition: bool) {
+        if (condition) 1 + 1 else 2;
+    }
+}

external-crates/move/crates/move-compiler/tests/linter/suppress_unnecessary_conditional.move

@@ -0,0 +1,17 @@
+module a::m {
+
+    #[allow(lint(unnecessary_conditional))]
+    public fun t0(condition: bool) {
+        if (condition) true else false;
+    }
+
+    #[allow(lint(unnecessary_conditional))]
+    public fun t1(condition: bool) {
+        if (condition) vector<u8>[] else vector[];
+    }
+
+    #[allow(lint(unnecessary_conditional))]
+    public fun t2(condition: bool) {
+        if (condition) @0 else @0;
+    }
+}

external-crates/move/crates/move-compiler/tests/linter/true_negative_unnecessary_conditional.move

@@ -0,0 +1,17 @@
+// true negative cases for redundant conditional
+module a::m {
+    public fun t0(condition: bool) {
+        if (condition) 1 else { 0 };
+        if (condition) vector[] else vector[1];
+    }
+
+    public fun t1(x: u64, y: u64) {
+        let _ = if (x > y) { x } else y;
+    }
+
+    // has side effects, too complex to analyze
+    public fun t2(condition: &mut bool) {
+        let _ = if (*condition) { *condition = false; true } else { *condition = true; false };
+    }
+
+}

external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_conitional.exp

@@ -0,0 +1,52 @@
+warning[Lint W01007]: 'if' expression can be removed
+  ┌─ tests/linter/true_positive_unnecessary_conitional.move:4:9
+  │
+4 │         if (!condition) true else false;
+  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Detected an unnecessary conditional expression 'if (cond)'. Consider using the condition directly, i.e. 'cond'
+  │
+  = This warning can be suppressed with '#[allow(lint(unnecessary_conditional))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01007]: 'if' expression can be removed
+  ┌─ tests/linter/true_positive_unnecessary_conitional.move:5:9
+  │
+5 │         if (condition) { { false } } else { (true: bool) };
+  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Detected an unnecessary conditional expression 'if (cond)'. Consider using the condition directly, i.e. '!cond'
+  │
+  = This warning can be suppressed with '#[allow(lint(unnecessary_conditional))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01007]: 'if' expression can be removed
+  ┌─ tests/linter/true_positive_unnecessary_conitional.move:9:9
+  │
+9 │         if (true) true else true;
+  │         ^^^^^^^^^^^^^^^^^^^^^^^^
+  │         │         │         │
+  │         │         │         is the same as this value
+  │         │         This value
+  │         Detected a redundant conditional expression 'if (..) v else v', where each branch results in the same value 'v'. Consider using the value directly
+  │
+  = This warning can be suppressed with '#[allow(lint(unnecessary_conditional))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01007]: 'if' expression can be removed
+   ┌─ tests/linter/true_positive_unnecessary_conitional.move:10:9
+   │
+10 │         if (foo()) 0 else 0;
+   │         ^^^^^^^^^^^^^^^^^^^
+   │         │          │      │
+   │         │          │      is the same as this value
+   │         │          This value
+   │         Detected a redundant conditional expression 'if (..) v else v', where each branch results in the same value 'v'. Consider using the value directly
+   │
+   = This warning can be suppressed with '#[allow(lint(unnecessary_conditional))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[Lint W01007]: 'if' expression can be removed
+   ┌─ tests/linter/true_positive_unnecessary_conitional.move:11:9
+   │
+11 │         if (!foo()) b"" else x"";
+   │         ^^^^^^^^^^^^^^^^^^^^^^^^
+   │         │           │        │
+   │         │           │        is the same as this value
+   │         │           This value
+   │         Detected a redundant conditional expression 'if (..) v else v', where each branch results in the same value 'v'. Consider using the value directly
+   │
+   = This warning can be suppressed with '#[allow(lint(unnecessary_conditional))]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

external-crates/move/crates/move-compiler/tests/linter/true_positive_unnecessary_conitional.move

@@ -0,0 +1,15 @@
+// tests cases where a lint is reported for a redundant conditional expression
+module a::m {
+    public fun t0(condition: bool) {
+        if (!condition) true else false;
+        if (condition) { { false } } else { (true: bool) };
+    }
+
+    public fun t1() {
+        if (true) true else true;
+        if (foo()) 0 else 0;
+        if (!foo()) b"" else x"";
+    }
+
+    fun foo(): bool { true }
+}