From cd682446535c16b8c2d0b0e323bb8ee210371604 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Mon, 23 Jun 2025 10:57:51 -0700 Subject: [PATCH] Handle potentially-shadowing bindings in manual_let_else This commit also adds more test cases, which work fine but were mentioned in the issue. --- clippy_lints/src/manual_let_else.rs | 39 ++++++++++---- tests/ui/manual_let_else_match.fixed | 45 +++++++++++++++++ tests/ui/manual_let_else_match.rs | 73 +++++++++++++++++++++++++++ tests/ui/manual_let_else_match.stderr | 72 +++++++++++++++++++++++++- 4 files changed, 218 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 9ff82cdcb664..1f9a943f13dc 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -245,17 +245,36 @@ fn replace_in_pattern( } match pat.kind { - PatKind::Binding(_ann, _id, binding_name, opt_subpt) => { - let Some((pat_to_put, binding_mode)) = ident_map.get(&binding_name.name) else { - break 'a; - }; - let sn_pfx = binding_mode.prefix_str(); - 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_pfx}{sn_ptp} @ {subpt}"); + PatKind::Binding(ann, _id, binding_name, opt_subpt) => { + match (ident_map.get(&binding_name.name), opt_subpt) { + (Some((pat_to_put, binding_mode)), opt_subpt) => { + let sn_pfx = binding_mode.prefix_str(); + 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_pfx}{sn_ptp} @ {subpt}"); + } + return format!("{sn_pfx}{sn_ptp}"); + }, + (None, Some(subpt)) => { + let subpt = replace_in_pattern(cx, span, ident_map, subpt, app, false); + // scanning for a value that matches is not sensitive to order + #[expect(rustc::potential_query_instability)] + if ident_map.values().any(|(other_pat, _)| { + if let PatKind::Binding(_, _, other_name, _) = other_pat.kind { + other_name == binding_name + } else { + false + } + }) { + // this name is shadowed, and, therefore, not usable + return subpt; + } + let binding_pfx = ann.prefix_str(); + return format!("{binding_pfx}{binding_name} @ {subpt}"); + }, + (None, None) => break 'a, } - return format!("{sn_pfx}{sn_ptp}"); }, PatKind::Or(pats) => { let patterns = pats diff --git a/tests/ui/manual_let_else_match.fixed b/tests/ui/manual_let_else_match.fixed index 588ba5edd8f1..15f604aec292 100644 --- a/tests/ui/manual_let_else_match.fixed +++ b/tests/ui/manual_let_else_match.fixed @@ -137,3 +137,48 @@ fn not_fire() { fn issue11579() { let Some(msg) = Some("hi") else { unreachable!("can't happen") }; } + +#[derive(Clone, Copy)] +struct Issue9939 { + avalanche: T, +} + +fn issue9939() { + let issue = Some(Issue9939 { avalanche: 1 }); + let Some(Issue9939 { avalanche: tornado }) = issue else { unreachable!("can't happen") }; + let issue = Some(Issue9939 { avalanche: true }); + let Some(Issue9939 { avalanche: acid_rain }) = issue else { unreachable!("can't happen") }; + assert_eq!(tornado, 1); + assert!(acid_rain); + + // without shadowing + let _x @ Some(Issue9939 { avalanche: _y }) = issue else { unreachable!("can't happen") }; + + // with shadowing + let Some(Issue9939 { avalanche: _x }) = issue else { unreachable!("can't happen") }; +} + +#[derive(Clone, Copy)] +struct Issue9939b { + earthquake: T, + hurricane: U, +} + +fn issue9939b() { + let issue = Some(Issue9939b { + earthquake: true, + hurricane: 1, + }); + let issue @ Some(Issue9939b { earthquake: flood, hurricane: drought }) = issue else { unreachable!("can't happen") }; + assert_eq!(drought, 1); + assert!(flood); + assert!(issue.is_some()); + + // without shadowing + let _x @ Some(Issue9939b { earthquake: erosion, hurricane: _y }) = issue else { unreachable!("can't happen") }; + assert!(erosion); + + // with shadowing + let Some(Issue9939b { earthquake: erosion, hurricane: _x }) = issue else { unreachable!("can't happen") }; + assert!(erosion); +} diff --git a/tests/ui/manual_let_else_match.rs b/tests/ui/manual_let_else_match.rs index 6416753bac10..44a044b142bd 100644 --- a/tests/ui/manual_let_else_match.rs +++ b/tests/ui/manual_let_else_match.rs @@ -177,3 +177,76 @@ fn issue11579() { _ => unreachable!("can't happen"), }; } + +#[derive(Clone, Copy)] +struct Issue9939 { + avalanche: T, +} + +fn issue9939() { + let issue = Some(Issue9939 { avalanche: 1 }); + let tornado = match issue { + //~^ manual_let_else + Some(Issue9939 { avalanche }) => avalanche, + _ => unreachable!("can't happen"), + }; + let issue = Some(Issue9939 { avalanche: true }); + let acid_rain = match issue { + //~^ manual_let_else + Some(Issue9939 { avalanche: tornado }) => tornado, + _ => unreachable!("can't happen"), + }; + assert_eq!(tornado, 1); + assert!(acid_rain); + + // without shadowing + let _y = match issue { + //~^ manual_let_else + _x @ Some(Issue9939 { avalanche }) => avalanche, + None => unreachable!("can't happen"), + }; + + // with shadowing + let _x = match issue { + //~^ manual_let_else + _x @ Some(Issue9939 { avalanche }) => avalanche, + None => unreachable!("can't happen"), + }; +} + +#[derive(Clone, Copy)] +struct Issue9939b { + earthquake: T, + hurricane: U, +} + +fn issue9939b() { + let issue = Some(Issue9939b { + earthquake: true, + hurricane: 1, + }); + let (issue, drought, flood) = match issue { + //~^ manual_let_else + flood @ Some(Issue9939b { earthquake, hurricane }) => (flood, hurricane, earthquake), + None => unreachable!("can't happen"), + }; + assert_eq!(drought, 1); + assert!(flood); + assert!(issue.is_some()); + + // without shadowing + let (_y, erosion) = match issue { + //~^ manual_let_else + _x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake), + None => unreachable!("can't happen"), + }; + assert!(erosion); + + // with shadowing + let (_x, erosion) = match issue { + //~^ manual_let_else + _x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake), + None => unreachable!("can't happen"), + }; + assert!(erosion); +} diff --git a/tests/ui/manual_let_else_match.stderr b/tests/ui/manual_let_else_match.stderr index 393562c629ba..ed6117ebffb7 100644 --- a/tests/ui/manual_let_else_match.stderr +++ b/tests/ui/manual_let_else_match.stderr @@ -101,5 +101,75 @@ LL | | _ => unreachable!("can't happen"), LL | | }; | |______^ help: consider writing: `let Some(msg) = Some("hi") else { unreachable!("can't happen") };` -error: aborting due to 10 previous errors +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else_match.rs:188:5 + | +LL | / let tornado = match issue { +LL | | +LL | | Some(Issue9939 { avalanche }) => avalanche, +LL | | _ => unreachable!("can't happen"), +LL | | }; + | |______^ help: consider writing: `let Some(Issue9939 { avalanche: tornado }) = issue else { unreachable!("can't happen") };` + +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else_match.rs:194:5 + | +LL | / let acid_rain = match issue { +LL | | +LL | | Some(Issue9939 { avalanche: tornado }) => tornado, +LL | | _ => unreachable!("can't happen"), +LL | | }; + | |______^ help: consider writing: `let Some(Issue9939 { avalanche: acid_rain }) = issue else { unreachable!("can't happen") };` + +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else_match.rs:203:5 + | +LL | / let _y = match issue { +LL | | +LL | | _x @ Some(Issue9939 { avalanche }) => avalanche, +LL | | None => unreachable!("can't happen"), +LL | | }; + | |______^ help: consider writing: `let _x @ Some(Issue9939 { avalanche: _y }) = issue else { unreachable!("can't happen") };` + +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else_match.rs:210:5 + | +LL | / let _x = match issue { +LL | | +LL | | _x @ Some(Issue9939 { avalanche }) => avalanche, +LL | | None => unreachable!("can't happen"), +LL | | }; + | |______^ help: consider writing: `let Some(Issue9939 { avalanche: _x }) = issue else { unreachable!("can't happen") };` + +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else_match.rs:228:5 + | +LL | / let (issue, drought, flood) = match issue { +LL | | +LL | | flood @ Some(Issue9939b { earthquake, hurricane }) => (flood, hurricane, earthquake), +LL | | None => unreachable!("can't happen"), +LL | | }; + | |______^ help: consider writing: `let issue @ Some(Issue9939b { earthquake: flood, hurricane: drought }) = issue else { unreachable!("can't happen") };` + +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else_match.rs:238:5 + | +LL | / let (_y, erosion) = match issue { +LL | | +LL | | _x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake), +LL | | None => unreachable!("can't happen"), +LL | | }; + | |______^ help: consider writing: `let _x @ Some(Issue9939b { earthquake: erosion, hurricane: _y }) = issue else { unreachable!("can't happen") };` + +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else_match.rs:246:5 + | +LL | / let (_x, erosion) = match issue { +LL | | +LL | | _x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake), +LL | | None => unreachable!("can't happen"), +LL | | }; + | |______^ help: consider writing: `let Some(Issue9939b { earthquake: erosion, hurricane: _x }) = issue else { unreachable!("can't happen") };` + +error: aborting due to 17 previous errors