Auto merge of rust-lang#13496 - y21:issue10619, r=Alexendoo

Show interior mutability chain in `mutable_key_type`

Fixes rust-lang#10619

Just ran into this myself and I definitely agree it's not very nice to have to manually go through all the types involved to figure out why this happens and to evaluate if this is really a problem (knowing if the field of a struct is something that a hash impl relies on), so this changes the lint to emit notes for each step involved.

changelog: none

Author: bors

clippy_lints/src/mut_key.rs

@@ -1,9 +1,10 @@
 use clippy_config::Conf;
-use clippy_utils::diagnostics::span_lint;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::trait_ref_of_method;
 use clippy_utils::ty::InteriorMut;
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::print::with_forced_trimmed_paths;
 use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_session::impl_lint_pass;
 use rustc_span::Span;
@@ -132,8 +133,14 @@ impl<'tcx> MutableKeyType<'tcx> {
             )
         {
             let subst_ty = args.type_at(0);
-            if self.interior_mut.is_interior_mut_ty(cx, subst_ty) {
-                span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type");
+            if let Some(chain) = self.interior_mut.interior_mut_ty_chain(cx, subst_ty) {
+                span_lint_and_then(cx, MUTABLE_KEY_TYPE, span, "mutable key type", |diag| {
+                    for ty in chain.iter().rev() {
+                        diag.note(with_forced_trimmed_paths!(format!(
+                            "... because it contains `{ty}`, which has interior mutability"
+                        )));
+                    }
+                });
             }
         }
     }

clippy_utils/src/ty.rs

@@ -1168,7 +1168,7 @@ pub fn make_normalized_projection<'tcx>(
 pub struct InteriorMut<'tcx> {
     ignored_def_ids: FxHashSet<DefId>,
     ignore_pointers: bool,
-    tys: FxHashMap<Ty<'tcx>, Option<bool>>,
+    tys: FxHashMap<Ty<'tcx>, Option<&'tcx ty::List<Ty<'tcx>>>>,
 }
 
 impl<'tcx> InteriorMut<'tcx> {
@@ -1194,25 +1194,24 @@ impl<'tcx> InteriorMut<'tcx> {
         }
     }
 
