-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #4885 - rust-lang:mut-key-types, r=flip1995
new lint: mutable_key_type This fixes #732 - well, partly, it doesn't adress `Hash` impls, but the use of mutable types as map keys or set members changelog: add `mutable_key_type` lint r? @flip1995
- Loading branch information
Showing
7 changed files
with
194 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| use crate::utils::{match_def_path, paths, span_lint, trait_ref_of_method, walk_ptrs_ty}; | ||
| use rustc::declare_lint_pass; | ||
| use rustc::hir; | ||
| use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; | ||
| use rustc::ty::{Adt, Dynamic, Opaque, Param, RawPtr, Ref, Ty, TypeAndMut}; | ||
| use rustc_session::declare_tool_lint; | ||
| use syntax::source_map::Span; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// **What it does:** Checks for sets/maps with mutable key types. | ||
| /// | ||
| /// **Why is this bad?** All of `HashMap`, `HashSet`, `BTreeMap` and | ||
| /// `BtreeSet` rely on either the hash or the order of keys be unchanging, | ||
| /// so having types with interior mutability is a bad idea. | ||
| /// | ||
| /// **Known problems:** We don't currently account for `Rc` or `Arc`, so | ||
| /// this may yield false positives. | ||
| /// | ||
| /// **Example:** | ||
| /// ```rust | ||
| /// use std::cmp::{PartialEq, Eq}; | ||
| /// use std::collections::HashSet; | ||
| /// use std::hash::{Hash, Hasher}; | ||
| /// use std::sync::atomic::AtomicUsize; | ||
| ///# #[allow(unused)] | ||
| /// | ||
| /// struct Bad(AtomicUsize); | ||
| /// impl PartialEq for Bad { | ||
| /// fn eq(&self, rhs: &Self) -> bool { | ||
| /// .. | ||
| /// ; unimplemented!(); | ||
| /// } | ||
| /// } | ||
| /// | ||
| /// impl Eq for Bad {} | ||
| /// | ||
| /// impl Hash for Bad { | ||
| /// fn hash<H: Hasher>(&self, h: &mut H) { | ||
| /// .. | ||
| /// ; unimplemented!(); | ||
| /// } | ||
| /// } | ||
| /// | ||
| /// fn main() { | ||
| /// let _: HashSet<Bad> = HashSet::new(); | ||
| /// } | ||
| /// ``` | ||
| pub MUTABLE_KEY_TYPE, | ||
| correctness, | ||
| "Check for mutable Map/Set key type" | ||
| } | ||
|
|
||
| declare_lint_pass!(MutableKeyType => [ MUTABLE_KEY_TYPE ]); | ||
|
|
||
| impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MutableKeyType { | ||
| fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item<'tcx>) { | ||
| if let hir::ItemKind::Fn(ref sig, ..) = item.kind { | ||
| check_sig(cx, item.hir_id, &sig.decl); | ||
| } | ||
| } | ||
|
|
||
| fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::ImplItem<'tcx>) { | ||
| if let hir::ImplItemKind::Method(ref sig, ..) = item.kind { | ||
| if trait_ref_of_method(cx, item.hir_id).is_none() { | ||
| check_sig(cx, item.hir_id, &sig.decl); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::TraitItem<'tcx>) { | ||
| if let hir::TraitItemKind::Method(ref sig, ..) = item.kind { | ||
| check_sig(cx, item.hir_id, &sig.decl); | ||
| } | ||
| } | ||
|
|
||
| fn check_local(&mut self, cx: &LateContext<'_, '_>, local: &hir::Local) { | ||
| if let hir::PatKind::Wild = local.pat.kind { | ||
| return; | ||
| } | ||
| check_ty(cx, local.span, cx.tables.pat_ty(&*local.pat)); | ||
| } | ||
| } | ||
|
|
||
| fn check_sig<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, item_hir_id: hir::HirId, decl: &hir::FnDecl) { | ||
| let fn_def_id = cx.tcx.hir().local_def_id(item_hir_id); | ||
| let fn_sig = cx.tcx.fn_sig(fn_def_id); | ||
| for (hir_ty, ty) in decl.inputs.iter().zip(fn_sig.inputs().skip_binder().iter()) { | ||
| check_ty(cx, hir_ty.span, ty); | ||
| } | ||
| check_ty( | ||
| cx, | ||
| decl.output.span(), | ||
| cx.tcx.erase_late_bound_regions(&fn_sig.output()), | ||
| ); | ||
| } | ||
|
|
||
| // We want to lint 1. sets or maps with 2. not immutable key types and 3. no unerased | ||
| // generics (because the compiler cannot ensure immutability for unknown types). | ||
| fn check_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, span: Span, ty: Ty<'tcx>) { | ||
| let ty = walk_ptrs_ty(ty); | ||
| if let Adt(def, substs) = ty.kind { | ||
| if [&paths::HASHMAP, &paths::BTREEMAP, &paths::HASHSET, &paths::BTREESET] | ||
| .iter() | ||
| .any(|path| match_def_path(cx, def.did, &**path)) | ||
| { | ||
| let key_type = substs.type_at(0); | ||
| if is_concrete_type(key_type) && !key_type.is_freeze(cx.tcx, cx.param_env, span) { | ||
| span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn is_concrete_type(ty: Ty<'_>) -> bool { | ||
| match ty.kind { | ||
| RawPtr(TypeAndMut { ty: inner_ty, .. }) | Ref(_, inner_ty, _) => is_concrete_type(inner_ty), | ||
| Dynamic(..) | Opaque(..) | Param(..) => false, | ||
| _ => true, | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| use std::collections::{HashMap, HashSet}; | ||
| use std::hash::{Hash, Hasher}; | ||
| use std::sync::atomic::{AtomicUsize, Ordering::Relaxed}; | ||
|
|
||
| struct Key(AtomicUsize); | ||
|
|
||
| impl Clone for Key { | ||
| fn clone(&self) -> Self { | ||
| Key(AtomicUsize::new(self.0.load(Relaxed))) | ||
| } | ||
| } | ||
|
|
||
| impl PartialEq for Key { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.0.load(Relaxed) == other.0.load(Relaxed) | ||
| } | ||
| } | ||
|
|
||
| impl Eq for Key {} | ||
|
|
||
| impl Hash for Key { | ||
| fn hash<H: Hasher>(&self, h: &mut H) { | ||
| self.0.load(Relaxed).hash(h); | ||
| } | ||
| } | ||
|
|
||
| fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> { | ||
| let _other: HashMap<Key, bool> = HashMap::new(); | ||
| m.keys().cloned().collect() | ||
| } | ||
|
|
||
| fn this_is_ok(m: &mut HashMap<usize, Key>) {} | ||
|
|
||
| fn main() { | ||
| let _ = should_not_take_this_arg(&mut HashMap::new(), 1); | ||
| this_is_ok(&mut HashMap::new()); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| error: mutable key type | ||
| --> $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: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:28:5 | ||
| | | ||
| LL | let _other: HashMap<Key, bool> = HashMap::new(); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| error: aborting due to 3 previous errors | ||
|
|