mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Add implicit_hasher lint (#2101)

Browse Source
This commit is contained in:
sinkuu 2017-10-05 23:57:31 +09:00
parent 2b491d5bb3
commit 159cc8413c
4 changed files with 563 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
clippy_lints/src/lib.rs
View File

@ -333,6 +333,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_late_lint_pass(box infinite_iter::Pass);
reg.register_late_lint_pass(box invalid_ref::InvalidRef);
reg.register_late_lint_pass(box identity_conversion::IdentityConversion::default());
reg.register_late_lint_pass(box types::ImplicitHasher);
reg.register_lint_group("clippy_restrictions", vec![
arithmetic::FLOAT_ARITHMETIC,
@ -555,6 +556,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
types::BOX_VEC,
types::CAST_LOSSLESS,
types::CHAR_LIT_AS_U8,
types::IMPLICIT_HASHER,
types::LET_UNIT_VALUE,
types::LINKEDLIST,
types::TYPE_COMPLEXITY,

354
clippy_lints/src/types.rs
View File

@ -1,16 +1,19 @@
use reexport::*;
use rustc::hir;
use rustc::hir::*;
use rustc::hir::intravisit::{walk_ty, FnKind, NestedVisitorMap, Visitor};
use rustc::hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor};
use rustc::lint::*;
use rustc::ty::{self, Ty, TyCtxt};
use rustc::ty::subst::Substs;
use std::cmp::Ordering;
use std::collections::BTreeMap;
use std::borrow::Cow;
use syntax::ast::{FloatTy, IntTy, UintTy};
use syntax::attr::IntType;
use syntax::codemap::Span;
use utils::{comparisons, higher, in_external_macro, in_macro, last_path_segment, match_def_path, match_path,
opt_def_id, snippet, snippet_opt, span_help_and_lint, span_lint, span_lint_and_sugg, type_size};
opt_def_id, same_tys, snippet, snippet_opt, span_help_and_lint, span_lint, span_lint_and_sugg,
span_lint_and_then, type_size};
use utils::paths;
/// Handles all the linting of funky types
@ -182,21 +185,19 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) {
match *qpath {
QPath::Resolved(Some(ref ty), ref p) => {
check_ty(cx, ty, is_local);
for ty in p.segments
.iter()
.flat_map(|seg| seg.parameters.as_ref()
.map_or_else(|| [].iter(),
|params| params.types.iter()))
{
for ty in p.segments.iter().flat_map(|seg| {
seg.parameters
.as_ref()
.map_or_else(|| [].iter(), |params| params.types.iter())
}) {
check_ty(cx, ty, is_local);
}
},
QPath::Resolved(None, ref p) => for ty in p.segments
.iter()
.flat_map(|seg| seg.parameters.as_ref()
.map_or_else(|| [].iter(),
|params| params.types.iter()))
{
QPath::Resolved(None, ref p) => for ty in p.segments.iter().flat_map(|seg| {
seg.parameters
.as_ref()
.map_or_else(|| [].iter(), |params| params.types.iter())
}) {
check_ty(cx, ty, is_local);
},
QPath::TypeRelative(ref ty, ref seg) => {
@ -605,7 +606,7 @@ fn span_lossless_lint(cx: &LateContext, expr: &Expr, op: &Expr, cast_from: Ty, c
let opt = snippet_opt(cx, op.span);
let sugg = if let Some(ref snip) = opt {
if should_strip_parens(op, snip) {
&snip[1..snip.len()-1]
&snip[1..snip.len() - 1]
} else {
snip.as_str()
}
@ -1449,3 +1450,326 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
}
}
}
/// **What it does:** Checks for `impl` or `fn` missing generalization over
/// different hashers and implicitly defaulting to the default hashing
/// algorithm (SipHash). This lint ignores private free-functions.
///
/// **Why is this bad?** `HashMap` or `HashSet` with custom hashers cannot be
/// used with them.
///
/// **Known problems:** Suggestions for replacing constructors are not always
/// accurate.
///
/// **Example:**
/// ```rust
/// impl<K: Hash + Eq, V> Serialize for HashMap<K, V> { ... }
///
/// pub foo(map: &mut HashMap<i32, i32>) { .. }
/// ```
declare_lint! {
pub IMPLICIT_HASHER,
Warn,
"missing generalization over different hashers"
}
pub struct ImplicitHasher;
impl LintPass for ImplicitHasher {
fn get_lints(&self) -> LintArray {
lint_array!(IMPLICIT_HASHER)
}
}
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitHasher {
#[allow(cast_possible_truncation)]
fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) {
if let ItemImpl(_, _, _, ref generics, _, ref ty, ref items) = item.node {
let mut vis = ImplicitHasherTypeVisitor::new(cx);
vis.visit_ty(ty);
for target in vis.found {
let generics_snip = snippet(cx, generics.span, "");
let generics_snip_trimmed = if generics_snip.len() == 0 {
""
} else {
// trim `<` `>`
&generics_snip[1..generics_snip.len() - 1]
};
let generics_span = generics.span.substitute_dummy({
let pos = snippet_opt(cx, item.span.until(target.span()))
.and_then(|snip| {
Some(item.span.lo() + ::syntax_pos::BytePos(snip.find("impl")? as u32 + 4))
})
.expect("failed to create span for type arguments");
Span::new(pos, pos, item.span.data().ctxt)
});
let mut vis = ImplicitHasherConstructorVisitor::new(cx, target.clone());
for item in items.iter().map(|item| cx.tcx.hir.impl_item(item.id)) {
vis.visit_impl_item(item);
}
span_lint_and_then(
cx,
IMPLICIT_HASHER,
target.span(),
&format!("impl for `{}` should be generarized over different hashers", target.type_name()),
move |db| {
db.span_suggestion(
generics_span,
"consider adding a type parameter",
format!(
"<{}{}S: ::std::hash::BuildHasher{}>",
generics_snip_trimmed,
if generics_snip_trimmed.is_empty() {
""
} else {
", "
},
if vis.suggestions.is_empty() {
""
} else {
// request users to add `Default` bound so that generic constructors can be used
" + Default"
},
),
);
db.span_suggestion(
target.span(),
"...and change the type to",
format!("{}<{}, S>", target.type_name(), target.type_arguments(),),
);
for (span, sugg) in vis.suggestions {
db.span_suggestion(span, "...and use generic constructor here", sugg);
}
},
);
}
}
if let ItemFn(ref decl, .., ref generics, body) = item.node {
if item.vis != Public {
return;
}
for ty in &decl.inputs {
let mut vis = ImplicitHasherTypeVisitor::new(cx);
vis.visit_ty(ty);
for target in vis.found {
let generics_snip = snippet(cx, generics.span, "");
let generics_snip_trimmed = if generics_snip.len() == 0 {
""
} else {
// trim `<` `>`
&generics_snip[1..generics_snip.len() - 1]
};
let generics_span = generics.span.substitute_dummy({
let pos = snippet_opt(cx, item.span.until(ty.span))
.and_then(|snip| {
let i = snip.find("fn")?;
Some(item.span.lo() + ::syntax_pos::BytePos(i as u32 + (&snip[i..]).find('(')? as u32))
})
.expect("failed to create span for type parameters");
Span::new(pos, pos, item.span.data().ctxt)
});
let mut ctr_vis = ImplicitHasherConstructorVisitor::new(cx, target.clone());
ctr_vis.visit_body(cx.tcx.hir.body(body));
assert!(ctr_vis.suggestions.is_empty());
span_lint_and_then(
cx,
IMPLICIT_HASHER,
target.span(),
&format!(
"parameter of type `{}` should be generarized over different hashers",
target.type_name()
),
move |db| {
db.span_suggestion(
generics_span,
"consider adding a type parameter",
format!(
"<{}{}S: ::std::hash::BuildHasher>",
generics_snip_trimmed,
if generics_snip_trimmed.is_empty() {
""
} else {
", "
},
),
);
db.span_suggestion(
target.span(),
"...and change the type to",
format!("{}<{}, S>", target.type_name(), target.type_arguments(),),
);
},
);
}
}
}
}
}
#[derive(Clone)]
enum ImplicitHasherType<'tcx> {
HashMap(Span, Ty<'tcx>, Cow<'static, str>, Cow<'static, str>),
HashSet(Span, Ty<'tcx>, Cow<'static, str>),
}
impl<'tcx> ImplicitHasherType<'tcx> {
/// Checks that `ty` is a target type without a BuildHasher.
fn new<'a>(cx: &LateContext<'a, 'tcx>, hir_ty: &hir::Ty) -> Option<Self> {
if let TyPath(QPath::Resolved(None, ref path)) = hir_ty.node {
let params = &path.segments.last().as_ref()?.parameters.as_ref()?.types;
let params_len = params.len();
let ty = cx.tcx.type_of(opt_def_id(path.def)?);
if match_path(path, &paths::HASHMAP) && params_len == 2 {
Some(ImplicitHasherType::HashMap(
hir_ty.span,
ty,
snippet(cx, params[0].span, "K"),
snippet(cx, params[1].span, "V"),
))
} else if match_path(path, &paths::HASHSET) && params_len == 1 {
Some(ImplicitHasherType::HashSet(hir_ty.span, ty, snippet(cx, params[0].span, "T")))
} else {
None
}
} else {
None
}
}
fn type_name(&self) -> &'static str {
match *self {
ImplicitHasherType::HashMap(..) => "HashMap",
ImplicitHasherType::HashSet(..) => "HashSet",
}
}
fn type_arguments(&self) -> String {
match *self {
ImplicitHasherType::HashMap(.., ref k, ref v) => format!("{}, {}", k, v),
ImplicitHasherType::HashSet(.., ref t) => format!("{}", t),
}
}
fn ty(&self) -> Ty<'tcx> {
match *self {
ImplicitHasherType::HashMap(_, ty, ..) | ImplicitHasherType::HashSet(_, ty, ..) => ty,
}
}
fn span(&self) -> Span {
match *self {
ImplicitHasherType::HashMap(span, ..) | ImplicitHasherType::HashSet(span, ..) => span,
}
}
}
struct ImplicitHasherTypeVisitor<'a, 'tcx: 'a> {
cx: &'a LateContext<'a, 'tcx>,
found: Vec<ImplicitHasherType<'tcx>>,
}
impl<'a, 'tcx: 'a> ImplicitHasherTypeVisitor<'a, 'tcx> {
fn new(cx: &'a LateContext<'a, 'tcx>) -> Self {
Self { cx, found: vec![] }
}
}
impl<'a, 'tcx: 'a> Visitor<'tcx> for ImplicitHasherTypeVisitor<'a, 'tcx> {
fn visit_ty(&mut self, t: &'tcx hir::Ty) {
if let Some(target) = ImplicitHasherType::new(self.cx, t) {
self.found.push(target);
}
walk_ty(self, t);
}
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
NestedVisitorMap::None
}
}
/// Looks for default-hasher-dependent constructors like `HashMap::new`.
struct ImplicitHasherConstructorVisitor<'a, 'tcx: 'a> {
cx: &'a LateContext<'a, 'tcx>,
body: Option<BodyId>,
target: ImplicitHasherType<'tcx>,
suggestions: BTreeMap<Span, String>,
}
impl<'a, 'tcx: 'a> ImplicitHasherConstructorVisitor<'a, 'tcx> {
fn new(cx: &'a LateContext<'a, 'tcx>, target: ImplicitHasherType<'tcx>) -> Self {
Self {
cx,
body: None,
target,
suggestions: BTreeMap::new(),
}
}
}
impl<'a, 'tcx: 'a> Visitor<'tcx> for ImplicitHasherConstructorVisitor<'a, 'tcx> {
fn visit_body(&mut self, body: &'tcx Body) {
self.body = Some(body.id());
walk_body(self, body);
}
fn visit_expr(&mut self, e: &'tcx Expr) {
if_let_chain!{[
let Some(body) = self.body,
let ExprCall(ref fun, ref args) = e.node,
let ExprPath(QPath::TypeRelative(ref ty, ref method)) = fun.node,
let TyPath(QPath::Resolved(None, ref ty_path)) = ty.node,
], {
if same_tys(self.cx, self.cx.tcx.body_tables(body).expr_ty(e), self.target.ty()) {
return;
}
if match_path(ty_path, &paths::HASHMAP) {
if method.name == "new" {
self.suggestions
.insert(e.span, "HashMap::default()".to_string());
} else if method.name == "with_capacity" {
self.suggestions.insert(
e.span,
format!(
"HashMap::with_capacity_and_hasher({}, Default::default())",
snippet(self.cx, args[0].span, "capacity"),
),
);
}
} else if match_path(ty_path, &paths::HASHSET) {
if method.name == "new" {
self.suggestions
.insert(e.span, "HashSet::default()".to_string());
} else if method.name == "with_capacity" {
self.suggestions.insert(
e.span,
format!(
"HashSet::with_capacity_and_hasher({}, Default::default())",
snippet(self.cx, args[0].span, "capacity"),
),
);
}
}
}}
walk_expr(self, e);
}
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
NestedVisitorMap::OnlyBodies(&self.cx.tcx.hir)
}
}

