Auto merge of rust-lang#9692 - llogiq:mutable-key-more-arcs, r=Alexendoo

make ignored internally mutable types for `mutable-key` configurable

We had some false positives where people would create their own types that had interior mutability unrelated to hash/eq. This addition lets you configure this as e.g. `arc-like-types=["bytes::Bytes"]`

This fixes rust-lang#5325 by allowing users to specify the types whose innards like `Arc` should be ignored (the generic types are still checked) for the sake of detecting inner mutability.

r? `@Alexendoo`

---

changelog: Allow configuring types to ignore internal mutability in `mutable-key`

Author: bors

clippy_lints/src/lib.rs

@@ -727,7 +727,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let max_trait_bounds = conf.max_trait_bounds;
     store.register_late_pass(move |_| Box::new(trait_bounds::TraitBounds::new(max_trait_bounds)));
     store.register_late_pass(|_| Box::new(comparison_chain::ComparisonChain));
-    store.register_late_pass(|_| Box::new(mut_key::MutableKeyType));
+    let ignore_interior_mutability = conf.ignore_interior_mutability.clone();
+    store.register_late_pass(move |_| Box::new(mut_key::MutableKeyType::new(ignore_interior_mutability.clone())));
     store.register_early_pass(|| Box::new(reference::DerefAddrOf));
     store.register_early_pass(|| Box::new(double_parens::DoubleParens));
     store.register_late_pass(|_| Box::new(format_impl::FormatImpl::new()));

clippy_lints/src/mut_key.rs

@@ -1,10 +1,12 @@
 use clippy_utils::diagnostics::span_lint;
-use clippy_utils::trait_ref_of_method;
+use clippy_utils::{def_path_res, trait_ref_of_method};
+use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
+use rustc_hir::def::Namespace;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::TypeVisitable;
 use rustc_middle::ty::{Adt, Array, Ref, Slice, Tuple, Ty};
-use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::Span;
 use rustc_span::symbol::sym;
 use std::iter;
@@ -78,98 +80,128 @@ declare_clippy_lint! {
     "Check for mutable `Map`/`Set` key type"
 }
 
-declare_lint_pass!(MutableKeyType => [ MUTABLE_KEY_TYPE ]);
+#[derive(Clone)]
+pub struct MutableKeyType {
+    ignore_interior_mutability: Vec<String>,
+    ignore_mut_def_ids: FxHashSet<hir::def_id::DefId>,
+}
+
+impl_lint_pass!(MutableKeyType => [ MUTABLE_KEY_TYPE ]);
 
 impl<'tcx> LateLintPass<'tcx> for MutableKeyType {
+    fn check_crate(&mut self, cx: &LateContext<'tcx>) {
+        self.ignore_mut_def_ids.clear();
+        let mut path = Vec::new();
+        for ty in &self.ignore_interior_mutability {
+            path.extend(ty.split("::"));
+            if let Some(id) = def_path_res(cx, &path[..], Some(Namespace::TypeNS)).opt_def_id() {
+                self.ignore_mut_def_ids.insert(id);
+            }
+            path.clear();
+        }
+    }
+
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
         if let hir::ItemKind::Fn(ref sig, ..) = item.kind {
-            check_sig(cx, item.hir_id(), sig.decl);
+            self.check_sig(cx, item.hir_id(), sig.decl);
         }
     }
 
     fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'tcx>) {
         if let hir::ImplItemKind::Fn(ref sig, ..) = item.kind {
             if trait_ref_of_method(cx, item.def_id.def_id).is_none() {
-                check_sig(cx, item.hir_id(), sig.decl);
+                self.check_sig(cx, item.hir_id(), sig.decl);
             }
         }
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'tcx>) {
         if let hir::TraitItemKind::Fn(ref sig, ..) = item.kind {
-            check_sig(cx, item.hir_id(), sig.decl);
+            self.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.typeck_results().pat_ty(local.pat));
+        self.check_ty_(cx, local.span, cx.typeck_results().pat_ty(local.pat));
     }
 }
 
