Improve accuracy of mut_key

clippy_lints/src/mut_key.rs

@@ -3,7 +3,7 @@ use clippy_utils::trait_ref_of_method;
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::TypeFoldable;
-use rustc_middle::ty::{Adt, Array, RawPtr, Ref, Slice, Tuple, Ty, TypeAndMut};
+use rustc_middle::ty::{Adt, Array, Ref, Slice, Tuple, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
 use rustc_span::symbol::sym;
@@ -19,10 +19,29 @@ declare_clippy_lint! {
     /// so having types with interior mutability is a bad idea.
     ///
     /// ### Known problems
-    /// It's correct to use a struct, that contains interior mutability
-    /// as a key, when its `Hash` implementation doesn't access any of the interior mutable types.
-    /// However, this lint is unable to recognize this, so it causes a false positive in theses cases.
-    /// The `bytes` crate is a great example of this.
+    ///
+    /// #### False Positives
+    /// It's correct to use a struct that contains interior mutability as a key, when its
+    /// implementation of `Hash` or `Ord` doesn't access any of the interior mutable types.
+    /// However, this lint is unable to recognize this, so it will often cause false positives in
+    /// theses cases.  The `bytes` crate is a great example of this.
+    ///
+    /// #### False Negatives
+    /// For custom `struct`s/`enum`s, this lint is unable to check for interior mutability behind
+    /// indirection.  For example, `struct BadKey<'a>(&'a Cell<usize>)` will be seen as immutable
+    /// and cause a false negative if its implementation of `Hash`/`Ord` accesses the `Cell`.
+    ///
+    /// This lint does check a few cases for indirection.  Firstly, using some standard library
+    /// types (`Option`, `Result`, `Box`, `Rc`, `Arc`, `Vec`, `VecDeque`, `BTreeMap` and
+    /// `BTreeSet`) directly as keys (e.g. in `HashMap<Box<Cell<usize>>, ()>`) **will** trigger the
+    /// lint, because the impls of `Hash`/`Ord` for these types directly call `Hash`/`Ord` on their
+    /// contained type.
+    ///
+    /// Secondly, the implementations of `Hash` and `Ord` for raw pointers (`*const T` or `*mut T`)
+    /// apply only to the **address** of the contained value.  Therefore, interior mutability
+    /// behind raw pointers (e.g. in `HashSet<*mut Cell<usize>>`) can't impact the value of `Hash`
+    /// or `Ord`, and therefore will not trigger this link.  For more info, see issue
+    /// [#6745](https://github.com/rust-lang/rust-clippy/issues/6745).
     ///
     /// ### Example
     /// ```rust
@@ -103,30 +122,52 @@ fn check_sig<'tcx>(cx: &LateContext<'tcx>, item_hir_id: hir::HirId, decl: &hir::
 fn check_ty<'tcx>(cx: &LateContext<'tcx>, span: Span, ty: Ty<'tcx>) {
     let ty = ty.peel_refs();
     if let Adt(def, substs) = ty.kind() {
-        if [sym::hashmap_type, sym::BTreeMap, sym::hashset_type, sym::BTreeMap]
+        let is_keyed_type = [sym::hashmap_type, sym::BTreeMap, sym::hashset_type, sym::BTreeSet]
             .iter()
-            .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def.did))
-            && is_mutable_type(cx, substs.type_at(0), span)
-        {
+            .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def.did));
+        if is_keyed_type && is_interior_mutable_type(cx, substs.type_at(0), span) {
             span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type");
         }
     }
 }
 
-fn is_mutable_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, span: Span) -> bool {
+/// Determines if a type contains interior mutability which would affect its implementation of
+/// [`Hash`] or [`Ord`].
+fn is_interior_mutable_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, span: Span) -> bool {
     match *ty.kind() {
-        RawPtr(TypeAndMut { ty: inner_ty, mutbl }) | Ref(_, inner_ty, mutbl) => {
-            mutbl == hir::Mutability::Mut || is_mutable_type(cx, inner_ty, span)
-        },
-        Slice(inner_ty) => is_mutable_type(cx, inner_ty, span),
+        Ref(_, inner_ty, mutbl) => mutbl == hir::Mutability::Mut || is_interior_mutable_type(cx, inner_ty, span),
+        Slice(inner_ty) => is_interior_mutable_type(cx, inner_ty, span),
         Array(inner_ty, size) => {
-            size.try_eval_usize(cx.tcx, cx.param_env).map_or(true, |u| u != 0) && is_mutable_type(cx, inner_ty, span)
+            size.try_eval_usize(cx.tcx, cx.param_env).map_or(true, |u| u != 0)
+                && is_interior_mutable_type(cx, inner_ty, span)
         },
-        Tuple(..) => ty.tuple_fields().any(|ty| is_mutable_type(cx, ty, span)),
-        Adt(..) => {
-            !ty.has_escaping_bound_vars()
-                && cx.tcx.layout_of(cx.param_env.and(ty)).is_ok()
-                && !ty.is_freeze(cx.tcx.at(span), cx.param_env)
+        Tuple(..) => ty.tuple_fields().any(|ty| is_interior_mutable_type(cx, ty, span)),
+        Adt(def, substs) => {
+            // Special case for collections in `std` who's impl of `Hash` or `Ord` delegates to
+            // that of their type parameters.  Note: we don't include `HashSet` and `HashMap`
+            // because they have no impl for `Hash` or `Ord`.
+            let is_std_collection = [
+                sym::option_type,
+                sym::result_type,
+                sym::LinkedList,
+                sym::vec_type,
+                sym::vecdeque_type,
+                sym::BTreeMap,
+                sym::BTreeSet,
+                sym::Rc,
+                sym::Arc,
+            ]
+            .iter()
+            .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def.did));
+            let is_box = Some(def.did) == cx.tcx.lang_items().owned_box();
+            if is_std_collection || is_box {
+                // The type is mutable if any of its type parameters are
+                substs.types().any(|ty| is_interior_mutable_type(cx, ty, span))
+            } else {
+                !ty.has_escaping_bound_vars()
+                    && cx.tcx.layout_of(cx.param_env.and(ty)).is_ok()
+                    && !ty.is_freeze(cx.tcx.at(span), cx.param_env)
+            }
         },
         _ => false,
     }

