auto-fix if_not_else (rust-lang#13809)

fix rust-lang#13411 The `if_not_else` lint can be fixed automatically, but the issue above reports that there is no implementation to do so. Therefore, this PR implements it. ---- changelog: [`if_not_else`]: make suggestions for modified code
flip1995 · Dec 24, 2024 · 85b6094 · 85b6094
2 parents 988042e + 887aa26
commit 85b6094
Show file tree

Hide file tree

Showing 4 changed files with 279 additions and 6 deletions.
diff --git a/clippy_lints/src/if_not_else.rs b/clippy_lints/src/if_not_else.rs
@@ -1,9 +1,13 @@
 use clippy_utils::consts::{ConstEvalCtxt, Constant};
-use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
 use clippy_utils::is_else_clause;
+use clippy_utils::source::{HasSession, indent_of, reindent_multiline, snippet};
+use rustc_errors::Applicability;
 use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
+use rustc_span::Span;
+use std::borrow::Cow;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -54,7 +58,7 @@ fn is_zero_const(expr: &Expr<'_>, cx: &LateContext<'_>) -> bool {
 
 impl LateLintPass<'_> for IfNotElse {
     fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
-        if let ExprKind::If(cond, _, Some(els)) = e.kind
+        if let ExprKind::If(cond, cond_inner, Some(els)) = e.kind
             && let ExprKind::DropTemps(cond) = cond.kind
             && let ExprKind::Block(..) = els.kind
         {
@@ -79,8 +83,52 @@ impl LateLintPass<'_> for IfNotElse {
             // }
             // ```
             if !e.span.from_expansion() && !is_else_clause(cx.tcx, e) {
-                span_lint_and_help(cx, IF_NOT_ELSE, e.span, msg, None, help);
+                match cond.kind {
+                    ExprKind::Unary(UnOp::Not, _) | ExprKind::Binary(_, _, _) => span_lint_and_sugg(
+                        cx,
+                        IF_NOT_ELSE,
+                        e.span,
+                        msg,
+                        "try",
+                        make_sugg(cx, &cond.kind, cond_inner.span, els.span, "..", Some(e.span)).to_string(),
+                        Applicability::MachineApplicable,
+                    ),
+                    _ => span_lint_and_help(cx, IF_NOT_ELSE, e.span, msg, None, help),
+                }
             }
         }
     }
 }
+
+fn make_sugg<'a>(
+    sess: &impl HasSession,
+    cond_kind: &'a ExprKind<'a>,
+    cond_inner: Span,
+    els_span: Span,
+    default: &'a str,
+    indent_relative_to: Option<Span>,
+) -> Cow<'a, str> {
+    let cond_inner_snip = snippet(sess, cond_inner, default);
+    let els_snip = snippet(sess, els_span, default);
+    let indent = indent_relative_to.and_then(|s| indent_of(sess, s));
+
+    let suggestion = match cond_kind {
+        ExprKind::Unary(UnOp::Not, cond_rest) => {
+            format!(
+                "if {} {} else {}",
+                snippet(sess, cond_rest.span, default),
+                els_snip,
+                cond_inner_snip
+            )
+        },
+        ExprKind::Binary(_, lhs, rhs) => {
+            let lhs_snip = snippet(sess, lhs.span, default);
+            let rhs_snip = snippet(sess, rhs.span, default);
+
+            format!("if {lhs_snip} == {rhs_snip} {els_snip} else {cond_inner_snip}")
+        },
+        _ => String::new(),
+    };
+
+    reindent_multiline(suggestion.into(), true, indent)
+}
diff --git a/tests/ui/if_not_else.fixed b/tests/ui/if_not_else.fixed
@@ -0,0 +1,73 @@
+#![warn(clippy::all)]
+#![warn(clippy::if_not_else)]
+
+fn foo() -> bool {
+    unimplemented!()
+}
+fn bla() -> bool {
+    unimplemented!()
+}
+
+fn main() {
+    if bla() {
+        println!("Bunny");
+    } else {
+        //~^ ERROR: unnecessary boolean `not` operation
+        println!("Bugs");
+    }
+    if 4 == 5 {
+        println!("Bunny");
+    } else {
+        //~^ ERROR: unnecessary `!=` operation
+        println!("Bugs");
+    }
+    if !foo() {
+        println!("Foo");
+    } else if !bla() {
+        println!("Bugs");
+    } else {
+        println!("Bunny");
+    }
+
+    if (foo() && bla()) {
+        println!("both true");
+    } else {
+        #[cfg(not(debug_assertions))]
+        println!("not debug");
+        #[cfg(debug_assertions)]
+        println!("debug");
+        if foo() {
+            println!("foo");
+        } else if bla() {
+            println!("bla");
+        } else {
+            println!("both false");
+        }
+    }
+}
+
+fn with_comments() {
+    if foo() {
+        println!("foo"); /* foo */
+    } else {
+        /* foo is false */
+        println!("foo is false");
+    }
+
+    if bla() {
+        println!("bla"); // bla
+    } else {
+        // bla is false
+        println!("bla");
+    }
+}
+
+fn with_annotations() {
+    #[cfg(debug_assertions)]
+    if foo() {
+        println!("foo"); /* foo */
+    } else {
+        /* foo is false */
+        println!("foo is false");
+    }
+}
diff --git a/tests/ui/if_not_else.rs b/tests/ui/if_not_else.rs
@@ -28,4 +28,46 @@ fn main() {
     } else {
         println!("Bunny");
     }
