From 824a0408339fa1292080b15e0a1b7f7d3f0cba49 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 19 Jan 2025 17:51:10 +1100 Subject: [PATCH 1/4] Use let-chains for determining length in `prefix_slice_suffix` --- .../src/builder/matches/match_pair.rs | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_mir_build/src/builder/matches/match_pair.rs b/compiler/rustc_mir_build/src/builder/matches/match_pair.rs index 9d59ffc88ba23..19ba4016be2e3 100644 --- a/compiler/rustc_mir_build/src/builder/matches/match_pair.rs +++ b/compiler/rustc_mir_build/src/builder/matches/match_pair.rs @@ -40,19 +40,25 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { suffix: &'pat [Box>], ) { let tcx = self.tcx; - let (min_length, exact_size) = if let Some(place_resolved) = place.try_to_place(self) { - match place_resolved.ty(&self.local_decls, tcx).ty.kind() { - ty::Array(_, length) => ( - length - .try_to_target_usize(tcx) - .expect("expected len of array pat to be definite"), - true, - ), - _ => ((prefix.len() + suffix.len()).try_into().unwrap(), false), - } + + // Minimum length of the sequence being matched, needed by projections. + // If `exact_size` is true, that "minimum" is actually an exact length. + // + // The caller of `prefix_slice_suffix` is responsible for enforcing + // that length if necessary, e.g. by checking the scrutinee's length. + let min_length: u64; + let exact_size: bool; + + if let Some(place_resolved) = place.try_to_place(self) + && let ty::Array(_, length) = place_resolved.ty(&self.local_decls, tcx).ty.kind() + { + min_length = + length.try_to_target_usize(tcx).expect("expected len of array pat to be definite"); + exact_size = true; } else { - ((prefix.len() + suffix.len()).try_into().unwrap(), false) - }; + min_length = u64::try_from(prefix.len() + suffix.len()).unwrap(); + exact_size = false; + } match_pairs.extend(prefix.iter().enumerate().map(|(idx, subpattern)| { let elem = From be347c900bbcf73b16db7d994cafa81a0aa819c0 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 20 Jan 2025 15:08:03 +1100 Subject: [PATCH 2/4] Note an edge case in array/slice pattern handling --- .../src/builder/matches/match_pair.rs | 15 +++++++ ...re.prefix_only-{closure#0}.built.after.mir | 18 ++++++++ ...fix_slice_only-{closure#0}.built.after.mir | 18 ++++++++ ...x_slice_suffix-{closure#0}.built.after.mir | 18 ++++++++ .../building/match/array_non_capture.rs | 43 +++++++++++++++++++ 5 files changed, 112 insertions(+) create mode 100644 tests/mir-opt/building/match/array_non_capture.prefix_only-{closure#0}.built.after.mir create mode 100644 tests/mir-opt/building/match/array_non_capture.prefix_slice_only-{closure#0}.built.after.mir create mode 100644 tests/mir-opt/building/match/array_non_capture.prefix_slice_suffix-{closure#0}.built.after.mir create mode 100644 tests/mir-opt/building/match/array_non_capture.rs diff --git a/compiler/rustc_mir_build/src/builder/matches/match_pair.rs b/compiler/rustc_mir_build/src/builder/matches/match_pair.rs index 19ba4016be2e3..4f22c991080e2 100644 --- a/compiler/rustc_mir_build/src/builder/matches/match_pair.rs +++ b/compiler/rustc_mir_build/src/builder/matches/match_pair.rs @@ -60,6 +60,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { exact_size = false; } + // Normally `exact_size` is true iff this is an array pattern, but there + // is at least one edge-case exception. Consider code like this: + // + // ```ignore (illustrative) + // let arr = [1, 2, 3]; + // let closure = || match arr { + // [_, ..] => {} + // }; + // ``` + // + // Under Rust 2021 disjoint-capture rules, the array place isn't + // actually captured, because no part of it is actually read or bound + // by the match. So the above code can't resolve it, and falls back to + // `exact_size = false`. This appears to be benign, but keep it in mind. + match_pairs.extend(prefix.iter().enumerate().map(|(idx, subpattern)| { let elem = ProjectionElem::ConstantIndex { offset: idx as u64, min_length, from_end: false }; diff --git a/tests/mir-opt/building/match/array_non_capture.prefix_only-{closure#0}.built.after.mir b/tests/mir-opt/building/match/array_non_capture.prefix_only-{closure#0}.built.after.mir new file mode 100644 index 0000000000000..d92388f92f4e4 --- /dev/null +++ b/tests/mir-opt/building/match/array_non_capture.prefix_only-{closure#0}.built.after.mir @@ -0,0 +1,18 @@ +// MIR for `prefix_only::{closure#0}` after built + +fn prefix_only::{closure#0}(_1: &{closure@$DIR/array_non_capture.rs:21:19: 21:21}) -> u32 { + let mut _0: u32; + + bb0: { + _0 = const 101_u32; + goto -> bb2; + } + + bb1: { + unreachable; + } + + bb2: { + return; + } +} diff --git a/tests/mir-opt/building/match/array_non_capture.prefix_slice_only-{closure#0}.built.after.mir b/tests/mir-opt/building/match/array_non_capture.prefix_slice_only-{closure#0}.built.after.mir new file mode 100644 index 0000000000000..beb5ccae7e57e --- /dev/null +++ b/tests/mir-opt/building/match/array_non_capture.prefix_slice_only-{closure#0}.built.after.mir @@ -0,0 +1,18 @@ +// MIR for `prefix_slice_only::{closure#0}` after built + +fn prefix_slice_only::{closure#0}(_1: &{closure@$DIR/array_non_capture.rs:30:19: 30:21}) -> u32 { + let mut _0: u32; + + bb0: { + _0 = const 102_u32; + goto -> bb2; + } + + bb1: { + unreachable; + } + + bb2: { + return; + } +} diff --git a/tests/mir-opt/building/match/array_non_capture.prefix_slice_suffix-{closure#0}.built.after.mir b/tests/mir-opt/building/match/array_non_capture.prefix_slice_suffix-{closure#0}.built.after.mir new file mode 100644 index 0000000000000..1d5a8f2cefcc7 --- /dev/null +++ b/tests/mir-opt/building/match/array_non_capture.prefix_slice_suffix-{closure#0}.built.after.mir @@ -0,0 +1,18 @@ +// MIR for `prefix_slice_suffix::{closure#0}` after built + +fn prefix_slice_suffix::{closure#0}(_1: &{closure@$DIR/array_non_capture.rs:39:19: 39:21}) -> u32 { + let mut _0: u32; + + bb0: { + _0 = const 103_u32; + goto -> bb2; + } + + bb1: { + unreachable; + } + + bb2: { + return; + } +} diff --git a/tests/mir-opt/building/match/array_non_capture.rs b/tests/mir-opt/building/match/array_non_capture.rs new file mode 100644 index 0000000000000..afcceaafaa772 --- /dev/null +++ b/tests/mir-opt/building/match/array_non_capture.rs @@ -0,0 +1,43 @@ +//@ edition: 2021 +// skip-filecheck + +// Under the Rust 2021 disjoint capture rules, a "captured" place sometimes +// doesn't actually need to be captured, if it is only matched against +// irrefutable patterns that don't bind anything. +// +// When that happens, there is currently some MIR-building code +// (`Builder::prefix_slice_suffix`) that can no longer distinguish between +// array patterns and slice patterns, so it falls back to the code for dealing +// with slice patterns. +// +// That appears to be benign, but it's worth having a test that explicitly +// triggers the edge-case scenario. If someone makes a change that assumes the +// edge case can't happen, then hopefully this test will demand attention by +// either triggering an ICE, or needing its MIR to be re-blessed. + +// EMIT_MIR array_non_capture.prefix_only-{closure#0}.built.after.mir +fn prefix_only() -> u32 { + let arr = [1, 2, 3]; + let closure = || match arr { + [_, _, _] => 101u32, + }; + closure() +} + +// EMIT_MIR array_non_capture.prefix_slice_only-{closure#0}.built.after.mir +fn prefix_slice_only() -> u32 { + let arr = [1, 2, 3]; + let closure = || match arr { + [_, ..] => 102u32, + }; + closure() +} + +// EMIT_MIR array_non_capture.prefix_slice_suffix-{closure#0}.built.after.mir +fn prefix_slice_suffix() -> u32 { + let arr = [1, 2, 3]; + let closure = || match arr { + [_, .., _] => 103u32, + }; + closure() +} From b28c2c7e2961a85cd960516078728431ab6c8464 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 20 Jan 2025 17:04:29 +1100 Subject: [PATCH 3/4] Rename test to `unresolvable-upvar-issue-87987.rs` and add some notes --- src/tools/tidy/src/issues.txt | 1 - .../{issue-87987.rs => unresolvable-upvar-issue-87987.rs} | 6 ++++++ ...e-87987.stderr => unresolvable-upvar-issue-87987.stderr} | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) rename tests/ui/closures/2229_closure_analysis/{issue-87987.rs => unresolvable-upvar-issue-87987.rs} (60%) rename tests/ui/closures/2229_closure_analysis/{issue-87987.stderr => unresolvable-upvar-issue-87987.stderr} (84%) diff --git a/src/tools/tidy/src/issues.txt b/src/tools/tidy/src/issues.txt index 54de2ef83148f..bb317478c505b 100644 --- a/src/tools/tidy/src/issues.txt +++ b/src/tools/tidy/src/issues.txt @@ -410,7 +410,6 @@ ui/closure_context/issue-26046-fn-once.rs ui/closure_context/issue-42065.rs ui/closures/2229_closure_analysis/issue-118144.rs ui/closures/2229_closure_analysis/issue-87378.rs -ui/closures/2229_closure_analysis/issue-87987.rs ui/closures/2229_closure_analysis/issue-88118-2.rs ui/closures/2229_closure_analysis/issue-88476.rs ui/closures/2229_closure_analysis/issue-89606.rs diff --git a/tests/ui/closures/2229_closure_analysis/issue-87987.rs b/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.rs similarity index 60% rename from tests/ui/closures/2229_closure_analysis/issue-87987.rs rename to tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.rs index f79a8f1b57100..acaf37fa48fd2 100644 --- a/tests/ui/closures/2229_closure_analysis/issue-87987.rs +++ b/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.rs @@ -1,6 +1,12 @@ //@ run-pass //@ edition:2021 +// When a closure syntactically captures a place, but doesn't actually capture +// it, make sure MIR building doesn't ICE when handling that place. +// +// Under the Rust 2021 disjoint capture rules, this sort of non-capture can +// occur when a place is only inspected by infallible non-binding patterns. + struct Props { field_1: u32, //~ WARNING: fields `field_1` and `field_2` are never read field_2: u32, diff --git a/tests/ui/closures/2229_closure_analysis/issue-87987.stderr b/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.stderr similarity index 84% rename from tests/ui/closures/2229_closure_analysis/issue-87987.stderr rename to tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.stderr index 5696a010c3f80..9d174a728b85e 100644 --- a/tests/ui/closures/2229_closure_analysis/issue-87987.stderr +++ b/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.stderr @@ -1,5 +1,5 @@ warning: fields `field_1` and `field_2` are never read - --> $DIR/issue-87987.rs:5:5 + --> $DIR/unresolvable-upvar-issue-87987.rs:11:5 | LL | struct Props { | ----- fields in this struct From 7a2e3064d2dfd38d25ba8ed083da8e9773ec4506 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 20 Jan 2025 17:05:54 +1100 Subject: [PATCH 4/4] Add some more pattern variants to the unresolvable-upvar test --- .../unresolvable-upvar-issue-87987.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.rs b/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.rs index acaf37fa48fd2..59461fb3acf0c 100644 --- a/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.rs +++ b/tests/ui/closures/2229_closure_analysis/unresolvable-upvar-issue-87987.rs @@ -25,8 +25,16 @@ fn main() { let mref = &mut arr; + // These array patterns don't need to inspect the array, so the array + // isn't captured. let _c = || match arr { - [_, _, _, _] => println!("A"), + [_, _, _, _] => println!("C"), + }; + let _d = || match arr { + [_, .., _] => println!("D"), + }; + let _e = || match arr { + [_, ..] => println!("E"), }; println!("{:#?}", mref);