Skip to content

Commit 0b30af0

Browse files
llogiqllogiq
authored
Fix useless_let_if_seq FN when if is in the last expr of block (#16063)
Closes #16062 Closes #16064 changelog: [`useless_let_if_seq`] fix FN when `if` is in the last expr of block
2 parents 3c3452a + e1ac7da commit 0b30af0

File tree

3 files changed

+132
-69
lines changed

3 files changed

+132
-69
lines changed

‎clippy_lints/src/let_if_seq.rs‎

Lines changed: 76 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use clippy_utils::diagnostics::span_lint_hir_and_then;
22
use clippy_utils::res::MaybeResPath;
3-
use clippy_utils::source::snippet;
3+
use clippy_utils::source::snippet_with_context;
44
use clippy_utils::visitors::is_local_used;
55
use rustc_errors::Applicability;
66
use rustc_hir as hir;
@@ -59,80 +59,88 @@ declare_lint_pass!(LetIfSeq => [USELESS_LET_IF_SEQ]);
5959
impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
6060
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
6161
for [stmt, next] in block.stmts.array_windows::<2>() {
62-
if let hir::StmtKind::Let(local) = stmt.kind
63-
&& let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind
64-
&& let hir::StmtKind::Expr(if_) = next.kind
65-
&& let hir::ExprKind::If(cond, then, else_) = if_.kind
66-
&& !is_local_used(cx, cond, canonical_id)
67-
&& let hir::ExprKind::Block(then, _) = then.kind
68-
&& let Some(value) = check_assign(cx, canonical_id, then)
69-
&& !is_local_used(cx, value, canonical_id)
70-
{
71-
let span = stmt.span.to(if_.span);
62+
if let hir::StmtKind::Expr(if_) = next.kind {
63+
check_block_inner(cx, stmt, if_);
64+
}
65+
}
7266

73-
let has_interior_mutability = !cx
74-
.typeck_results()
75-
.node_type(canonical_id)
76-
.is_freeze(cx.tcx, cx.typing_env());
77-
if has_interior_mutability {
78-
return;
79-
}
67+
if let Some(expr) = block.expr
68+
&& let Some(stmt) = block.stmts.last()
69+
{
70+
check_block_inner(cx, stmt, expr);
71+
}
72+
}
73+
}
74+
75+
fn check_block_inner<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx hir::Stmt<'tcx>, if_: &'tcx hir::Expr<'tcx>) {
76+
if let hir::StmtKind::Let(local) = stmt.kind
77+
&& let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind
78+
&& let hir::ExprKind::If(cond, then, else_) = if_.kind
79+
&& !is_local_used(cx, cond, canonical_id)
80+
&& let hir::ExprKind::Block(then, _) = then.kind
81+
&& let Some(value) = check_assign(cx, canonical_id, then)
82+
&& !is_local_used(cx, value, canonical_id)
83+
{
84+
let span = stmt.span.to(if_.span);
8085

81-
let (default_multi_stmts, default) = if let Some(else_) = else_ {
82-
if let hir::ExprKind::Block(else_, _) = else_.kind {
83-
if let Some(default) = check_assign(cx, canonical_id, else_) {
84-
(else_.stmts.len() > 1, default)
85-
} else if let Some(default) = local.init {
86-
(true, default)
87-
} else {
88-
continue;
89-
}
90-
} else {
91-
continue;
92-
}
86+
let has_interior_mutability = !cx
87+
.typeck_results()
88+
.node_type(canonical_id)
89+
.is_freeze(cx.tcx, cx.typing_env());
90+
if has_interior_mutability {
91+
return;
92+
}
93+
94+
let (default_multi_stmts, default) = if let Some(else_) = else_ {
95+
if let hir::ExprKind::Block(else_, _) = else_.kind {
96+
if let Some(default) = check_assign(cx, canonical_id, else_) {
97+
(else_.stmts.len() > 1, default)
9398
} else if let Some(default) = local.init {
94-
(false, default)
99+
(true, default)
95100
} else {
96-
continue;
97-
};
101+
return;
102+
}
103+
} else {
104+
return;
105+
}
106+
} else if let Some(default) = local.init {
107+
(false, default)
108+
} else {
109+
return;
110+
};
98111

99-
let mutability = match mode {
100-
BindingMode(_, Mutability::Mut) => "<mut> ",
101-
_ => "",
102-
};
112+
let mutability = match mode {
113+
BindingMode(_, Mutability::Mut) => "<mut> ",
114+
_ => "",
115+
};
103116

104-
// FIXME: this should not suggest `mut` if we can detect that the variable is not
105-
// use mutably after the `if`
117+
// FIXME: this should not suggest `mut` if we can detect that the variable is not
118+
// use mutably after the `if`
106119

107-
let sug = format!(
108-
"let {mutability}{name} = if {cond} {{{then} {value} }} else {{{else} {default} }};",
109-
name=ident.name,
110-
cond=snippet(cx, cond.span, "_"),
111-
then=if then.stmts.len() > 1 { " ..;" } else { "" },
112-
else=if default_multi_stmts { " ..;" } else { "" },
113-
value=snippet(cx, value.span, "<value>"),
114-
default=snippet(cx, default.span, "<default>"),
115-
);
116-
span_lint_hir_and_then(
117-
cx,
118-
USELESS_LET_IF_SEQ,
119-
local.hir_id,
120-
span,
121-
"`if _ { .. } else { .. }` is an expression",
122-
|diag| {
123-
diag.span_suggestion(
124-
span,
125-
"it is more idiomatic to write",
126-
sug,
127-
Applicability::HasPlaceholders,
128-
);
129-
if !mutability.is_empty() {
130-
diag.note("you might not need `mut` at all");
131-
}
132-
},
133-
);
134-
}
135-
}
120+
let mut applicability = Applicability::HasPlaceholders;
121+
let (cond_snip, _) = snippet_with_context(cx, cond.span, if_.span.ctxt(), "_", &mut applicability);
122+
let (value_snip, _) = snippet_with_context(cx, value.span, if_.span.ctxt(), "<value>", &mut applicability);
123+
let (default_snip, _) =
124+
snippet_with_context(cx, default.span, if_.span.ctxt(), "<default>", &mut applicability);
125+
let sug = format!(
126+
"let {mutability}{name} = if {cond_snip} {{{then} {value_snip} }} else {{{else} {default_snip} }};",
127+
name=ident.name,
128+
then=if then.stmts.len() > 1 { " ..;" } else { "" },
129+
else=if default_multi_stmts { " ..;" } else { "" },
130+
);
131+
span_lint_hir_and_then(
132+
cx,
133+
USELESS_LET_IF_SEQ,
134+
local.hir_id,
135+
span,
136+
"`if _ { .. } else { .. }` is an expression",
137+
|diag| {
138+
diag.span_suggestion(span, "it is more idiomatic to write", sug, applicability);
139+
if !mutability.is_empty() {
140+
diag.note("you might not need `mut` at all");
141+
}
142+
},
143+
);
136144
}
137145
}
138146

