Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework interior mutability detection #12691

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
5 changes: 2 additions & 3 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down
44 changes: 20 additions & 24 deletions clippy_lints/src/copies.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
67 changes: 20 additions & 47 deletions clippy_lints/src/mut_key.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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
/// }
/// }
///
Expand All @@ -66,7 +52,7 @@ declare_clippy_lint! {
/// impl Hash for Bad {
/// fn hash<H: Hasher>(&self, h: &mut H) {
/// ..
/// ; unimplemented!();
/// # ;
/// }
/// }
///
Expand All @@ -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>) {
Expand All @@ -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);
Expand All @@ -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]
Expand All @@ -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");
}
}
Expand Down
Loading
Loading