Auto merge of rust-lang#8950 - Jarcho:derive_non_pub, r=dswij

Fixes for `derive_partial_eq_without_eq`

fixes  rust-lang#8875

changelog: Don't lint `derive_partial_eq_without_eq` on non-public types
changelog: Better handle generics in `derive_partial_eq_without_eq`

Author: bors

clippy_lints/src/derive.rs

@@ -4,14 +4,19 @@ use clippy_utils::ty::{implements_trait, implements_trait_with_env, is_copy};
 use clippy_utils::{is_lint_allowed, match_def_path};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
+use rustc_hir::def_id::DefId;
 use rustc_hir::intravisit::{walk_expr, walk_fn, walk_item, FnKind, Visitor};
 use rustc_hir::{
-    self as hir, BlockCheckMode, BodyId, Expr, ExprKind, FnDecl, HirId, Impl, Item, ItemKind, UnsafeSource, Unsafety,
+    self as hir, BlockCheckMode, BodyId, Constness, Expr, ExprKind, FnDecl, HirId, Impl, Item, ItemKind, UnsafeSource,
+    Unsafety,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::nested_filter;
-use rustc_middle::ty::subst::GenericArg;
-use rustc_middle::ty::{self, BoundConstness, ImplPolarity, ParamEnv, PredicateKind, TraitPredicate, TraitRef, Ty};
+use rustc_middle::traits::Reveal;
+use rustc_middle::ty::{
+    self, Binder, BoundConstness, GenericParamDefKind, ImplPolarity, ParamEnv, PredicateKind, TraitPredicate, TraitRef,
+    Ty, TyCtxt, Visibility,
+};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
 use rustc_span::sym;
@@ -459,50 +464,18 @@ impl<'tcx> Visitor<'tcx> for UnsafeVisitor<'_, 'tcx> {
 fn check_partial_eq_without_eq<'tcx>(cx: &LateContext<'tcx>, span: Span, trait_ref: &hir::TraitRef<'_>, ty: Ty<'tcx>) {
     if_chain! {
         if let ty::Adt(adt, substs) = ty.kind();
+        if cx.tcx.visibility(adt.did()) == Visibility::Public;
         if let Some(eq_trait_def_id) = cx.tcx.get_diagnostic_item(sym::Eq);
-        if let Some(peq_trait_def_id) = cx.tcx.get_diagnostic_item(sym::PartialEq);
         if let Some(def_id) = trait_ref.trait_def_id();
         if cx.tcx.is_diagnostic_item(sym::PartialEq, def_id);
-        // New `ParamEnv` replacing `T: PartialEq` with `T: Eq`
-        let param_env = ParamEnv::new(
-            cx.tcx.mk_predicates(cx.param_env.caller_bounds().iter().map(|p| {
-                let kind = p.kind();
-                match kind.skip_binder() {
-                    PredicateKind::Trait(p)
-                        if p.trait_ref.def_id == peq_trait_def_id
-                            && p.trait_ref.substs.get(0) == p.trait_ref.substs.get(1)
-                            && matches!(p.trait_ref.self_ty().kind(), ty::Param(_))
-                            && p.constness == BoundConstness::NotConst
-                            && p.polarity == ImplPolarity::Positive =>
-                    {
-                        cx.tcx.mk_predicate(kind.rebind(PredicateKind::Trait(TraitPredicate {
-                            trait_ref: TraitRef::new(
-                                eq_trait_def_id,
-                                cx.tcx.mk_substs([GenericArg::from(p.trait_ref.self_ty())].into_iter()),
-                            ),
-                            constness: BoundConstness::NotConst,
-                            polarity: ImplPolarity::Positive,
-                        })))
-                    },
-                    _ => p,
-                }
-            })),
-            cx.param_env.reveal(),
-            cx.param_env.constness(),
-        );
-        if !implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, substs);
+        let param_env = param_env_for_derived_eq(cx.tcx, adt.did(), eq_trait_def_id);
+        if !implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, &[]);
+        // If all of our fields implement `Eq`, we can implement `Eq` too
+        if adt
+            .all_fields()
+            .map(|f| f.ty(cx.tcx, substs))
+            .all(|ty| implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, &[]));
         then {
-            // If all of our fields implement `Eq`, we can implement `Eq` too
-            for variant in adt.variants() {
-                for field in &variant.fields {
-                    let ty = field.ty(cx.tcx, substs);
-
-                    if !implements_trait(cx, ty, eq_trait_def_id, substs) {
-                        return;
-                    }
-                }
-            }
-
             span_lint_and_sugg(
                 cx,
                 DERIVE_PARTIAL_EQ_WITHOUT_EQ,
@@ -515,3 +488,41 @@ fn check_partial_eq_without_eq<'tcx>(cx: &LateContext<'tcx>, span: Span, trait_r
         }
     }
 }
