Skip to content

Commit cd68244

Browse files
committed
Handle potentially-shadowing bindings in manual_let_else
This commit also adds more test cases, which work fine but were mentioned in the issue.
1 parent 76118ec commit cd68244

File tree

4 files changed

+218
-11
lines changed

4 files changed

+218
-11
lines changed

clippy_lints/src/manual_let_else.rs

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -245,17 +245,36 @@ fn replace_in_pattern(
245245
}
246246

247247
match pat.kind {
248-
PatKind::Binding(_ann, _id, binding_name, opt_subpt) => {
249-
let Some((pat_to_put, binding_mode)) = ident_map.get(&binding_name.name) else {
250-
break 'a;
251-
};
252-
let sn_pfx = binding_mode.prefix_str();
253-
let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app);
254-
if let Some(subpt) = opt_subpt {
255-
let subpt = replace_in_pattern(cx, span, ident_map, subpt, app, false);
256-
return format!("{sn_pfx}{sn_ptp} @ {subpt}");
248+
PatKind::Binding(ann, _id, binding_name, opt_subpt) => {
249+
match (ident_map.get(&binding_name.name), opt_subpt) {
250+
(Some((pat_to_put, binding_mode)), opt_subpt) => {
251+
let sn_pfx = binding_mode.prefix_str();
252+
let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app);
253+
if let Some(subpt) = opt_subpt {
254+
let subpt = replace_in_pattern(cx, span, ident_map, subpt, app, false);
255+
return format!("{sn_pfx}{sn_ptp} @ {subpt}");
256+
}
257+
return format!("{sn_pfx}{sn_ptp}");
258+
},
259+
(None, Some(subpt)) => {
260+
let subpt = replace_in_pattern(cx, span, ident_map, subpt, app, false);
261+
// scanning for a value that matches is not sensitive to order
262+
#[expect(rustc::potential_query_instability)]
263+
if ident_map.values().any(|(other_pat, _)| {
264+
if let PatKind::Binding(_, _, other_name, _) = other_pat.kind {
265+
other_name == binding_name
266+
} else {
267+
false
268+
}
269+
}) {
270+
// this name is shadowed, and, therefore, not usable
271+
return subpt;
272+
}
273+
let binding_pfx = ann.prefix_str();
274+
return format!("{binding_pfx}{binding_name} @ {subpt}");
275+
},
276+
(None, None) => break 'a,
257277
}
258-
return format!("{sn_pfx}{sn_ptp}");
259278
},
260279
PatKind::Or(pats) => {
261280
let patterns = pats

tests/ui/manual_let_else_match.fixed

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,3 +137,48 @@ fn not_fire() {
137137
fn issue11579() {
138138
let Some(msg) = Some("hi") else { unreachable!("can't happen") };
139139
}
140+
141+
#[derive(Clone, Copy)]
142+
struct Issue9939<T> {
143+
avalanche: T,
144+
}
145+
146+
fn issue9939() {
147+
let issue = Some(Issue9939 { avalanche: 1 });
148+
let Some(Issue9939 { avalanche: tornado }) = issue else { unreachable!("can't happen") };
149+
let issue = Some(Issue9939 { avalanche: true });
150+
let Some(Issue9939 { avalanche: acid_rain }) = issue else { unreachable!("can't happen") };
151+
assert_eq!(tornado, 1);
152+
assert!(acid_rain);
153+
154+
// without shadowing
155+
let _x @ Some(Issue9939 { avalanche: _y }) = issue else { unreachable!("can't happen") };
156+
157+
// with shadowing
158+
let Some(Issue9939 { avalanche: _x }) = issue else { unreachable!("can't happen") };
159+
}
160+
161+
#[derive(Clone, Copy)]
162+
struct Issue9939b<T, U> {
163+
earthquake: T,
164+
hurricane: U,
165+
}
166+
167+
fn issue9939b() {
168+
let issue = Some(Issue9939b {
169+
earthquake: true,
170+
hurricane: 1,
171+
});
172+
let issue @ Some(Issue9939b { earthquake: flood, hurricane: drought }) = issue else { unreachable!("can't happen") };
173+
assert_eq!(drought, 1);
174+
assert!(flood);
175+
assert!(issue.is_some());
176+
177+
// without shadowing
178+
let _x @ Some(Issue9939b { earthquake: erosion, hurricane: _y }) = issue else { unreachable!("can't happen") };
179+
assert!(erosion);
180+
181+
// with shadowing
182+
let Some(Issue9939b { earthquake: erosion, hurricane: _x }) = issue else { unreachable!("can't happen") };
183+
assert!(erosion);
184+
}

tests/ui/manual_let_else_match.rs

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,3 +177,76 @@ fn issue11579() {
177177
_ => unreachable!("can't happen"),
178178
};
179179
}
180+
181+
#[derive(Clone, Copy)]
182+
struct Issue9939<T> {
183+
avalanche: T,
184+
}
185+
186+
fn issue9939() {
187+
let issue = Some(Issue9939 { avalanche: 1 });
188+
let tornado = match issue {
189+
//~^ manual_let_else
190+
Some(Issue9939 { avalanche }) => avalanche,
191+
_ => unreachable!("can't happen"),
192+
};
193+
let issue = Some(Issue9939 { avalanche: true });
194+
let acid_rain = match issue {
195+
//~^ manual_let_else
196+
Some(Issue9939 { avalanche: tornado }) => tornado,
197+
_ => unreachable!("can't happen"),
198+
};
199+
assert_eq!(tornado, 1);
200+
assert!(acid_rain);
201+
202+
// without shadowing
203+
let _y = match issue {
204+
//~^ manual_let_else
205+
_x @ Some(Issue9939 { avalanche }) => avalanche,
206+
None => unreachable!("can't happen"),
207+
};
208+
209+
// with shadowing
210+
let _x = match issue {
211+
//~^ manual_let_else
212+
_x @ Some(Issue9939 { avalanche }) => avalanche,
213+
None => unreachable!("can't happen"),
214+
};
215+
}
216+
217+
#[derive(Clone, Copy)]
218+
struct Issue9939b<T, U> {
219+
earthquake: T,
220+
hurricane: U,
221+
}
222+
223+
fn issue9939b() {
224+
let issue = Some(Issue9939b {
225+
earthquake: true,
226+
hurricane: 1,
227+
});
228+
let (issue, drought, flood) = match issue {
229+
//~^ manual_let_else
230+
flood @ Some(Issue9939b { earthquake, hurricane }) => (flood, hurricane, earthquake),
231+
None => unreachable!("can't happen"),
232+
};
233+
assert_eq!(drought, 1);
234+
assert!(flood);
235+
assert!(issue.is_some());
236+
237+
// without shadowing
238+
let (_y, erosion) = match issue {
239+
//~^ manual_let_else
240+
_x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake),
241+
None => unreachable!("can't happen"),
242+
};
243+
assert!(erosion);
244+
245+
// with shadowing
246+
let (_x, erosion) = match issue {
247+
//~^ manual_let_else
248+
_x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake),
249+
None => unreachable!("can't happen"),
250+
};
251+
assert!(erosion);
252+
}

