diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 389b0a4a62dc..59e421c16220 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -6,17 +6,18 @@ use clippy_utils::source::snippet_with_context; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{Descend, Visitable}; use if_chain::if_chain; -use rustc_data_structures::fx::FxHashSet; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, Visitor}; use rustc_hir::{Expr, ExprKind, HirId, ItemId, Local, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind, Ty}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::symbol::sym; +use rustc_span::symbol::{sym, Symbol}; use rustc_span::Span; use serde::Deserialize; use std::ops::ControlFlow; +use std::slice; declare_clippy_lint! { /// ### What it does @@ -81,11 +82,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { { match if_let_or_match { IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! { - if expr_is_simple_identity(let_pat, if_then); + if let Some(ident_map) = expr_simple_identity_map(local.pat, let_pat, if_then); if let Some(if_else) = if_else; if expr_diverges(cx, if_else); then { - emit_manual_let_else(cx, stmt.span, if_let_expr, local.pat, let_pat, if_else); + emit_manual_let_else(cx, stmt.span, if_let_expr, &ident_map, let_pat, if_else); } }, IfLetOrMatch::Match(match_expr, arms, source) => { @@ -118,11 +119,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { return; } let pat_arm = &arms[1 - idx]; - if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) { - return; - } + let Some(ident_map) = expr_simple_identity_map(local.pat, pat_arm.pat, pat_arm.body) else { + return + }; - emit_manual_let_else(cx, stmt.span, match_expr, local.pat, pat_arm.pat, diverging_arm.body); + emit_manual_let_else(cx, stmt.span, match_expr, &ident_map, pat_arm.pat, diverging_arm.body); }, } }; @@ -135,7 +136,7 @@ fn emit_manual_let_else( cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, - local: &Pat<'_>, + ident_map: &FxHashMap>, pat: &Pat<'_>, else_body: &Expr<'_>, ) { @@ -146,8 +147,8 @@ fn emit_manual_let_else( "this could be rewritten as `let...else`", |diag| { // This is far from perfect, for example there needs to be: - // * tracking for multi-binding cases: let (foo, bar) = if let (Some(foo), Ok(bar)) = ... - // * renamings of the bindings for many `PatKind`s like structs, slices, etc. + // * renamings of the bindings for many `PatKind`s like slices, etc. + // * limitations in the existing replacement algorithms // * unused binding collision detection with existing ones // for this to be machine applicable. let mut app = Applicability::HasPlaceholders; @@ -159,57 +160,126 @@ fn emit_manual_let_else( } else { format!("{{ {sn_else} }}") }; - let sn_bl = replace_in_pattern(cx, span, local, pat, &mut app); + let sn_bl = replace_in_pattern(cx, span, ident_map, pat, &mut app, true); let sugg = format!("let {sn_bl} = {sn_expr} else {else_bl};"); diag.span_suggestion(span, "consider writing", sugg, app); }, ); } -// replaces the locals in the pattern +/// Replaces the locals in the pattern +/// +/// For this example: +/// +/// ```ignore +/// let (a, FooBar { b, c }) = if let Bar { Some(a_i), b_i } = ex { (a_i, b_i) } else { return }; +/// ``` +/// +/// We have: +/// +/// ```ignore +/// pat: Bar { Some(a_i), b_i } +/// ident_map: (a_i) -> (a), (b_i) -> (FooBar { b, c }) +/// ``` +/// +/// We return: +/// +/// ```ignore +/// Bar { Some(a), b_i: FooBar { b, c } } +/// ``` fn replace_in_pattern( cx: &LateContext<'_>, span: Span, - local: &Pat<'_>, + ident_map: &FxHashMap>, pat: &Pat<'_>, app: &mut Applicability, + top_level: bool, ) -> String { - let mut bindings_count = 0; - pat.each_binding_or_first(&mut |_, _, _, _| bindings_count += 1); - // If the pattern creates multiple bindings, exit early, - // as otherwise we might paste the pattern to the positions of multiple bindings. - if bindings_count > 1 { - let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app); - return sn_pat.into_owned(); - } + // We put a labeled block here so that we can implement the fallback in this function. + // As the function has multiple call sites, implementing the fallback via an Option + // return type and unwrap_or_else would cause repetition. Similarly, the function also + // invokes the fall back multiple times. + 'a: { + // If the ident map is empty, there is no replacement to do. + // The code following this if assumes a non-empty ident_map. + if ident_map.is_empty() { + break 'a; + } - match pat.kind { - PatKind::Binding(..) => { - let (sn_bdg, _) = snippet_with_context(cx, local.span, span.ctxt(), "", app); - return sn_bdg.to_string(); - }, - PatKind::Or(pats) => { - let patterns = pats - .iter() - .map(|pat| replace_in_pattern(cx, span, local, pat, app)) - .collect::>(); - let or_pat = patterns.join(" | "); - return format!("({or_pat})"); - }, - // Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`. - PatKind::TupleStruct(ref w, args, dot_dot_pos) => { - let mut args = args - .iter() - .map(|pat| replace_in_pattern(cx, span, local, pat, app)) - .collect::>(); - if let Some(pos) = dot_dot_pos.as_opt_usize() { - args.insert(pos, "..".to_owned()); - } - let args = args.join(", "); - let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default(); - return format!("{sn_wrapper}({args})"); - }, - _ => {}, + match pat.kind { + PatKind::Binding(_ann, _id, binding_name, opt_subpt) => { + let Some(pat_to_put) = ident_map.get(&binding_name.name) else { break 'a }; + let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app); + if let Some(subpt) = opt_subpt { + let subpt = replace_in_pattern(cx, span, ident_map, subpt, app, false); + return format!("{sn_ptp} @ {subpt}"); + } + return sn_ptp.to_string(); + }, + PatKind::Or(pats) => { + let patterns = pats + .iter() + .map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false)) + .collect::>(); + let or_pat = patterns.join(" | "); + if top_level { + return format!("({or_pat})"); + } + return or_pat; + }, + PatKind::Struct(path, fields, has_dot_dot) => { + let fields = fields + .iter() + .map(|fld| { + if let PatKind::Binding(_, _, name, None) = fld.pat.kind && + let Some(pat_to_put) = ident_map.get(&name.name) + { + let (sn_fld_name, _) = snippet_with_context(cx, fld.ident.span, span.ctxt(), "", app); + let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app); + // TODO: this is a bit of a hack, but it does its job. Ideally, we'd check if pat_to_put is + // a PatKind::Binding but that is also hard to get right. + if sn_fld_name == sn_ptp { + // Field init shorthand + return format!("{sn_fld_name}"); + } + return format!("{sn_fld_name}: {sn_ptp}"); + } + let (sn_fld, _) = snippet_with_context(cx, fld.span, span.ctxt(), "", app); + sn_fld.into_owned() + }) + .collect::>(); + let fields_string = fields.join(", "); + + let dot_dot_str = if has_dot_dot { " .." } else { "" }; + let (sn_pth, _) = snippet_with_context(cx, path.span(), span.ctxt(), "", app); + return format!("{sn_pth} {{ {fields_string}{dot_dot_str} }}"); + }, + // Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`. + PatKind::TupleStruct(ref w, args, dot_dot_pos) => { + let mut args = args + .iter() + .map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false)) + .collect::>(); + if let Some(pos) = dot_dot_pos.as_opt_usize() { + args.insert(pos, "..".to_owned()); + } + let args = args.join(", "); + let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default(); + return format!("{sn_wrapper}({args})"); + }, + PatKind::Tuple(args, dot_dot_pos) => { + let mut args = args + .iter() + .map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false)) + .collect::>(); + if let Some(pos) = dot_dot_pos.as_opt_usize() { + args.insert(pos, "..".to_owned()); + } + let args = args.join(", "); + return format!("({args})"); + }, + _ => {}, + } } let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app); sn_pat.into_owned() @@ -353,37 +423,74 @@ fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>, check_types: boo !has_disallowed } -/// Checks if the passed block is a simple identity referring to bindings created by the pattern -fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool { - // We support patterns with multiple bindings and tuples, like: - // let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... } +/// Checks if the passed block is a simple identity referring to bindings created by the pattern, +/// and if yes, returns a mapping between the relevant sub-pattern and the identifier it corresponds +/// to. +/// +/// We support patterns with multiple bindings and tuples, e.g.: +/// +/// ```ignore +/// let (foo_o, bar_o) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... } +/// ``` +/// +/// The expected params would be: +/// +/// ```ignore +/// local_pat: (foo_o, bar_o) +/// let_pat: (Some(foo), bar) +/// expr: (foo, bar) +/// ``` +/// +/// We build internal `sub_pats` so that it looks like `[foo_o, bar_o]` and `paths` so that it looks +/// like `[foo, bar]`. Then we turn that into `FxHashMap [(foo) -> (foo_o), (bar) -> (bar_o)]` which +/// we return. +fn expr_simple_identity_map<'a, 'hir>( + local_pat: &'a Pat<'hir>, + let_pat: &'_ Pat<'hir>, + expr: &'_ Expr<'hir>, +) -> Option>> { let peeled = peel_blocks(expr); - let paths = match peeled.kind { - ExprKind::Tup(exprs) | ExprKind::Array(exprs) => exprs, - ExprKind::Path(_) => std::slice::from_ref(peeled), - _ => return false, + let (sub_pats, paths) = match (local_pat.kind, peeled.kind) { + (PatKind::Tuple(pats, _), ExprKind::Tup(exprs)) | (PatKind::Slice(pats, ..), ExprKind::Array(exprs)) => { + (pats, exprs) + }, + (_, ExprKind::Path(_)) => (slice::from_ref(local_pat), slice::from_ref(peeled)), + _ => return None, }; + + // There is some length mismatch, which indicates usage of .. in the patterns above e.g.: + // let (a, ..) = if let [a, b, _c] = ex { (a, b) } else { ... }; + // We bail in these cases as they should be rare. + if paths.len() != sub_pats.len() { + return None; + } + let mut pat_bindings = FxHashSet::default(); - pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| { + let_pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| { pat_bindings.insert(ident); }); if pat_bindings.len() < paths.len() { - return false; + // This rebinds some bindings from the outer scope, or it repeats some copy-able bindings multiple + // times. We don't support these cases so we bail here. E.g.: + // let foo = 0; + // let (new_foo, bar, bar_copied) = if let Some(bar) = Some(0) { (foo, bar, bar) } else { .. }; + return None; } - for path in paths { - if_chain! { - if let ExprKind::Path(QPath::Resolved(_ty, path)) = path.kind; - if let [path_seg] = path.segments; - then { - if !pat_bindings.remove(&path_seg.ident) { - return false; - } - } else { - return false; + let mut ident_map = FxHashMap::default(); + for (sub_pat, path) in sub_pats.iter().zip(paths.iter()) { + if let ExprKind::Path(QPath::Resolved(_ty, path)) = path.kind && + let [path_seg] = path.segments + { + let ident = path_seg.ident; + if !pat_bindings.remove(&ident) { + return None; } + ident_map.insert(ident.name, sub_pat); + } else { + return None; } } - true + Some(ident_map) } #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Deserialize)] diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index 351ea0e4f509..b5c2a5dba34b 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -127,8 +127,8 @@ fn fire() { return; }; - // Tuples supported for the identity block and pattern - let v = if let (Some(v_some), w_some) = (g(), 0) { + // Tuples supported with multiple bindings + let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) { (w_some, v_some) } else { return; @@ -160,6 +160,30 @@ fn fire() { }; // dot dot works let v = if let Variant::A(.., a) = e() { a } else { return }; + + // () is preserved: a bit of an edge case but make sure it stays around + let w = if let (Some(v), ()) = (g(), ()) { v } else { return }; + + // Tuple structs work + let w = if let Some(S { v: x }) = Some(S { v: 0 }) { + x + } else { + return; + }; + + // Field init shorthand is suggested + let v = if let Some(S { v: x }) = Some(S { v: 0 }) { + x + } else { + return; + }; + + // Multi-field structs also work + let (x, S { v }, w) = if let Some(U { v, w, x }) = None::>> { + (x, v, w) + } else { + return; + }; } fn not_fire() { @@ -284,4 +308,23 @@ fn not_fire() { }; 1 }; + + // This would require creation of a suggestion of the form + // let v @ (Some(_), _) = (...) else { return }; + // Which is too advanced for our code, so we just bail. + let v = if let (Some(v_some), w_some) = (g(), 0) { + (w_some, v_some) + } else { + return; + }; +} + +struct S { + v: T, +} + +struct U { + v: T, + w: T, + x: T, } diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index 0e8767971347..9d4aef4cad66 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -234,7 +234,7 @@ LL + }; error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:131:5 | -LL | / let v = if let (Some(v_some), w_some) = (g(), 0) { +LL | / let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) { LL | | (w_some, v_some) LL | | } else { LL | | return; @@ -243,7 +243,7 @@ LL | | }; | help: consider writing | -LL ~ let (Some(v_some), w_some) = (g(), 0) else { +LL ~ let (Some(S { v }), w) = (g().map(|_| S { v: 0 }), 0) else { LL + return; LL + }; | @@ -295,7 +295,64 @@ LL | let v = if let Variant::A(.., a) = e() { a } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(.., v) = e() else { return };` error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:272:5 + --> $DIR/manual_let_else.rs:165:5 + | +LL | let w = if let (Some(v), ()) = (g(), ()) { v } else { return }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let (Some(w), ()) = (g(), ()) else { return };` + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:168:5 + | +LL | / let w = if let Some(S { v: x }) = Some(S { v: 0 }) { +LL | | x +LL | | } else { +LL | | return; +LL | | }; + | |______^ + | +help: consider writing + | +LL ~ let Some(S { v: w }) = Some(S { v: 0 }) else { +LL + return; +LL + }; + | + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:175:5 + | +LL | / let v = if let Some(S { v: x }) = Some(S { v: 0 }) { +LL | | x +LL | | } else { +LL | | return; +LL | | }; + | |______^ + | +help: consider writing + | +LL ~ let Some(S { v }) = Some(S { v: 0 }) else { +LL + return; +LL + }; + | + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:182:5 + | +LL | / let (x, S { v }, w) = if let Some(U { v, w, x }) = None::>> { +LL | | (x, v, w) +LL | | } else { +LL | | return; +LL | | }; + | |______^ + | +help: consider writing + | +LL ~ let Some(U { v: S { v }, w, x }) = None::>> else { +LL + return; +LL + }; + | + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:296:5 | LL | / let _ = match ff { LL | | Some(value) => value, @@ -303,5 +360,5 @@ LL | | _ => macro_call!(), LL | | }; | |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };` -error: aborting due to 22 previous errors +error: aborting due to 26 previous errors diff --git a/tests/ui/manual_let_else_match.rs b/tests/ui/manual_let_else_match.rs index dfca3b023cd5..07cd5a53a7c4 100644 --- a/tests/ui/manual_let_else_match.rs +++ b/tests/ui/manual_let_else_match.rs @@ -72,6 +72,11 @@ fn fire() { _ => return, }; + let _value = match Some(build_enum()) { + Some(Variant::Bar(v) | Variant::Baz(v)) => v, + _ => return, + }; + let data = [1_u8, 2, 3, 4, 0, 0, 0, 0]; let data = match data.as_slice() { [data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0] => data, diff --git a/tests/ui/manual_let_else_match.stderr b/tests/ui/manual_let_else_match.stderr index 13ed35bc1d5d..ead2f28e609a 100644 --- a/tests/ui/manual_let_else_match.stderr +++ b/tests/ui/manual_let_else_match.stderr @@ -64,7 +64,16 @@ LL | | }; | |______^ help: consider writing: `let (Variant::Bar(_value) | Variant::Baz(_value)) = f else { return };` error: this could be rewritten as `let...else` - --> $DIR/manual_let_else_match.rs:76:5 + --> $DIR/manual_let_else_match.rs:75:5 + | +LL | / let _value = match Some(build_enum()) { +LL | | Some(Variant::Bar(v) | Variant::Baz(v)) => v, +LL | | _ => return, +LL | | }; + | |______^ help: consider writing: `let Some(Variant::Bar(_value) | Variant::Baz(_value)) = Some(build_enum()) else { return };` + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else_match.rs:81:5 | LL | / let data = match data.as_slice() { LL | | [data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0] => data, @@ -72,5 +81,5 @@ LL | | _ => return, LL | | }; | |______^ help: consider writing: `let ([data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0]) = data.as_slice() else { return };` -error: aborting due to 8 previous errors +error: aborting due to 9 previous errors