-fn check_sig<'tcx>(cx: &LateContext<'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 iter::zip(decl.inputs, fn_sig.inputs().skip_binder()) {
-        check_ty(cx, hir_ty.span, *ty);
+impl MutableKeyType {
+    pub fn new(ignore_interior_mutability: Vec<String>) -> Self {
+        Self {
+            ignore_interior_mutability,
+            ignore_mut_def_ids: FxHashSet::default(),
+        }
     }
-    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<'tcx>(cx: &LateContext<'tcx>, span: Span, ty: Ty<'tcx>) {
-    let ty = ty.peel_refs();
-    if let Adt(def, substs) = ty.kind() {
-        let is_keyed_type = [sym::HashMap, sym::BTreeMap, sym::HashSet, sym::BTreeSet]
-            .iter()
-            .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 check_sig<'tcx>(&self, cx: &LateContext<'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 iter::zip(decl.inputs, fn_sig.inputs().skip_binder()) {
+            self.check_ty_(cx, hir_ty.span, *ty);
         }
+        self.check_ty_(cx, decl.output.span(), cx.tcx.erase_late_bound_regions(fn_sig.output()));
     }
-}
 
-/// 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() {
-        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_interior_mutable_type(cx, inner_ty, span)
-        },
-        Tuple(fields) => fields.iter().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,
-                sym::Result,
-                sym::LinkedList,
-                sym::Vec,
-                sym::VecDeque,
-                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)
+    // 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_<'tcx>(&self, cx: &LateContext<'tcx>, span: Span, ty: Ty<'tcx>) {
+        let ty = ty.peel_refs();
+        if let Adt(def, substs) = ty.kind() {
+            let is_keyed_type = [sym::HashMap, sym::BTreeMap, sym::HashSet, sym::BTreeSet]
+                .iter()
+                .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def.did()));
+            if is_keyed_type && self.is_interior_mutable_type(cx, substs.type_at(0), span) {
+                span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type");
             }
-        },
-        _ => false,
+        }
+    }
+
+    /// Determines if a type contains interior mutability which would affect its implementation of
+    /// [`Hash`] or [`Ord`].
+    fn is_interior_mutable_type<'tcx>(&self, cx: &LateContext<'tcx>, ty: Ty<'tcx>, span: Span) -> bool {
+        match *ty.kind() {
+            Ref(_, inner_ty, mutbl) => {
+                mutbl == hir::Mutability::Mut || self.is_interior_mutable_type(cx, inner_ty, span)
+            },
+            Slice(inner_ty) => self.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)
+                    && self.is_interior_mutable_type(cx, inner_ty, span)
+            },
+            Tuple(fields) => fields.iter().any(|ty| self.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 def_id = def.did();
+                let is_std_collection = [
+                    sym::Option,
+                    sym::Result,
+                    sym::LinkedList,
+                    sym::Vec,
+                    sym::VecDeque,
+                    sym::BTreeMap,
+                    sym::BTreeSet,
+                    sym::Rc,
+                    sym::Arc,
+                ]
+                .iter()
+                .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def_id));
+                let is_box = Some(def_id) == cx.tcx.lang_items().owned_box();
+                if is_std_collection || is_box || self.ignore_mut_def_ids.contains(&def_id) {
+                    // The type is mutable if any of its type parameters are
+                    substs.types().any(|ty| self.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,
+        }
     }
 }

clippy_lints/src/utils/conf.rs

@@ -389,10 +389,15 @@ define_Conf! {
     ///
     /// Whether `dbg!` should be allowed in test functions
     (allow_dbg_in_tests: bool = false),
-    /// Lint: RESULT_LARGE_ERR
+    /// Lint: RESULT_LARGE_ERR.
     ///
     /// The maximum size of the `Err`-variant in a `Result` returned from a function
     (large_error_threshold: u64 = 128),
+    /// Lint: MUTABLE_KEY.
+    ///
+    /// A list of paths to types that should be treated like `Arc`, i.e. ignored but
+    /// for the generic parameters for determining interior mutability
+    (ignore_interior_mutability: Vec<String> = Vec::from(["bytes::Bytes".into()])),
 }
 
 /// Search for the configuration file.

