Skip to content

Commit

Permalink
implicit_hasher: use a single multipart suggestion
Browse files Browse the repository at this point in the history
  • Loading branch information
Alexendoo committed Jul 29, 2024
1 parent 79783e9 commit 33b16d3
Show file tree
Hide file tree
Showing 5 changed files with 181 additions and 91 deletions.
53 changes: 26 additions & 27 deletions clippy_lints/src/implicit_hasher.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::borrow::Cow;
use std::collections::BTreeMap;

use rustc_errors::Diag;
use rustc_errors::{Applicability, Diag};
use rustc_hir as hir;
use rustc_hir::intravisit::{walk_body, walk_expr, walk_inf, walk_ty, Visitor};
use rustc_hir::{Body, Expr, ExprKind, GenericArg, Item, ItemKind, QPath, TyKind};
Expand All @@ -13,7 +13,7 @@ use rustc_session::declare_lint_pass;
use rustc_span::symbol::sym;
use rustc_span::Span;

use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::{snippet, IntoSpan, SpanRangeExt};
use clippy_utils::ty::is_type_diagnostic_item;

Expand Down Expand Up @@ -77,33 +77,32 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitHasher {
&generics_snip[1..generics_snip.len() - 1]
};

multispan_sugg(
diag,
"consider adding a type parameter",
vec![
(
generics_suggestion_span,
format!(
"<{generics_snip}{}S: ::std::hash::BuildHasher{}>",
if generics_snip.is_empty() { "" } else { ", " },
if vis.suggestions.is_empty() {
""
} else {
// request users to add `Default` bound so that generic constructors can be used
" + Default"
},
),
),
(
target.span(),
format!("{}<{}, S>", target.type_name(), target.type_arguments(),),
let mut suggestions = vec![
(
generics_suggestion_span,
format!(
"<{generics_snip}{}S: ::std::hash::BuildHasher{}>",
if generics_snip.is_empty() { "" } else { ", " },
if vis.suggestions.is_empty() {
""
} else {
// request users to add `Default` bound so that generic constructors can be used
" + Default"
},
),
],
),
(
target.span(),
format!("{}<{}, S>", target.type_name(), target.type_arguments(),),
),
];
suggestions.extend(vis.suggestions);

diag.multipart_suggestion(
"add a type parameter for `BuildHasher`",
suggestions,
Applicability::MaybeIncorrect,
);

if !vis.suggestions.is_empty() {
multispan_sugg(diag, "...and use generic constructor", vis.suggestions);
}
}

if !cx.effective_visibilities.is_exported(item.owner_id.def_id) {
Expand Down
11 changes: 5 additions & 6 deletions tests/ui/crashes/ice-3717.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,13 @@ note: the lint level is defined here
|
LL | #![deny(clippy::implicit_hasher)]
| ^^^^^^^^^^^^^^^^^^^^^^^
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | pub fn ice_3717<S: ::std::hash::BuildHasher + Default>(_: &HashSet<usize, S>) {
| +++++++++++++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~~~
help: ...and use generic constructor
LL ~ pub fn ice_3717<S: ::std::hash::BuildHasher + Default>(_: &HashSet<usize, S>) {
LL |
LL | let _ = [0u8; 0];
LL ~ let _: HashSet<usize> = HashSet::default();
|
LL | let _: HashSet<usize> = HashSet::default();
| ~~~~~~~~~~~~~~~~~~

error: aborting due to 1 previous error

102 changes: 102 additions & 0 deletions tests/ui/implicit_hasher.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
//@aux-build:proc_macros.rs
#![deny(clippy::implicit_hasher)]

#[macro_use]
extern crate proc_macros;

use std::cmp::Eq;
use std::collections::{HashMap, HashSet};
use std::hash::{BuildHasher, Hash};

pub trait Foo<T>: Sized {
fn make() -> (Self, Self);
}

impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashMap<K, V, S> {
fn make() -> (Self, Self) {
// OK, don't suggest to modify these
let _: HashMap<i32, i32> = HashMap::new();
let _: HashSet<i32> = HashSet::new();

(HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
}
}
impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for (HashMap<K, V, S>,) {
fn make() -> (Self, Self) {
((HashMap::default(),), (HashMap::with_capacity_and_hasher(10, Default::default()),))
}
}
impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashMap<String, String, S> {
fn make() -> (Self, Self) {
(HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
}
}

impl<K: Hash + Eq, V, S: BuildHasher + Default> Foo<i32> for HashMap<K, V, S> {
fn make() -> (Self, Self) {
(HashMap::default(), HashMap::with_capacity_and_hasher(10, S::default()))
}
}
impl<S: BuildHasher + Default> Foo<i64> for HashMap<String, String, S> {
fn make() -> (Self, Self) {
(HashMap::default(), HashMap::with_capacity_and_hasher(10, S::default()))
}
}

impl<T: Hash + Eq, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashSet<T, S> {
fn make() -> (Self, Self) {
(HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
}
}
impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashSet<String, S> {
fn make() -> (Self, Self) {
(HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
}
}

impl<T: Hash + Eq, S: BuildHasher + Default> Foo<i32> for HashSet<T, S> {
fn make() -> (Self, Self) {
(HashSet::default(), HashSet::with_capacity_and_hasher(10, S::default()))
}
}
impl<S: BuildHasher + Default> Foo<i64> for HashSet<String, S> {
fn make() -> (Self, Self) {
(HashSet::default(), HashSet::with_capacity_and_hasher(10, S::default()))
}
}

pub fn map<S: ::std::hash::BuildHasher>(map: &mut HashMap<i32, i32, S>) {}

pub fn set<S: ::std::hash::BuildHasher>(set: &mut HashSet<i32, S>) {}

#[inline_macros]
pub mod gen {
use super::*;
inline! {
impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<u8> for HashMap<K, V, S> {
fn make() -> (Self, Self) {
(HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
}
}

pub fn bar(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
}
}

// When the macro is in a different file, the suggestion spans can't be combined properly
// and should not cause an ICE
// See #2707
#[macro_use]
#[path = "auxiliary/test_macro.rs"]
pub mod test_macro;
__implicit_hasher_test_macro!(impl<K, V> for HashMap<K, V> where V: test_macro::A);

// #4260
external! {
pub fn f(input: &HashMap<u32, u32>) {}
}

// #7712
pub async fn election_vote<S: ::std::hash::BuildHasher>(_data: HashMap<i32, i32, S>) {}

fn main() {}
6 changes: 3 additions & 3 deletions tests/ui/implicit_hasher.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
//@aux-build:proc_macros.rs
//@no-rustfix
#![deny(clippy::implicit_hasher)]
#![allow(unused)]

#[macro_use]
extern crate proc_macros;
Expand Down Expand Up @@ -67,7 +65,9 @@ impl<S: BuildHasher + Default> Foo<i64> for HashSet<String, S> {
}
}

pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
pub fn map(map: &mut HashMap<i32, i32>) {}

pub fn set(set: &mut HashSet<i32>) {}

#[inline_macros]
pub mod gen {
Expand Down
100 changes: 45 additions & 55 deletions tests/ui/implicit_hasher.stderr
Original file line number Diff line number Diff line change
@@ -1,104 +1,96 @@
error: impl for `HashMap` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:17:35
--> tests/ui/implicit_hasher.rs:15:35
|
LL | impl<K: Hash + Eq, V> Foo<i8> for HashMap<K, V> {
| ^^^^^^^^^^^^^
|
note: the lint level is defined here
--> tests/ui/implicit_hasher.rs:3:9
--> tests/ui/implicit_hasher.rs:2:9
|
LL | #![deny(clippy::implicit_hasher)]
| ^^^^^^^^^^^^^^^^^^^^^^^
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashMap<K, V, S> {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~
help: ...and use generic constructor
LL ~ impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashMap<K, V, S> {
LL | fn make() -> (Self, Self) {
...
LL |
LL ~ (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
|
LL | (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
| ~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: impl for `HashMap` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:26:36
--> tests/ui/implicit_hasher.rs:24:36
|
LL | impl<K: Hash + Eq, V> Foo<i8> for (HashMap<K, V>,) {
| ^^^^^^^^^^^^^
|
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for (HashMap<K, V, S>,) {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~
help: ...and use generic constructor
LL ~ impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for (HashMap<K, V, S>,) {
LL | fn make() -> (Self, Self) {
LL ~ ((HashMap::default(),), (HashMap::with_capacity_and_hasher(10, Default::default()),))
|
LL | ((HashMap::default(),), (HashMap::with_capacity_and_hasher(10, Default::default()),))
| ~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: impl for `HashMap` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:31:19
--> tests/ui/implicit_hasher.rs:29:19
|
LL | impl Foo<i16> for HashMap<String, String> {
| ^^^^^^^^^^^^^^^^^^^^^^^
|
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashMap<String, String, S> {
| +++++++++++++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~~~~~~~~~~~~
help: ...and use generic constructor
LL ~ impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashMap<String, String, S> {
LL | fn make() -> (Self, Self) {
LL ~ (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
|
LL | (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
| ~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: impl for `HashSet` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:48:32
--> tests/ui/implicit_hasher.rs:46:32
|
LL | impl<T: Hash + Eq> Foo<i8> for HashSet<T> {
| ^^^^^^^^^^
|
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | impl<T: Hash + Eq, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashSet<T, S> {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~
help: ...and use generic constructor
LL ~ impl<T: Hash + Eq, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashSet<T, S> {
LL | fn make() -> (Self, Self) {
LL ~ (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
|
LL | (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
| ~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: impl for `HashSet` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:53:19
--> tests/ui/implicit_hasher.rs:51:19
|
LL | impl Foo<i16> for HashSet<String> {
| ^^^^^^^^^^^^^^^
|
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashSet<String, S> {
| +++++++++++++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~~~~
help: ...and use generic constructor
LL ~ impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashSet<String, S> {
LL | fn make() -> (Self, Self) {
LL ~ (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
|
LL | (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
| ~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: parameter of type `HashMap` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:70:23
--> tests/ui/implicit_hasher.rs:68:22
|
LL | pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
| ^^^^^^^^^^^^^^^^^
LL | pub fn map(map: &mut HashMap<i32, i32>) {}
| ^^^^^^^^^^^^^^^^^
|
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | pub fn foo<S: ::std::hash::BuildHasher>(_map: &mut HashMap<i32, i32, S>, _set: &mut HashSet<i32>) {}
| +++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~~~~~~
LL | pub fn map<S: ::std::hash::BuildHasher>(map: &mut HashMap<i32, i32, S>) {}
| +++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~~~~~~

error: parameter of type `HashSet` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:70:53
--> tests/ui/implicit_hasher.rs:70:22
|
LL | pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {}
| ^^^^^^^^^^^^
LL | pub fn set(set: &mut HashSet<i32>) {}
| ^^^^^^^^^^^^
|
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | pub fn foo<S: ::std::hash::BuildHasher>(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32, S>) {}
| +++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~
LL | pub fn set<S: ::std::hash::BuildHasher>(set: &mut HashSet<i32, S>) {}
| +++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~

error: impl for `HashMap` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:76:43
Expand All @@ -107,22 +99,20 @@ LL | impl<K: Hash + Eq, V> Foo<u8> for HashMap<K, V> {
| ^^^^^^^^^^^^^
|
= note: this error originates in the macro `__inline_mac_mod_gen` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<u8> for HashMap<K, V, S> {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~
help: ...and use generic constructor
LL ~ impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<u8> for HashMap<K, V, S> {
LL | fn make() -> (Self, Self) {
LL ~ (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
|
LL | (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
| ~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: parameter of type `HashMap` should be generalized over different hashers
--> tests/ui/implicit_hasher.rs:100:35
|
LL | pub async fn election_vote(_data: HashMap<i32, i32>) {}
| ^^^^^^^^^^^^^^^^^
|
help: consider adding a type parameter
help: add a type parameter for `BuildHasher`
|
LL | pub async fn election_vote<S: ::std::hash::BuildHasher>(_data: HashMap<i32, i32, S>) {}
| +++++++++++++++++++++++++++++ ~~~~~~~~~~~~~~~~~~~~
Expand Down

0 comments on commit 33b16d3

Please sign in to comment.