make arc-likes for mutable-key configurable

llogiq · llogiq · commit ab02459241f4 · 2022-10-25T00:57:16.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
@@ -735,7 +735,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()));
diff --git a/clippy_lints/src/mut_key.rs b/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,
+        }
     }
 }
diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -383,10 +383,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.
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -586,14 +586,19 @@ pub fn def_path_res(cx: &LateContext<'_>, path: &[&str], namespace_hint: Option<
         }
     }
 
-    fn find_crate(tcx: TyCtxt<'_>, name: &str) -> Option<DefId> {
-        tcx.crates(())
+    fn find_crate(cx: &LateContext<'_>, name: &str) -> Option<DefId> {
+        cx.tcx
+            .crates(())
             .iter()
             .copied()
-            .find(|&num| tcx.crate_name(num).as_str() == name)
+            .find(|&num| cx.tcx.crate_name(num).as_str() == name)
             .map(CrateNum::as_def_id)
     }
 
+    if path.first() == Some(&"crate") || path.first().copied() == cx.sess().opts.crate_name.as_deref() {
+        return def_path_res_local(cx, &path[1..]);
+    }
+
     let (base, path) = match *path {
         [primitive] => {
             return PrimTy::from_name(Symbol::intern(primitive)).map_or(Res::Err, Res::PrimTy);
@@ -603,7 +608,7 @@ pub fn def_path_res(cx: &LateContext<'_>, path: &[&str], namespace_hint: Option<
     };
     let tcx = cx.tcx;
     let starts = find_primitive(tcx, base)
-        .chain(find_crate(tcx, base))
+        .chain(find_crate(cx, base))
         .map(|id| Res::Def(tcx.def_kind(id), id));
 
     for first in starts {
@@ -642,7 +647,60 @@ 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..];
+                    let def_kind = match kind {
+                        ItemKind::Mod(m) => {
+                            next_ids = Some(m.item_ids);
+                            DefKind::Mod
+                        },
+                        ItemKind::Struct(..) => DefKind::Struct,
+                        ItemKind::Enum(..) => DefKind::Enum,
+                        ItemKind::ExternCrate(_) => DefKind::ExternCrate,
+                        ItemKind::Use(_, _) => DefKind::Use,
+                        ItemKind::Static(_, hir::Mutability::Mut, _) => DefKind::Static(ast::Mutability::Mut),
+                        ItemKind::Static(_, hir::Mutability::Not, _) => DefKind::Static(ast::Mutability::Not),
+                        ItemKind::Const(_, _) => DefKind::Const,
+                        ItemKind::Fn(_, _, _) => DefKind::Fn,
+                        ItemKind::Macro(_, mkind) => DefKind::Macro(*mkind),
+                        ItemKind::ForeignMod { .. } => DefKind::ForeignMod,
+                        ItemKind::GlobalAsm(_) => DefKind::GlobalAsm,
+                        ItemKind::TyAlias(_, _) => DefKind::TyAlias,
+                        ItemKind::OpaqueTy(_) => DefKind::OpaqueTy,
+                        ItemKind::Union(_, _) => DefKind::Union,
+                        ItemKind::Trait(_, _, _, _, _) => DefKind::Trait,
+                        ItemKind::TraitAlias(_, _) => DefKind::TraitAlias,
+                        ItemKind::Impl(_) => DefKind::Impl,
+                    };
+                    if path.is_empty() {
+                        return Res::Def(def_kind, item_def_id.to_def_id());
+                    }
+                    break;
+                }
+            }
+        }
+        if let Some(next_ids) = next_ids {
+            ids = next_ids;
+        } else {
+            break;
+        }
+    }
     Res::Err
 }
 
@@ -1811,6 +1869,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 @@
+ignore-interior-mutability = ["crate::Counted"]
diff --git a/tests/ui-toml/mut_key/mut_key.rs b/tests/ui-toml/mut_key/mut_key.rs
diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+ignore-interior-mutability = ["crate::Counted"]`