Auto merge of #12691 - Alexendoo:ignore-interior-mutability-indirect, r=llogiq

Rework interior mutability detection

Replaces the existing interior mutability detection, the two main changes being

- It now follows references/pointers e.g. `struct S(&Cell)`
  - `mutable_key_type` ignores pointers as it did before
- The `ignore_interior_mutability` config now applies to types containing the ignored type, e.g. `http::HeaderName`

Fixes #7752
Fixes #9776
Fixes #9801

changelog: [`mutable_key_type`], [`declare_interior_mutable_const`]: now considers types that have references to interior mutable types as interior mutable

Author: bors

book/src/lint_configuration.md

@@ -506,13 +506,14 @@ The maximum byte size a `Future` can have, before it triggers the `clippy::large
 
 
 ## `ignore-interior-mutability`
-A list of paths to types that should be treated like `Arc`, i.e. ignored but
-for the generic parameters for determining interior mutability
+A list of paths to types that should be treated as if they do not contain interior mutability
 
 **Default Value:** `["bytes::Bytes"]`
 
 ---
 **Affected lints:**
+* [`borrow_interior_mutable_const`](https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const)
+* [`declare_interior_mutable_const`](https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const)
 * [`ifs_same_cond`](https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond)
 * [`mutable_key_type`](https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type)
 

clippy_config/src/conf.rs

@@ -467,10 +467,9 @@ define_Conf! {
     ///
     /// The maximum size of the `Err`-variant in a `Result` returned from a function
     (large_error_threshold: u64 = 128),
-    /// Lint: MUTABLE_KEY_TYPE, IFS_SAME_COND.
+    /// Lint: MUTABLE_KEY_TYPE, IFS_SAME_COND, BORROW_INTERIOR_MUTABLE_CONST, DECLARE_INTERIOR_MUTABLE_CONST.
     ///
-    /// A list of paths to types that should be treated like `Arc`, i.e. ignored but
-    /// for the generic parameters for determining interior mutability
+    /// A list of paths to types that should be treated as if they do not contain interior mutability
     (ignore_interior_mutability: Vec<String> = Vec::from(["bytes::Bytes".into()])),
     /// Lint: UNINLINED_FORMAT_ARGS.
     ///

clippy_lints/src/copies.rs

@@ -1,15 +1,14 @@
 use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_then};
 use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, snippet, snippet_opt};
-use clippy_utils::ty::{is_interior_mut_ty, needs_ordered_drop};
+use clippy_utils::ty::{needs_ordered_drop, InteriorMut};
 use clippy_utils::visitors::for_each_expr;
 use clippy_utils::{
-    capture_local_usage, def_path_def_ids, eq_expr_value, find_binding_init, get_enclosing_block, hash_expr, hash_stmt,
-    if_sequence, is_else_clause, is_lint_allowed, path_to_local, search_same, ContainsName, HirEqInterExpr, SpanlessEq,
+    capture_local_usage, eq_expr_value, find_binding_init, get_enclosing_block, hash_expr, hash_stmt, if_sequence,
+    is_else_clause, is_lint_allowed, path_to_local, search_same, ContainsName, HirEqInterExpr, SpanlessEq,
 };
 use core::iter;
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
-use rustc_hir::def_id::DefIdSet;
 use rustc_hir::{intravisit, BinOpKind, Block, Expr, ExprKind, HirId, HirIdSet, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
@@ -159,40 +158,36 @@ declare_clippy_lint! {
     "`if` statement with shared code in all blocks"
 }
 
-pub struct CopyAndPaste {
+pub struct CopyAndPaste<'tcx> {
     ignore_interior_mutability: Vec<String>,
-    ignored_ty_ids: DefIdSet,
+    interior_mut: InteriorMut<'tcx>,
 }
 
-impl CopyAndPaste {
+impl CopyAndPaste<'_> {
     pub fn new(ignore_interior_mutability: Vec<String>) -> Self {
         Self {
             ignore_interior_mutability,
-            ignored_ty_ids: DefIdSet::new(),
+            interior_mut: InteriorMut::default(),
         }
     }
 }
 
-impl_lint_pass!(CopyAndPaste => [
+impl_lint_pass!(CopyAndPaste<'_> => [
     IFS_SAME_COND,
     SAME_FUNCTIONS_IN_IF_CONDITION,
     IF_SAME_THEN_ELSE,
     BRANCHES_SHARING_CODE
 ]);
 
-impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
+impl<'tcx> LateLintPass<'tcx> for CopyAndPaste<'tcx> {
     fn check_crate(&mut self, cx: &LateContext<'tcx>) {
-        for ignored_ty in &self.ignore_interior_mutability {
-            let path: Vec<&str> = ignored_ty.split("::").collect();
-            for id in def_path_def_ids(cx, path.as_slice()) {
-                self.ignored_ty_ids.insert(id);
-            }
-        }
+        self.interior_mut = InteriorMut::new(cx, &self.ignore_interior_mutability);
     }
+
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if !expr.span.from_expansion() && matches!(expr.kind, ExprKind::If(..)) && !is_else_clause(cx.tcx, expr) {
             let (conds, blocks) = if_sequence(expr);
-            lint_same_cond(cx, &conds, &self.ignored_ty_ids);
+            lint_same_cond(cx, &conds, &mut self.interior_mut);
             lint_same_fns_in_if_cond(cx, &conds);
             let all_same =
                 !is_lint_allowed(cx, IF_SAME_THEN_ELSE, expr.hir_id) && lint_if_same_then_else(cx, &conds, &blocks);
@@ -570,13 +565,14 @@ fn check_for_warn_of_moved_symbol(cx: &LateContext<'_>, symbols: &[(HirId, Symbo
     })
 }
 
-fn method_caller_is_mutable(cx: &LateContext<'_>, caller_expr: &Expr<'_>, ignored_ty_ids: &DefIdSet) -> bool {
+fn method_caller_is_mutable<'tcx>(
+    cx: &LateContext<'tcx>,
+    caller_expr: &Expr<'_>,
+    interior_mut: &mut InteriorMut<'tcx>,
+) -> bool {
     let caller_ty = cx.typeck_results().expr_ty(caller_expr);
-    // Check if given type has inner mutability and was not set to ignored by the configuration
-    let is_inner_mut_ty = is_interior_mut_ty(cx, caller_ty)
-        && !matches!(caller_ty.ty_adt_def(), Some(adt) if ignored_ty_ids.contains(&adt.did()));
 
-    is_inner_mut_ty
+    interior_mut.is_interior_mut_ty(cx, caller_ty)
         || caller_ty.is_mutable_ptr()
         // `find_binding_init` will return the binding iff its not mutable
         || path_to_local(caller_expr)
@@ -585,15 +581,15 @@ fn method_caller_is_mutable(cx: &LateContext<'_>, caller_expr: &Expr<'_>, ignore
 }
 
 /// Implementation of `IFS_SAME_COND`.
-fn lint_same_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>], ignored_ty_ids: &DefIdSet) {
+fn lint_same_cond<'tcx>(cx: &LateContext<'tcx>, conds: &[&Expr<'_>], interior_mut: &mut InteriorMut<'tcx>) {
     for (i, j) in search_same(
         conds,
         |e| hash_expr(cx, e),
         |lhs, rhs| {
             // Ignore eq_expr side effects iff one of the expression kind is a method call
             // and the caller is not a mutable, including inner mutable type.
             if let ExprKind::MethodCall(_, caller, _, _) = lhs.kind {
-                if method_caller_is_mutable(cx, caller, ignored_ty_ids) {
+                if method_caller_is_mutable(cx, caller, interior_mut) {
                     false
                 } else {
                     SpanlessEq::new(cx).eq_expr(lhs, rhs)

clippy_lints/src/mut_key.rs

@@ -1,7 +1,6 @@
 use clippy_utils::diagnostics::span_lint;
-use clippy_utils::ty::is_interior_mut_ty;
-use clippy_utils::{def_path_def_ids, trait_ref_of_method};
-use rustc_data_structures::fx::FxHashSet;
+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::{self, Ty};
@@ -23,41 +22,28 @@ declare_clippy_lint! {
     /// ### Known problems
     ///
     /// #### False Positives
-    /// It's correct to use a struct that contains interior mutability as a key, when its
+    /// 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.
+    /// these cases.
     ///
     /// #### 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).
+    /// This lint does not follow raw pointers (`*const T` or `*mut T`) as `Hash` and `Ord`
+    /// apply only to the **address** of the contained value. This can cause false negatives for
+    /// custom collections that use raw pointers internally.
     ///
     /// ### Example
     /// ```no_run
     /// use std::cmp::{PartialEq, Eq};
     /// use std::collections::HashSet;
     /// use std::hash::{Hash, Hasher};
     /// use std::sync::atomic::AtomicUsize;
