Skip to content

Commit fb6d939

Browse files
Rollup merge of rust-lang#146832 - Natural-selection1:not-in-chains, r=petrochenkov
Not linting irrefutable_let_patterns on let chains *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/146832)* # Description this PR makes the lint `irrefutable_let_patterns` not check for `let chains`, only check for single `if let`, `while let`, and `if let guard`. # Motivation Since `let chains` were stabilized, the following code has become common: ```rust fn max() -> usize { 42 } fn main() { if let mx = max() && mx < usize::MAX { /* */ } } ``` This code naturally expresses "please call that function and then do something if the return value satisfies a condition". Putting the let binding outside the if would be bad as then it remains in scope after the if, which is not the intent. Current Output: ```bash warning: leading irrefutable pattern in let chain --> src/main.rs:7:8 | 7 | if let mx = max() && mx < usize::MAX { | ^^^^^^^^^^^^^^ | = note: this pattern will always match = help: consider moving it outside of the construct = note: `#[warn(irrefutable_let_patterns)]` on by default ``` Another common case is progressively destructuring a struct with enum fields, or an enum with struct variants: ```rust struct NameOfOuterStruct { middle: NameOfMiddleEnum, other: (), } enum NameOfMiddleEnum { Inner(NameOfInnerStruct), Other(()), } struct NameOfInnerStruct { id: u32, } fn test(outer: NameOfOuterStruct) { if let NameOfOuterStruct { middle, .. } = outer && let NameOfMiddleEnum::Inner(inner) = middle && let NameOfInnerStruct { id } = inner { /* */ } } ``` Current Output: ```bash warning: leading irrefutable pattern in let chain --> src\main.rs:17:8 | 17 | if let NameOfOuterStruct { middle, .. } = outer | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this pattern will always match = help: consider moving it outside of the construct = note: `#[warn(irrefutable_let_patterns)]` on by default warning: trailing irrefutable pattern in let chain --> src\main.rs:19:12 | 19 | && let NameOfInnerStruct { id } = inner | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this pattern will always match = help: consider moving it into the body ``` To avoid the warning, the readability would be much worse: ```rust fn test(outer: NameOfOuterStruct) { if let NameOfOuterStruct { middle: NameOfMiddleEnum::Inner(NameOfInnerStruct { id }), .. } = outer { /* */ } } ``` # related issue * rust-lang#139369 # possible questions 1. Moving the irrefutable pattern at the head of the chain out of it would cause a variable that was intended to be temporary to remain in scope, so we remove it. However, should we keep the check for moving the irrefutable pattern at the tail into the body? 2. Should we still lint `entire chain is made up of irrefutable let`? --- This is my first time contributing non-documentation code to Rust. If there are any irregularities, please feel free to point them out. : )
2 parents 0376d43 + 4f31ff8 commit fb6d939

10 files changed

Lines changed: 196 additions & 307 deletions

File tree

compiler/rustc_mir_build/src/errors.rs

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -874,52 +874,6 @@ pub(crate) struct UpperRangeBoundCannotBeMin {
874874
pub(crate) span: Span,
875875
}
876876

