From 1ace535bc25f64404c5d0435b93fe43e6146f2a7 Mon Sep 17 00:00:00 2001 From: lapla-cogito Date: Wed, 11 Dec 2024 12:38:03 +0900 Subject: [PATCH 1/2] suggest modified code for if_not_else lint --- clippy_lints/src/if_not_else.rs | 54 +++++++++++++++++++++++++++++++-- tests/ui/if_not_else.fixed | 31 +++++++++++++++++++ tests/ui/if_not_else.stderr | 20 ++++++++++-- 3 files changed, 100 insertions(+), 5 deletions(-) create mode 100644 tests/ui/if_not_else.fixed diff --git a/clippy_lints/src/if_not_else.rs b/clippy_lints/src/if_not_else.rs index 120c5396a1cd..2806d4d0e5d6 100644 --- 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, +) -> 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 new file mode 100644 index 000000000000..6c646b0b963d --- /dev/null +++ b/tests/ui/if_not_else.fixed @@ -0,0 +1,31 @@ +#![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"); + } +} diff --git a/tests/ui/if_not_else.stderr b/tests/ui/if_not_else.stderr index 92fed7b1bf70..01d6767004d9 100644 --- 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,15 @@ 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: aborting due to 2 previous errors From 887aa269b6657d53d4d074b6b34ad417fef1e271 Mon Sep 17 00:00:00 2001 From: lapla-cogito Date: Tue, 24 Dec 2024 21:10:43 +0900 Subject: [PATCH 2/2] add more tests --- tests/ui/if_not_else.fixed | 42 ++++++++++++++++ tests/ui/if_not_else.rs | 42 ++++++++++++++++ tests/ui/if_not_else.stderr | 96 ++++++++++++++++++++++++++++++++++++- 3 files changed, 179 insertions(+), 1 deletion(-) diff --git a/tests/ui/if_not_else.fixed b/tests/ui/if_not_else.fixed index 6c646b0b963d..11d1e13179c4 100644 --- a/tests/ui/if_not_else.fixed +++ b/tests/ui/if_not_else.fixed @@ -28,4 +28,46 @@ fn main() { } 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 index fd30e3702a27..fcc67e163e8a 100644 --- 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 index 01d6767004d9..b01cb5af11f7 100644 --- a/tests/ui/if_not_else.stderr +++ b/tests/ui/if_not_else.stderr @@ -42,5 +42,99 @@ LL + println!("Bugs"); LL + } | -error: aborting due to 2 previous errors +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 6 previous errors