Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auto-fix if_not_else #13809

Merged
merged 2 commits into from
Dec 24, 2024
+279 −6
Merged

auto-fix if_not_else #13809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ace535
suggest modified code for if_not_else lint
lapla-cogito Dec 11, 2024
887aa26
add more tests
lapla-cogito Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 51 additions & 3 deletions clippy_lints/src/if_not_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
{
Expand All @@ -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)
}
73 changes: 73 additions & 0 deletions tests/ui/if_not_else.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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");
}
}
42 changes: 42 additions & 0 deletions tests/ui/if_not_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Comment on lines +32 to +46
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the following code, which is similar in structure to this one, except that the parts to be linted are nested:

if !(foo() && bla()) {
    if !foo() {
        println!("not foo");
    } else {
        println!("some output");
    }
} else {
    println!("both true");
}

Then, when I run cargo bless, I get a cannot replace slice of data that was already replaced error. But I don't think this is a problem since rustfix will apply the first fix and ignore the subsequent when put to practical use.

}

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 */
}
}
116 changes: 113 additions & 3 deletions tests/ui/if_not_else.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Loading