Skip to content

Commit ce47a4c

Browse files
committed
ExprUseVisitor: properly report discriminant reads
This solves the "can't find the upvar" ICEs that resulted from `maybe_read_scrutinee` being unfit for purpose.
1 parent 908504e commit ce47a4c

9 files changed

+371
-49
lines changed

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

+90-11
Original file line numberDiff line numberDiff line change
@@ -941,6 +941,19 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
941941
}
942942

943943
/// The core driver for walking a pattern
944+
///
945+
/// This should mirror how pattern-matching gets lowered to MIR, as
946+
/// otherwise lowering will ICE when trying to resolve the upvars.
947+
///
948+
/// However, it is okay to approximate it here by doing *more* accesses than
949+
/// the actual MIR builder will, which is useful when some checks are too
950+
/// cumbersome to perform here. For example, if after typeck it becomes
951+
/// clear that only one variant of an enum is inhabited, and therefore a
952+
/// read of the discriminant is not necessary, `walk_pat` will have
953+
/// over-approximated the necessary upvar capture granularity.
954+
///
955+
/// Do note that discrepancies like these do still create obscure corners
956+
/// in the semantics of the language, and should be avoided if possible.
944957
#[instrument(skip(self), level = "debug")]
945958
fn walk_pat(
946959
&self,
@@ -950,6 +963,11 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
950963
) -> Result<(), Cx::Error> {
951964
let tcx = self.cx.tcx();
952965
self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| {
966+
debug!("walk_pat: pat.kind={:?}", pat.kind);
967+
let read_discriminant = || {
968+
self.delegate.borrow_mut().borrow(place, discr_place.hir_id, BorrowKind::Immutable);
969+
};
970+
953971
match pat.kind {
954972
PatKind::Binding(_, canonical_id, ..) => {
955973
debug!("walk_pat: binding place={:?} pat={:?}", place, pat);
@@ -972,11 +990,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
972990
// binding when lowering pattern guards to ensure that the guard does not
973991
// modify the scrutinee.
974992
if has_guard {
975-
self.delegate.borrow_mut().borrow(
976-
place,
977-
discr_place.hir_id,
978-
BorrowKind::Immutable,
979-
);
993+
read_discriminant();
980994
}
981995

982996
// It is also a borrow or copy/move of the value being matched.
@@ -1008,13 +1022,70 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
10081022
PatKind::Never => {
10091023
// A `!` pattern always counts as an immutable read of the discriminant,
10101024
// even in an irrefutable pattern.
1011-
self.delegate.borrow_mut().borrow(
1012-
place,
1013-
discr_place.hir_id,
1014-
BorrowKind::Immutable,
1015-
);
1025+
read_discriminant();
1026+
}
1027+
PatKind::Expr(PatExpr { kind: PatExprKind::Path(qpath), hir_id, span }) => {
1028+
// A `Path` pattern is just a name like `Foo`. This is either a
1029+
// named constant or else it refers to an ADT variant
1030+
1031+
let res = self.cx.typeck_results().qpath_res(qpath, *hir_id);
1032+
match res {
1033+
Res::Def(DefKind::Const, _) | Res::Def(DefKind::AssocConst, _) => {
1034+
// Named constants have to be equated with the value
1035+
// being matched, so that's a read of the value being matched.
1036+
//
1037+
// FIXME: Does the MIR code skip this read when matching on a ZST?
1038+
// If so, we can also skip it here.
1039+
read_discriminant();
1040+
}
1041+
_ => {
1042+
// Otherwise, this is a struct/enum variant, and so it's
1043+
// only a read if we need to read the discriminant.
1044+
if self.is_multivariant_adt(place.place.ty(), *span) {
1045+
read_discriminant();
1046+
}
1047+
}
1048+
}
1049+
}
1050+
PatKind::Expr(_) | PatKind::Range(..) => {
1051+
// When matching against a literal or range, we need to
1052+
// borrow the place to compare it against the pattern.
1053+
//
1054+
// FIXME: What if the type being matched only has one
1055+
// possible value?
1056+
// FIXME: What if the range is the full range of the type
1057+
// and doesn't actually require a discriminant read?
1058+
read_discriminant();
1059+
}
1060+
PatKind::Struct(..) | PatKind::TupleStruct(..) => {
1061+
if self.is_multivariant_adt(place.place.ty(), pat.span) {
1062+
read_discriminant();
1063+
}
1064+
}
1065+
PatKind::Slice(lhs, wild, rhs) => {
1066+
// We don't need to test the length if the pattern is `[..]`
1067+
if matches!((lhs, wild, rhs), (&[], Some(_), &[]))
1068+
// Arrays have a statically known size, so
1069+
// there is no need to read their length
1070+
|| place.place.ty().peel_refs().is_array()
1071+
{
1072+
// No read necessary
1073+
} else {
1074+
read_discriminant();
1075+
}
1076+
}
1077+
PatKind::Or(_)
1078+
| PatKind::Box(_)
1079+
| PatKind::Ref(..)
1080+
| PatKind::Guard(..)
1081+
| PatKind::Tuple(..)
1082+
| PatKind::Wild
1083+
| PatKind::Err(_) => {
1084+
// If the PatKind is Or, Box, Ref, Guard, or Tuple, the relevant accesses
1085+
// are made later as these patterns contains subpatterns.
1086+
// If the PatKind is Wild or Err, they are made when processing the other patterns
1087+
// being examined
10161088
}
1017-
_ => {}
10181089
}
10191090

10201091
Ok(())
@@ -1849,6 +1920,14 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
18491920
Ok(())
18501921
}
18511922

1923+
/// Checks whether a type has multiple variants, and therefore, whether a
1924+
/// read of the discriminant might be necessary. Note that the actual MIR
1925+
/// builder code does a more specific check, filtering out variants that
1926+
/// happen to be uninhabited.
1927+
///
1928+
/// Here, we cannot perform such an accurate checks, because querying
1929+
/// whether a type is inhabited requires that it has been fully inferred,
1930+
/// which cannot be guaranteed at this point.
18521931
fn is_multivariant_adt(&self, ty: Ty<'tcx>, span: Span) -> bool {
18531932
if let ty::Adt(def, _) = self.cx.try_structurally_resolve_type(span, ty).kind() {
18541933
// Note that if a non-exhaustive SingleVariant is defined in another crate, we need

tests/ui/closures/2229_closure_analysis/capture-enums.rs

+2
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,15 @@ fn multi_variant_enum() {
2222
//~| Min Capture analysis includes:
2323
if let Info::Point(_, _, str) = point {
2424
//~^ NOTE: Capturing point[] -> Immutable
25+
//~| NOTE: Capturing point[] -> Immutable
2526
//~| NOTE: Capturing point[(2, 0)] -> ByValue
2627
//~| NOTE: Min Capture point[] -> ByValue
2728
println!("{}", str);
2829
}
2930

3031
if let Info::Meta(_, v) = meta {
3132
//~^ NOTE: Capturing meta[] -> Immutable
33+
//~| NOTE: Capturing meta[] -> Immutable
3234
//~| NOTE: Capturing meta[(1, 1)] -> ByValue
3335
//~| NOTE: Min Capture meta[] -> ByValue
3436
println!("{:?}", v);

tests/ui/closures/2229_closure_analysis/capture-enums.stderr

+18-8
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ LL | let c = #[rustc_capture_analysis]
99
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date
1010

1111
error[E0658]: attributes on expressions are experimental
12-
--> $DIR/capture-enums.rs:48:13
12+
--> $DIR/capture-enums.rs:50:13
1313
|
1414
LL | let c = #[rustc_capture_analysis]
1515
| ^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -34,18 +34,28 @@ note: Capturing point[] -> Immutable
3434
|
3535
LL | if let Info::Point(_, _, str) = point {
3636
| ^^^^^
37+
note: Capturing point[] -> Immutable
38+
--> $DIR/capture-enums.rs:23:41
39+
|
40+
LL | if let Info::Point(_, _, str) = point {
41+
| ^^^^^
3742
note: Capturing point[(2, 0)] -> ByValue
3843
--> $DIR/capture-enums.rs:23:41
3944
|
4045
LL | if let Info::Point(_, _, str) = point {
4146
| ^^^^^
4247
note: Capturing meta[] -> Immutable
43-
--> $DIR/capture-enums.rs:30:35
48+
--> $DIR/capture-enums.rs:31:35
49+
|
50+
LL | if let Info::Meta(_, v) = meta {
51+
| ^^^^
52+
note: Capturing meta[] -> Immutable
53+
--> $DIR/capture-enums.rs:31:35
4454
|
4555
LL | if let Info::Meta(_, v) = meta {
4656
| ^^^^
4757
note: Capturing meta[(1, 1)] -> ByValue
48-
--> $DIR/capture-enums.rs:30:35
58+
--> $DIR/capture-enums.rs:31:35
4959
|
5060
LL | if let Info::Meta(_, v) = meta {
5161
| ^^^^
@@ -67,13 +77,13 @@ note: Min Capture point[] -> ByValue
6777
LL | if let Info::Point(_, _, str) = point {
6878
| ^^^^^
6979
note: Min Capture meta[] -> ByValue
70-
--> $DIR/capture-enums.rs:30:35
80+
--> $DIR/capture-enums.rs:31:35
7181
|
7282
LL | if let Info::Meta(_, v) = meta {
7383
| ^^^^
7484

7585
error: First Pass analysis includes:
76-
--> $DIR/capture-enums.rs:52:5
86+
--> $DIR/capture-enums.rs:54:5
7787
|
7888
LL | / || {
7989
LL | |
@@ -85,13 +95,13 @@ LL | | };
8595
| |_____^
8696
|
8797
note: Capturing point[(2, 0)] -> ByValue
88-
--> $DIR/capture-enums.rs:55:47
98+
--> $DIR/capture-enums.rs:57:47
8999
|
90100
LL | let SingleVariant::Point(_, _, str) = point;
91101
| ^^^^^
92102

93103
error: Min Capture analysis includes:
94-
--> $DIR/capture-enums.rs:52:5
104+
--> $DIR/capture-enums.rs:54:5
95105
|
96106
LL | / || {
97107
LL | |
@@ -103,7 +113,7 @@ LL | | };
103113
| |_____^
104114
|
105115
note: Min Capture point[(2, 0)] -> ByValue
106-
--> $DIR/capture-enums.rs:55:47
116+
--> $DIR/capture-enums.rs:57:47
107117
|
108118
LL | let SingleVariant::Point(_, _, str) = point;
109119
| ^^^^^

tests/ui/closures/2229_closure_analysis/match/patterns-capture-analysis.rs

+5
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ fn test_1_should_capture() {
1414
//~| Min Capture analysis includes:
1515
match variant {
1616
//~^ NOTE: Capturing variant[] -> Immutable
17+
//~| NOTE: Capturing variant[] -> Immutable
1718
//~| NOTE: Min Capture variant[] -> Immutable
1819
Some(_) => {}
1920
_ => {}
@@ -132,6 +133,7 @@ fn test_5_should_capture_multi_variant() {
132133
//~| Min Capture analysis includes:
133134
match variant {
134135
//~^ NOTE: Capturing variant[] -> Immutable
136+
//~| NOTE: Capturing variant[] -> Immutable
135137
//~| NOTE: Min Capture variant[] -> Immutable
136138
MVariant::A => {}
137139
_ => {}
@@ -150,6 +152,7 @@ fn test_7_should_capture_slice_len() {
150152
//~| Min Capture analysis includes:
151153
match slice {
152154
//~^ NOTE: Capturing slice[] -> Immutable
155+
//~| NOTE: Capturing slice[Deref] -> Immutable
153156
//~| NOTE: Min Capture slice[] -> Immutable
154157
[_,_,_] => {},
155158
_ => {}
@@ -162,6 +165,7 @@ fn test_7_should_capture_slice_len() {
162165
//~| Min Capture analysis includes:
163166
match slice {
164167
//~^ NOTE: Capturing slice[] -> Immutable
168+
//~| NOTE: Capturing slice[Deref] -> Immutable
165169
//~| NOTE: Min Capture slice[] -> Immutable
166170
[] => {},
167171
_ => {}
@@ -174,6 +178,7 @@ fn test_7_should_capture_slice_len() {
174178
//~| Min Capture analysis includes:
175179
match slice {
176180
//~^ NOTE: Capturing slice[] -> Immutable
181+
//~| NOTE: Capturing slice[Deref] -> Immutable
177182
//~| NOTE: Min Capture slice[] -> Immutable
178183
[_, .. ,_] => {},
179184
_ => {}

0 commit comments

Comments
 (0)