Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow implementing Hash with derived PartialEq (derive_hash_xor_eq) #10184

Merged
merged 3 commits into from Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -4137,6 +4137,7 @@ Released 2018-09-13
[`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
[`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
[`derive_partial_eq_without_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq
[`derived_hash_with_manual_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derived_hash_with_manual_eq
[`disallowed_macros`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros
[`disallowed_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_method
[`disallowed_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/declared_lints.rs
Expand Up @@ -111,7 +111,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::dereference::NEEDLESS_BORROW_INFO,
crate::dereference::REF_BINDING_TO_REFERENCE_INFO,
crate::derivable_impls::DERIVABLE_IMPLS_INFO,
crate::derive::DERIVE_HASH_XOR_EQ_INFO,
crate::derive::DERIVED_HASH_WITH_MANUAL_EQ_INFO,
crate::derive::DERIVE_ORD_XOR_PARTIAL_ORD_INFO,
crate::derive::DERIVE_PARTIAL_EQ_WITHOUT_EQ_INFO,
crate::derive::EXPL_IMPL_CLONE_ON_COPY_INFO,
Expand Down
18 changes: 6 additions & 12 deletions clippy_lints/src/derive.rs
Expand Up @@ -46,7 +46,7 @@ declare_clippy_lint! {
/// }
/// ```
#[clippy::version = "pre 1.29.0"]
pub DERIVE_HASH_XOR_EQ,
pub DERIVED_HASH_WITH_MANUAL_EQ,
correctness,
"deriving `Hash` but implementing `PartialEq` explicitly"
}
Expand Down Expand Up @@ -197,7 +197,7 @@ declare_clippy_lint! {

declare_lint_pass!(Derive => [
EXPL_IMPL_CLONE_ON_COPY,
DERIVE_HASH_XOR_EQ,
DERIVED_HASH_WITH_MANUAL_EQ,
DERIVE_ORD_XOR_PARTIAL_ORD,
UNSAFE_DERIVE_DESERIALIZE,
DERIVE_PARTIAL_EQ_WITHOUT_EQ
Expand Down Expand Up @@ -226,7 +226,7 @@ impl<'tcx> LateLintPass<'tcx> for Derive {
}
}

/// Implementation of the `DERIVE_HASH_XOR_EQ` lint.
/// Implementation of the `DERIVED_HASH_WITH_MANUAL_EQ` lint.
fn check_hash_peq<'tcx>(
cx: &LateContext<'tcx>,
span: Span,
Expand All @@ -243,7 +243,7 @@ fn check_hash_peq<'tcx>(
cx.tcx.for_each_relevant_impl(peq_trait_def_id, ty, |impl_id| {
let peq_is_automatically_derived = cx.tcx.has_attr(impl_id, sym::automatically_derived);

if peq_is_automatically_derived == hash_is_automatically_derived {
if !hash_is_automatically_derived || peq_is_automatically_derived {
return;
}

Expand All @@ -252,17 +252,11 @@ fn check_hash_peq<'tcx>(
// Only care about `impl PartialEq<Foo> for Foo`
// For `impl PartialEq<B> for A, input_types is [A, B]
if trait_ref.substs.type_at(1) == ty {
let mess = if peq_is_automatically_derived {
"you are implementing `Hash` explicitly but have derived `PartialEq`"
} else {
"you are deriving `Hash` but have implemented `PartialEq` explicitly"
};

span_lint_and_then(
cx,
DERIVE_HASH_XOR_EQ,
DERIVED_HASH_WITH_MANUAL_EQ,
span,
mess,
"you are deriving `Hash` but have implemented `PartialEq` explicitly",
|diag| {
if let Some(local_def_id) = impl_id.as_local() {
let hir_id = cx.tcx.hir().local_def_id_to_hir_id(local_def_id);
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/renamed_lints.rs
Expand Up @@ -9,6 +9,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
("clippy::box_vec", "clippy::box_collection"),
("clippy::const_static_lifetime", "clippy::redundant_static_lifetimes"),
("clippy::cyclomatic_complexity", "clippy::cognitive_complexity"),
("clippy::derive_hash_xor_eq", "clippy::derived_hash_with_manual_eq"),
("clippy::disallowed_method", "clippy::disallowed_methods"),
("clippy::disallowed_type", "clippy::disallowed_types"),
("clippy::eval_order_dependence", "clippy::mixed_read_write_in_expression"),
Expand Down
59 changes: 0 additions & 59 deletions tests/ui/derive_hash_xor_eq.stderr

This file was deleted.

Expand Up @@ -27,30 +27,13 @@ impl PartialEq<Baz> for Baz {
}
}

// Implementing `Hash` with a derived `PartialEq` is fine. See #2627

#[derive(PartialEq)]
struct Bah;

impl std::hash::Hash for Bah {
fn hash<H: std::hash::Hasher>(&self, _: &mut H) {}
}

#[derive(PartialEq)]
struct Foo2;

trait Hash {}

// We don't want to lint on user-defined traits called `Hash`
impl Hash for Foo2 {}

mod use_hash {
use std::hash::{Hash, Hasher};

#[derive(PartialEq)]
struct Foo3;

impl Hash for Foo3 {
fn hash<H: std::hash::Hasher>(&self, _: &mut H) {}
}
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
}

fn main() {}
29 changes: 29 additions & 0 deletions tests/ui/derived_hash_with_manual_eq.stderr
@@ -0,0 +1,29 @@
error: you are deriving `Hash` but have implemented `PartialEq` explicitly
--> $DIR/derived_hash_with_manual_eq.rs:12:10
|
LL | #[derive(Hash)]
| ^^^^
|
note: `PartialEq` implemented here
--> $DIR/derived_hash_with_manual_eq.rs:15:1
|
LL | impl PartialEq for Bar {
| ^^^^^^^^^^^^^^^^^^^^^^
= note: `#[deny(clippy::derived_hash_with_manual_eq)]` on by default
= note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

error: you are deriving `Hash` but have implemented `PartialEq` explicitly
--> $DIR/derived_hash_with_manual_eq.rs:21:10
|
LL | #[derive(Hash)]
| ^^^^
|
note: `PartialEq` implemented here
--> $DIR/derived_hash_with_manual_eq.rs:24:1
|
LL | impl PartialEq<Baz> for Baz {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

2 changes: 2 additions & 0 deletions tests/ui/rename.fixed
Expand Up @@ -10,6 +10,7 @@
#![allow(clippy::box_collection)]
#![allow(clippy::redundant_static_lifetimes)]
#![allow(clippy::cognitive_complexity)]
#![allow(clippy::derived_hash_with_manual_eq)]
#![allow(clippy::disallowed_methods)]
#![allow(clippy::disallowed_types)]
#![allow(clippy::mixed_read_write_in_expression)]
Expand Down Expand Up @@ -45,6 +46,7 @@
#![warn(clippy::box_collection)]
#![warn(clippy::redundant_static_lifetimes)]
#![warn(clippy::cognitive_complexity)]
#![warn(clippy::derived_hash_with_manual_eq)]
#![warn(clippy::disallowed_methods)]
#![warn(clippy::disallowed_types)]
#![warn(clippy::mixed_read_write_in_expression)]
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/rename.rs
Expand Up @@ -10,6 +10,7 @@
#![allow(clippy::box_collection)]
#![allow(clippy::redundant_static_lifetimes)]
#![allow(clippy::cognitive_complexity)]
#![allow(clippy::derived_hash_with_manual_eq)]
robertbastian marked this conversation as resolved.
Show resolved Hide resolved
#![allow(clippy::disallowed_methods)]
#![allow(clippy::disallowed_types)]
#![allow(clippy::mixed_read_write_in_expression)]
Expand Down Expand Up @@ -45,6 +46,7 @@
#![warn(clippy::box_vec)]
#![warn(clippy::const_static_lifetime)]
#![warn(clippy::cyclomatic_complexity)]
#![warn(clippy::derive_hash_xor_eq)]
#![warn(clippy::disallowed_method)]
#![warn(clippy::disallowed_type)]
#![warn(clippy::eval_order_dependence)]
Expand Down