877-
#[derive(LintDiagnostic)]
878-
#[diag(
879-
"leading irrefutable {$count ->
880-
[one] pattern
881-
*[other] patterns
882-
} in let chain"
883-
)]
884-
#[note(
885-
"{$count ->
886-
[one] this pattern
887-
*[other] these patterns
888-
} will always match"
889-
)]
890-
#[help(
891-
"consider moving {$count ->
892-
[one] it
893-
*[other] them
894-
} outside of the construct"
895-
)]
896-
pub(crate) struct LeadingIrrefutableLetPatterns {
897-
pub(crate) count: usize,
898-
}
899-
900-
#[derive(LintDiagnostic)]
901-
#[diag(
902-
"trailing irrefutable {$count ->
903-
[one] pattern
904-
*[other] patterns
905-
} in let chain"
906-
)]
907-
#[note(
908-
"{$count ->
909-
[one] this pattern
910-
*[other] these patterns
911-
} will always match"
912-
)]
913-
#[help(
914-
"consider moving {$count ->
915-
[one] it
916-
*[other] them
917-
} into the body"
918-
)]
919-
pub(crate) struct TrailingIrrefutableLetPatterns {
920-
pub(crate) count: usize,
921-
}
922-
923877
#[derive(LintDiagnostic)]
924878
#[diag("pattern binding `{$name}` is named the same as one of the variants of the type `{$ty_path}`", code = E0170)]
925879
pub(crate) struct BindingsWithVariantName {

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

Lines changed: 8 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -167,9 +167,9 @@ impl<'p, 'tcx> Visitor<'p, 'tcx> for MatchVisitor<'p, 'tcx> {
167167
{
168168
let mut chain_refutabilities = Vec::new();
169169
let Ok(()) = self.visit_land(ex, &mut chain_refutabilities) else { return };
170-
// If at least one of the operands is a `let ... = ...`.
171-
if chain_refutabilities.iter().any(|x| x.is_some()) {
172-
self.check_let_chain(chain_refutabilities, ex.span);
170+
// Lint only single irrefutable let binding.
171+
if let [Some((_, Irrefutable))] = chain_refutabilities[..] {
172+
self.lint_single_let(ex.span);
173173
}
174174
return;
175175
}
@@ -430,18 +430,9 @@ impl<'p, 'tcx> MatchVisitor<'p, 'tcx> {
430430
assert!(self.let_source != LetSource::None);
431431
let scrut = scrutinee.map(|id| &self.thir[id]);
432432
if let LetSource::PlainLet = self.let_source {
433-
self.check_binding_is_irrefutable(pat, "local binding", scrut, Some(span))
434-
} else {
435-
let Ok(refutability) = self.is_let_irrefutable(pat, scrut) else { return };
436-
if matches!(refutability, Irrefutable) {
437-
report_irrefutable_let_patterns(
438-
self.tcx,
439-
self.hir_source,
440-
self.let_source,
441-
1,
442-
span,
443-
);
444-
}
433+
self.check_binding_is_irrefutable(pat, "local binding", scrut, Some(span));
434+
} else if let Ok(Irrefutable) = self.is_let_irrefutable(pat, scrut) {
435+
self.lint_single_let(span);
445436
}
446437
}
447438

@@ -549,74 +540,8 @@ impl<'p, 'tcx> MatchVisitor<'p, 'tcx> {
549540
}
550541

551542
#[instrument(level = "trace", skip(self))]
552-
fn check_let_chain(
553-
&mut self,
554-
chain_refutabilities: Vec<Option<(Span, RefutableFlag)>>,
555-
whole_chain_span: Span,
556-
) {
557-
assert!(self.let_source != LetSource::None);
558-
559-
if chain_refutabilities.iter().all(|r| matches!(*r, Some((_, Irrefutable)))) {
560-
// The entire chain is made up of irrefutable `let` statements
561-
report_irrefutable_let_patterns(
562-
self.tcx,
563-
self.hir_source,
564-
self.let_source,
565-
chain_refutabilities.len(),
566-
whole_chain_span,
567-
);
568-
return;
569-
}
570-
571-
if let Some(until) =
572-
chain_refutabilities.iter().position(|r| !matches!(*r, Some((_, Irrefutable))))
573-
&& until > 0
574-
{
575-
// The chain has a non-zero prefix of irrefutable `let` statements.
576-
577-
// Check if the let source is while, for there is no alternative place to put a prefix,
578-
// and we shouldn't lint.
579-
// For let guards inside a match, prefixes might use bindings of the match pattern,
580-
// so can't always be moved out.
581-
// For `else if let`, an extra indentation level would be required to move the bindings.
582-
// FIXME: Add checking whether the bindings are actually used in the prefix,
583-
// and lint if they are not.
584-
if !matches!(
585-
self.let_source,
586-
LetSource::WhileLet | LetSource::IfLetGuard | LetSource::ElseIfLet
587-
) {
588-
// Emit the lint
589-
let prefix = &chain_refutabilities[..until];
590-
let span_start = prefix[0].unwrap().0;
591-
let span_end = prefix.last().unwrap().unwrap().0;
592-
let span = span_start.to(span_end);
593-
let count = prefix.len();
594-
self.tcx.emit_node_span_lint(
595-
IRREFUTABLE_LET_PATTERNS,
596-
self.hir_source,
597-
span,
598-
LeadingIrrefutableLetPatterns { count },
599-
);
600-
}
601-
}
602-
603-
if let Some(from) =
604-
chain_refutabilities.iter().rposition(|r| !matches!(*r, Some((_, Irrefutable))))
605-
&& from != (chain_refutabilities.len() - 1)
606-
{
607-
// The chain has a non-empty suffix of irrefutable `let` statements
608-
let suffix = &chain_refutabilities[from + 1..];
609-
let span_start = suffix[0].unwrap().0;
610-
let span_end = suffix.last().unwrap().unwrap().0;
611-
let span = span_start.to(span_end);
612-
let count = suffix.len();
613-
self.tcx.emit_node_span_lint(
614-
IRREFUTABLE_LET_PATTERNS,
615-
self.hir_source,
616-
span,
617-
TrailingIrrefutableLetPatterns { count },
618-
);
619-
}
543+
fn lint_single_let(&mut self, let_span: Span) {
544+
report_irrefutable_let_patterns(self.tcx, self.hir_source, self.let_source, 1, let_span);
620545
}
621546

622547
fn analyze_binding(

compiler/rustc_mir_transform/src/check_alignment.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ fn insert_alignment_check<'tcx>(
8282

8383
// If this target does not have reliable alignment, further limit the mask by anding it with
8484
// the mask for the highest reliable alignment.
85-
#[allow(irrefutable_let_patterns)]
85+
#[cfg_attr(bootstrap, expect(irrefutable_let_patterns))]
8686
if let max_align = tcx.sess.target.max_reliable_alignment()
8787
&& max_align < Align::MAX
8888
{

src/tools/miri/src/shims/foreign_items.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -851,7 +851,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
851851
// Fallback to shims in submodules.
852852
_ => {
853853
// Math shims
854-
#[expect(irrefutable_let_patterns)]
854+
#[cfg_attr(bootstrap, expect(irrefutable_let_patterns))]
855855
if let res = shims::math::EvalContextExt::emulate_foreign_item_inner(
856856
this, link_name, abi, args, dest,
857857
)? && !matches!(res, EmulateItemResult::NotSupported)
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
// https://github.com/rust-lang/rust/issues/139369
2+
// Test that the lint `irrefutable_let_patterns` now
3+
// only checks single let binding.
4+
//@ edition: 2024
5+
//@ check-pass
6+
7+
use std::ops::Range;
8+
9+
fn main() {
10+
let opt = Some(None..Some(1));
11+
12+
// test `if let`
13+
if let first = &opt {}
14+
//~^ WARN irrefutable `if let` pattern
15+
16+
if let first = &opt && let Some(second) = first {}
17+
18+
if let first = &opt && let (a, b) = (1, 2) {}
19+
20+
if let first = &opt && let None = Some(1) {}
21+
22+
if 4 * 2 == 0 && let first = &opt {}
23+
24+
if let first = &opt
25+
&& let Some(second) = first
26+
&& let None = second.start
27+
&& let v = 0
28+
{}
29+
30+
if let Range { start: local_start, end: _ } = (None..Some(1)) {}
31+
//~^ WARN irrefutable `if let` pattern
32+
33+
if let Range { start: local_start, end: _ } = (None..Some(1))
34+
&& let None = local_start
35+
{}
36+
37+
if let (a, b, c) = (Some(1), Some(1), Some(1)) {}
38+
//~^ WARN irrefutable `if let` pattern
39+
40+
if let (a, b, c) = (Some(1), Some(1), Some(1)) && let None = Some(1) {}
41+
42+
if let Some(ref first) = opt
43+
&& let Range { start: local_start, end: _ } = first
44+
&& let None = local_start
45+
{}
46+
47+
// test `else if let`
48+
if opt == Some(None..None) {
49+
} else if let x = opt.clone().map(|_| 1) {
50+
//~^ WARN irrefutable `if let` pattern
51+
}
52+
53+
if opt == Some(None..None) {
54+
} else if let x = opt.clone().map(|_| 1)
55+
&& x == Some(1)
56+
{}
57+
58+
if opt == Some(None..None) {
59+
} else if opt.is_some() && let x = &opt
60+
{}
61+
62+
if opt == Some(None..None) {
63+
} else {
64+
if let x = opt.clone().map(|_| 1) && x == Some(1)
65+
{}
66+
}
67+
68+
// test `if let guard`
69+
match opt {
70+
Some(ref first) if let second = first => {}
71+
//~^ WARN irrefutable `if let` guard pattern
72+
_ => {}
73+
}
74+
75+
match opt {
76+
Some(ref first)
77+
if let second = first
78+
&& let _third = second
79+
&& let v = 4 + 4 => {}
80+
_ => {}
81+
}
82+
83+
match opt {
84+
Some(ref first)
85+
if let Range { start: local_start, end: _ } = first
86+
&& let None = local_start => {}
87+
_ => {}
88+
}
89+
90+
match opt {
91+
Some(ref first)
92+
if let Range { start: Some(_), end: local_end } = first
93+
&& let v = local_end
94+
&& let w = v => {}
95+
_ => {}
96+
}
97+
98+
// test `while let`
99+
while let first = &opt {}
100+
//~^ WARN irrefutable `while let` pattern
101+
102+
while let first = &opt
103+
&& let (a, b) = (1, 2)
104+
{}
105+
106+
while let first = &opt
107+
&& let Some(second) = first
108+
&& let None = second.start
109+
{}
110+
111+
while let Some(ref first) = opt
112+
&& let second = first
113+
&& let _third = second
114+
{}
115+
116+
while let Some(ref first) = opt
117+
&& let Range { start: local_start, end: _ } = first
118+
&& let None = local_start
119+
{}
120+
}
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
warning: irrefutable `if let` pattern
2+
--> $DIR/irrefutable-in-let-chains.rs:13:8
3+
|
4+
LL | if let first = &opt {}
5+
| ^^^^^^^^^^^^^^^^
6+
|
7+
= note: this pattern will always match, so the `if let` is useless
8+
= help: consider replacing the `if let` with a `let`
9+
= note: `#[warn(irrefutable_let_patterns)]` on by default
10+
11+
warning: irrefutable `if let` pattern
12+
--> $DIR/irrefutable-in-let-chains.rs:30:8
13+
|
14+
LL | if let Range { start: local_start, end: _ } = (None..Some(1)) {}
15+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
16+
|
17+
= note: this pattern will always match, so the `if let` is useless
18+
= help: consider replacing the `if let` with a `let`
19+
20+
warning: irrefutable `if let` pattern
21+
--> $DIR/irrefutable-in-let-chains.rs:37:8
22+
|
23+
LL | if let (a, b, c) = (Some(1), Some(1), Some(1)) {}
24+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
25+
|
26+
= note: this pattern will always match, so the `if let` is useless
27+
= help: consider replacing the `if let` with a `let`
28+
29+
warning: irrefutable `if let` pattern
30+
--> $DIR/irrefutable-in-let-chains.rs:49:15
31+
|
32+
LL | } else if let x = opt.clone().map(|_| 1) {
33+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
34+
|
35+
= note: this pattern will always match, so the `if let` is useless
36+
= help: consider replacing the `if let` with a `let`
37+
38+
warning: irrefutable `if let` guard pattern
39+
--> $DIR/irrefutable-in-let-chains.rs:70:28
40+
|
41+
LL | Some(ref first) if let second = first => {}
42+
| ^^^^^^^^^^^^^^^^^^
43+
|
44+
= note: this pattern will always match, so the guard is useless
45+
= help: consider removing the guard and adding a `let` inside the match arm
46+
47+
warning: irrefutable `while let` pattern
48+
--> $DIR/irrefutable-in-let-chains.rs:99:11
49+
|
50+
LL | while let first = &opt {}
51+
| ^^^^^^^^^^^^^^^^
52+
|
53+
= note: this pattern will always match, so the loop will never exit
54+
= help: consider instead using a `loop { ... }` with a `let` inside it
55+
56+
warning: 6 warnings emitted
57+

tests/ui/rfcs/rfc-2294-if-let-guard/warns.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
#[deny(irrefutable_let_patterns)]
2-
32
fn irrefutable_let_guard() {
43
match Some(()) {
54
Some(x) if let () = x => {}
@@ -21,7 +20,6 @@ fn trailing_irrefutable_pattern_binding() {
2120
fn trailing_irrefutable_in_let_chain() {
2221
match Some(5) {
2322
Some(x) if let Some(y) = Some(x) && let z = 0 => {}
24-
//~^ ERROR trailing irrefutable pattern in let chain
2523
_ => {}
2624
}
2725
}

0 commit comments

Comments
 (0)