From 4cd30197eb126727b791a1c845c5ec47dbd3b1de Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sun, 1 Nov 2020 01:58:48 +0000 Subject: [PATCH 1/2] Fix #78549 Before #78430, string literals worked because `specialize_constructor` didn't actually care too much which constructor was passed to it unless needed. Since then, string literals are special cased and a bit hacky. I did not anticipate patterns for the `&str` type other than string literals, hence this bug. This makes string literals less hacky. --- .../src/thir/pattern/_match.rs | 60 ++++++++++++++----- src/test/ui/issues/issue-30240.rs | 4 +- src/test/ui/issues/issue-30240.stderr | 8 +-- .../usefulness/issue-78549-ref-pat-and-str.rs | 25 ++++++++ 4 files changed, 75 insertions(+), 22 deletions(-) create mode 100644 src/test/ui/pattern/usefulness/issue-78549-ref-pat-and-str.rs diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 843a6c0e461f9..868923be757c1 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -327,9 +327,23 @@ struct LiteralExpander; impl<'tcx> PatternFolder<'tcx> for LiteralExpander { fn fold_pattern(&mut self, pat: &Pat<'tcx>) -> Pat<'tcx> { debug!("fold_pattern {:?} {:?} {:?}", pat, pat.ty.kind(), pat.kind); - match (pat.ty.kind(), &*pat.kind) { - (_, &PatKind::Binding { subpattern: Some(ref s), .. }) => s.fold_with(self), - (_, &PatKind::AscribeUserType { subpattern: ref s, .. }) => s.fold_with(self), + match (pat.ty.kind(), pat.kind.as_ref()) { + (_, PatKind::Binding { subpattern: Some(s), .. }) => s.fold_with(self), + (_, PatKind::AscribeUserType { subpattern: s, .. }) => s.fold_with(self), + (ty::Ref(_, t, _), PatKind::Constant { .. }) if t.is_str() => { + // Treat string literal patterns as deref patterns to a `str` constant, i.e. + // `&CONST`. This expands them like other const patterns. This could have been done + // in `const_to_pat`, but that causes issues with the rest of the matching code. + let mut new_pat = pat.super_fold_with(self); + // Make a fake const pattern of type `str` (instead of `&str`). That the carried + // constant value still knows it is of type `&str`. + new_pat.ty = t; + Pat { + kind: Box::new(PatKind::Deref { subpattern: new_pat }), + span: pat.span, + ty: pat.ty, + } + } _ => pat.super_fold_with(self), } } @@ -788,7 +802,7 @@ enum Constructor<'tcx> { /// Fake extra constructor for enums that aren't allowed to be matched exhaustively. NonExhaustive, /// Fake constructor for those types for which we can't list constructors explicitly, like - /// `f64` and `&str`. + /// `f64` and `str`. Unlistable, /// Wildcard pattern. Wildcard, @@ -931,7 +945,12 @@ impl<'tcx> Constructor<'tcx> { // Otherwise, only a wildcard pattern can match the special extra constructor. (Unlistable, _) => false, - _ => bug!("trying to compare incompatible constructors {:?} and {:?}", self, other), + _ => span_bug!( + pcx.span, + "trying to compare incompatible constructors {:?} and {:?}", + self, + other + ), } } @@ -1009,6 +1028,10 @@ impl<'tcx> Constructor<'tcx> { PatKind::Leaf { subpatterns } } } + // Note: given the expansion of `&str` patterns done in `expand_pattern`, we should + // be careful to reconstruct the correct constant pattern here. However a string + // literal pattern will never be reported as a non-exhaustiveness witness, so we + // can ignore this issue. ty::Ref(..) => PatKind::Deref { subpattern: subpatterns.next().unwrap() }, ty::Slice(_) | ty::Array(..) => bug!("bad slice pattern {:?} {:?}", self, pcx.ty), _ => PatKind::Wild, @@ -1303,9 +1326,13 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { /// [Some(0), ..] => {} /// } /// ``` + /// This is guaranteed to preserve the number of patterns in `self`. fn replace_with_pattern_arguments(&self, pat: &'p Pat<'tcx>) -> Self { match pat.kind.as_ref() { - PatKind::Deref { subpattern } => Self::from_single_pattern(subpattern), + PatKind::Deref { subpattern } => { + assert_eq!(self.len(), 1); + Fields::from_single_pattern(subpattern) + } PatKind::Leaf { subpatterns } | PatKind::Variant { subpatterns, .. } => { self.replace_with_fieldpats(subpatterns) } @@ -1596,9 +1623,8 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec vec![], - ty::Adt(..) | ty::Tuple(..) => vec![Single], - ty::Ref(_, t, _) if !t.is_str() => vec![Single], - // This type is one for which we don't know how to list constructors, like `&str` or `f64`. + ty::Adt(..) | ty::Tuple(..) | ty::Ref(..) => vec![Single], + // This type is one for which we don't know how to list constructors, like `str` or `f64`. _ => vec![Unlistable], } } @@ -2161,20 +2187,23 @@ fn pat_constructor<'p, 'tcx>( cx: &MatchCheckCtxt<'p, 'tcx>, pat: &'p Pat<'tcx>, ) -> Constructor<'tcx> { - match *pat.kind { + match pat.kind.as_ref() { PatKind::AscribeUserType { .. } => bug!(), // Handled by `expand_pattern` PatKind::Binding { .. } | PatKind::Wild => Wildcard, PatKind::Leaf { .. } | PatKind::Deref { .. } => Single, - PatKind::Variant { adt_def, variant_index, .. } => { + &PatKind::Variant { adt_def, variant_index, .. } => { Variant(adt_def.variants[variant_index].def_id) } PatKind::Constant { value } => { if let Some(int_range) = IntRange::from_const(cx.tcx, cx.param_env, value, pat.span) { IntRange(int_range) } else { - match value.ty.kind() { + match pat.ty.kind() { ty::Float(_) => FloatRange(value, value, RangeEnd::Included), - ty::Ref(_, t, _) if t.is_str() => Str(value), + // In `expand_pattern`, we convert string literals to `&CONST` patterns with + // `CONST` a pattern of type `str`. In truth this contains a constant of type + // `&str`. + ty::Str => Str(value), // All constants that can be structurally matched have already been expanded // into the corresponding `Pat`s by `const_to_pat`. Constants that remain are // opaque. @@ -2182,7 +2211,7 @@ fn pat_constructor<'p, 'tcx>( } } } - PatKind::Range(PatRange { lo, hi, end }) => { + &PatKind::Range(PatRange { lo, hi, end }) => { let ty = lo.ty; if let Some(int_range) = IntRange::from_range( cx.tcx, @@ -2197,8 +2226,7 @@ fn pat_constructor<'p, 'tcx>( FloatRange(lo, hi, end) } } - PatKind::Array { ref prefix, ref slice, ref suffix } - | PatKind::Slice { ref prefix, ref slice, ref suffix } => { + PatKind::Array { prefix, slice, suffix } | PatKind::Slice { prefix, slice, suffix } => { let array_len = match pat.ty.kind() { ty::Array(_, length) => Some(length.eval_usize(cx.tcx, cx.param_env)), ty::Slice(_) => None, diff --git a/src/test/ui/issues/issue-30240.rs b/src/test/ui/issues/issue-30240.rs index 8075532c37d76..a0c0d1626ec45 100644 --- a/src/test/ui/issues/issue-30240.rs +++ b/src/test/ui/issues/issue-30240.rs @@ -1,9 +1,9 @@ fn main() { - match "world" { //~ ERROR non-exhaustive patterns: `_` + match "world" { //~ ERROR non-exhaustive patterns: `&_` "hello" => {} } - match "world" { //~ ERROR non-exhaustive patterns: `_` + match "world" { //~ ERROR non-exhaustive patterns: `&_` ref _x if false => {} "hello" => {} } diff --git a/src/test/ui/issues/issue-30240.stderr b/src/test/ui/issues/issue-30240.stderr index 71a8bcb50cda4..a2c58d6e051b5 100644 --- a/src/test/ui/issues/issue-30240.stderr +++ b/src/test/ui/issues/issue-30240.stderr @@ -1,17 +1,17 @@ -error[E0004]: non-exhaustive patterns: `_` not covered +error[E0004]: non-exhaustive patterns: `&_` not covered --> $DIR/issue-30240.rs:2:11 | LL | match "world" { - | ^^^^^^^ pattern `_` not covered + | ^^^^^^^ pattern `&_` not covered | = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms = note: the matched value is of type `&str` -error[E0004]: non-exhaustive patterns: `_` not covered +error[E0004]: non-exhaustive patterns: `&_` not covered --> $DIR/issue-30240.rs:6:11 | LL | match "world" { - | ^^^^^^^ pattern `_` not covered + | ^^^^^^^ pattern `&_` not covered | = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms = note: the matched value is of type `&str` diff --git a/src/test/ui/pattern/usefulness/issue-78549-ref-pat-and-str.rs b/src/test/ui/pattern/usefulness/issue-78549-ref-pat-and-str.rs new file mode 100644 index 0000000000000..2879caf2c4c70 --- /dev/null +++ b/src/test/ui/pattern/usefulness/issue-78549-ref-pat-and-str.rs @@ -0,0 +1,25 @@ +// check-pass +// From https://github.com/rust-lang/rust/issues/78549 + +fn main() { + match "foo" { + "foo" => {}, + &_ => {}, + } + + match "foo" { + &_ => {}, + "foo" => {}, + } + + match ("foo", 0, "bar") { + (&_, 0, &_) => {}, + ("foo", _, "bar") => {}, + (&_, _, &_) => {}, + } + + match (&"foo", "bar") { + (&"foo", &_) => {}, + (&&_, &_) => {}, + } +} From 1bdcd02a70ef15d3602b52a340169ac68e757460 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sun, 1 Nov 2020 02:04:11 +0000 Subject: [PATCH 2/2] The need for `Single` to cover `Unlistable` was a hack It is now unneeded, since we handle `&str` patterns in a consistent way. --- .../src/thir/pattern/_match.rs | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 868923be757c1..4bbbf416bec4e 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -799,11 +799,9 @@ enum Constructor<'tcx> { /// boxes for the purposes of exhaustiveness: we must not inspect them, and they /// don't count towards making a match exhaustive. Opaque, - /// Fake extra constructor for enums that aren't allowed to be matched exhaustively. + /// Fake extra constructor for enums that aren't allowed to be matched exhaustively. Also used + /// for those types for which we cannot list constructors explicitly, like `f64` and `str`. NonExhaustive, - /// Fake constructor for those types for which we can't list constructors explicitly, like - /// `f64` and `str`. - Unlistable, /// Wildcard pattern. Wildcard, } @@ -897,6 +895,7 @@ impl<'tcx> Constructor<'tcx> { /// For the simple cases, this is simply checking for equality. For the "grouped" constructors, /// this checks for inclusion. fn is_covered_by<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, other: &Self) -> bool { + // This must be kept in sync with `is_covered_by_any`. match (self, other) { // Wildcards cover anything (_, Wildcard) => true, @@ -939,11 +938,6 @@ impl<'tcx> Constructor<'tcx> { (Opaque, _) | (_, Opaque) => false, // Only a wildcard pattern can match the special extra constructor. (NonExhaustive, _) => false, - // If we encounter a `Single` here, this means there was only one constructor for this - // type after all. - (Unlistable, Single) => true, - // Otherwise, only a wildcard pattern can match the special extra constructor. - (Unlistable, _) => false, _ => span_bug!( pcx.span, @@ -955,7 +949,8 @@ impl<'tcx> Constructor<'tcx> { } /// Faster version of `is_covered_by` when applied to many constructors. `used_ctors` is - /// assumed to be built from `matrix.head_ctors()`, and `self` is assumed to have been split. + /// assumed to be built from `matrix.head_ctors()` with wildcards filtered out, and `self` is + /// assumed to have been split from a wildcard. fn is_covered_by_any<'p>( &self, pcx: PatCtxt<'_, 'p, 'tcx>, @@ -965,8 +960,9 @@ impl<'tcx> Constructor<'tcx> { return false; } + // This must be kept in sync with `is_covered_by`. match self { - // `used_ctors` cannot contain anything else than `Single`s. + // If `self` is `Single`, `used_ctors` cannot contain anything else than `Single`s. Single => !used_ctors.is_empty(), Variant(_) => used_ctors.iter().any(|c| c == self), IntRange(range) => used_ctors @@ -979,8 +975,6 @@ impl<'tcx> Constructor<'tcx> { .any(|other| slice.is_covered_by(other)), // This constructor is never covered by anything else NonExhaustive => false, - // This constructor is only covered by `Single`s - Unlistable => used_ctors.iter().any(|c| *c == Single), Str(..) | FloatRange(..) | Opaque | Wildcard => { bug!("found unexpected ctor in all_ctors: {:?}", self) } @@ -1064,7 +1058,7 @@ impl<'tcx> Constructor<'tcx> { &Str(value) => PatKind::Constant { value }, &FloatRange(lo, hi, end) => PatKind::Range(PatRange { lo, hi, end }), IntRange(range) => return range.to_pat(pcx.cx.tcx), - NonExhaustive | Unlistable => PatKind::Wild, + NonExhaustive => PatKind::Wild, Opaque => bug!("we should not try to apply an opaque constructor"), Wildcard => bug!( "trying to apply a wildcard constructor; this should have been done in `apply_constructors`" @@ -1213,8 +1207,9 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { } _ => bug!("bad slice pattern {:?} {:?}", constructor, ty), }, - Str(..) | FloatRange(..) | IntRange(..) | NonExhaustive | Opaque | Unlistable - | Wildcard => Fields::empty(), + Str(..) | FloatRange(..) | IntRange(..) | NonExhaustive | Opaque | Wildcard => { + Fields::empty() + } }; debug!("Fields::wildcards({:?}, {:?}) = {:#?}", constructor, ty, ret); ret @@ -1624,8 +1619,8 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec vec![], ty::Adt(..) | ty::Tuple(..) | ty::Ref(..) => vec![Single], - // This type is one for which we don't know how to list constructors, like `str` or `f64`. - _ => vec![Unlistable], + // This type is one for which we cannot list constructors, like `str` or `f64`. + _ => vec![NonExhaustive], } }