-    ///# #[allow(unused)]
     ///
     /// struct Bad(AtomicUsize);
     /// impl PartialEq for Bad {
     ///     fn eq(&self, rhs: &Self) -> bool {
     ///          ..
-    /// ; unimplemented!();
+    /// # ; true
     ///     }
     /// }
     ///
@@ -66,7 +52,7 @@ declare_clippy_lint! {
     /// impl Hash for Bad {
     ///     fn hash<H: Hasher>(&self, h: &mut H) {
     ///         ..
-    /// ; unimplemented!();
+    /// # ;
     ///     }
     /// }
     ///
@@ -80,25 +66,16 @@ declare_clippy_lint! {
     "Check for mutable `Map`/`Set` key type"
 }
 
-#[derive(Clone)]
-pub struct MutableKeyType {
+pub struct MutableKeyType<'tcx> {
     ignore_interior_mutability: Vec<String>,
-    ignore_mut_def_ids: FxHashSet<hir::def_id::DefId>,
+    interior_mut: InteriorMut<'tcx>,
 }
 
-impl_lint_pass!(MutableKeyType => [ MUTABLE_KEY_TYPE ]);
+impl_lint_pass!(MutableKeyType<'_> => [ MUTABLE_KEY_TYPE ]);
 
-impl<'tcx> LateLintPass<'tcx> for MutableKeyType {
+impl<'tcx> LateLintPass<'tcx> for MutableKeyType<'tcx> {
     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("::"));
-            for id in def_path_def_ids(cx, &path[..]) {
-                self.ignore_mut_def_ids.insert(id);
-            }
-            path.clear();
-        }
+        self.interior_mut = InteriorMut::without_pointers(cx, &self.ignore_interior_mutability);
     }
 
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
@@ -121,23 +98,23 @@ impl<'tcx> LateLintPass<'tcx> for MutableKeyType {
         }
     }
 