+
+/// Creates the `ParamEnv` used for the give type's derived `Eq` impl.
+fn param_env_for_derived_eq(tcx: TyCtxt<'_>, did: DefId, eq_trait_id: DefId) -> ParamEnv<'_> {
+    // Initial map from generic index to param def.
+    // Vec<(param_def, needs_eq)>
+    let mut params = tcx
+        .generics_of(did)
+        .params
+        .iter()
+        .map(|p| (p, matches!(p.kind, GenericParamDefKind::Type { .. })))
+        .collect::<Vec<_>>();
+
+    let ty_predicates = tcx.predicates_of(did).predicates;
+    for (p, _) in ty_predicates {
+        if let PredicateKind::Trait(p) = p.kind().skip_binder()
+            && p.trait_ref.def_id == eq_trait_id
+            && let ty::Param(self_ty) = p.trait_ref.self_ty().kind()
+            && p.constness == BoundConstness::NotConst
+        {
+            // Flag types which already have an `Eq` bound.
+            params[self_ty.index as usize].1 = false;
+        }
+    }
+
+    ParamEnv::new(
+        tcx.mk_predicates(ty_predicates.iter().map(|&(p, _)| p).chain(
+            params.iter().filter(|&&(_, needs_eq)| needs_eq).map(|&(param, _)| {
+                tcx.mk_predicate(Binder::dummy(PredicateKind::Trait(TraitPredicate {
+                    trait_ref: TraitRef::new(eq_trait_id, tcx.mk_substs([tcx.mk_param_from_def(param)].into_iter())),
+                    constness: BoundConstness::NotConst,
+                    polarity: ImplPolarity::Positive,
+                })))
+            }),
+        )),
+        Reveal::UserFacing,
+        Constness::NotConst,
+    )
+}

tests/ui/derive_partial_eq_without_eq.fixed

@@ -4,28 +4,28 @@
 #![warn(clippy::derive_partial_eq_without_eq)]
 
 // Don't warn on structs that aren't PartialEq
-struct NotPartialEq {
+pub struct NotPartialEq {
     foo: u32,
     bar: String,
 }
 
 // Eq can be derived but is missing
 #[derive(Debug, PartialEq, Eq)]
-struct MissingEq {
+pub struct MissingEq {
     foo: u32,
     bar: String,
 }
 
 // Eq is derived
 #[derive(PartialEq, Eq)]
-struct NotMissingEq {
+pub struct NotMissingEq {
     foo: u32,
     bar: String,
 }
 
 // Eq is manually implemented
 #[derive(PartialEq)]
-struct ManualEqImpl {
+pub struct ManualEqImpl {
     foo: u32,
     bar: String,
 }
@@ -34,13 +34,13 @@ impl Eq for ManualEqImpl {}
 
 // Cannot be Eq because f32 isn't Eq
 #[derive(PartialEq)]
-struct CannotBeEq {
+pub struct CannotBeEq {
     foo: u32,
     bar: f32,
 }
 
 // Don't warn if PartialEq is manually implemented
-struct ManualPartialEqImpl {
+pub struct ManualPartialEqImpl {
     foo: u32,
     bar: String,
 }
@@ -52,53 +52,75 @@ impl PartialEq for ManualPartialEqImpl {
 }
 
 // Generic fields should be properly checked for Eq-ness
-#[derive(PartialEq)]
-struct GenericNotEq<T: Eq, U: PartialEq> {
+#[derive(PartialEq, Eq)]
+pub struct GenericNotEq<T: Eq, U: PartialEq> {
     foo: T,
     bar: U,
 }
 
 #[derive(PartialEq, Eq)]
-struct GenericEq<T: Eq, U: Eq> {
+pub struct GenericEq<T: Eq, U: Eq> {
     foo: T,
     bar: U,
 }
 
 #[derive(PartialEq, Eq)]
-struct TupleStruct(u32);
+pub struct TupleStruct(u32);
 
 #[derive(PartialEq, Eq)]
-struct GenericTupleStruct<T: Eq>(T);
+pub struct GenericTupleStruct<T: Eq>(T);
 
 #[derive(PartialEq)]
-struct TupleStructNotEq(f32);
+pub struct TupleStructNotEq(f32);
 
 #[derive(PartialEq, Eq)]
-enum Enum {
+pub enum Enum {
     Foo(u32),
     Bar { a: String, b: () },
 }
 
 #[derive(PartialEq, Eq)]
-enum GenericEnum<T: Eq, U: Eq, V: Eq> {
+pub enum GenericEnum<T: Eq, U: Eq, V: Eq> {
     Foo(T),
     Bar { a: U, b: V },
 }
 
 #[derive(PartialEq)]
-enum EnumNotEq {
+pub enum EnumNotEq {
     Foo(u32),
     Bar { a: String, b: f32 },
 }
 
 // Ensure that rustfix works properly when `PartialEq` has other derives on either side
 #[derive(Debug, PartialEq, Eq, Clone)]
-struct RustFixWithOtherDerives;
+pub struct RustFixWithOtherDerives;
 
-#[derive(PartialEq)]
-struct Generic<T>(T);
+#[derive(PartialEq, Eq)]
+pub struct Generic<T>(T);
 
 #[derive(PartialEq, Eq)]
-struct GenericPhantom<T>(core::marker::PhantomData<T>);
+pub struct GenericPhantom<T>(core::marker::PhantomData<T>);
+
+mod _hidden {
+    #[derive(PartialEq, Eq)]
+    pub struct Reexported;
+
+    #[derive(PartialEq, Eq)]
+    pub struct InPubFn;
+
+    #[derive(PartialEq)]
+    pub(crate) struct PubCrate;
+
+    #[derive(PartialEq)]
+    pub(super) struct PubSuper;
+}
+
+pub use _hidden::Reexported;
+pub fn _from_mod() -> _hidden::InPubFn {
+    _hidden::InPubFn
+}
+
+#[derive(PartialEq)]
+struct InternalTy;
 
 fn main() {}

tests/ui/derive_partial_eq_without_eq.rs

@@ -4,28 +4,28 @@
 #![warn(clippy::derive_partial_eq_without_eq)]
 
 // Don't warn on structs that aren't PartialEq
-struct NotPartialEq {
+pub struct NotPartialEq {
     foo: u32,
     bar: String,
 }
 
 // Eq can be derived but is missing
 #[derive(Debug, PartialEq)]
-struct MissingEq {
+pub struct MissingEq {
     foo: u32,
     bar: String,
 }
 
 // Eq is derived
 #[derive(PartialEq, Eq)]
-struct NotMissingEq {
+pub struct NotMissingEq {
     foo: u32,
     bar: String,
 }
 
 // Eq is manually implemented
 #[derive(PartialEq)]
-struct ManualEqImpl {
+pub struct ManualEqImpl {
     foo: u32,
     bar: String,
 }
@@ -34,13 +34,13 @@ impl Eq for ManualEqImpl {}
 
 // Cannot be Eq because f32 isn't Eq
 #[derive(PartialEq)]
-struct CannotBeEq {
+pub struct CannotBeEq {
     foo: u32,
     bar: f32,
 }
 
 // Don't warn if PartialEq is manually implemented
-struct ManualPartialEqImpl {
+pub struct ManualPartialEqImpl {
     foo: u32,
     bar: String,
 }
@@ -53,52 +53,74 @@ impl PartialEq for ManualPartialEqImpl {
 
 // Generic fields should be properly checked for Eq-ness
 #[derive(PartialEq)]
-struct GenericNotEq<T: Eq, U: PartialEq> {
+pub struct GenericNotEq<T: Eq, U: PartialEq> {
     foo: T,
     bar: U,
 }
 
 #[derive(PartialEq)]
-struct GenericEq<T: Eq, U: Eq> {
+pub struct GenericEq<T: Eq, U: Eq> {
     foo: T,
     bar: U,
 }
 
 #[derive(PartialEq)]
-struct TupleStruct(u32);
+pub struct TupleStruct(u32);
 
 #[derive(PartialEq)]
-struct GenericTupleStruct<T: Eq>(T);
+pub struct GenericTupleStruct<T: Eq>(T);
 
 #[derive(PartialEq)]
-struct TupleStructNotEq(f32);
+pub struct TupleStructNotEq(f32);
 
 #[derive(PartialEq)]
-enum Enum {
+pub enum Enum {
     Foo(u32),
     Bar { a: String, b: () },
 }
 
 #[derive(PartialEq)]
-enum GenericEnum<T: Eq, U: Eq, V: Eq> {
+pub enum GenericEnum<T: Eq, U: Eq, V: Eq> {
     Foo(T),
     Bar { a: U, b: V },
 }
 
 #[derive(PartialEq)]
-enum EnumNotEq {
+pub enum EnumNotEq {
     Foo(u32),
     Bar { a: String, b: f32 },
 }
 
 // Ensure that rustfix works properly when `PartialEq` has other derives on either side
 #[derive(Debug, PartialEq, Clone)]
-struct RustFixWithOtherDerives;
+pub struct RustFixWithOtherDerives;
 
 #[derive(PartialEq)]
-struct Generic<T>(T);
+pub struct Generic<T>(T);
 
 #[derive(PartialEq, Eq)]
-struct GenericPhantom<T>(core::marker::PhantomData<T>);
+pub struct GenericPhantom<T>(core::marker::PhantomData<T>);
+
+mod _hidden {
+    #[derive(PartialEq)]
+    pub struct Reexported;
+
+    #[derive(PartialEq)]
+    pub struct InPubFn;
+
+    #[derive(PartialEq)]
+    pub(crate) struct PubCrate;
+
+    #[derive(PartialEq)]
+    pub(super) struct PubSuper;
+}
+
+pub use _hidden::Reexported;
+pub fn _from_mod() -> _hidden::InPubFn {
+    _hidden::InPubFn
+}
+
+#[derive(PartialEq)]
+struct InternalTy;
 
 fn main() {}