From 159cc8413cf0f228a5e590a1d3a88f84c6e6cc4a Mon Sep 17 00:00:00 2001 From: sinkuu Date: Thu, 5 Oct 2017 23:57:31 +0900 Subject: [PATCH] Add implicit_hasher lint (#2101) --- clippy_lints/src/lib.rs | 2 + clippy_lints/src/types.rs | 354 ++++++++++++++++++++++++++++++-- tests/ui/implicit_hasher.rs | 68 ++++++ tests/ui/implicit_hasher.stderr | 154 ++++++++++++++ 4 files changed, 563 insertions(+), 15 deletions(-) create mode 100644 tests/ui/implicit_hasher.rs create mode 100644 tests/ui/implicit_hasher.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0f27a74ac8e..d8f99ae6cb3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -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, diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index acea709123b..006833ba934 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -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 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidUpcastComparisons { } } } + +/// **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 Serialize for HashMap { ... } +/// +/// pub foo(map: &mut HashMap) { .. } +/// ``` +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 { + 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>, +} + +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, + target: ImplicitHasherType<'tcx>, + suggestions: BTreeMap, +} + +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) + } +} diff --git a/tests/ui/implicit_hasher.rs b/tests/ui/implicit_hasher.rs new file mode 100644 index 00000000000..7e1eef9b11c --- /dev/null +++ b/tests/ui/implicit_hasher.rs @@ -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: Sized { + fn make() -> (Self, Self); +} + +impl Foo for HashMap { + fn make() -> (Self, Self) { + // OK, don't suggest to modify these + let _: HashMap = HashMap::new(); + let _: HashSet = HashSet::new(); + + (HashMap::new(), HashMap::with_capacity(10)) + } +} +impl Foo for (HashMap,) { + fn make() -> (Self, Self) { + ((HashMap::new(),), (HashMap::with_capacity(10),)) + } +} +impl Foo for HashMap { + fn make() -> (Self, Self) { + (HashMap::new(), HashMap::with_capacity(10)) + } +} + +impl Foo for HashMap { + fn make() -> (Self, Self) { + (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default())) + } +} +impl Foo for HashMap { + fn make() -> (Self, Self) { + (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default())) + } +} + + +impl Foo for HashSet { + fn make() -> (Self, Self) { + (HashSet::new(), HashSet::with_capacity(10)) + } +} +impl Foo for HashSet { + fn make() -> (Self, Self) { + (HashSet::new(), HashSet::with_capacity(10)) + } +} + +impl Foo for HashSet { + fn make() -> (Self, Self) { + (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default())) + } +} +impl Foo for HashSet { + fn make() -> (Self, Self) { + (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default())) + } +} + +pub fn foo(_map: &mut HashMap, _set: &mut HashSet) { +} + +fn main() {} diff --git a/tests/ui/implicit_hasher.stderr b/tests/ui/implicit_hasher.stderr new file mode 100644 index 00000000000..086362185cf --- /dev/null +++ b/tests/ui/implicit_hasher.stderr @@ -0,0 +1,154 @@ +error: impl for `HashMap` should be generarized over different hashers + --> $DIR/implicit_hasher.rs:11:35 + | +11 | impl Foo for HashMap { + | ^^^^^^^^^^^^^ + | + = note: `-D implicit-hasher` implied by `-D warnings` +help: consider adding a type parameter + | +11 | impl Foo for HashMap { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and change the type to + | +11 | impl Foo for HashMap { + | ^^^^^^^^^^^^^^^^ +help: ...and use generic constructor here + | +14 | let _: HashMap = HashMap::default(); + | ^^^^^^^^^^^^^^^^^^ +help: ...and use generic constructor here + | +15 | let _: HashSet = 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 Foo for (HashMap,) { + | ^^^^^^^^^^^^^ + | +help: consider adding a type parameter + | +20 | impl Foo for (HashMap,) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and change the type to + | +20 | impl Foo for (HashMap,) { + | ^^^^^^^^^^^^^^^^ +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 for HashMap { + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider adding a type parameter + | +25 | impl Foo for HashMap { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and change the type to + | +25 | impl Foo for HashMap { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +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 Foo for HashSet { + | ^^^^^^^^^^ + | +help: consider adding a type parameter + | +43 | impl Foo for HashSet { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and change the type to + | +43 | impl Foo for HashSet { + | ^^^^^^^^^^^^^ +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 for HashSet { + | ^^^^^^^^^^^^^^^ + | +help: consider adding a type parameter + | +48 | impl Foo for HashSet { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and change the type to + | +48 | impl Foo for HashSet { + | ^^^^^^^^^^^^^^^^^^ +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, _set: &mut HashSet) { + | ^^^^^^^^^^^^^^^^^ + | +help: consider adding a type parameter + | +65 | pub fn foo(_map: &mut HashMap, _set: &mut HashSet) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and change the type to + | +65 | pub fn foo(_map: &mut HashMap, _set: &mut HashSet) { + | ^^^^^^^^^^^^^^^^^^^^ + +error: parameter of type `HashSet` should be generarized over different hashers + --> $DIR/implicit_hasher.rs:65:53 + | +65 | pub fn foo(_map: &mut HashMap, _set: &mut HashSet) { + | ^^^^^^^^^^^^ + | +help: consider adding a type parameter + | +65 | pub fn foo(_map: &mut HashMap, _set: &mut HashSet) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and change the type to + | +65 | pub fn foo(_map: &mut HashMap, _set: &mut HashSet) { + | ^^^^^^^^^^^^^^^ +