make arc-likes for mutable-key configurable

llogiq · llogiq · commit 1bd11c1451cc · 2022-10-22T15:10:41.000+02:00
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"]`
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -734,7 +734,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 arc_likes = conf.arc_like_types.clone();
+    store.register_late_pass(move |_| Box::new(mut_key::MutableKeyType::new(arc_likes.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()));
diff --git a/clippy_lints/src/mut_key.rs b/clippy_lints/src/mut_key.rs
@@ -1,10 +1,10 @@
 use clippy_utils::diagnostics::span_lint;
-use clippy_utils::trait_ref_of_method;
+use clippy_utils::{match_any_def_paths_iter, trait_ref_of_method};
 use rustc_hir as hir;
 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 +78,116 @@ declare_clippy_lint! {
     "Check for mutable `Map`/`Set` key type"
 }
 
-declare_lint_pass!(MutableKeyType => [ MUTABLE_KEY_TYPE ]);
+#[derive(Clone)]
+pub struct MutableKeyType {
+    arc_likes: Vec<String>,
+}
+
+impl_lint_pass!(MutableKeyType => [ MUTABLE_KEY_TYPE ]);
 
 impl<'tcx> LateLintPass<'tcx> for MutableKeyType {
     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(arc_likes: Vec<String>) -> Self {
+        Self { arc_likes }
     }
-    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 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 || self.is_arc_like(cx, def.did()) {
+                    // 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,
+        }
+    }
+
+    fn is_arc_like(&self, cx: &LateContext<'_>, did: hir::def_id::DefId) -> bool {
+        !self.arc_likes.is_empty()
+            && match_any_def_paths_iter(cx, did, self.arc_likes.iter().map(|s| s.split("::"))).is_some()
     }
 }
diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -387,6 +387,11 @@ define_Conf! {
     ///
     /// 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
+    (arc_like_types: Vec<String> = Vec::new()),
 }
 
 /// Search for the configuration file.
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -1811,6 +1811,22 @@ pub fn match_any_def_paths(cx: &LateContext<'_>, did: DefId, paths: &[&[&str]])
         .position(|p| p.iter().map(|x| Symbol::intern(x)).eq(search_path.iter().copied()))
 }
 
+/// The same as [`match_any_def_paths`], but for iterators over path-like things.
+/// Useful e.g. with `str::split`
+pub fn match_any_def_paths_iter<'a, P>(
+    cx: &LateContext<'_>,
+    did: DefId,
+    paths: impl IntoIterator<Item = P> + 'a,
+) -> Option<usize>
+where
+    P: IntoIterator<Item = &'a str>,
+{
+    let search_path = cx.get_def_path(did);
+    paths
+        .into_iter()
+        .position(|p| p.into_iter().map(Symbol::intern).eq(search_path.iter().copied()))
+}
+
 /// Checks if the given `DefId` matches the path.
 pub fn match_def_path<'tcx>(cx: &LateContext<'tcx>, did: DefId, syms: &[&str]) -> bool {
     // We should probably move to Symbols in Clippy as well rather than interning every time.
diff --git a/tests/ui-toml/mut_key/clippy.toml b/tests/ui-toml/mut_key/clippy.toml
@@ -0,0 +1 @@
+arc-like-types = ["mut_key::Counted"]
diff --git a/tests/ui-toml/mut_key/mut_key.rs b/tests/ui-toml/mut_key/mut_key.rs
@@ -0,0 +1,51 @@
+#![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());
+}
diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
@@ -3,6 +3,7 @@ error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown fie
            allow-expect-in-tests
            allow-unwrap-in-tests
            allowed-scripts
+           arc-like-types
            arithmetic-side-effects-allowed
            array-size-threshold
            avoid-breaking-exported-api
