Skip to content

Commit 2759284

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 077cedc commit 2759284

14 files changed

+426
-92
lines changed

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

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

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

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

10231095
Ok(())
@@ -1880,6 +1952,14 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
18801952
self.cat_deref(hir_id, base)
18811953
}
18821954

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

tests/crashes/137467-1.rs

-17
This file was deleted.

tests/crashes/137467-2.rs

-18
This file was deleted.

tests/crashes/137467-3.rs

-8
This file was deleted.

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
//~| ERROR 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
//~| ERROR 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
//~| ERROR 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
//~| ERROR 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
//~| ERROR 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
//~| ERROR 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)