Rollup merge of rust-lang#156807 - kpreid:speq, r=petrochenkov

Add `T: PartialEq` bounds to derived `StructuralPartialEq` impls.

Fixes <rust-lang#147714>.

This is a breaking change to fix a bug, so it will need a crater run if it is to be done at all.

The changes in `const_to_pat.rs` are entirely to avoid regressing diagnostic quality, and should not make any difference to what code is accepted. The changes in `compute_applicable_impls_for_diagnostics` and its callers are entirely to be able to reuse that algorithm for this purpose.

@rustbot label +T-lang

Author: JonathanBrouwer

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

@@ -22,7 +22,10 @@ pub(crate) fn expand_deriving_partial_eq(
         path: path_std!(marker::StructuralPartialEq),
         skip_path_as_bound: true, // crucial!
         needs_copy_as_bound_if_packed: false,
-        additional_bounds: Vec::new(),
+        // The `StructuralPartialEq` impl must have the *same* bounds as the `PartialEq` impl,
+        // or it will apply in situations where it should not, such as in the bug
+        // <https://github.com/rust-lang/rust/issues/147714>.
+        additional_bounds: vec![ty::Ty::Path(path_std!(cmp::PartialEq))],
         // We really don't support unions, but that's already checked by the impl generated below;
         // a second check here would lead to redundant error messages.
         supports_unions: true,

compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs

@@ -17,6 +17,9 @@ use rustc_middle::ty::{
 };
 use rustc_span::def_id::DefId;
 use rustc_span::{DUMMY_SP, Span};
+use rustc_trait_selection::error_reporting::traits::ambiguity::{
+    CandidateSource, compute_applicable_impls_for_diagnostics,
+};
 use rustc_trait_selection::traits::ObligationCause;
 use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
 use tracing::{debug, instrument, trace};
@@ -232,15 +235,35 @@ impl<'tcx> ConstToPat<'tcx> {
 
         let kind = match ty.kind() {
             // Extremely important check for all ADTs!
-            // Make sure they are eligible to be used in patterns, and if not, emit an error.
+            // Make sure they are eligible to be used in patterns (structural), and if not, emit an
+            // error.
             ty::Adt(adt_def, _) if !self.type_marked_structural(ty) => {
                 // This ADT cannot be used as a constant in patterns.
                 debug!(?adt_def, ?value.ty, "ADT type in pattern is not `type_marked_structural`");
                 let PartialEqImplStatus {
-                    is_derived, structural_partial_eq, non_blanket_impl, ..
+                    is_derived,
+                    possibly_inapplicable_structural_partial_eq,
+                    non_blanket_impl,
+                    possibly_inapplicable_derived_partial_eq,
+                    has_impl,
+                    ..
                 } = type_has_partial_eq_impl(self.tcx, self.typing_env, ty);
+
+                // If we have a derived PartialEq impl but it does not apply,
+                // then error about that instead, because `TypeNotStructural` gives advice that is
+                // relevant only when the problem is that `ty` does not derive `PartialEq`.
+                //
+                // Note that this is a duplicate of a check in `unevaluated_to_pat()`,
+                // which we would run later if we weren’t emitting an error now.
+                if possibly_inapplicable_derived_partial_eq && !has_impl {
+                    let mut err =
+                        self.tcx.dcx().create_err(TypeNotPartialEq { span: self.span, ty });
+                    extend_type_not_partial_eq(self.tcx, self.typing_env, ty, &mut err);
+                    return self.mk_err(err, ty);
+                }
+
                 let (manual_partialeq_impl_span, manual_partialeq_impl_note) =
-                    match (structural_partial_eq, non_blanket_impl) {
+                    match (possibly_inapplicable_structural_partial_eq, non_blanket_impl) {
                         (true, _) => (None, false),
                         (_, Some(def_id)) if def_id.is_local() && !is_derived => {
                             (Some(tcx.def_span(def_id)), false)
@@ -488,8 +511,9 @@ fn extend_type_not_partial_eq<'tcx>(
                     let PartialEqImplStatus {
                         has_impl,
                         is_derived,
-                        structural_partial_eq,
+                        possibly_inapplicable_structural_partial_eq: structural_partial_eq,
                         non_blanket_impl,
+                        possibly_inapplicable_derived_partial_eq: _,
                     } = type_has_partial_eq_impl(self.tcx, self.typing_env, ty);
                     match (has_impl, is_derived, structural_partial_eq, non_blanket_impl) {
                         (_, _, true, _) => {}
@@ -558,10 +582,20 @@ fn extend_type_not_partial_eq<'tcx>(
 
 #[derive(Debug)]
 struct PartialEqImplStatus {
+    /// There is a `PartialEq` impl that applies to the type.
     has_impl: bool,
+
+    /// The `PartialEq` impl is `#[automatically_derived]`.
     is_derived: bool,
-    structural_partial_eq: bool,
+    /// The `DefId` of the same impl that `is_derived` refers to.
     non_blanket_impl: Option<DefId>,
+
+    /// If true, there is a `StructuralPartialEq` implementation,
+    /// but its bounds might not be satisfied.
+    possibly_inapplicable_structural_partial_eq: bool,
+    /// If true, there is a derived `PartialEq` implementation for the type,
+    /// but its bounds might not be satisfied.
+    possibly_inapplicable_derived_partial_eq: bool,
 }
 
 #[instrument(level = "trace", skip(tcx), ret)]
@@ -580,23 +614,6 @@ fn type_has_partial_eq_impl<'tcx>(
     let structural_partial_eq_trait_id =
         tcx.require_lang_item(hir::LangItem::StructuralPeq, DUMMY_SP);
 
-    let partial_eq_obligation = Obligation::new(
-        tcx,
-        ObligationCause::dummy(),
-        param_env,
-        ty::TraitRef::new(tcx, partial_eq_trait_id, [ty, ty]),
-    );
-
-    let mut automatically_derived = false;
-    let mut structural_peq = false;
-    let mut impl_def_id = None;
-    for def_id in tcx.non_blanket_impls_for_ty(partial_eq_trait_id, ty) {
-        automatically_derived = find_attr!(tcx, def_id, AutomaticallyDerived);
-        impl_def_id = Some(def_id);
-    }
-    for _ in tcx.non_blanket_impls_for_ty(structural_partial_eq_trait_id, ty) {
-        structural_peq = true;
-    }
     // This *could* accept a type that isn't actually `PartialEq`, because region bounds get
     // ignored. However that should be pretty much impossible since consts that do not depend on
     // generics can only mention the `'static` lifetime, and how would one have a type that's
@@ -607,10 +624,60 @@ fn type_has_partial_eq_impl<'tcx>(
     // that patterns can only do things that the code could also do without patterns, but it is
     // needed for backwards compatibility. The actual pattern matching compares primitive values,
     // `PartialEq::eq` never gets invoked, so there's no risk of us running non-const code.
+    let has_impl = {
+        let obligation = Obligation::new(
+            tcx,
+            ObligationCause::dummy(),
+            param_env,
+            ty::TraitRef::new(tcx, partial_eq_trait_id, [ty, ty]),
+        );
+        infcx.predicate_must_hold_modulo_regions(&obligation)
+    };
+
+    // Determine whether there are is a derived `PartialEq` implementation, whether or not its
+    // bounds are met.
+    let possibly_inapplicable_derived_partial_eq = {
+        let obligation = Obligation::new(
+            tcx,
+            ObligationCause::dummy(),
+            param_env,
+            ty::Binder::dummy(ty::TraitRef::new(tcx, partial_eq_trait_id, [ty, ty])),
+        );
+        compute_applicable_impls_for_diagnostics(&infcx, &obligation, true).iter().any(
+            |candidate_source| {
+                matches!(
+                    candidate_source,
+                    &CandidateSource::DefId(def_id)
+                    if find_attr!(tcx, def_id, AutomaticallyDerived)
+                )
+            },
+        )
+    };
+
+    let possibly_inapplicable_structural_partial_eq = {
+        let obligation = Obligation::new(
+            tcx,
+            ObligationCause::dummy(),
+            param_env,
+            ty::Binder::dummy(ty::TraitRef::new(tcx, structural_partial_eq_trait_id, [ty])),
+        );
+        compute_applicable_impls_for_diagnostics(&infcx, &obligation, true)
+            .iter()
+            .any(|candidate_source| matches!(candidate_source, CandidateSource::DefId(_)))
+    };
+
+    let mut automatically_derived = false;
+    let mut impl_def_id = None;
+    for def_id in tcx.non_blanket_impls_for_ty(partial_eq_trait_id, ty) {
+        automatically_derived = find_attr!(tcx, def_id, AutomaticallyDerived);
+        impl_def_id = Some(def_id);
+    }
+
     PartialEqImplStatus {
-        has_impl: infcx.predicate_must_hold_modulo_regions(&partial_eq_obligation),
+        has_impl,
         is_derived: automatically_derived,
-        structural_partial_eq: structural_peq,
+        possibly_inapplicable_structural_partial_eq,
         non_blanket_impl: impl_def_id,
+        possibly_inapplicable_derived_partial_eq,
     }
 }

compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs

@@ -296,7 +296,8 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
         let obligation =
             Obligation::new(tcx, ObligationCause::dummy(), param_env, ty::Binder::dummy(trait_ref));
 
-        let applicable_impls = compute_applicable_impls_for_diagnostics(self.infcx, &obligation);
+        let applicable_impls =
+            compute_applicable_impls_for_diagnostics(self.infcx, &obligation, false);
 
         for candidate in applicable_impls {
             let impl_def_id = match candidate {

compiler/rustc_trait_selection/src/error_reporting/traits/ambiguity.rs

@@ -32,6 +32,7 @@ pub enum CandidateSource {
 pub fn compute_applicable_impls_for_diagnostics<'tcx>(
     infcx: &InferCtxt<'tcx>,
     obligation: &PolyTraitObligation<'tcx>,
+    ignore_predicates_of_impls: bool,
 ) -> Vec<CandidateSource> {
     let tcx = infcx.tcx;
     let param_env = obligation.param_env;
@@ -71,26 +72,28 @@ pub fn compute_applicable_impls_for_diagnostics<'tcx>(
                 _ => return false,
             }
 
-            let obligations = tcx
-                .predicates_of(impl_def_id)
-                .instantiate(tcx, impl_args)
-                .into_iter()
-                .map(|(predicate, _)| {
-                    Obligation::new(
-                        tcx,
-                        ObligationCause::dummy(),
-                        param_env,
-                        predicate.skip_norm_wip(),
-                    )
-                })
-                // Kinda hacky, but let's just throw away obligations that overflow.
-                // This may reduce the accuracy of this check (if the obligation guides
-                // inference or it actually resulted in error after others are processed)
-                // ... but this is diagnostics code.
-                .filter(|obligation| {
-                    infcx.next_trait_solver() || infcx.evaluate_obligation(obligation).is_ok()
-                });
-            ocx.register_obligations(obligations);
+            if !ignore_predicates_of_impls {
+                let obligations = tcx
+                    .predicates_of(impl_def_id)
+                    .instantiate(tcx, impl_args)
+                    .into_iter()
+                    .map(|(predicate, _)| {
+                        Obligation::new(
+                            tcx,
+                            ObligationCause::dummy(),
+                            param_env,
+                            predicate.skip_norm_wip(),
+                        )
+                    })
+                    // Kinda hacky, but let's just throw away obligations that overflow.
+                    // This may reduce the accuracy of this check (if the obligation guides
+                    // inference or it actually resulted in error after others are processed)
+                    // ... but this is diagnostics code.
+                    .filter(|obligation| {
+                        infcx.next_trait_solver() || infcx.evaluate_obligation(obligation).is_ok()
+                    });
+                ocx.register_obligations(obligations);
+            }
 
             ocx.try_evaluate_obligations().is_empty()
         })
@@ -306,6 +309,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
                 let mut ambiguities = compute_applicable_impls_for_diagnostics(
                     self.infcx,
                     &obligation.with(self.tcx, trait_pred),
+                    false,
                 );
                 let has_non_region_infer = trait_pred
                     .skip_binder()

tests/ui/consts/const_in_pattern/derive-and-manual-partialeq-issue-147714.rs

@@ -0,0 +1,31 @@
+// `derive(PartialEq)`, which also implements `StructuralPartialEq`, must not allow the latter impl
+// to be used with other non-derived implementations of `PartialEq`.
+//
+// Test case by theemathas from <https://github.com/rust-lang/rust/issues/147714>.
+
+#[allow(dead_code)]
+#[derive(PartialEq)]
+enum Thing<T> {
+    A(T),
+    B,
+}
+
+struct Incomparable;
+
+// This impl does not obey StructuralPartialEq's rules.
+impl PartialEq for Thing<Incomparable> {
+    fn eq(&self, _: &Self) -> bool {
+        panic!()
+    }
+}
+
+// This constant does not obey StructuralPartialEq's rules, so it should not
+// implement StructuralPartialEq.
+const X: Thing<Incomparable> = Thing::B;
+
+fn main() {
+    if let X = X {
+        //~^ ERROR constant of non-structural type `Thing<Incomparable>` in a pattern
+        println!("equal");
+    }
+}

tests/ui/consts/const_in_pattern/derive-and-manual-partialeq-issue-147714.stderr

@@ -0,0 +1,20 @@
+error: constant of non-structural type `Thing<Incomparable>` in a pattern
+  --> $DIR/derive-and-manual-partialeq-issue-147714.rs:27:12
+   |
+LL | enum Thing<T> {
+   | ------------- `Thing<Incomparable>` must be annotated with `#[derive(PartialEq)]` to be usable in patterns
+...
+LL | const X: Thing<Incomparable> = Thing::B;
+   | ---------------------------- constant defined here
+...
+LL |     if let X = X {
+   |            ^ constant of non-structural type
+   |
+help: if `Thing<Incomparable>` manually implemented `PartialEq`, you could check for equality instead of pattern matching
+   |
+LL -     if let X = X {
+LL +     if X == X {
+   |
+
+error: aborting due to 1 previous error
+

tests/ui/consts/const_in_pattern/issue-65466.rs

@@ -11,7 +11,7 @@ const C: &[O<B>] = &[O::None];
 fn main() {
     let x = O::None;
     match &[x][..] {
-        C => (), //~ ERROR constant of non-structural type `&[O<B>]` in a pattern
+        C => (), //~ ERROR constant of non-structural type `O<B>` in a pattern
         _ => (),
     }
 }

tests/ui/consts/const_in_pattern/issue-65466.stderr

@@ -1,4 +1,4 @@
-error: constant of non-structural type `&[O<B>]` in a pattern
+error: constant of non-structural type `O<B>` in a pattern
   --> $DIR/issue-65466.rs:14:9
    |
 LL | struct B;

tests/ui/derives/deriving-all-codegen.stdout

@@ -786,7 +786,10 @@ impl<T: ::core::hash::Hash + Trait, U: ::core::hash::Hash> ::core::hash::Hash
     }
 }
 #[automatically_derived]
-impl<T: Trait, U> ::core::marker::StructuralPartialEq for Generic<T, U> { }
+impl<T: ::core::cmp::PartialEq + Trait, U: ::core::cmp::PartialEq>
+    ::core::marker::StructuralPartialEq for Generic<T, U> where
+    T::A: ::core::cmp::PartialEq {
+}
 #[automatically_derived]
 impl<T: ::core::cmp::PartialEq + Trait, U: ::core::cmp::PartialEq>
     ::core::cmp::PartialEq for Generic<T, U> where
@@ -903,8 +906,9 @@ impl<T: ::core::hash::Hash + ::core::marker::Copy + Trait,
     }
 }
 #[automatically_derived]
-impl<T: Trait, U> ::core::marker::StructuralPartialEq for PackedGeneric<T, U>
-    {
+impl<T: ::core::cmp::PartialEq + Trait, U: ::core::cmp::PartialEq>
+    ::core::marker::StructuralPartialEq for PackedGeneric<T, U> where
+    T::A: ::core::cmp::PartialEq {
 }
 #[automatically_derived]
 impl<T: ::core::cmp::PartialEq + ::core::marker::Copy + Trait,
@@ -1595,7 +1599,9 @@ impl<T: ::core::hash::Hash, U: ::core::hash::Hash> ::core::hash::Hash for
     }
 }
 #[automatically_derived]
-impl<T, U> ::core::marker::StructuralPartialEq for EnumGeneric<T, U> { }
+impl<T: ::core::cmp::PartialEq, U: ::core::cmp::PartialEq>
+    ::core::marker::StructuralPartialEq for EnumGeneric<T, U> {
+}
 #[automatically_derived]
 impl<T: ::core::cmp::PartialEq, U: ::core::cmp::PartialEq>
     ::core::cmp::PartialEq for EnumGeneric<T, U> {