review comments

Replace tuple with struct and remove unnecessary early return.

Author: estebank

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

@@ -187,7 +187,7 @@ impl<'tcx> ConstToPat<'tcx> {
 
         if !inlined_const_as_pat.references_error() {
             // Always check for `PartialEq` if we had no other errors yet.
-            if !type_has_partial_eq_impl(self.tcx, typing_env, ty).0 {
+            if !type_has_partial_eq_impl(self.tcx, typing_env, ty).has_impl {
                 let mut err = self.tcx.dcx().create_err(TypeNotPartialEq { span: self.span, ty });
                 extend_type_not_partial_eq(self.tcx, typing_env, ty, &mut err);
                 return self.mk_err(err, ty);
@@ -221,12 +221,13 @@ impl<'tcx> ConstToPat<'tcx> {
                 // Extremely important check for all ADTs! Make sure they opted-in to be used in
                 // patterns.
                 debug!("adt_def {:?} has !type_marked_structural for cv.ty: {:?}", adt_def, ty);
-                let (_impls_partial_eq, derived, structural, impl_def_id) =
-                    type_has_partial_eq_impl(self.tcx, self.typing_env, ty);
+                let PartialEqImplStatus {
+                    is_derived, structural_partial_eq, non_blanket_impl, ..
+                } = type_has_partial_eq_impl(self.tcx, self.typing_env, ty);
                 let (manual_partialeq_impl_span, manual_partialeq_impl_note) =
-                    match (structural, impl_def_id) {
+                    match (structural_partial_eq, non_blanket_impl) {
                         (true, _) => (None, false),
-                        (_, Some(def_id)) if def_id.is_local() && !derived => {
+                        (_, Some(def_id)) if def_id.is_local() && !is_derived => {
                             (Some(tcx.def_span(def_id)), false)
                         }
                         _ => (None, true),
@@ -385,40 +386,50 @@ fn extend_type_not_partial_eq<'tcx>(
         /// The type has no `PartialEq` implementation, neither manual or derived, but
         /// we don't have a span to point at, so we'll just add them as a `note`.
         without: FxHashSet<Ty<'tcx>>,
-        /// If any of the subtypes has escaping bounds (`for<'a>`), we won't emit a note.
-        has_escaping_bound_vars: bool,
+        /// If any of the subtypes has escaping bounds (`for<'a>`) or involves a trait object, we
+        /// won't emit a note.
+        should_note: bool,
     }
 
     impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for UsedParamsNeedInstantiationVisitor<'tcx> {
-        fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result {
-            if ty.has_escaping_bound_vars() {
-                self.has_escaping_bound_vars = true;
-            } else if let ty::Adt(def, _args) = ty.kind() {
-                let ty_def_id = def.did();
-                let ty_def_span = self.tcx.def_span(ty_def_id);
-                let (impls_partial_eq, derived, structural, impl_def_id) =
-                    type_has_partial_eq_impl(self.tcx, self.typing_env, ty);
-                match (impls_partial_eq, derived, structural, impl_def_id) {
-                    (_, _, true, _) => {}
-                    (true, false, _, Some(def_id)) if def_id.is_local() => {
-                        self.adts_with_manual_partialeq.insert(self.tcx.def_span(def_id));
-                    }
-                    (true, false, _, _) if ty_def_id.is_local() => {
-                        self.adts_with_manual_partialeq.insert(ty_def_span);
-                    }
-                    (false, _, _, _) if ty_def_id.is_local() => {
-                        self.adts_without_partialeq.insert(ty_def_span);
-                    }
-                    (true, false, _, _) => {
-                        self.manual.insert(ty);
-                    }
-                    (false, _, _, _) => {
-                        self.without.insert(ty);
-                    }
-                    _ => {}
-                };
+        fn visit_ty(&mut self, ty: Ty<'tcx>) {
+            match ty.kind() {
+                ty::Dynamic(..) => {
+                    self.should_note = false;
+                }
+                ty::FnPtr(..) => {}
+                ty::Adt(def, _args) => {
+                    let ty_def_id = def.did();
+                    let ty_def_span = self.tcx.def_span(ty_def_id);
+                    let PartialEqImplStatus {
+                        has_impl,
+                        is_derived,
+                        structural_partial_eq,
+                        non_blanket_impl,
+                    } = type_has_partial_eq_impl(self.tcx, self.typing_env, ty);
+                    match (has_impl, is_derived, structural_partial_eq, non_blanket_impl) {
+                        (_, _, true, _) => {}
+                        (true, false, _, Some(def_id)) if def_id.is_local() => {
+                            self.adts_with_manual_partialeq.insert(self.tcx.def_span(def_id));
+                        }
+                        (true, false, _, _) if ty_def_id.is_local() => {
+                            self.adts_with_manual_partialeq.insert(ty_def_span);
+                        }
+                        (false, _, _, _) if ty_def_id.is_local() => {
+                            self.adts_without_partialeq.insert(ty_def_span);
+                        }
+                        (true, false, _, _) => {
+                            self.manual.insert(ty);
+                        }
+                        (false, _, _, _) => {
+                            self.without.insert(ty);
+                        }
+                        _ => {}
+                    };
+                    ty.super_visit_with(self);
+                }
+                _ => ty.super_visit_with(self),
             }
-            ty.super_visit_with(self)
         }
     }
     let mut v = UsedParamsNeedInstantiationVisitor {
@@ -428,10 +439,10 @@ fn extend_type_not_partial_eq<'tcx>(
         adts_without_partialeq: FxHashSet::default(),
         manual: FxHashSet::default(),
         without: FxHashSet::default(),
-        has_escaping_bound_vars: false,
+        should_note: true,
     };
     v.visit_ty(ty);
-    if v.has_escaping_bound_vars {
+    if !v.should_note {
         return;
     }
     #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering
@@ -445,31 +456,38 @@ fn extend_type_not_partial_eq<'tcx>(
             "must be annotated with `#[derive(PartialEq)]` to be usable in patterns",
         );
     }
-    #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering
-    for ty in v.manual {
+    #[allow(rustc::potential_query_instability)]
+    let mut manual: Vec<_> = v.manual.into_iter().map(|t| t.to_string()).collect();
+    manual.sort();
+    for ty in manual {
         err.note(format!(
             "`{ty}` must be annotated with `#[derive(PartialEq)]` to be usable in patterns, manual `impl`s are not sufficient; see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details"
         ));
     }
-    #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering
-    for ty in v.without {
+    #[allow(rustc::potential_query_instability)]
+    let mut without: Vec<_> = v.without.into_iter().map(|t| t.to_string()).collect();
+    without.sort();
+    for ty in without {
         err.note(format!(
             "`{ty}` must be annotated with `#[derive(PartialEq)]` to be usable in patterns"
         ));
     }
 }
 
+#[derive(Debug)]
+struct PartialEqImplStatus {
+    has_impl: bool,
+    is_derived: bool,
+    structural_partial_eq: bool,
+    non_blanket_impl: Option<DefId>,
+}
+
 #[instrument(level = "trace", skip(tcx), ret)]
 fn type_has_partial_eq_impl<'tcx>(
     tcx: TyCtxt<'tcx>,
     typing_env: ty::TypingEnv<'tcx>,
     ty: Ty<'tcx>,
-) -> (
-    /* has impl */ bool,
-    /* is derived */ bool,
-    /* structural partial eq */ bool,
-    /* non-blanket impl */ Option<DefId>,
-) {
+) -> PartialEqImplStatus {
     let (infcx, param_env) = tcx.infer_ctxt().build_with_typing_env(typing_env);
     // double-check there even *is* a semantic `PartialEq` to dispatch to.
     //
@@ -479,10 +497,6 @@ fn type_has_partial_eq_impl<'tcx>(
     let partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::PartialEq, None);
     let structural_partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::StructuralPeq, None);
 
-    if ty.has_escaping_bound_vars() {
-        return (false, false, false, None);
-    }
-
     let partial_eq_obligation = Obligation::new(
         tcx,
         ObligationCause::dummy(),
@@ -510,10 +524,10 @@ 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.
-    (
-        infcx.predicate_must_hold_modulo_regions(&partial_eq_obligation),
-        automatically_derived,
-        structural_peq,
-        impl_def_id,
-    )
+    PartialEqImplStatus {
+        has_impl: infcx.predicate_must_hold_modulo_regions(&partial_eq_obligation),
+        is_derived: automatically_derived,
+        structural_partial_eq: structural_peq,
+        non_blanket_impl: impl_def_id,
+    }
 }