tests/ui/manual_let_else_match.stderr

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,5 +101,75 @@ LL | | _ => unreachable!("can't happen"),
101101
LL | | };
102102
| |______^ help: consider writing: `let Some(msg) = Some("hi") else { unreachable!("can't happen") };`
103103

104-
error: aborting due to 10 previous errors
104+
error: this could be rewritten as `let...else`
105+
--> tests/ui/manual_let_else_match.rs:188:5
106+
|
107+
LL | / let tornado = match issue {
108+
LL | |
109+
LL | | Some(Issue9939 { avalanche }) => avalanche,
110+
LL | | _ => unreachable!("can't happen"),
111+
LL | | };
112+
| |______^ help: consider writing: `let Some(Issue9939 { avalanche: tornado }) = issue else { unreachable!("can't happen") };`
113+
114+
error: this could be rewritten as `let...else`
115+
--> tests/ui/manual_let_else_match.rs:194:5
116+
|
117+
LL | / let acid_rain = match issue {
118+
LL | |
119+
LL | | Some(Issue9939 { avalanche: tornado }) => tornado,
120+
LL | | _ => unreachable!("can't happen"),
121+
LL | | };
122+
| |______^ help: consider writing: `let Some(Issue9939 { avalanche: acid_rain }) = issue else { unreachable!("can't happen") };`
123+
124+
error: this could be rewritten as `let...else`
125+
--> tests/ui/manual_let_else_match.rs:203:5
126+
|
127+
LL | / let _y = match issue {
128+
LL | |
129+
LL | | _x @ Some(Issue9939 { avalanche }) => avalanche,
130+
LL | | None => unreachable!("can't happen"),
131+
LL | | };
132+
| |______^ help: consider writing: `let _x @ Some(Issue9939 { avalanche: _y }) = issue else { unreachable!("can't happen") };`
133+
134+
error: this could be rewritten as `let...else`
135+
--> tests/ui/manual_let_else_match.rs:210:5
136+
|
137+
LL | / let _x = match issue {
138+
LL | |
139+
LL | | _x @ Some(Issue9939 { avalanche }) => avalanche,
140+
LL | | None => unreachable!("can't happen"),
141+
LL | | };
142+
| |______^ help: consider writing: `let Some(Issue9939 { avalanche: _x }) = issue else { unreachable!("can't happen") };`
143+
144+
error: this could be rewritten as `let...else`
145+
--> tests/ui/manual_let_else_match.rs:228:5
146+
|
147+
LL | / let (issue, drought, flood) = match issue {
148+
LL | |
149+
LL | | flood @ Some(Issue9939b { earthquake, hurricane }) => (flood, hurricane, earthquake),
150+
LL | | None => unreachable!("can't happen"),
151+
LL | | };
152+
| |______^ help: consider writing: `let issue @ Some(Issue9939b { earthquake: flood, hurricane: drought }) = issue else { unreachable!("can't happen") };`
153+
154+
error: this could be rewritten as `let...else`
155+
--> tests/ui/manual_let_else_match.rs:238:5
156+
|
157+
LL | / let (_y, erosion) = match issue {
158+
LL | |
159+
LL | | _x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake),
160+
LL | | None => unreachable!("can't happen"),
161+
LL | | };
162+
| |______^ help: consider writing: `let _x @ Some(Issue9939b { earthquake: erosion, hurricane: _y }) = issue else { unreachable!("can't happen") };`
163+
164+
error: this could be rewritten as `let...else`
165+
--> tests/ui/manual_let_else_match.rs:246:5
166+
|
167+
LL | / let (_x, erosion) = match issue {
168+
LL | |
169+
LL | | _x @ Some(Issue9939b { earthquake, hurricane }) => (hurricane, earthquake),
170+
LL | | None => unreachable!("can't happen"),
171+
LL | | };
172+
| |______^ help: consider writing: `let Some(Issue9939b { earthquake: erosion, hurricane: _x }) = issue else { unreachable!("can't happen") };`
173+
174+
error: aborting due to 17 previous errors
105175

0 commit comments

Comments
 (0)