From b56080162bc45e1cfd561b0a21e50b827c9df679 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 3 Jul 2019 16:53:42 +0200 Subject: [PATCH] Rewrite with future-compat lint for indirect pattern omitting `#[structural_match]`. Outline of changes: * Recur as deeply as necessary when searching for `#[structural_match]`. * `#[structural_match]`: handle case of `const A: & &Wrap(NoDerive)` by including the fields of an ADT during traversal of input type. (We continue to not traverse the substs of an ADT, though, so that we continue to handle `PhantomData` and `*NoDerive` properly.) * Refactored code to use `match` instead of `if let`. This ends up *with less* right-ward drift by moving the handling of the main *`ty::Adt` case *outside* the match. * Using lint (rather than hard error) mmeans we need to check that type is `PartialEq` to avoid ICE'ing the compiler in scneario where MIR codegen dispatches to `PartialEq::eq`. Added said check, and fatal error in that case. --- src/librustc_mir/hair/pattern/check_match.rs | 2 + src/librustc_mir/hair/pattern/mod.rs | 189 ++++++++++++++++++- 2 files changed, 185 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/hair/pattern/check_match.rs b/src/librustc_mir/hair/pattern/check_match.rs index ed850379af60b..a619a5c35a7ca 100644 --- a/src/librustc_mir/hair/pattern/check_match.rs +++ b/src/librustc_mir/hair/pattern/check_match.rs @@ -170,6 +170,7 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> { let mut patcx = PatternContext::new(self.tcx, self.param_env.and(self.identity_substs), self.tables); + patcx.include_lint_checks(); let pattern = expand_pattern(cx, patcx.lower_pattern(&pat)); if !patcx.errors.is_empty() { patcx.report_inlining_errors(pat.span); @@ -266,6 +267,7 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> { let mut patcx = PatternContext::new(self.tcx, self.param_env.and(self.identity_substs), self.tables); + patcx.include_lint_checks(); let pattern = patcx.lower_pattern(pat); let pattern_ty = pattern.ty; let pats: Matrix<'_, '_> = vec![smallvec![ diff --git a/src/librustc_mir/hair/pattern/mod.rs b/src/librustc_mir/hair/pattern/mod.rs index cf597ce0b6319..a15452043b35d 100644 --- a/src/librustc_mir/hair/pattern/mod.rs +++ b/src/librustc_mir/hair/pattern/mod.rs @@ -10,9 +10,11 @@ use crate::const_eval::const_variant_index; use crate::hair::util::UserAnnotatedTyHelpers; use crate::hair::constant::*; +use rustc::lint; use rustc::mir::{Field, BorrowKind, Mutability}; use rustc::mir::{UserTypeProjection}; use rustc::mir::interpret::{GlobalId, ConstValue, sign_extend, AllocId, Pointer}; +use rustc::traits::{ObligationCause, PredicateObligation}; use rustc::ty::{self, Region, TyCtxt, AdtDef, Ty, UserType, DefIdTree}; use rustc::ty::{CanonicalUserType, CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations}; use rustc::ty::subst::{SubstsRef, Kind}; @@ -22,6 +24,7 @@ use rustc::hir::def::{CtorOf, Res, DefKind, CtorKind}; use rustc::hir::pat_util::EnumerateAndAdjustIterator; use rustc_data_structures::indexed_vec::Idx; +use rustc_data_structures::fx::FxHashSet; use std::cmp::Ordering; use std::fmt; @@ -332,6 +335,7 @@ pub struct PatternContext<'a, 'tcx> { pub tables: &'a ty::TypeckTables<'tcx>, pub substs: SubstsRef<'tcx>, pub errors: Vec, + include_lint_checks: bool, } impl<'a, 'tcx> Pattern<'tcx> { @@ -363,10 +367,16 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { param_env: param_env_and_substs.param_env, tables, substs: param_env_and_substs.value, - errors: vec![] + errors: vec![], + include_lint_checks: false, } } + pub fn include_lint_checks(&mut self) -> &mut Self { + self.include_lint_checks = true; + self + } + pub fn lower_pattern(&mut self, pat: &'tcx hir::Pat) -> Pattern<'tcx> { // When implicit dereferences have been inserted in this pattern, the unadjusted lowered // pattern has the type that results *after* dereferencing. For example, in this code: @@ -942,7 +952,7 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { /// Converts an evaluated constant to a pattern (if possible). /// This means aggregate values (like structs and enums) are converted - /// to a pattern that matches the value (as if you'd compared via equality). + /// to a pattern that matches the value (as if you'd compared via structural equality). fn const_to_pat( &self, instance: ty::Instance<'tcx>, @@ -950,15 +960,86 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { id: hir::HirId, span: Span, ) -> Pattern<'tcx> { + // This method is just a warpper handling a validity check; the heavy lifting is + // performed by the recursive const_to_pat_inner method, which is not meant to be + // invoked except by this method. + // + // once indirect_structural_match is a full fledged error, this + // level of indirection can be eliminated + debug!("const_to_pat: cv={:#?} id={:?}", cv, id); - let adt_subpattern = |i, variant_opt| { + debug!("const_to_pat: cv.ty={:?} span={:?}", cv.ty, span); + + let mut saw_error = false; + let inlined_const_as_pat = self.const_to_pat_inner(instance, cv, id, span, &mut saw_error); + + if self.include_lint_checks && !saw_error { + // If we were able to successfully convert the const to some pat, double-check + // that the type of the const obeys `#[structural_match]` constraint. + if let Some(adt_def) = search_for_adt_without_structural_match(self.tcx, cv.ty) { + + let path = self.tcx.def_path_str(adt_def.did); + let msg = format!( + "to use a constant of type `{}` in a pattern, \ + `{}` must be annotated with `#[derive(PartialEq, Eq)]`", + path, + path, + ); + + // before issuing lint, double-check there even *is* a + // semantic PartialEq for us to dispatch to. + // + // (If there isn't, then we can safely issue a hard + // error, because that's never worked, due to compiler + // using PartialEq::eq in this scenario in the past.) + + let ty_is_partial_eq: bool = { + let partial_eq_trait_id = self.tcx.lang_items().eq_trait().unwrap(); + let obligation: PredicateObligation<'_> = + self.tcx.predicate_for_trait_def(self.param_env, + ObligationCause::misc(span, id), + partial_eq_trait_id, + 0, + cv.ty, + &[]); + self.tcx + .infer_ctxt() + .enter(|infcx| infcx.predicate_may_hold(&obligation)) + }; + + if !ty_is_partial_eq { + // span_fatal avoids ICE from resolution of non-existent method (rare case). + self.tcx.sess.span_fatal(span, &msg); + } else { + self.tcx.lint_hir(lint::builtin::INDIRECT_STRUCTURAL_MATCH, id, span, &msg); + } + } + } + + inlined_const_as_pat + } + + /// Recursive helper for `const_to_pat`; invoke that (instead of calling this directly). + fn const_to_pat_inner( + &self, + instance: ty::Instance<'tcx>, + cv: &'tcx ty::Const<'tcx>, + id: hir::HirId, + span: Span, + // This tracks if we signal some hard error for a given const + // value, so that we will not subsequently issue an irrelevant + // lint for the same const value. + saw_const_match_error: &mut bool, + ) -> Pattern<'tcx> { + + let mut adt_subpattern = |i, variant_opt| { let field = Field::new(i); let val = crate::const_eval::const_field( self.tcx, self.param_env, variant_opt, field, cv ); - self.const_to_pat(instance, val, id, span) + self.const_to_pat_inner(instance, val, id, span, saw_const_match_error) }; - let adt_subpatterns = |n, variant_opt| { + let mut adt_subpatterns = |n, variant_opt| { (0..n).map(|i| { let field = Field::new(i); FieldPattern { @@ -967,7 +1048,8 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { } }).collect::>() }; - debug!("const_to_pat: cv.ty={:?} span={:?}", cv.ty, span); + + let kind = match cv.ty.sty { ty::Float(_) => { self.tcx.lint_hir( @@ -982,9 +1064,11 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { } ty::Adt(adt_def, _) if adt_def.is_union() => { // Matching on union fields is unsafe, we can't hide it in constants + *saw_const_match_error = true; self.tcx.sess.span_err(span, "cannot use unions in constant patterns"); PatternKind::Wild } + // keep old code until future-compat upgraded to errors. ty::Adt(adt_def, _) if !self.tcx.has_attr(adt_def.did, sym::structural_match) => { let path = self.tcx.def_path_str(adt_def.did); let msg = format!( @@ -993,9 +1077,11 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { path, path, ); + *saw_const_match_error = true; self.tcx.sess.span_err(span, &msg); PatternKind::Wild } + // keep old code until future-compat upgraded to errors. ty::Ref(_, ty::TyS { sty: ty::Adt(adt_def, _), .. }, _) if !self.tcx.has_attr(adt_def.did, sym::structural_match) => { // HACK(estebank): Side-step ICE #53708, but anything other than erroring here @@ -1007,6 +1093,7 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { path, path, ); + *saw_const_match_error = true; self.tcx.sess.span_err(span, &msg); PatternKind::Wild } @@ -1058,6 +1145,96 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { } } +fn search_for_adt_without_structural_match<'tcx>(tcx: TyCtxt<'tcx>, + ty: Ty<'tcx>) + -> Option<&'tcx AdtDef> +{ + // Import here (not mod level), because `TypeFoldable::fold_with` + // conflicts with `PatternFoldable::fold_with` + use crate::rustc::ty::fold::TypeVisitor; + use crate::rustc::ty::TypeFoldable; + + let mut search = Search { tcx, found: None, seen: FxHashSet::default() }; + ty.visit_with(&mut search); + return search.found; + + struct Search<'tcx> { + tcx: TyCtxt<'tcx>, + + // records the first ADT we find without `#[structural_match` + found: Option<&'tcx AdtDef>, + + // tracks ADT's previously encountered during search, so that + // we will not recur on them again. + seen: FxHashSet<&'tcx AdtDef>, + } + + impl<'tcx> TypeVisitor<'tcx> for Search<'tcx> { + fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool { + debug!("Search visiting ty: {:?}", ty); + + let (adt_def, substs) = match ty.sty { + ty::Adt(adt_def, substs) => (adt_def, substs), + ty::RawPtr(..) => { + // `#[structural_match]` ignores substructure of + // `*const _`/`*mut _`, so skip super_visit_with + + // (But still tell caller to continue search.) + return false; + } + ty::Array(_, n) if n.assert_usize(self.tcx) == Some(0) => { + // rust-lang/rust#62336: ignore type of contents + // for empty array. + return false; + } + _ => { + ty.super_visit_with(self); + return false; + } + }; + + if !self.tcx.has_attr(adt_def.did, sym::structural_match) { + self.found = Some(&adt_def); + debug!("Search found adt_def: {:?}", adt_def); + return true // Halt visiting! + } + + if self.seen.contains(adt_def) { + debug!("Search already seen adt_def: {:?}", adt_def); + // let caller continue its search + return false; + } + + self.seen.insert(adt_def); + + // `#[structural_match]` does not care about the + // instantiation of the generics in an ADT (it + // instead looks directly at its fields outside + // this match), so we skip super_visit_with. + // + // (Must not recur on substs for `PhantomData` cf + // rust-lang/rust#55028 and rust-lang/rust#55837; but also + // want to skip substs when only uses of generic are + // behind unsafe pointers `*const T`/`*mut T`.) + + // even though we skip super_visit_with, we must recur on + // fields of ADT. + let tcx = self.tcx; + for field_ty in adt_def.all_fields().map(|field| field.ty(tcx, substs)) { + if field_ty.visit_with(self) { + // found an ADT without `#[structural_match]`; halt visiting! + assert!(self.found.is_some()); + return true; + } + } + + // Even though we do not want to recur on substs, we do + // want our caller to continue its own search. + false + } + } +} + impl UserAnnotatedTyHelpers<'tcx> for PatternContext<'_, 'tcx> { fn tcx(&self) -> TyCtxt<'tcx> { self.tcx