Skip to content

Commit b2ffb28

Browse files
kneaslekneasle
committed
Fix FN for collections/smart ptrs in std 
1 parent e009073 commit b2ffb28

File tree

3 files changed

+161
-37
lines changed

3 files changed

+161
-37
lines changed

‎clippy_lints/src/mut_key.rs

Lines changed: 59 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use clippy_utils::trait_ref_of_method;
33
use rustc_hir as hir;
44
use rustc_lint::{LateContext, LateLintPass};
55
use rustc_middle::ty::TypeFoldable;
6-
use rustc_middle::ty::{Adt, Array, RawPtr, Ref, Slice, Tuple, Ty, TypeAndMut};
6+
use rustc_middle::ty::{Adt, Array, Ref, Slice, Tuple, Ty};
77
use rustc_session::{declare_lint_pass, declare_tool_lint};
88
use rustc_span::source_map::Span;
99
use rustc_span::symbol::sym;
@@ -19,10 +19,29 @@ declare_clippy_lint! {
1919
/// so having types with interior mutability is a bad idea.
2020
///
2121
/// ### Known problems
22-
/// It's correct to use a struct, that contains interior mutability
23-
/// as a key, when its `Hash` implementation doesn't access any of the interior mutable types.
24-
/// However, this lint is unable to recognize this, so it causes a false positive in theses cases.
25-
/// The `bytes` crate is a great example of this.
22+
///
23+
/// #### False Positives
24+
/// It's correct to use a struct that contains interior mutability as a key, when its
25+
/// implementation of `Hash` or `Ord` doesn't access any of the interior mutable types.
26+
/// However, this lint is unable to recognize this, so it will often cause false positives in
27+
/// theses cases. The `bytes` crate is a great example of this.
28+
///
29+
/// #### False Negatives
30+
/// For custom `struct`s/`enum`s, this lint is unable to check for interior mutability behind
31+
/// indirection. For example, `struct BadKey<'a>(&'a Cell<usize>)` will be seen as immutable
32+
/// and cause a false negative if its implementation of `Hash`/`Ord` accesses the `Cell`.
33+
///
34+
/// This lint does check a few cases for indirection. Firstly, using some standard library
35+
/// types (`Option`, `Result`, `Box`, `Rc`, `Arc`, `Vec`, `VecDeque`, `BTreeMap` and
36+
/// `BTreeSet`) directly as keys (e.g. in `HashMap<Box<Cell<usize>>, ()>`) **will** trigger the
37+
/// lint, because the impls of `Hash`/`Ord` for these types directly call `Hash`/`Ord` on their
38+
/// contained type.
39+
///
40+
/// Secondly, the implementations of `Hash` and `Ord` for raw pointers (`*const T` or `*mut T`)
41+
/// apply only to the **address** of the contained value. Therefore, interior mutability
42+
/// behind raw pointers (e.g. in `HashSet<*mut Cell<usize>>`) can't impact the value of `Hash`
43+
/// or `Ord`, and therefore will not trigger this link. For more info, see issue
44+
/// [#6745](https://github.com/rust-lang/rust-clippy/issues/6745).
2645
///
2746
/// ### Example
2847
/// ```rust
@@ -103,43 +122,52 @@ fn check_sig<'tcx>(cx: &LateContext<'tcx>, item_hir_id: hir::HirId, decl: &hir::
103122
fn check_ty<'tcx>(cx: &LateContext<'tcx>, span: Span, ty: Ty<'tcx>) {
104123
let ty = ty.peel_refs();
105124
if let Adt(def, substs) = ty.kind() {
106-
let is_map_type = [sym::hashmap_type, sym::BTreeMap, sym::hashset_type, sym::BTreeMap]
125+
let is_keyed_type = [sym::hashmap_type, sym::BTreeMap, sym::hashset_type, sym::BTreeSet]
107126
.iter()
108127
.any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def.did));
109-
if is_map_type && is_mutable_type(cx, substs.type_at(0), span, true) {
128+
if is_keyed_type && is_interior_mutable_type(cx, substs.type_at(0), span) {
110129
span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type");
111130
}
112131
}
113132
}
114133

