Skip to content

Commit c225f17

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 c225f17

9 files changed

+373
-49
lines changed

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

+92-11
Original file line numberDiff line numberDiff line change
@@ -941,6 +941,21 @@ 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
949+
/// than the actual MIR builder will, which is useful when some checks
950+
/// are too cumbersome to perform here. For example, if only after type
951+
/// inference it becomes clear that only one variant of an enum is
952+
/// inhabited, and therefore a read of the discriminant is not necessary,
953+
/// `walk_pat` will have over-approximated the necessary upvar capture
954+
/// granularity. (Or, at least, that's what the code seems to be saying.
955+
/// I didn't bother trying to craft an example where this actually happens).
956+
///
957+
/// Do note that discrepancies like these do still create weird language
958+
/// semantics, and should be avoided if possible.
944959
#[instrument(skip(self), level = "debug")]
945960
fn walk_pat(
946961
&self,
@@ -950,6 +965,11 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
950965
) -> Result<(), Cx::Error> {
951966
let tcx = self.cx.tcx();
952967
self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| {
968+
debug!("walk_pat: pat.kind={:?}", pat.kind);
969+
let read_discriminant = || {
970+
self.delegate.borrow_mut().borrow(place, discr_place.hir_id, BorrowKind::Immutable);
971+
};
972+
953973
match pat.kind {
954974
PatKind::Binding(_, canonical_id, ..) => {
955975
debug!("walk_pat: binding place={:?} pat={:?}", place, pat);
@@ -972,11 +992,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
972992
// binding when lowering pattern guards to ensure that the guard does not
973993
// modify the scrutinee.
974994
if has_guard {
975-
self.delegate.borrow_mut().borrow(
976-
place,
977-
discr_place.hir_id,
978-
BorrowKind::Immutable,
979-
);
995+
read_discriminant();
980996
}
981997

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

10201093
Ok(())
@@ -1849,6 +1922,14 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
18491922
Ok(())
18501923
}
18511924

1925+
/// Checks whether a type has multiple variants, and therefore, whether a
1926+
/// read of the discriminant might be necessary. Note that the actual MIR
1927+
/// builder code does a more specific check, filtering out variants that
1928+
/// happen to be uninhabited.
1929+
///
1930+
/// Here, we cannot perform such an accurate checks, because querying
1931+
/// whether a type is inhabited requires that it has been fully inferred,
1932+
/// which cannot be guaranteed at this point.
18521933
fn is_multivariant_adt(&self, ty: Ty<'tcx>, span: Span) -> bool {
18531934
if let ty::Adt(def, _) = self.cx.try_structurally_resolve_type(span, ty).kind() {
18541935
// 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)