Skip to content

Commit

Permalink
Auto merge of rust-lang#5411 - dtolnay:hasher, r=flip1995
Browse files Browse the repository at this point in the history
Downgrade implicit_hasher to pedantic

From the [documentation](https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher), this lint is intended to suggest:

```diff
- pub fn foo(map: &mut HashMap<i32, i32>) { }

+ pub fn foo<S: BuildHasher>(map: &mut HashMap<i32, i32, S>) { }
```

I think this is pedantic. I get that this lint can benefit core libraries like serde, but that's exactly the use case for pedantic lints; a library like serde will [enable clippy_pedantic](https://github.com/serde-rs/json/blob/fd6741f4b0b3fc90a58a6f578e33a9adc6403f3f/src/lib.rs#L304) and take the time to go through everything possible. Similar for libraries doing a libz blitz style checkup before committing to a 1.0 release; it would make sense to run through all the available pedantic lints then.

But otherwise, for most codebases and certainly for industrial codebases, the above suggested change just makes the codebase more obtuse for questionable benefit.

changelog: Remove implicit_hasher from default set of enabled lints
  • Loading branch information
bors committed Apr 8, 2020
2 parents 940bbd6 + 5f92fae commit c25f26d
Show file tree
Hide file tree
Showing 9 changed files with 31 additions and 23 deletions.
3 changes: 1 addition & 2 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&types::CAST_POSSIBLE_WRAP),
LintId::of(&types::CAST_PRECISION_LOSS),
LintId::of(&types::CAST_SIGN_LOSS),
LintId::of(&types::IMPLICIT_HASHER),
LintId::of(&types::INVALID_UPCAST_COMPARISONS),
LintId::of(&types::LET_UNIT_VALUE),
LintId::of(&types::LINKEDLIST),
Expand Down Expand Up @@ -1383,7 +1384,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&types::CHAR_LIT_AS_U8),
LintId::of(&types::FN_TO_NUMERIC_CAST),
LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
LintId::of(&types::IMPLICIT_HASHER),
LintId::of(&types::REDUNDANT_ALLOCATION),
LintId::of(&types::TYPE_COMPLEXITY),
LintId::of(&types::UNIT_ARG),
Expand Down Expand Up @@ -1494,7 +1494,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&try_err::TRY_ERR),
LintId::of(&types::FN_TO_NUMERIC_CAST),
LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
LintId::of(&types::IMPLICIT_HASHER),
LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
LintId::of(&write::PRINTLN_EMPTY_STRING),
LintId::of(&write::PRINT_LITERAL),
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2169,7 +2169,7 @@ declare_clippy_lint! {
/// pub fn foo<S: BuildHasher>(map: &mut HashMap<i32, i32, S>) { }
/// ```
pub IMPLICIT_HASHER,
style,
pedantic,
"missing generalization over different hashers"
}

