Skip to content

Commit 85b6094

Browse files
llogiqllogiq
authored
auto-fix if_not_else (#13809)
fix #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
2 parents 988042e + 887aa26 commit 85b6094

File tree

4 files changed

+279
-6
lines changed

4 files changed

+279
-6
lines changed

‎clippy_lints/src/if_not_else.rs

+51-3
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
use clippy_utils::consts::{ConstEvalCtxt, Constant};
2-
use clippy_utils::diagnostics::span_lint_and_help;
2+
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
33
use clippy_utils::is_else_clause;
4+
use clippy_utils::source::{HasSession, indent_of, reindent_multiline, snippet};
5+
use rustc_errors::Applicability;
46
use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
57
use rustc_lint::{LateContext, LateLintPass};
68
use rustc_session::declare_lint_pass;
9+
use rustc_span::Span;
10+
use std::borrow::Cow;
711

812
declare_clippy_lint! {
913
/// ### What it does
@@ -54,7 +58,7 @@ fn is_zero_const(expr: &Expr<'_>, cx: &LateContext<'_>) -> bool {
5458

5559
impl LateLintPass<'_> for IfNotElse {
5660
fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
57-
if let ExprKind::If(cond, _, Some(els)) = e.kind
61+
if let ExprKind::If(cond, cond_inner, Some(els)) = e.kind
5862
&& let ExprKind::DropTemps(cond) = cond.kind
5963
&& let ExprKind::Block(..) = els.kind
6064
{
@@ -79,8 +83,52 @@ impl LateLintPass<'_> for IfNotElse {
7983
// }
8084
// ```
8185
if !e.span.from_expansion() && !is_else_clause(cx.tcx, e) {
82-
span_lint_and_help(cx, IF_NOT_ELSE, e.span, msg, None, help);
86+
match cond.kind {
87+
ExprKind::Unary(UnOp::Not, _) | ExprKind::Binary(_, _, _) => span_lint_and_sugg(
88+
cx,
89+
IF_NOT_ELSE,
90+
e.span,
91+
msg,
92+
"try",
93+
make_sugg(cx, &cond.kind, cond_inner.span, els.span, "..", Some(e.span)).to_string(),
94+
Applicability::MachineApplicable,
95+
),
96+
_ => span_lint_and_help(cx, IF_NOT_ELSE, e.span, msg, None, help),
97+
}
8398
}
8499
}
85100
}
86101
}
102+
103+
fn make_sugg<'a>(
104+
sess: &impl HasSession,
105+
cond_kind: &'a ExprKind<'a>,
106+
cond_inner: Span,
107+
els_span: Span,
108+
default: &'a str,
109+
indent_relative_to: Option<Span>,
110+
) -> Cow<'a, str> {
111+
let cond_inner_snip = snippet(sess, cond_inner, default);
112+
let els_snip = snippet(sess, els_span, default);
113+
let indent = indent_relative_to.and_then(|s| indent_of(sess, s));
114+
115+
let suggestion = match cond_kind {
116+
ExprKind::Unary(UnOp::Not, cond_rest) => {
117+
format!(
118+
"if {} {} else {}",
119+
snippet(sess, cond_rest.span, default),
120+
els_snip,
121+
cond_inner_snip
122+
)
123+
},
124+
ExprKind::Binary(_, lhs, rhs) => {
125+
let lhs_snip = snippet(sess, lhs.span, default);
126+
let rhs_snip = snippet(sess, rhs.span, default);
127+
128+
format!("if {lhs_snip} == {rhs_snip} {els_snip} else {cond_inner_snip}")
129+
},
130+
_ => String::new(),
131+
};
132+
133+
reindent_multiline(suggestion.into(), true, indent)
134+
}

‎tests/ui/if_not_else.fixed

+73
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
#![warn(clippy::all)]
2+
#![warn(clippy::if_not_else)]
3+
4+
fn foo() -> bool {
5+
unimplemented!()
6+
}
7+
fn bla() -> bool {
8+
unimplemented!()
9+
}
10+
11+
fn main() {
12+
if bla() {
13+
println!("Bunny");
14+
} else {
15+
//~^ ERROR: unnecessary boolean `not` operation
16+
println!("Bugs");
17+
}
18+
if 4 == 5 {
19+
println!("Bunny");
20+
} else {
21+
//~^ ERROR: unnecessary `!=` operation
22+
println!("Bugs");
23+
}
24+
if !foo() {
25+
println!("Foo");
26+
} else if !bla() {
27+
println!("Bugs");
28+
} else {
29+
println!("Bunny");
30+
}
31+
32+
if (foo() && bla()) {
33+
println!("both true");
34+
} else {
35+
#[cfg(not(debug_assertions))]
36+
println!("not debug");
37+
#[cfg(debug_assertions)]
38+
println!("debug");
39+
if foo() {
40+
println!("foo");
41+
} else if bla() {
42+
println!("bla");
43+
} else {
44+
println!("both false");
45+
}
46+
}
47+
}
48+
49+
fn with_comments() {
50+
if foo() {
51+
println!("foo"); /* foo */
52+
} else {
53+
/* foo is false */
54+
println!("foo is false");
55+
}
56+
57+
if bla() {
58+
println!("bla"); // bla
59+
} else {
60+
// bla is false
61+
println!("bla");
62+
}
63+
}
64+
65+
fn with_annotations() {
66+
#[cfg(debug_assertions)]
67+
if foo() {
68+
println!("foo"); /* foo */
69+
} else {
70+
/* foo is false */
71+
println!("foo is false");
72+
}
73+
}