-    /// Check if given type has inner mutability such as [`std::cell::Cell`] or
-    /// [`std::cell::RefCell`] etc.
-    pub fn is_interior_mut_ty(&mut self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    /// Check if given type has interior mutability such as [`std::cell::Cell`] or
+    /// [`std::cell::RefCell`] etc. and if it does, returns a chain of types that causes
+    /// this type to be interior mutable
+    pub fn interior_mut_ty_chain(&mut self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'tcx ty::List<Ty<'tcx>>> {
         match self.tys.entry(ty) {
-            Entry::Occupied(o) => return *o.get() == Some(true),
+            Entry::Occupied(o) => return *o.get(),
             // Temporarily insert a `None` to break cycles
             Entry::Vacant(v) => v.insert(None),
         };
 
-        let interior_mut = match *ty.kind() {
-            ty::RawPtr(inner_ty, _) if !self.ignore_pointers => self.is_interior_mut_ty(cx, inner_ty),
-            ty::Ref(_, inner_ty, _) | ty::Slice(inner_ty) => self.is_interior_mut_ty(cx, inner_ty),
-            ty::Array(inner_ty, size) => {
-                size.try_eval_target_usize(cx.tcx, cx.param_env)
-                    .map_or(true, |u| u != 0)
-                    && self.is_interior_mut_ty(cx, inner_ty)
+        let chain = match *ty.kind() {
+            ty::RawPtr(inner_ty, _) if !self.ignore_pointers => self.interior_mut_ty_chain(cx, inner_ty),
+            ty::Ref(_, inner_ty, _) | ty::Slice(inner_ty) => self.interior_mut_ty_chain(cx, inner_ty),
+            ty::Array(inner_ty, size) if size.try_eval_target_usize(cx.tcx, cx.param_env) != Some(0) => {
+                self.interior_mut_ty_chain(cx, inner_ty)
             },
-            ty::Tuple(fields) => fields.iter().any(|ty| self.is_interior_mut_ty(cx, ty)),
-            ty::Adt(def, _) if def.is_unsafe_cell() => true,
+            ty::Tuple(fields) => fields.iter().find_map(|ty| self.interior_mut_ty_chain(cx, ty)),
+            ty::Adt(def, _) if def.is_unsafe_cell() => Some(ty::List::empty()),
             ty::Adt(def, args) => {
                 let is_std_collection = matches!(
                     cx.tcx.get_diagnostic_name(def.did()),
@@ -1231,19 +1230,28 @@ impl<'tcx> InteriorMut<'tcx> {
 
                 if is_std_collection || def.is_box() {
                     // Include the types from std collections that are behind pointers internally
-                    args.types().any(|ty| self.is_interior_mut_ty(cx, ty))
+                    args.types().find_map(|ty| self.interior_mut_ty_chain(cx, ty))
                 } else if self.ignored_def_ids.contains(&def.did()) || def.is_phantom_data() {
-                    false
+                    None
                 } else {
                     def.all_fields()
-                        .any(|f| self.is_interior_mut_ty(cx, f.ty(cx.tcx, args)))
+                        .find_map(|f| self.interior_mut_ty_chain(cx, f.ty(cx.tcx, args)))
                 }
             },
-            _ => false,
+            _ => None,
         };
 
-        self.tys.insert(ty, Some(interior_mut));
-        interior_mut
+        chain.map(|chain| {
+            let list = cx.tcx.mk_type_list_from_iter(chain.iter().chain([ty]));
+            self.tys.insert(ty, Some(list));
+            list
+        })
+    }
+
+    /// Check if given type has interior mutability such as [`std::cell::Cell`] or
+    /// [`std::cell::RefCell`] etc.
+    pub fn is_interior_mut_ty(&mut self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+        self.interior_mut_ty_chain(cx, ty).is_some()
     }
 }
 

tests/ui/mut_key.stderr

@@ -4,6 +4,9 @@ error: mutable key type
 LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^
    |
+   = note: ... because it contains `Key`, which has interior mutability
+   = note: ... because it contains `AtomicUsize`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
    = note: `-D clippy::mutable-key-type` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::mutable_key_type)]`
 
@@ -12,84 +15,141 @@ error: mutable key type
    |
 LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
    |                                                                        ^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Key`, which has interior mutability
+   = note: ... because it contains `AtomicUsize`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:35:5
    |
 LL |     let _other: HashMap<Key, bool> = HashMap::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Key`, which has interior mutability
+   = note: ... because it contains `AtomicUsize`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:63:22
    |
 LL | fn tuples_bad<U>(_m: &mut HashMap<(Key, U), bool>) {}
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `(Key, U)`, which has interior mutability
+   = note: ... because it contains `Key`, which has interior mutability
+   = note: ... because it contains `AtomicUsize`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:76:5
    |
 LL |     let _map = HashMap::<Cell<usize>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:78:5
    |
 LL |     let _map = HashMap::<&mut Cell<usize>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `&mut Cell<usize>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:81:5
    |
 LL |     let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Vec<Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:83:5
    |
 LL |     let _map = HashMap::<BTreeMap<Cell<usize>, ()>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `BTreeMap<Cell<usize>, ()>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:85:5
    |
 LL |     let _map = HashMap::<BTreeMap<(), Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `BTreeMap<(), Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:87:5
    |
 LL |     let _map = HashMap::<BTreeSet<Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `BTreeSet<Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:89:5
    |
 LL |     let _map = HashMap::<Option<Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Option<Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:91:5
    |
 LL |     let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Option<Vec<Cell<usize>>>`, which has interior mutability
+   = note: ... because it contains `Vec<Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:94:5
    |
 LL |     let _map = HashMap::<Box<Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Box<Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:96:5
    |
 LL |     let _map = HashMap::<Rc<Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Rc<Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: mutable key type
   --> tests/ui/mut_key.rs:98:5
    |
 LL |     let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ... because it contains `Arc<Cell<usize>>`, which has interior mutability
+   = note: ... because it contains `Cell<usize>`, which has interior mutability
+   = note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
 
 error: aborting due to 15 previous errors