From 8fcfd6e136a31ea2a199b489e4df315a937fcd6a Mon Sep 17 00:00:00 2001 From: Roxane Date: Thu, 26 Aug 2021 20:56:45 -0400 Subject: [PATCH 1/4] Add missing const edge case --- compiler/rustc_typeck/src/expr_use_visitor.rs | 7 ++++ .../2229_closure_analysis/issue-88331.rs | 33 +++++++++++++++++++ .../2229_closure_analysis/issue-88331.stderr | 27 +++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 src/test/ui/closures/2229_closure_analysis/issue-88331.rs create mode 100644 src/test/ui/closures/2229_closure_analysis/issue-88331.stderr diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index cd00d181ed0..e7c3a366cc4 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -267,6 +267,13 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { if let ty::Adt(def, _) = place_ty.kind() { if def.variants.len() > 1 { needs_to_be_read = true; + } else if let Some(variant) = def.variants.iter().next() { + // If the pat kind is a Path we want to check whether the + // variant contains at least one field. If that's the case, + // we want to borrow discr. + if matches!(pat.kind, PatKind::Path(..)) && variant.fields.len() > 0 { + needs_to_be_read = true; + } } } else { // If it is not ty::Adt, then it should be read diff --git a/src/test/ui/closures/2229_closure_analysis/issue-88331.rs b/src/test/ui/closures/2229_closure_analysis/issue-88331.rs new file mode 100644 index 00000000000..0a6d71c68ae --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/issue-88331.rs @@ -0,0 +1,33 @@ +// edition:2021 + +#[derive(Copy, Clone, PartialEq, Eq)] +pub struct Opcode(pub u8); + +impl Opcode { + pub const OP1: Opcode = Opcode(0x1); +} + +pub fn example1(msg_type: Opcode) -> impl FnMut(&[u8]) { + move |i| match msg_type { + //~^ ERROR: non-exhaustive patterns: `Opcode(0_u8)` and `Opcode(2_u8..=u8::MAX)` not covered + Opcode::OP1 => unimplemented!(), + } +} + +#[derive(Copy, Clone, PartialEq, Eq)] +pub struct Opcode2(Opcode); + +impl Opcode2 { + pub const OP2: Opcode2 = Opcode2(Opcode(0x1)); +} + + +pub fn example2(msg_type: Opcode2) -> impl FnMut(&[u8]) { + + move |i| match msg_type { + //~^ ERROR: non-exhaustive patterns: `Opcode2(Opcode(0_u8))` and `Opcode2(Opcode(2_u8..=u8::MAX))` not covered + Opcode2::OP2=> unimplemented!(), + } +} + +fn main() {} diff --git a/src/test/ui/closures/2229_closure_analysis/issue-88331.stderr b/src/test/ui/closures/2229_closure_analysis/issue-88331.stderr new file mode 100644 index 00000000000..f02d23464f1 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/issue-88331.stderr @@ -0,0 +1,27 @@ +error[E0004]: non-exhaustive patterns: `Opcode(0_u8)` and `Opcode(2_u8..=u8::MAX)` not covered + --> $DIR/issue-88331.rs:11:20 + | +LL | pub struct Opcode(pub u8); + | -------------------------- `Opcode` defined here +... +LL | move |i| match msg_type { + | ^^^^^^^^ patterns `Opcode(0_u8)` and `Opcode(2_u8..=u8::MAX)` 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 `Opcode` + +error[E0004]: non-exhaustive patterns: `Opcode2(Opcode(0_u8))` and `Opcode2(Opcode(2_u8..=u8::MAX))` not covered + --> $DIR/issue-88331.rs:27:20 + | +LL | pub struct Opcode2(Opcode); + | --------------------------- `Opcode2` defined here +... +LL | move |i| match msg_type { + | ^^^^^^^^ patterns `Opcode2(Opcode(0_u8))` and `Opcode2(Opcode(2_u8..=u8::MAX))` 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 `Opcode2` + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0004`. From 110a9b3b1ca1fddd34a3ecb8ec47fd8bb5ca7424 Mon Sep 17 00:00:00 2001 From: Roxane Date: Fri, 27 Aug 2021 09:00:50 -0400 Subject: [PATCH 2/4] Add comment and fix fmt issue --- compiler/rustc_typeck/src/expr_use_visitor.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index e7c3a366cc4..7c99a37f6e7 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -268,10 +268,21 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { if def.variants.len() > 1 { needs_to_be_read = true; } else if let Some(variant) = def.variants.iter().next() { + // We need to handle `const` in match arms slightly differently + // as they are not processed the same way as other match arms. + // Consider this const `const OP1: Opcode = Opcode(0x1)`, this + // will generate a pattern with kind Path while if use Opcode(0x1) + // this will generate pattern TupleStruct and Lit. + // When dealing with pat kind Path we need to make additional checks + // to ensure we have all the info needed to make a decision on whether + // to borrow discr. + // // If the pat kind is a Path we want to check whether the // variant contains at least one field. If that's the case, // we want to borrow discr. - if matches!(pat.kind, PatKind::Path(..)) && variant.fields.len() > 0 { + if matches!(pat.kind, PatKind::Path(..)) + && variant.fields.len() > 0 + { needs_to_be_read = true; } } From f34909d68f1410c89e3a97bdee04d7e6e7cfbe5e Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 27 Aug 2021 16:30:45 -0400 Subject: [PATCH 3/4] simplify the logic and document --- compiler/rustc_typeck/src/expr_use_visitor.rs | 66 +++++++++---------- 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index 7c99a37f6e7..d999c17b579 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -2,6 +2,7 @@ //! normal visitor, which just walks the entire body in one shot, the //! `ExprUseVisitor` determines how expressions are being used. +use hir::def::DefKind; // Export these here so that Clippy can use them. pub use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, Projection}; @@ -14,7 +15,7 @@ use rustc_index::vec::Idx; use rustc_infer::infer::InferCtxt; use rustc_middle::hir::place::ProjectionKind; use rustc_middle::mir::FakeReadCause; -use rustc_middle::ty::{self, adjustment, TyCtxt}; +use rustc_middle::ty::{self, adjustment, Ty, TyCtxt}; use rustc_target::abi::VariantIdx; use std::iter; @@ -251,43 +252,34 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { needs_to_be_read = true; } } - PatKind::TupleStruct(..) - | PatKind::Path(..) - | PatKind::Struct(..) - | PatKind::Tuple(..) => { - // If the PatKind is a TupleStruct, Path, Struct or Tuple then we want to check - // whether the Variant is a MultiVariant or a SingleVariant. We only want - // to borrow discr if it is a MultiVariant. - // If it is a SingleVariant and creates a binding we will handle that when - // this callback gets called again. + PatKind::Path(qpath) => { + // A `Path` pattern is just a name like `Foo`. This is either a + // named constant or else it refers to an ADT variant - // Get the type of the Place after all projections have been applied - let place_ty = place.place.ty(); - - if let ty::Adt(def, _) = place_ty.kind() { - if def.variants.len() > 1 { + let res = self.mc.typeck_results.qpath_res(qpath, pat.hir_id); + match res { + Res::Def(DefKind::Const, _) + | Res::Def(DefKind::AssocConst, _) => { + // Named constants have to be equated with the value + // being matched, so that's a read of the value being matched. needs_to_be_read = true; - } else if let Some(variant) = def.variants.iter().next() { - // We need to handle `const` in match arms slightly differently - // as they are not processed the same way as other match arms. - // Consider this const `const OP1: Opcode = Opcode(0x1)`, this - // will generate a pattern with kind Path while if use Opcode(0x1) - // this will generate pattern TupleStruct and Lit. - // When dealing with pat kind Path we need to make additional checks - // to ensure we have all the info needed to make a decision on whether - // to borrow discr. - // - // If the pat kind is a Path we want to check whether the - // variant contains at least one field. If that's the case, - // we want to borrow discr. - if matches!(pat.kind, PatKind::Path(..)) - && variant.fields.len() > 0 - { - needs_to_be_read = true; - } } - } else { - // If it is not ty::Adt, then it should be read + _ => { + // Otherwise, this is a struct/enum variant, and so it's + // only a read if we need to read the discriminant. + needs_to_be_read = is_multivariant_adt(place.place.ty()); + } + } + } + PatKind::TupleStruct(..) | PatKind::Struct(..) | PatKind::Tuple(..) => { + // For `Foo(..)`, `Foo { ... }` and `(...)` patterns, check if we are matching + // against a multivariant enum or struct. In that case, we have to read + // the discriminant. Otherwise this kind of pattern doesn't actually + // read anything (we'll get invoked for the `...`, which may indeed + // perform some reads). + + let place_ty = place.place.ty(); + if is_multivariant_adt(place_ty) { needs_to_be_read = true; } } @@ -854,3 +846,7 @@ fn delegate_consume<'a, 'tcx>( } } } + +fn is_multivariant_adt(ty: Ty<'tcx>) -> bool { + if let ty::Adt(def, _) = ty.kind() { def.variants.len() > 1 } else { false } +} From c4dba5a64efe340a779d8a1ee8c332140c51180e Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 27 Aug 2021 17:13:41 -0400 Subject: [PATCH 4/4] use `|=` --- compiler/rustc_typeck/src/expr_use_visitor.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index d999c17b579..1c60ef7bda0 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -262,12 +262,14 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { | Res::Def(DefKind::AssocConst, _) => { // Named constants have to be equated with the value // being matched, so that's a read of the value being matched. + // + // FIXME: We don't actually reads for ZSTs. needs_to_be_read = true; } _ => { // Otherwise, this is a struct/enum variant, and so it's // only a read if we need to read the discriminant. - needs_to_be_read = is_multivariant_adt(place.place.ty()); + needs_to_be_read |= is_multivariant_adt(place.place.ty()); } } } @@ -279,9 +281,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { // perform some reads). let place_ty = place.place.ty(); - if is_multivariant_adt(place_ty) { - needs_to_be_read = true; - } + needs_to_be_read |= is_multivariant_adt(place_ty); } PatKind::Lit(_) | PatKind::Range(..) => { // If the PatKind is a Lit or a Range then we want