tests/ui/mut_key.rs

@@ -1,6 +1,9 @@
-use std::collections::{HashMap, HashSet};
+use std::cell::Cell;
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
 use std::hash::{Hash, Hasher};
+use std::rc::Rc;
 use std::sync::atomic::{AtomicUsize, Ordering::Relaxed};
+use std::sync::Arc;
 
 struct Key(AtomicUsize);
 
@@ -31,11 +34,19 @@ fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<K
 
 fn this_is_ok(_m: &mut HashMap<usize, Key>) {}
 
+// Raw pointers are hashed by the address they point to, so it doesn't matter if they point to a
+// type with interior mutability.  See:
+// - clippy issue: https://github.com/rust-lang/rust-clippy/issues/6745
+// - std lib: https://github.com/rust-lang/rust/blob/1.54.0/library/core/src/hash/mod.rs#L717-L736
+// So these are OK:
+fn raw_ptr_is_ok(_m: &mut HashMap<*const Key, ()>) {}
+fn raw_mut_ptr_is_ok(_m: &mut HashMap<*mut Key, ()>) {}
+
 #[allow(unused)]
 trait Trait {
     type AssociatedType;
 
-    fn trait_fn(&self, set: std::collections::HashSet<Self::AssociatedType>);
+    fn trait_fn(&self, set: HashSet<Self::AssociatedType>);
 }
 
 fn generics_are_ok_too<K>(_m: &mut HashSet<K>) {
@@ -52,4 +63,23 @@ fn main() {
     tuples::<Key>(&mut HashMap::new());
     tuples::<()>(&mut HashMap::new());
     tuples_bad::<()>(&mut HashMap::new());
+
+    raw_ptr_is_ok(&mut HashMap::new());
+    raw_mut_ptr_is_ok(&mut HashMap::new());
+
+    let _map = HashMap::<Cell<usize>, usize>::new();
+    let _map = HashMap::<&mut Cell<usize>, usize>::new();
+    let _map = HashMap::<&mut usize, usize>::new();
+    // Collection types from `std` who's impl of `Hash` or `Ord` delegate their type parameters
+    let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
+    let _map = HashMap::<BTreeMap<Cell<usize>, ()>, usize>::new();
+    let _map = HashMap::<BTreeMap<(), Cell<usize>>, usize>::new();
+    let _map = HashMap::<BTreeSet<Cell<usize>>, usize>::new();
+    let _map = HashMap::<Option<Cell<usize>>, usize>::new();
+    let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
+    let _map = HashMap::<Result<&mut usize, ()>, usize>::new();
+    // Smart pointers from `std` who's impl of `Hash` or `Ord` delegate their type parameters
+    let _map = HashMap::<Box<Cell<usize>>, usize>::new();
+    let _map = HashMap::<Rc<Cell<usize>>, usize>::new();
+    let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
 }

tests/ui/mut_key.stderr

@@ -1,28 +1,106 @@
 error: mutable key type
-  --> $DIR/mut_key.rs:27:32
+  --> $DIR/mut_key.rs:30:32
    |
 LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::mutable-key-type` implied by `-D warnings`
 
 error: mutable key type
-  --> $DIR/mut_key.rs:27:72
+  --> $DIR/mut_key.rs:30: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
+  --> $DIR/mut_key.rs:31:5
    |
 LL |     let _other: HashMap<Key, bool> = HashMap::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: mutable key type
-  --> $DIR/mut_key.rs:47:22
+  --> $DIR/mut_key.rs:58:22
    |
 LL | fn tuples_bad<U>(_m: &mut HashMap<(Key, U), bool>) {}
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 4 previous errors
+error: mutable key type
+  --> $DIR/mut_key.rs:70:5
+   |
+LL |     let _map = HashMap::<Cell<usize>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:71:5
+   |
+LL |     let _map = HashMap::<&mut Cell<usize>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:72:5
+   |
+LL |     let _map = HashMap::<&mut usize, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:74:5
+   |
+LL |     let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:75:5
+   |
+LL |     let _map = HashMap::<BTreeMap<Cell<usize>, ()>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:76:5
+   |
+LL |     let _map = HashMap::<BTreeMap<(), Cell<usize>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:77:5
+   |
+LL |     let _map = HashMap::<BTreeSet<Cell<usize>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:78:5
+   |
+LL |     let _map = HashMap::<Option<Cell<usize>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:79:5
+   |
+LL |     let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:80:5
+   |
+LL |     let _map = HashMap::<Result<&mut usize, ()>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:82:5
+   |
+LL |     let _map = HashMap::<Box<Cell<usize>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:83:5
+   |
+LL |     let _map = HashMap::<Rc<Cell<usize>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: mutable key type
+  --> $DIR/mut_key.rs:84:5
+   |
+LL |     let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 17 previous errors