68
tests/ui/implicit_hasher.rs Normal file
View File

@ -0,0 +1,68 @@
#![allow(unused)]
//#![feature(plugin)]#![plugin(clippy)]
use std::collections::{HashMap, HashSet};
use std::cmp::Eq;
use std::hash::{Hash, BuildHasher};
trait Foo<T>: Sized {
fn make() -> (Self, Self);
}
impl<K: Hash + Eq, V> Foo<i8> for HashMap<K, V> {
fn make() -> (Self, Self) {
// OK, don't suggest to modify these
let _: HashMap<i32, i32> = HashMap::new();
let _: HashSet<i32> = HashSet::new();
(HashMap::new(), HashMap::with_capacity(10))
}
}
impl<K: Hash + Eq, V> Foo<i8> for (HashMap<K, V>,) {
fn make() -> (Self, Self) {
((HashMap::new(),), (HashMap::with_capacity(10),))
}
}
impl Foo<i16> for HashMap<String, String> {
fn make() -> (Self, Self) {
(HashMap::new(), HashMap::with_capacity(10))
}
}
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, Default::default()))
}
}
impl<S: BuildHasher + Default> Foo<i64> for HashMap<String, String, S> {
fn make() -> (Self, Self) {
(HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
}
}
impl<T: Hash + Eq> Foo<i8> for HashSet<T> {
fn make() -> (Self, Self) {
(HashSet::new(), HashSet::with_capacity(10))
}
}
impl Foo<i16> for HashSet<String> {
fn make() -> (Self, Self) {
(HashSet::new(), HashSet::with_capacity(10))
}
}
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, Default::default()))
}
}
impl<S: BuildHasher + Default> Foo<i64> for HashSet<String, S> {
fn make() -> (Self, Self) {
(HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
}
}
pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {
}
fn main() {}

154
tests/ui/implicit_hasher.stderr Normal file
View File

@ -0,0 +1,154 @@
error: impl for `HashMap` should be generarized over different hashers
--> $DIR/implicit_hasher.rs:11:35
|
11 | impl<K: Hash + Eq, V> Foo<i8> for HashMap<K, V> {
| ^^^^^^^^^^^^^
|
= note: `-D implicit-hasher` implied by `-D warnings`
help: consider adding a type parameter
|
11 | impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashMap<K, V> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the type to
|
11 | impl<K: Hash + Eq, V> Foo<i8> for HashMap<K, V, S> {
| ^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
14 | let _: HashMap<i32, i32> = HashMap::default();
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
15 | let _: HashSet<i32> = HashSet::default();
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
17 | (HashMap::default(), HashMap::with_capacity(10))
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
17 | (HashMap::new(), HashMap::with_capacity_and_hasher(10, Default::default()))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: impl for `HashMap` should be generarized over different hashers
--> $DIR/implicit_hasher.rs:20:36
|
20 | impl<K: Hash + Eq, V> Foo<i8> for (HashMap<K, V>,) {
| ^^^^^^^^^^^^^
|
help: consider adding a type parameter
|
20 | impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for (HashMap<K, V>,) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the type to
|
20 | impl<K: Hash + Eq, V> Foo<i8> for (HashMap<K, V, S>,) {
| ^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
22 | ((HashMap::default(),), (HashMap::with_capacity(10),))
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
22 | ((HashMap::new(),), (HashMap::with_capacity_and_hasher(10, Default::default()),))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: impl for `HashMap` should be generarized over different hashers
--> $DIR/implicit_hasher.rs:25:19
|
25 | impl Foo<i16> for HashMap<String, String> {
| ^^^^^^^^^^^^^^^^^^^^^^^
|
help: consider adding a type parameter
|
25 | impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashMap<String, String> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the type to
|
25 | impl Foo<i16> for HashMap<String, String, S> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
27 | (HashMap::default(), HashMap::with_capacity(10))
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
27 | (HashMap::new(), HashMap::with_capacity_and_hasher(10, Default::default()))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: impl for `HashSet` should be generarized over different hashers
--> $DIR/implicit_hasher.rs:43:32
|
43 | impl<T: Hash + Eq> Foo<i8> for HashSet<T> {
| ^^^^^^^^^^
|
help: consider adding a type parameter
|
43 | impl<T: Hash + Eq, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashSet<T> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the type to
|
43 | impl<T: Hash + Eq> Foo<i8> for HashSet<T, S> {
| ^^^^^^^^^^^^^
help: ...and use generic constructor here
|
45 | (HashSet::default(), HashSet::with_capacity(10))
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
45 | (HashSet::new(), HashSet::with_capacity_and_hasher(10, Default::default()))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: impl for `HashSet` should be generarized over different hashers
--> $DIR/implicit_hasher.rs:48:19
|
48 | impl Foo<i16> for HashSet<String> {
| ^^^^^^^^^^^^^^^
|
help: consider adding a type parameter
|
48 | impl<S: ::std::hash::BuildHasher + Default> Foo<i16> for HashSet<String> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the type to
|
48 | impl Foo<i16> for HashSet<String, S> {
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
50 | (HashSet::default(), HashSet::with_capacity(10))
| ^^^^^^^^^^^^^^^^^^
help: ...and use generic constructor here
|
50 | (HashSet::new(), HashSet::with_capacity_and_hasher(10, Default::default()))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: parameter of type `HashMap` should be generarized over different hashers
--> $DIR/implicit_hasher.rs:65:23
|
65 | pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {
| ^^^^^^^^^^^^^^^^^
|
help: consider adding a type parameter
|
65 | pub fn foo<S: ::std::hash::BuildHasher>(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the type to
|
65 | pub fn foo(_map: &mut HashMap<i32, i32, S>, _set: &mut HashSet<i32>) {
| ^^^^^^^^^^^^^^^^^^^^
error: parameter of type `HashSet` should be generarized over different hashers
--> $DIR/implicit_hasher.rs:65:53
|
65 | pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {
| ^^^^^^^^^^^^
|
help: consider adding a type parameter
|
65 | pub fn foo<S: ::std::hash::BuildHasher>(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32>) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the type to
|
65 | pub fn foo(_map: &mut HashMap<i32, i32>, _set: &mut HashSet<i32, S>) {
| ^^^^^^^^^^^^^^^