-    fn check_local(&mut self, cx: &LateContext<'_>, local: &hir::LetStmt<'_>) {
+    fn check_local(&mut self, cx: &LateContext<'tcx>, local: &hir::LetStmt<'tcx>) {
         if let hir::PatKind::Wild = local.pat.kind {
             return;
         }
         self.check_ty_(cx, local.span, cx.typeck_results().pat_ty(local.pat));
     }
 }
 
-impl MutableKeyType {
+impl<'tcx> MutableKeyType<'tcx> {
     pub fn new(ignore_interior_mutability: Vec<String>) -> Self {
         Self {
             ignore_interior_mutability,
-            ignore_mut_def_ids: FxHashSet::default(),
+            interior_mut: InteriorMut::default(),
         }
     }
 
-    fn check_sig(&self, cx: &LateContext<'_>, fn_def_id: LocalDefId, decl: &hir::FnDecl<'_>) {
+    fn check_sig(&mut self, cx: &LateContext<'tcx>, fn_def_id: LocalDefId, decl: &hir::FnDecl<'tcx>) {
         let fn_sig = cx.tcx.fn_sig(fn_def_id).instantiate_identity();
         for (hir_ty, ty) in iter::zip(decl.inputs, fn_sig.inputs().skip_binder()) {
             self.check_ty_(cx, hir_ty.span, *ty);
@@ -151,7 +128,7 @@ impl MutableKeyType {
 
     // 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>) {
+    fn check_ty_(&mut self, cx: &LateContext<'tcx>, span: Span, ty: Ty<'tcx>) {
         let ty = ty.peel_refs();
         if let ty::Adt(def, args) = ty.kind() {
             let is_keyed_type = [sym::HashMap, sym::BTreeMap, sym::HashSet, sym::BTreeSet]
@@ -162,11 +139,7 @@ impl MutableKeyType {
             }
 
             let subst_ty = args.type_at(0);
-            // Determines if a type contains interior mutability which would affect its implementation of
-            // [`Hash`] or [`Ord`].
-            if is_interior_mut_ty(cx, subst_ty)
-                && !matches!(subst_ty.ty_adt_def(), Some(adt) if self.ignore_mut_def_ids.contains(&adt.did()))
-            {
+            if self.interior_mut.is_interior_mut_ty(cx, subst_ty) {
                 span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type");
             }
         }