Expand Down
2 changes: 1 addition & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
},
Lint {
name: "implicit_hasher",
group: "style",
group: "pedantic",
desc: "missing generalization over different hashers",
deprecation: None,
module: "types",
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/crashes/ice-3717.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![deny(clippy::implicit_hasher)]

use std::collections::HashSet;

fn main() {}
Expand Down
8 changes: 6 additions & 2 deletions tests/ui/crashes/ice-3717.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
error: parameter of type `HashSet` should be generalized over different hashers
--> $DIR/ice-3717.rs:5:21
--> $DIR/ice-3717.rs:7:21
|
LL | pub fn ice_3717(_: &HashSet<usize>) {
| ^^^^^^^^^^^^^^
|
= note: `-D clippy::implicit-hasher` implied by `-D warnings`
note: the lint level is defined here
--> $DIR/ice-3717.rs:1:9
|
LL | #![deny(clippy::implicit_hasher)]
| ^^^^^^^^^^^^^^^^^^^^^^^
help: consider adding a type parameter
|
LL | pub fn ice_3717<S: ::std::hash::BuildHasher + Default>(_: &HashSet<usize, S>) {
Expand Down
1 change: 1 addition & 0 deletions tests/ui/implicit_hasher.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// aux-build:implicit_hasher_macros.rs
#![deny(clippy::implicit_hasher)]
#![allow(unused)]

#[macro_use]
Expand Down
26 changes: 15 additions & 11 deletions tests/ui/implicit_hasher.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
error: impl for `HashMap` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:15:35
--> $DIR/implicit_hasher.rs:16:35
|
LL | impl<K: Hash + Eq, V> Foo<i8> for HashMap<K, V> {
| ^^^^^^^^^^^^^
|
= note: `-D clippy::implicit-hasher` implied by `-D warnings`
note: the lint level is defined here
--> $DIR/implicit_hasher.rs:2:9
|
LL | #![deny(clippy::implicit_hasher)]
| ^^^^^^^^^^^^^^^^^^^^^^^
help: consider adding a type parameter
|
LL | impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashMap<K, V, S> {
Expand All @@ -15,7 +19,7 @@ LL | (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default:
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: impl for `HashMap` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:24:36
--> $DIR/implicit_hasher.rs:25:36
|
LL | impl<K: Hash + Eq, V> Foo<i8> for (HashMap<K, V>,) {
| ^^^^^^^^^^^^^
Expand All @@ -30,7 +34,7 @@ LL | ((HashMap::default(),), (HashMap::with_capacity_and_hasher(10, Defa
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: impl for `HashMap` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:29:19
--> $DIR/implicit_hasher.rs:30:19
|
LL | impl Foo<i16> for HashMap<String, String> {
| ^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -45,7 +49,7 @@ LL | (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default:
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: impl for `HashSet` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:46:32
--> $DIR/implicit_hasher.rs:47:32
|
LL | impl<T: Hash + Eq> Foo<i8> for HashSet<T> {
| ^^^^^^^^^^
Expand All @@ -60,7 +64,7 @@ LL | (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default:
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: impl for `HashSet` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:51:19
--> $DIR/implicit_hasher.rs:52:19
|
LL | impl Foo<i16> for HashSet<String> {
| ^^^^^^^^^^^^^^^
Expand All @@ -75,7 +79,7 @@ LL | (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default:
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: parameter of type `HashMap` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:68:23
--> $DIR/implicit_hasher.rs:69:23
|
LL | pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
| ^^^^^^^^^^^^^^^^^
Expand All @@ -86,7 +90,7 @@ LL | pub fn foo<S: ::std::hash::BuildHasher>(_map: &mut HashMap<i32, i32, S>, _s
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^

error: parameter of type `HashSet` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:68:53
--> $DIR/implicit_hasher.rs:69:53
|
LL | pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
| ^^^^^^^^^^^^
Expand All @@ -97,7 +101,7 @@ LL | pub fn foo<S: ::std::hash::BuildHasher>(_map: &mut HashMap<i32, i32>, _set:
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^

error: impl for `HashMap` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:72:43
--> $DIR/implicit_hasher.rs:73:43
|
LL | impl<K: Hash + Eq, V> Foo<u8> for HashMap<K, V> {
| ^^^^^^^^^^^^^
Expand All @@ -116,7 +120,7 @@ LL | (HashMap::default(), HashMap::with_capacity_and_hasher(10,
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: parameter of type `HashMap` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:80:33
--> $DIR/implicit_hasher.rs:81:33
|
LL | pub fn $name(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
| ^^^^^^^^^^^^^^^^^
Expand All @@ -131,7 +135,7 @@ LL | pub fn $name<S: ::std::hash::BuildHasher>(_map: &mut HashMap<i32, i
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^

error: parameter of type `HashSet` should be generalized over different hashers
--> $DIR/implicit_hasher.rs:80:63
--> $DIR/implicit_hasher.rs:81:63
|
LL | pub fn $name(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
| ^^^^^^^^^^^^
Expand Down
2 changes: 0 additions & 2 deletions tests/ui/mut_key.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#![allow(clippy::implicit_hasher)]

use std::collections::{HashMap, HashSet};
use std::hash::{Hash, Hasher};
use std::sync::atomic::{AtomicUsize, Ordering::Relaxed};
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/mut_key.stderr
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
error: mutable key type
--> $DIR/mut_key.rs:29:32
--> $DIR/mut_key.rs:27:32
|
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[deny(clippy::mutable_key_type)]` on by default

error: mutable key type
--> $DIR/mut_key.rs:29:72
--> $DIR/mut_key.rs:27:72
|
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
| ^^^^^^^^^^^^

error: mutable key type
--> $DIR/mut_key.rs:30:5
--> $DIR/mut_key.rs:28:5
|
LL | let _other: HashMap<Key, bool> = HashMap::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: mutable key type
--> $DIR/mut_key.rs:49:22
--> $DIR/mut_key.rs:47:22
|
LL | fn tuples_bad<U>(_m: &mut HashMap<(Key, U), bool>) {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down

0 comments on commit c25f26d

Please sign in to comment.