+
+    if !(foo() && bla()) {
+        #[cfg(not(debug_assertions))]
+        println!("not debug");
+        #[cfg(debug_assertions)]
+        println!("debug");
+        if foo() {
+            println!("foo");
+        } else if bla() {
+            println!("bla");
+        } else {
+            println!("both false");
+        }
+    } else {
+        println!("both true");
+    }
+}
+
+fn with_comments() {
+    if !foo() {
+        /* foo is false */
+        println!("foo is false");
+    } else {
+        println!("foo"); /* foo */
+    }
+
+    if !bla() {
+        // bla is false
+        println!("bla");
+    } else {
+        println!("bla"); // bla
+    }
+}
+
+fn with_annotations() {
+    #[cfg(debug_assertions)]
+    if !foo() {
+        /* foo is false */
+        println!("foo is false");
+    } else {
+        println!("foo"); /* foo */
+    }
 }
diff --git a/tests/ui/if_not_else.stderr b/tests/ui/if_not_else.stderr
@@ -9,9 +9,17 @@ LL | |         println!("Bunny");
 LL | |     }
    | |_____^
    |
-   = help: remove the `!` and swap the blocks of the `if`/`else`
    = note: `-D clippy::if-not-else` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::if_not_else)]`
+help: try
+   |
+LL ~     if bla() {
+LL +         println!("Bunny");
+LL +     } else {
+LL +
+LL +         println!("Bugs");
+LL +     }
+   |
 
 error: unnecessary `!=` operation
   --> tests/ui/if_not_else.rs:18:5
@@ -24,7 +32,109 @@ LL | |         println!("Bunny");
 LL | |     }
    | |_____^
    |
-   = help: change to `==` and swap the blocks of the `if`/`else`
+help: try
+   |
+LL ~     if 4 == 5 {
+LL +         println!("Bunny");
+LL +     } else {
+LL +
+LL +         println!("Bugs");
+LL +     }
+   |
+
+error: unnecessary boolean `not` operation
+  --> tests/ui/if_not_else.rs:32:5
+   |
+LL | /     if !(foo() && bla()) {
+LL | |         #[cfg(not(debug_assertions))]
+LL | |         println!("not debug");
+LL | |         #[cfg(debug_assertions)]
+...  |
+LL | |         println!("both true");
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     if (foo() && bla()) {
+LL +         println!("both true");
+LL +     } else {
+LL +         #[cfg(not(debug_assertions))]
+LL +         println!("not debug");
+LL +         #[cfg(debug_assertions)]
+LL +         println!("debug");
+LL +         if foo() {
+LL +             println!("foo");
+LL +         } else if bla() {
+LL +             println!("bla");
+LL +         } else {
+LL +             println!("both false");
+LL +         }
+LL +     }
+   |
+
+error: unnecessary boolean `not` operation
+  --> tests/ui/if_not_else.rs:50:5
+   |
+LL | /     if !foo() {
+LL | |         /* foo is false */
+LL | |         println!("foo is false");
+LL | |     } else {
+LL | |         println!("foo"); /* foo */
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     if foo() {
+LL +         println!("foo"); /* foo */
+LL +     } else {
+LL +         /* foo is false */
+LL +         println!("foo is false");
+LL +     }
+   |
+
+error: unnecessary boolean `not` operation
+  --> tests/ui/if_not_else.rs:57:5
+   |
+LL | /     if !bla() {
+LL | |         // bla is false
+LL | |         println!("bla");
+LL | |     } else {
+LL | |         println!("bla"); // bla
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     if bla() {
+LL +         println!("bla"); // bla
+LL +     } else {
+LL +         // bla is false
+LL +         println!("bla");
+LL +     }
+   |
+
+error: unnecessary boolean `not` operation
+  --> tests/ui/if_not_else.rs:67:5
+   |
+LL | /     if !foo() {
+LL | |         /* foo is false */
+LL | |         println!("foo is false");
+LL | |     } else {
+LL | |         println!("foo"); /* foo */
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     if foo() {
+LL +         println!("foo"); /* foo */
+LL +     } else {
+LL +         /* foo is false */
+LL +         println!("foo is false");
+LL +     }
+   |
 
-error: aborting due to 2 previous errors
+error: aborting due to 6 previous errors