115-
fn is_mutable_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, span: Span, is_top_level_type: bool) -> bool {
134+
/// Determines if a type contains interior mutability which would affect its implementation of
135+
/// [`Hash`] or [`Ord`].
136+
fn is_interior_mutable_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, span: Span) -> bool {
116137
match *ty.kind() {
117-
RawPtr(TypeAndMut { ty: inner_ty, mutbl }) => {
118-
if is_top_level_type {
119-
// Raw pointers are hashed by the address they point to, not what is pointed to.
120-
// Therefore, using a raw pointer to any type as the top-level key type is OK.
121-
// Using raw pointers _in_ the key type is not, because the wrapper type could
122-
// provide a custom `impl` for `Hash` (which could deref the raw pointer).
123-
//
124-
// see:
125-
// - clippy issue: https://github.com/rust-lang/rust-clippy/issues/6745
126-
// - std code: https://github.com/rust-lang/rust/blob/1.54.0/library/core/src/hash/mod.rs#L717-L736
127-
false
128-
} else {
129-
mutbl == hir::Mutability::Mut || is_mutable_type(cx, inner_ty, span, false)
130-
}
131-
},
132-
Ref(_, inner_ty, mutbl) => mutbl == hir::Mutability::Mut || is_mutable_type(cx, inner_ty, span, false),
133-
Slice(inner_ty) => is_mutable_type(cx, inner_ty, span, false),
138+
Ref(_, inner_ty, mutbl) => mutbl == hir::Mutability::Mut || is_interior_mutable_type(cx, inner_ty, span),
139+
Slice(inner_ty) => is_interior_mutable_type(cx, inner_ty, span),
134140
Array(inner_ty, size) => {
135141
size.try_eval_usize(cx.tcx, cx.param_env).map_or(true, |u| u != 0)
136-
&& is_mutable_type(cx, inner_ty, span, false)
142+
&& is_interior_mutable_type(cx, inner_ty, span)
137143
},
138-
Tuple(..) => ty.tuple_fields().any(|ty| is_mutable_type(cx, ty, span, false)),
139-
Adt(..) => {
140-
!ty.has_escaping_bound_vars()
141-
&& cx.tcx.layout_of(cx.param_env.and(ty)).is_ok()
142-
&& !ty.is_freeze(cx.tcx.at(span), cx.param_env)
144+
Tuple(..) => ty.tuple_fields().any(|ty| is_interior_mutable_type(cx, ty, span)),
145+
Adt(def, substs) => {
146+
// Special case for collections in `std` who's impl of `Hash` or `Ord` delegates to
147+
// that of their type parameters. Note: we don't include `HashSet` and `HashMap`
148+
// because they have no impl for `Hash` or `Ord`.
149+
let is_std_collection = [
150+
sym::option_type,
151+
sym::result_type,
152+
sym::LinkedList,
153+
sym::vec_type,
154+
sym::vecdeque_type,
155+
sym::BTreeMap,
156+
sym::BTreeSet,
157+
sym::Rc,
158+
sym::Arc,
159+
]
160+
.iter()
161+
.any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def.did));
162+
let is_box = Some(def.did) == cx.tcx.lang_items().owned_box();
163+
if is_std_collection || is_box {
164+
// The type is mutable if any of its type parameters are
165+
substs.types().any(|ty| is_interior_mutable_type(cx, ty, span))
166+
} else {
167+
!ty.has_escaping_bound_vars()
168+
&& cx.tcx.layout_of(cx.param_env.and(ty)).is_ok()
169+
&& !ty.is_freeze(cx.tcx.at(span), cx.param_env)
170+
}
143171
},
144172
_ => false,
145173
}

‎tests/ui/mut_key.rs

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
use std::cell::Cell;
2-
use std::collections::{HashMap, HashSet};
2+
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
33
use std::hash::{Hash, Hasher};
4+
use std::rc::Rc;
45
use std::sync::atomic::{AtomicUsize, Ordering::Relaxed};
6+
use std::sync::Arc;
57

68
struct Key(AtomicUsize);
79

