Auto merge of #65519 - pnkfelix:issue-63438-trait-based-structural-match, r=matthewjasper

trait-based structural match implementation

Moves from using a `#[structural_match]` attribute to using a marker trait (or pair of such traits, really) instead.

Fix #63438.

(This however does not remove the hacks that I believe were put into place to support the previous approach of injecting the attribute based on the presence of both derives... I have left that for follow-on work.)

Author: bors

src/libcore/cmp.rs

@@ -211,7 +211,7 @@ pub trait PartialEq<Rhs: ?Sized = Self> {
 /// Derive macro generating an impl of the trait `PartialEq`.
 #[rustc_builtin_macro]
 #[stable(feature = "builtin_macro_prelude", since = "1.38.0")]
-#[allow_internal_unstable(core_intrinsics)]
+#[allow_internal_unstable(core_intrinsics, structural_match)]
 pub macro PartialEq($item:item) { /* compiler built-in */ }
 
 /// Trait for equality comparisons which are [equivalence relations](
@@ -273,7 +273,7 @@ pub trait Eq: PartialEq<Self> {
 /// Derive macro generating an impl of the trait `Eq`.
 #[rustc_builtin_macro]
 #[stable(feature = "builtin_macro_prelude", since = "1.38.0")]
-#[allow_internal_unstable(core_intrinsics, derive_eq)]
+#[allow_internal_unstable(core_intrinsics, derive_eq, structural_match)]
 pub macro Eq($item:item) { /* compiler built-in */ }
 
 // FIXME: this struct is used solely by #[derive] to

src/libcore/marker.rs

@@ -126,6 +126,85 @@ pub trait Unsize<T: ?Sized> {
     // Empty.
 }
 
+/// Required trait for constants used in pattern matches.
+///
+/// Any type that derives `PartialEq` automatically implements this trait,
+/// *regardless* of whether its type-parameters implement `Eq`.
+///
+/// If a `const` item contains some type that does not implement this trait,
+/// then that type either (1.) does not implement `PartialEq` (which means the
+/// constant will not provide that comparison method, which code generation
+/// assumes is available), or (2.) it implements *its own* version of
+/// `PartialEq` (which we assume does not conform to a structural-equality
+/// comparison).
+///
+/// In either of the two scenarios above, we reject usage of such a constant in
+/// a pattern match.
+///
+/// See also the [structural match RFC][RFC1445], and [issue 63438][] which
+/// motivated migrating from attribute-based design to this trait.
+///
+/// [RFC1445]: https://github.com/rust-lang/rfcs/blob/master/text/1445-restrict-constants-in-patterns.md
+/// [issue 63438]: https://github.com/rust-lang/rust/issues/63438
+#[cfg(not(bootstrap))]
+#[unstable(feature = "structural_match", issue = "31434")]
+#[rustc_on_unimplemented(message="the type `{Self}` does not `#[derive(PartialEq)]`")]
+#[lang = "structural_peq"]
+pub trait StructuralPartialEq {
+    // Empty.
+}
+
+/// Required trait for constants used in pattern matches.
+///
+/// Any type that derives `Eq` automatically implements this trait, *regardless*
+/// of whether its type-parameters implement `Eq`.
+///
+/// This is a hack to workaround a limitation in our type-system.
+///
+/// Background:
+///
+/// We want to require that types of consts used in pattern matches
+/// have the attribute `#[derive(PartialEq, Eq)]`.
+///
+/// In a more ideal world, we could check that requirement by just checking that
+/// the given type implements both (1.) the `StructuralPartialEq` trait *and*
+/// (2.) the `Eq` trait. However, you can have ADTs that *do* `derive(PartialEq, Eq)`,
+/// and be a case that we want the compiler to accept, and yet the constant's
+/// type fails to implement `Eq`.
+///
+/// Namely, a case like this:
+///
+/// ```rust
+/// #[derive(PartialEq, Eq)]
+/// struct Wrap<X>(X);
+/// fn higher_order(_: &()) { }
+/// const CFN: Wrap<fn(&())> = Wrap(higher_order);
+/// fn main() {
+///     match CFN {
+///         CFN => {}
+///         _ => {}
+///     }
+/// }
+/// ```
+///
+/// (The problem in the above code is that `Wrap<fn(&())>` does not implement
+/// `PartialEq`, nor `Eq`, because `for<'a> fn(&'a _)` does not implement those
+/// traits.)
+///
+/// Therefore, we cannot rely on naive check for `StructuralPartialEq` and
+/// mere `Eq`.
+///
+/// As a hack to work around this, we use two separate traits injected by each
+/// of the two derives (`#[derive(PartialEq)]` and `#[derive(Eq)]`) and check
+/// that both of them are present as part of structural-match checking.
+#[cfg(not(bootstrap))]
+#[unstable(feature = "structural_match", issue = "31434")]
+#[rustc_on_unimplemented(message="the type `{Self}` does not `#[derive(Eq)]`")]
+#[lang = "structural_teq"]
+pub trait StructuralEq {
+    // Empty.
+}
+
 /// Types whose values can be duplicated simply by copying bits.
 ///
 /// By default, variable bindings have 'move semantics.' In other
@@ -437,6 +516,14 @@ macro_rules! impls{
                 $t
             }
         }
+
+        #[cfg(not(bootstrap))]
+        #[unstable(feature = "structural_match", issue = "31434")]
+        impl<T: ?Sized> StructuralPartialEq for $t<T> { }
+
+        #[cfg(not(bootstrap))]
+        #[unstable(feature = "structural_match", issue = "31434")]
+        impl<T: ?Sized> StructuralEq for $t<T> { }
         )
 }
 

src/librustc/middle/lang_items.rs