‎tests/ui/let_if_seq.rs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,3 +139,34 @@ fn main() {
139139
}
140140
println!("{}", val.get());
141141
}
142+
143+
fn issue16062(bar: fn() -> bool) {
144+
let foo;
145+
//~^ useless_let_if_seq
146+
if bar() {
147+
foo = 42;
148+
} else {
149+
foo = 0;
150+
}
151+
}
152+
153+
fn issue16064(bar: fn() -> bool) {
154+
macro_rules! mac {
155+
($e:expr) => {
156+
$e()
157+
};
158+
($base:expr, $lit:expr) => {
159+
$lit * $base + 2
160+
};
161+
}
162+
163+
let foo;
164+
//~^ useless_let_if_seq
165+
if mac!(bar) {
166+
foo = mac!(10, 4);
167+
} else {
168+
foo = 0;
169+
}
170+
171+
let bar = 1;
172+
}

‎tests/ui/let_if_seq.stderr‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,5 +52,29 @@ LL | | }
5252
|
5353
= note: you might not need `mut` at all
5454

55-
error: aborting due to 4 previous errors
55+
error: `if _ { .. } else { .. }` is an expression
56+
--> tests/ui/let_if_seq.rs:144:5
57+
|
58+
LL | / let foo;
59+
LL | |
60+
LL | | if bar() {
61+
LL | | foo = 42;
62+
LL | | } else {
63+
LL | | foo = 0;
64+
LL | | }
65+
| |_____^ help: it is more idiomatic to write: `let foo = if bar() { 42 } else { 0 };`
66+
67+
error: `if _ { .. } else { .. }` is an expression
68+
--> tests/ui/let_if_seq.rs:163:5
69+
|
70+
LL | / let foo;
71+
LL | |
72+
LL | | if mac!(bar) {
73+
LL | | foo = mac!(10, 4);
74+
LL | | } else {
75+
LL | | foo = 0;
76+
LL | | }
77+
| |_____^ help: it is more idiomatic to write: `let foo = if mac!(bar) { mac!(10, 4) } else { 0 };`
78+
79+
error: aborting due to 6 previous errors
5680

0 commit comments

Comments
 (0)