‎tests/ui/if_not_else.rs

+42
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,46 @@ fn main() {
2828
} else {
2929
println!("Bunny");
3030
}
31+
32+
if !(foo() && bla()) {
33+
#[cfg(not(debug_assertions))]
34+
println!("not debug");
35+
#[cfg(debug_assertions)]
36+
println!("debug");
37+
if foo() {
38+
println!("foo");
39+
} else if bla() {
40+
println!("bla");
41+
} else {
42+
println!("both false");
43+
}
44+
} else {
45+
println!("both true");
46+
}
47+
}
48+
49+
fn with_comments() {
50+
if !foo() {
51+
/* foo is false */
52+
println!("foo is false");
53+
} else {
54+
println!("foo"); /* foo */
55+
}
56+
57+
if !bla() {
58+
// bla is false
59+
println!("bla");
60+
} else {
61+
println!("bla"); // bla
62+
}
63+
}
64+
65+
fn with_annotations() {
66+
#[cfg(debug_assertions)]
67+
if !foo() {
68+
/* foo is false */
69+
println!("foo is false");
70+
} else {
71+
println!("foo"); /* foo */
72+
}
3173
}

‎tests/ui/if_not_else.stderr

+113-3
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,17 @@ LL | | println!("Bunny");
99
LL | | }
1010
| |_____^
1111
|
12-
= help: remove the `!` and swap the blocks of the `if`/`else`
1312
= note: `-D clippy::if-not-else` implied by `-D warnings`
1413
= help: to override `-D warnings` add `#[allow(clippy::if_not_else)]`
14+
help: try
15+
|
16+
LL ~ if bla() {
17+
LL + println!("Bunny");
18+
LL + } else {
19+
LL +
20+
LL + println!("Bugs");
21+
LL + }
22+
|
1523

1624
error: unnecessary `!=` operation
1725
--> tests/ui/if_not_else.rs:18:5
@@ -24,7 +32,109 @@ LL | | println!("Bunny");
2432
LL | | }
2533
| |_____^
2634
|
27-
= help: change to `==` and swap the blocks of the `if`/`else`
35+
help: try
36+
|
37+
LL ~ if 4 == 5 {
38+
LL + println!("Bunny");
39+
LL + } else {
40+
LL +
41+
LL + println!("Bugs");
42+
LL + }
43+
|
44+
45+
error: unnecessary boolean `not` operation
46+
--> tests/ui/if_not_else.rs:32:5
47+
|
48+
LL | / if !(foo() && bla()) {
49+
LL | | #[cfg(not(debug_assertions))]
50+
LL | | println!("not debug");
51+
LL | | #[cfg(debug_assertions)]
52+
... |
53+
LL | | println!("both true");
54+
LL | | }
55+
| |_____^
56+
|
57+
help: try
58+
|
59+
LL ~ if (foo() && bla()) {
60+
LL + println!("both true");
61+
LL + } else {
62+
LL + #[cfg(not(debug_assertions))]
63+
LL + println!("not debug");
64+
LL + #[cfg(debug_assertions)]
65+
LL + println!("debug");
66+
LL + if foo() {
67+
LL + println!("foo");
68+
LL + } else if bla() {
69+
LL + println!("bla");
70+
LL + } else {
71+
LL + println!("both false");
72+
LL + }
73+
LL + }
74+
|
75+
76+
error: unnecessary boolean `not` operation
77+
--> tests/ui/if_not_else.rs:50:5
78+
|
79+
LL | / if !foo() {
80+
LL | | /* foo is false */
81+
LL | | println!("foo is false");
82+
LL | | } else {
83+
LL | | println!("foo"); /* foo */
84+
LL | | }
85+
| |_____^
86+
|
87+
help: try
88+
|
89+
LL ~ if foo() {
90+
LL + println!("foo"); /* foo */
91+
LL + } else {
92+
LL + /* foo is false */
93+
LL + println!("foo is false");
94+
LL + }
95+
|
96+
97+
error: unnecessary boolean `not` operation
98+
--> tests/ui/if_not_else.rs:57:5
99+
|
100+
LL | / if !bla() {
101+
LL | | // bla is false
102+
LL | | println!("bla");
103+
LL | | } else {
104+
LL | | println!("bla"); // bla
105+
LL | | }
106+
| |_____^
107+
|
108+
help: try
109+
|
110+
LL ~ if bla() {
111+
LL + println!("bla"); // bla
112+
LL + } else {
113+
LL + // bla is false
114+
LL + println!("bla");
115+
LL + }
116+
|
117+
118+
error: unnecessary boolean `not` operation
119+
--> tests/ui/if_not_else.rs:67:5
120+
|
121+
LL | / if !foo() {
122+
LL | | /* foo is false */
123+
LL | | println!("foo is false");
124+
LL | | } else {
125+
LL | | println!("foo"); /* foo */
126+
LL | | }
127+
| |_____^
128+
|
129+
help: try
130+
|
131+
LL ~ if foo() {
132+
LL + println!("foo"); /* foo */
133+
LL + } else {
134+
LL + /* foo is false */
135+
LL + println!("foo is false");
136+
LL + }
137+
|
28138

29-
error: aborting due to 2 previous errors
139+
error: aborting due to 6 previous errors
30140

0 commit comments

Comments
 (0)