clippy_utils/src/lib.rs

@@ -598,7 +598,13 @@ pub fn def_path_res(cx: &LateContext<'_>, path: &[&str], namespace_hint: Option<
         [primitive] => {
             return PrimTy::from_name(Symbol::intern(primitive)).map_or(Res::Err, Res::PrimTy);
         },
-        [base, ref path @ ..] => (base, path),
+        [base, ref path @ ..] => {
+            let crate_name = cx.sess().opts.crate_name.as_deref();
+            if Some(base) == crate_name {
+                return def_path_res_local(cx, path);
+            }
+            (base, path)
+        },
         _ => return Res::Err,
     };
     let tcx = cx.tcx;
@@ -642,7 +648,41 @@ pub fn def_path_res(cx: &LateContext<'_>, path: &[&str], namespace_hint: Option<
             return last;
         }
     }
+    Res::Err
+}
 
+fn def_path_res_local(cx: &LateContext<'_>, mut path: &[&str]) -> Res {
+    let map = cx.tcx.hir();
+    let mut ids = map.root_module().item_ids;
+    while let Some(&segment) = path.first() {
+        let mut next_ids = None;
+        for i in ids {
+            if let Some(Node::Item(hir::Item {
+                ident,
+                kind,
+                def_id: item_def_id,
+                ..
+            })) = map.find(i.hir_id())
+            {
+                if ident.name.as_str() == segment {
+                    path = &path[1..];
+                    if path.is_empty() {
+                        let def_id = item_def_id.to_def_id();
+                        return Res::Def(cx.tcx.def_kind(def_id), def_id);
+                    }
+                    if let ItemKind::Mod(m) = kind {
+                        next_ids = Some(m.item_ids);
+                    };
+                    break;
+                }
+            }
+        }
+        if let Some(next_ids) = next_ids {
+            ids = next_ids;
+        } else {
+            break;
+        }
+    }
     Res::Err
 }
 

tests/ui-toml/mut_key/clippy.toml

@@ -0,0 +1 @@
+ignore-interior-mutability = ["mut_key::Counted"]

tests/ui-toml/mut_key/mut_key.rs

@@ -0,0 +1,53 @@
+// compile-flags: --crate-name mut_key
+
+#![warn(clippy::mutable_key_type)]
+
+use std::cmp::{Eq, PartialEq};
+use std::collections::{HashMap, HashSet};
+use std::hash::{Hash, Hasher};
+use std::ops::Deref;
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+struct Counted<T> {
+    count: AtomicUsize,
+    val: T,
+}
+
+impl<T: Clone> Clone for Counted<T> {
+    fn clone(&self) -> Self {
+        Self {
+            count: AtomicUsize::new(0),
+            val: self.val.clone(),
+        }
+    }
+}
+
+impl<T: PartialEq> PartialEq for Counted<T> {
+    fn eq(&self, other: &Self) -> bool {
+        self.val == other.val
+    }
+}
+impl<T: PartialEq + Eq> Eq for Counted<T> {}
+
+impl<T: Hash> Hash for Counted<T> {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.val.hash(state);
+    }
+}
+
+impl<T> Deref for Counted<T> {
+    type Target = T;
+
+    fn deref(&self) -> &T {
+        self.count.fetch_add(1, Ordering::AcqRel);
+        &self.val
+    }
+}
+
+// This is not linted because `"mut_key::Counted"` is in
+// `arc_like_types` in `clippy.toml`
+fn should_not_take_this_arg(_v: HashSet<Counted<String>>) {}
+
+fn main() {
+    should_not_take_this_arg(HashSet::new());
+}

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -20,6 +20,7 @@ error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown fie
            enforced-import-renames
            enum-variant-name-threshold
            enum-variant-size-threshold
+           ignore-interior-mutability
            large-error-threshold
            literal-representation-threshold
            matches-for-let-else