@@ -64,4 +66,20 @@ fn main() {
6466

6567
raw_ptr_is_ok(&mut HashMap::new());
6668
raw_mut_ptr_is_ok(&mut HashMap::new());
69+
70+
let _map = HashMap::<Cell<usize>, usize>::new();
71+
let _map = HashMap::<&mut Cell<usize>, usize>::new();
72+
let _map = HashMap::<&mut usize, usize>::new();
73+
// Collection types from `std` who's impl of `Hash` or `Ord` delegate their type parameters
74+
let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
75+
let _map = HashMap::<BTreeMap<Cell<usize>, ()>, usize>::new();
76+
let _map = HashMap::<BTreeMap<(), Cell<usize>>, usize>::new();
77+
let _map = HashMap::<BTreeSet<Cell<usize>>, usize>::new();
78+
let _map = HashMap::<Option<Cell<usize>>, usize>::new();
79+
let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
80+
let _map = HashMap::<Result<&mut usize, ()>, usize>::new();
81+
// Smart pointers from `std` who's impl of `Hash` or `Ord` delegate their type parameters
82+
let _map = HashMap::<Box<Cell<usize>>, usize>::new();
83+
let _map = HashMap::<Rc<Cell<usize>>, usize>::new();
84+
let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
6785
}

‎tests/ui/mut_key.stderr

Lines changed: 83 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,106 @@
11
error: mutable key type
2-
--> $DIR/mut_key.rs:28:32
2+
--> $DIR/mut_key.rs:30:32
33
|
44
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
55
| ^^^^^^^^^^^^^^^^^^^^^^^^
66
|
77
= note: `-D clippy::mutable-key-type` implied by `-D warnings`
88

99
error: mutable key type
10-
--> $DIR/mut_key.rs:28:72
10+
--> $DIR/mut_key.rs:30:72
1111
|
1212
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
1313
| ^^^^^^^^^^^^
1414

1515
error: mutable key type
16-
--> $DIR/mut_key.rs:29:5
16+
--> $DIR/mut_key.rs:31:5
1717
|
1818
LL | let _other: HashMap<Key, bool> = HashMap::new();
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2020

2121
error: mutable key type
22-
--> $DIR/mut_key.rs:56:22
22+
--> $DIR/mut_key.rs:58:22
2323
|
2424
LL | fn tuples_bad<U>(_m: &mut HashMap<(Key, U), bool>) {}
2525
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2626

27-
error: aborting due to 4 previous errors
27+
error: mutable key type
28+
--> $DIR/mut_key.rs:70:5
29+
|
30+
LL | let _map = HashMap::<Cell<usize>, usize>::new();
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
32+
33+
error: mutable key type
34+
--> $DIR/mut_key.rs:71:5
35+
|
36+
LL | let _map = HashMap::<&mut Cell<usize>, usize>::new();
37+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
38+
39+
error: mutable key type
40+
--> $DIR/mut_key.rs:72:5
41+
|
42+
LL | let _map = HashMap::<&mut usize, usize>::new();
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
44+
45+
error: mutable key type
46+
--> $DIR/mut_key.rs:74:5
47+
|
48+
LL | let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
49+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
50+
51+
error: mutable key type
52+
--> $DIR/mut_key.rs:75:5
53+
|
54+
LL | let _map = HashMap::<BTreeMap<Cell<usize>, ()>, usize>::new();
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
56+
57+
error: mutable key type
58+
--> $DIR/mut_key.rs:76:5
59+
|
60+
LL | let _map = HashMap::<BTreeMap<(), Cell<usize>>, usize>::new();
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
62+
63+
error: mutable key type
64+
--> $DIR/mut_key.rs:77:5
65+
|
66+
LL | let _map = HashMap::<BTreeSet<Cell<usize>>, usize>::new();
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
68+
69+
error: mutable key type
70+
--> $DIR/mut_key.rs:78:5
71+
|
72+
LL | let _map = HashMap::<Option<Cell<usize>>, usize>::new();
73+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
74+
75+
error: mutable key type
76+
--> $DIR/mut_key.rs:79:5
77+
|
78+
LL | let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
79+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
80+
81+
error: mutable key type
82+
--> $DIR/mut_key.rs:80:5
83+
|
84+
LL | let _map = HashMap::<Result<&mut usize, ()>, usize>::new();
85+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
86+
87+
error: mutable key type
88+
--> $DIR/mut_key.rs:82:5
89+
|
90+
LL | let _map = HashMap::<Box<Cell<usize>>, usize>::new();
91+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
92+
93+
error: mutable key type
94+
--> $DIR/mut_key.rs:83:5
95+
|
96+
LL | let _map = HashMap::<Rc<Cell<usize>>, usize>::new();
97+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
98+
99+
error: mutable key type
100+
--> $DIR/mut_key.rs:84:5
101+
|
102+
LL | let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
103+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
104+
105+
error: aborting due to 17 previous errors
28106

0 commit comments

Comments
 (0)