@@ -297,6 +297,10 @@ language_item_table! {
 
     SizedTraitLangItem,          "sized",              sized_trait,             Target::Trait;
     UnsizeTraitLangItem,         "unsize",             unsize_trait,            Target::Trait;
+    // trait injected by #[derive(PartialEq)], (i.e. "Partial EQ").
+    StructuralPeqTraitLangItem,  "structural_peq",     structural_peq_trait,    Target::Trait;
+    // trait injected by #[derive(Eq)], (i.e. "Total EQ"; no, I will not apologize).
+    StructuralTeqTraitLangItem,  "structural_teq",     structural_teq_trait,    Target::Trait;
     CopyTraitLangItem,           "copy",               copy_trait,              Target::Trait;
     CloneTraitLangItem,          "clone",              clone_trait,             Target::Trait;
     SyncTraitLangItem,           "sync",               sync_trait,              Target::Trait;

src/librustc/traits/error_reporting.rs

@@ -2167,6 +2167,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
             ObligationCauseCode::ConstSized => {
                 err.note("constant expressions must have a statically known size");
             }
+            ObligationCauseCode::ConstPatternStructural => {
+                err.note("constants used for pattern-matching must derive `PartialEq` and `Eq`");
+            }
             ObligationCauseCode::SharedStatic => {
                 err.note("shared static variables must have a type that implements `Sync`");
             }

src/librustc/traits/mod.rs

@@ -239,6 +239,9 @@ pub enum ObligationCauseCode<'tcx> {
     /// Computing common supertype in the pattern guard for the arms of a match expression
     MatchExpressionArmPattern { span: Span, ty: Ty<'tcx> },
 
+    /// Constants in patterns must have `Structural` type.
+    ConstPatternStructural,
+
     /// Computing common supertype in an if expression
     IfExpression(Box<IfExpressionCause>),
 

src/librustc/traits/structural_impls.rs

@@ -497,6 +497,7 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
             super::RepeatVec => Some(super::RepeatVec),
             super::FieldSized { adt_kind, last } => Some(super::FieldSized { adt_kind, last }),
             super::ConstSized => Some(super::ConstSized),
+            super::ConstPatternStructural => Some(super::ConstPatternStructural),
             super::SharedStatic => Some(super::SharedStatic),
             super::BuiltinDerivedObligation(ref cause) => {
                 tcx.lift(cause).map(super::BuiltinDerivedObligation)

src/librustc/ty/mod.rs

@@ -51,7 +51,7 @@ use syntax::symbol::{kw, sym, Symbol};
 use syntax_pos::Span;
 
 use smallvec;
-use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
+use rustc_data_structures::fx::{FxIndexMap};
 use rustc_data_structures::stable_hasher::{StableHasher, HashStable};
 use rustc_index::vec::{Idx, IndexVec};
 
@@ -84,6 +84,10 @@ pub use self::context::{
 
 pub use self::instance::{Instance, InstanceDef};
 
+pub use self::structural_match::search_for_structural_match_violation;
+pub use self::structural_match::type_marked_structural;
+pub use self::structural_match::NonStructuralMatchTy;
+
 pub use self::trait_def::TraitDef;
 
 pub use self::query::queries;
@@ -116,6 +120,7 @@ pub mod util;
 mod context;
 mod instance;
 mod structural_impls;
+mod structural_match;
 mod sty;
 
 // Data types
@@ -3396,130 +3401,6 @@ fn asyncness(tcx: TyCtxt<'_>, def_id: DefId) -> hir::IsAsync {
     fn_like.asyncness()
 }
 
-pub enum NonStructuralMatchTy<'tcx> {
-    Adt(&'tcx AdtDef),
-    Param,
-}
-
-/// 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, or a generic type parameter
-/// (which cannot be determined to be `structural_match`).
-///
-/// 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 ADTs
-/// 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.
-pub fn search_for_structural_match_violation<'tcx>(
-    tcx: TyCtxt<'tcx>,
-    ty: Ty<'tcx>,
-) -> Option<NonStructuralMatchTy<'tcx>> {
-    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 or type parameter we find without `#[structural_match`.
-        found: Option<NonStructuralMatchTy<'tcx>>,
-
-        // Tracks ADTs previously encountered during search, so that
-        // we will not recurse on them again.
-        seen: FxHashSet<hir::def_id::DefId>,
-    }
-
-    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.kind {
-                ty::Adt(adt_def, substs) => (adt_def, substs),
-                ty::Param(_) => {
-                    self.found = Some(NonStructuralMatchTy::Param);
-                    return true; // Stop visiting.
-                }
-                ty::RawPtr(..) => {
-                    // `#[structural_match]` ignores substructure of
-                    // `*const _`/`*mut _`, so skip super_visit_with
-                    //
-                    // (But still tell caller to continue search.)
-                    return false;
-                }
-                ty::FnDef(..) | ty::FnPtr(..) => {
-                    // types of formals and return in `fn(_) -> _` are also irrelevant
-                    //
-                    // (But still tell caller to continue search.)
-                    return false;
-                }
-                ty::Array(_, n) if n.try_eval_usize(self.tcx, ty::ParamEnv::reveal_all()) == 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(NonStructuralMatchTy::Adt(&adt_def));
-                debug!("Search found adt_def: {:?}", adt_def);
-                return true; // Stop visiting.
-            }
-
-            if !self.seen.insert(adt_def.did) {
-                debug!("Search already seen adt_def: {:?}", adt_def);
-                // let caller continue its search
-                return false;
-            }
-
-            // `#[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
-        }
-    }
-}
-
 pub fn provide(providers: &mut ty::query::Providers<'_>) {
     context::provide(providers);
     erase_regions::provide(providers);