Auto merge of rust-lang#62339 - pnkfelix:issue-61188-use-visitor-for-structural-match-check, r=nikomatsakis

use visitor for #[structural_match] check

This changes the code so that we recur down the structure of a type of a const (rather than just inspecting at a shallow one or two levels) when we are looking to see if it has an ADT that did not derive `PartialEq` and `Eq`.

Fix rust-lang#61188

Fix rust-lang#62307

Cc rust-lang#62336

Author: bors

src/librustc/lint/builtin.rs

@@ -348,6 +348,12 @@ declare_lint! {
     "outlives requirements can be inferred"
 }
 
+declare_lint! {
+    pub INDIRECT_STRUCTURAL_MATCH,
+    Warn,
+    "pattern with const indirectly referencing non-`#[structural_match]` type"
+}
+
 /// Some lints that are buffered from `libsyntax`. See `syntax::early_buffered_lints`.
 pub mod parser {
     declare_lint! {

src/librustc/middle/lang_items.rs

@@ -326,6 +326,7 @@ language_item_table! {
     UnpinTraitLangItem,          "unpin",              unpin_trait,             Target::Trait;
     PinTypeLangItem,             "pin",                pin_type,                Target::Struct;
 
+    // Don't be fooled by the naming here: this lang item denotes `PartialEq`, not `Eq`.
     EqTraitLangItem,             "eq",                 eq_trait,                Target::Trait;
     PartialOrdTraitLangItem,     "partial_ord",        partial_ord_trait,       Target::Trait;
     OrdTraitLangItem,            "ord",                ord_trait,               Target::Trait;

src/librustc_lint/lib.rs

@@ -429,6 +429,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             id: LintId::of(MUTABLE_BORROW_RESERVATION_CONFLICT),
             reference: "issue #59159 <https://github.com/rust-lang/rust/issues/59159>",
             edition: None,
+        },
+        FutureIncompatibleInfo {
+            id: LintId::of(INDIRECT_STRUCTURAL_MATCH),
+            reference: "issue #62411 <https://github.com/rust-lang/rust/issues/62411>",
+            edition: None,
         }
         ]);
 

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![

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};
@@ -23,6 +25,7 @@ use rustc::hir::pat_util::EnumerateAndAdjustIterator;
 use rustc::hir::ptr::P;
 
 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<PatternError>,
+    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,23 +952,94 @@ 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>,
         cv: &'tcx ty::Const<'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::<Vec<_>>()
         };
-        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,120 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
     }
 }
 
+/// This method traverses the structure of `ty`, trying to find an
+/// instance of an ADT (i.e. struct or enum) that was declared without
+/// the `#[structural_match]` attribute.
+///
+/// The "structure of a type" includes all components that would be
+/// considered when doing a pattern match on a constant of that
+/// type.
+///
+///  * This means this method descends into fields of structs/enums,
+///    and also descends into the inner type `T` of `&T` and `&mut T`
+///
+///  * The traversal doesn't dereference unsafe pointers (`*const T`,
+///    `*mut T`), and it does not visit the type arguments of an
+///    instantiated generic like `PhantomData<T>`.
+///
+/// The reason we do this search is Rust currently require all ADT's
+/// reachable from a constant's type to be annotated with
+/// `#[structural_match]`, an attribute which essentially says that
+/// the implementation of `PartialEq::eq` behaves *equivalently* to a
+/// comparison against the unfolded structure.
+///
+/// For more background on why Rust has this requirement, and issues
+/// that arose when the requirement was not enforced completely, see
+/// Rust RFC 1445, rust-lang/rust#61188, and rust-lang/rust#62307.
+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<T>` 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

src/test/ui/issues/issue-55511.rs

@@ -14,6 +14,8 @@ fn main() {
     //~^ ERROR `a` does not live long enough [E0597]
     match b {
         <() as Foo<'static>>::C => { }
+        //~^ WARN must be annotated with `#[derive(PartialEq, Eq)]`
+        //~| WARN will become a hard error in a future release
         _ => { }
     }
 }

src/test/ui/issues/issue-55511.stderr

@@ -1,3 +1,13 @@
+warning: to use a constant of type `std::cell::Cell` in a pattern, `std::cell::Cell` must be annotated with `#[derive(PartialEq, Eq)]`
+  --> $DIR/issue-55511.rs:16:9
+   |
+LL |         <() as Foo<'static>>::C => { }
+   |         ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: #[warn(indirect_structural_match)] on by default
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #62411 <https://github.com/rust-lang/rust/issues/62411>
+
 error[E0597]: `a` does not live long enough
   --> $DIR/issue-55511.rs:13:28
    |