From 8a1dde1449dc45aa56a8438228a6e8b0f8bc8a7c Mon Sep 17 00:00:00 2001 From: Linshu Yang Date: Mon, 10 Nov 2025 19:30:50 +0000 Subject: [PATCH 1/2] fix: `useless_let_if_seq` FN when `if` is in the last expr of block --- clippy_lints/src/let_if_seq.rs | 141 ++++++++++++++++++--------------- tests/ui/let_if_seq.rs | 10 +++ tests/ui/let_if_seq.stderr | 14 +++- 3 files changed, 99 insertions(+), 66 deletions(-) diff --git a/clippy_lints/src/let_if_seq.rs b/clippy_lints/src/let_if_seq.rs index 2dbf55a8540b..1e72fc5618f9 100644 --- a/clippy_lints/src/let_if_seq.rs +++ b/clippy_lints/src/let_if_seq.rs @@ -59,80 +59,91 @@ declare_lint_pass!(LetIfSeq => [USELESS_LET_IF_SEQ]); impl<'tcx> LateLintPass<'tcx> for LetIfSeq { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) { for [stmt, next] in block.stmts.array_windows::<2>() { - if let hir::StmtKind::Let(local) = stmt.kind - && let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind - && let hir::StmtKind::Expr(if_) = next.kind - && let hir::ExprKind::If(cond, then, else_) = if_.kind - && !is_local_used(cx, cond, canonical_id) - && let hir::ExprKind::Block(then, _) = then.kind - && let Some(value) = check_assign(cx, canonical_id, then) - && !is_local_used(cx, value, canonical_id) - { - let span = stmt.span.to(if_.span); + if let hir::StmtKind::Expr(if_) = next.kind { + check_block_inner(cx, stmt, if_); + } + } - let has_interior_mutability = !cx - .typeck_results() - .node_type(canonical_id) - .is_freeze(cx.tcx, cx.typing_env()); - if has_interior_mutability { - return; - } + if let Some(expr) = block.expr + && let Some(stmt) = block.stmts.last() + { + check_block_inner(cx, stmt, expr); + } + } +} + +fn check_block_inner<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx hir::Stmt<'tcx>, if_: &'tcx hir::Expr<'tcx>) { + if let hir::StmtKind::Let(local) = stmt.kind + && let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind + && let hir::ExprKind::If(cond, then, else_) = if_.kind + && !is_local_used(cx, cond, canonical_id) + && let hir::ExprKind::Block(then, _) = then.kind + && let Some(value) = check_assign(cx, canonical_id, then) + && !is_local_used(cx, value, canonical_id) + { + let span = stmt.span.to(if_.span); - let (default_multi_stmts, default) = if let Some(else_) = else_ { - if let hir::ExprKind::Block(else_, _) = else_.kind { - if let Some(default) = check_assign(cx, canonical_id, else_) { - (else_.stmts.len() > 1, default) - } else if let Some(default) = local.init { - (true, default) - } else { - continue; - } - } else { - continue; - } + let has_interior_mutability = !cx + .typeck_results() + .node_type(canonical_id) + .is_freeze(cx.tcx, cx.typing_env()); + if has_interior_mutability { + return; + } + + let (default_multi_stmts, default) = if let Some(else_) = else_ { + if let hir::ExprKind::Block(else_, _) = else_.kind { + if let Some(default) = check_assign(cx, canonical_id, else_) { + (else_.stmts.len() > 1, default) } else if let Some(default) = local.init { - (false, default) + (true, default) } else { - continue; - }; + return; + } + } else { + return; + } + } else if let Some(default) = local.init { + (false, default) + } else { + return; + }; - let mutability = match mode { - BindingMode(_, Mutability::Mut) => " ", - _ => "", - }; + let mutability = match mode { + BindingMode(_, Mutability::Mut) => " ", + _ => "", + }; - // FIXME: this should not suggest `mut` if we can detect that the variable is not - // use mutably after the `if` + // FIXME: this should not suggest `mut` if we can detect that the variable is not + // use mutably after the `if` - let sug = format!( - "let {mutability}{name} = if {cond} {{{then} {value} }} else {{{else} {default} }};", - name=ident.name, - cond=snippet(cx, cond.span, "_"), - then=if then.stmts.len() > 1 { " ..;" } else { "" }, - else=if default_multi_stmts { " ..;" } else { "" }, - value=snippet(cx, value.span, ""), - default=snippet(cx, default.span, ""), - ); - span_lint_hir_and_then( - cx, - USELESS_LET_IF_SEQ, - local.hir_id, + let sug = format!( + "let {mutability}{name} = if {cond} {{{then} {value} }} else {{{else} {default} }};", + name=ident.name, + cond=snippet(cx, cond.span, "_"), + then=if then.stmts.len() > 1 { " ..;" } else { "" }, + else=if default_multi_stmts { " ..;" } else { "" }, + value=snippet(cx, value.span, ""), + default=snippet(cx, default.span, ""), + ); + span_lint_hir_and_then( + cx, + USELESS_LET_IF_SEQ, + local.hir_id, + span, + "`if _ { .. } else { .. }` is an expression", + |diag| { + diag.span_suggestion( span, - "`if _ { .. } else { .. }` is an expression", - |diag| { - diag.span_suggestion( - span, - "it is more idiomatic to write", - sug, - Applicability::HasPlaceholders, - ); - if !mutability.is_empty() { - diag.note("you might not need `mut` at all"); - } - }, + "it is more idiomatic to write", + sug, + Applicability::HasPlaceholders, ); - } - } + if !mutability.is_empty() { + diag.note("you might not need `mut` at all"); + } + }, + ); } } diff --git a/tests/ui/let_if_seq.rs b/tests/ui/let_if_seq.rs index 2db206212aa5..144abf578cf1 100644 --- a/tests/ui/let_if_seq.rs +++ b/tests/ui/let_if_seq.rs @@ -139,3 +139,13 @@ fn main() { } println!("{}", val.get()); } + +fn issue16062(bar: fn() -> bool) { + let foo; + //~^ useless_let_if_seq + if bar() { + foo = 42; + } else { + foo = 0; + } +} diff --git a/tests/ui/let_if_seq.stderr b/tests/ui/let_if_seq.stderr index f59d42bf4c8d..d7f600adb626 100644 --- a/tests/ui/let_if_seq.stderr +++ b/tests/ui/let_if_seq.stderr @@ -52,5 +52,17 @@ LL | | } | = note: you might not need `mut` at all -error: aborting due to 4 previous errors +error: `if _ { .. } else { .. }` is an expression + --> tests/ui/let_if_seq.rs:144:5 + | +LL | / let foo; +LL | | +LL | | if bar() { +LL | | foo = 42; +LL | | } else { +LL | | foo = 0; +LL | | } + | |_____^ help: it is more idiomatic to write: `let foo = if bar() { 42 } else { 0 };` + +error: aborting due to 5 previous errors From e1ac7da6a1542b02b376cce56c2668a17fce0329 Mon Sep 17 00:00:00 2001 From: Linshu Yang Date: Mon, 10 Nov 2025 19:59:21 +0000 Subject: [PATCH 2/2] fix: `useless_let_if_seq` wrongly unmangled macros --- clippy_lints/src/let_if_seq.rs | 19 ++++++++----------- tests/ui/let_if_seq.rs | 21 +++++++++++++++++++++ tests/ui/let_if_seq.stderr | 14 +++++++++++++- 3 files changed, 42 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/let_if_seq.rs b/clippy_lints/src/let_if_seq.rs index 1e72fc5618f9..dc7a916614be 100644 --- a/clippy_lints/src/let_if_seq.rs +++ b/clippy_lints/src/let_if_seq.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::res::MaybeResPath; -use clippy_utils::source::snippet; +use clippy_utils::source::snippet_with_context; use clippy_utils::visitors::is_local_used; use rustc_errors::Applicability; use rustc_hir as hir; @@ -117,14 +117,16 @@ fn check_block_inner<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx hir::Stmt<'tcx>, // FIXME: this should not suggest `mut` if we can detect that the variable is not // use mutably after the `if` + let mut applicability = Applicability::HasPlaceholders; + let (cond_snip, _) = snippet_with_context(cx, cond.span, if_.span.ctxt(), "_", &mut applicability); + let (value_snip, _) = snippet_with_context(cx, value.span, if_.span.ctxt(), "", &mut applicability); + let (default_snip, _) = + snippet_with_context(cx, default.span, if_.span.ctxt(), "", &mut applicability); let sug = format!( - "let {mutability}{name} = if {cond} {{{then} {value} }} else {{{else} {default} }};", + "let {mutability}{name} = if {cond_snip} {{{then} {value_snip} }} else {{{else} {default_snip} }};", name=ident.name, - cond=snippet(cx, cond.span, "_"), then=if then.stmts.len() > 1 { " ..;" } else { "" }, else=if default_multi_stmts { " ..;" } else { "" }, - value=snippet(cx, value.span, ""), - default=snippet(cx, default.span, ""), ); span_lint_hir_and_then( cx, @@ -133,12 +135,7 @@ fn check_block_inner<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx hir::Stmt<'tcx>, span, "`if _ { .. } else { .. }` is an expression", |diag| { - diag.span_suggestion( - span, - "it is more idiomatic to write", - sug, - Applicability::HasPlaceholders, - ); + diag.span_suggestion(span, "it is more idiomatic to write", sug, applicability); if !mutability.is_empty() { diag.note("you might not need `mut` at all"); } diff --git a/tests/ui/let_if_seq.rs b/tests/ui/let_if_seq.rs index 144abf578cf1..69d6319fa8bf 100644 --- a/tests/ui/let_if_seq.rs +++ b/tests/ui/let_if_seq.rs @@ -149,3 +149,24 @@ fn issue16062(bar: fn() -> bool) { foo = 0; } } + +fn issue16064(bar: fn() -> bool) { + macro_rules! mac { + ($e:expr) => { + $e() + }; + ($base:expr, $lit:expr) => { + $lit * $base + 2 + }; + } + + let foo; + //~^ useless_let_if_seq + if mac!(bar) { + foo = mac!(10, 4); + } else { + foo = 0; + } + + let bar = 1; +} diff --git a/tests/ui/let_if_seq.stderr b/tests/ui/let_if_seq.stderr index d7f600adb626..b86bca6b384b 100644 --- a/tests/ui/let_if_seq.stderr +++ b/tests/ui/let_if_seq.stderr @@ -64,5 +64,17 @@ LL | | foo = 0; LL | | } | |_____^ help: it is more idiomatic to write: `let foo = if bar() { 42 } else { 0 };` -error: aborting due to 5 previous errors +error: `if _ { .. } else { .. }` is an expression + --> tests/ui/let_if_seq.rs:163:5 + | +LL | / let foo; +LL | | +LL | | if mac!(bar) { +LL | | foo = mac!(10, 4); +LL | | } else { +LL | | foo = 0; +LL | | } + | |_____^ help: it is more idiomatic to write: `let foo = if mac!(bar) { mac!(10, 4) } else { 0 };` + +error: aborting due to 6 previous errors