mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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 https://github.com/rust-lang/rust-clippy/issues/7752
Fixes https://github.com/rust-lang/rust-clippy/issues/9776
Fixes https://github.com/rust-lang/rust-clippy/issues/9801

changelog: [`mutable_key_type`], [`declare_interior_mutable_const`]: now considers types that have references to interior mutable types as interior mutable
This commit is contained in:
bors 2024-04-20 13:54:55 +00:00
commit 8eafeeb47a
15 changed files with 225 additions and 223 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
book/src/lint_configuration.md
View File

@ -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)

5
clippy_config/src/conf.rs
View File

@ -467,10 +467,9 @@ pub fn get_configuration_metadata() -> Vec<ClippyConfiguration> {
///
/// 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.
///

44
clippy_lints/src/copies.rs
View File

@ -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 @@
"`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,7 +581,7 @@ 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),
@ -593,7 +589,7 @@ fn lint_same_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>], ignored_ty_ids: &De
// 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)

67
clippy_lints/src/mut_key.rs
View File

@ -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,27 +22,15 @@
/// ### 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
@ -51,13 +38,12 @@
/// 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 @@
/// impl Hash for Bad {
/// fn hash<H: Hasher>(&self, h: &mut H) {
/// ..
/// ; unimplemented!();
/// # ;
/// }
/// }
///
@ -80,25 +66,16 @@
"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,7 +98,7 @@ fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitIte
}
}
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;
}
@ -129,15 +106,15 @@ fn check_local(&mut self, cx: &LateContext<'_>, local: &hir::LetStmt<'_>) {
}
}
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 @@ fn check_sig(&self, cx: &LateContext<'_>, fn_def_id: LocalDefId, decl: &hir::FnD
// 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 @@ fn check_ty_<'tcx>(&self, cx: &LateContext<'tcx>, span: Span, ty: Ty<'tcx>) {
}
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");
}
}

99
clippy_lints/src/non_copy_const.rs
View File

@ -5,9 +5,9 @@
use std::ptr;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::in_constant;
use clippy_utils::macros::macro_backtrace;
use clippy_utils::{def_path_def_ids, in_constant};
use rustc_data_structures::fx::FxHashSet;
use clippy_utils::ty::InteriorMut;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::{
@ -52,8 +52,8 @@
/// There're other enums plus associated constants cases that the lint cannot handle.
///
/// Types that have underlying or potential interior mutability trigger the lint whether
/// the interior mutable field is used or not. See issues
/// [#5812](https://github.com/rust-lang/rust-clippy/issues/5812) and
/// the interior mutable field is used or not. See issue
/// [#5812](https://github.com/rust-lang/rust-clippy/issues/5812)
///
/// ### Example
/// ```no_run
@ -170,42 +170,22 @@ fn lint(cx: &LateContext<'_>, source: Source) {
});
}
#[derive(Clone)]
pub struct NonCopyConst {
pub struct NonCopyConst<'tcx> {
ignore_interior_mutability: Vec<String>,
ignore_mut_def_ids: FxHashSet<DefId>,
interior_mut: InteriorMut<'tcx>,
}
impl_lint_pass!(NonCopyConst => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST]);
impl_lint_pass!(NonCopyConst<'_> => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST]);
impl NonCopyConst {
impl<'tcx> NonCopyConst<'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 is_ty_ignored(&self, ty: Ty<'_>) -> bool {
matches!(ty.ty_adt_def(), Some(adt) if self.ignore_mut_def_ids.contains(&adt.did()))
}
fn is_unfrozen<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
// Ignore types whose layout is unknown since `is_freeze` reports every generic types as `!Freeze`,
// making it indistinguishable from `UnsafeCell`. i.e. it isn't a tool to prove a type is
// 'unfrozen'. However, this code causes a false negative in which
// a type contains a layout-unknown type, but also an unsafe cell like `const CELL: Cell<T>`.
// Yet, it's better than `ty.has_type_flags(TypeFlags::HAS_TY_PARAM | TypeFlags::HAS_PROJECTION)`
// since it works when a pointer indirection involves (`Cell<*const T>`).
// Making up a `ParamEnv` where every generic params and assoc types are `Freeze`is another option;
// but I'm not sure whether it's a decent way, if possible.
cx.tcx.layout_of(cx.param_env.and(ty)).is_ok() && !ty.is_freeze(cx.tcx, cx.param_env)
}
fn is_value_unfrozen_raw_inner<'tcx>(&self, cx: &LateContext<'tcx>, val: ty::ValTree<'tcx>, ty: Ty<'tcx>) -> bool {
if self.is_ty_ignored(ty) {
return false;
}
fn is_value_unfrozen_raw_inner(cx: &LateContext<'tcx>, val: ty::ValTree<'tcx>, ty: Ty<'tcx>) -> bool {
match *ty.kind() {
// the fact that we have to dig into every structs to search enums
// leads us to the point checking `UnsafeCell` directly is the only option.
@ -216,8 +196,7 @@ fn is_value_unfrozen_raw_inner<'tcx>(&self, cx: &LateContext<'tcx>, val: ty::Val
ty::Array(ty, _) => val
.unwrap_branch()
.iter()
.any(|field| self.is_value_unfrozen_raw_inner(cx, *field, ty)),
ty::Adt(def, _) if def.is_union() => false,
.any(|field| Self::is_value_unfrozen_raw_inner(cx, *field, ty)),
ty::Adt(def, args) if def.is_enum() => {
let (&variant_index, fields) = val.unwrap_branch().split_first().unwrap();
let variant_index = VariantIdx::from_u32(variant_index.unwrap_leaf().try_to_u32().ok().unwrap());
@ -230,24 +209,23 @@ fn is_value_unfrozen_raw_inner<'tcx>(&self, cx: &LateContext<'tcx>, val: ty::Val
.iter()
.map(|field| field.ty(cx.tcx, args)),
)
.any(|(field, ty)| self.is_value_unfrozen_raw_inner(cx, field, ty))
.any(|(field, ty)| Self::is_value_unfrozen_raw_inner(cx, field, ty))
},
ty::Adt(def, args) => val
.unwrap_branch()
.iter()
.zip(def.non_enum_variant().fields.iter().map(|field| field.ty(cx.tcx, args)))
.any(|(field, ty)| self.is_value_unfrozen_raw_inner(cx, *field, ty)),
.any(|(field, ty)| Self::is_value_unfrozen_raw_inner(cx, *field, ty)),
ty::Tuple(tys) => val
.unwrap_branch()
.iter()
.zip(tys)
.any(|(field, ty)| self.is_value_unfrozen_raw_inner(cx, *field, ty)),
.any(|(field, ty)| Self::is_value_unfrozen_raw_inner(cx, *field, ty)),
_ => false,
}
}
fn is_value_unfrozen_raw<'tcx>(
&self,
fn is_value_unfrozen_raw(
cx: &LateContext<'tcx>,
result: Result<Option<ty::ValTree<'tcx>>, ErrorHandled>,
ty: Ty<'tcx>,
@ -277,11 +255,11 @@ fn is_value_unfrozen_raw<'tcx>(
// I chose this way because unfrozen enums as assoc consts are rare (or, hopefully, none).
matches!(err, ErrorHandled::TooGeneric(..))
},
|val| val.map_or(true, |val| self.is_value_unfrozen_raw_inner(cx, val, ty)),
|val| val.map_or(true, |val| Self::is_value_unfrozen_raw_inner(cx, val, ty)),
)
}
fn is_value_unfrozen_poly<'tcx>(&self, cx: &LateContext<'tcx>, body_id: BodyId, ty: Ty<'tcx>) -> bool {
fn is_value_unfrozen_poly(cx: &LateContext<'tcx>, body_id: BodyId, ty: Ty<'tcx>) -> bool {
let def_id = body_id.hir_id.owner.to_def_id();
let args = ty::GenericArgs::identity_for_item(cx.tcx, def_id);
let instance = ty::Instance::new(def_id, args);
@ -291,17 +269,17 @@ fn is_value_unfrozen_poly<'tcx>(&self, cx: &LateContext<'tcx>, body_id: BodyId,
};
let param_env = cx.tcx.param_env(def_id).with_reveal_all_normalized(cx.tcx);
let result = cx.tcx.const_eval_global_id_for_typeck(param_env, cid, DUMMY_SP);
self.is_value_unfrozen_raw(cx, result, ty)
Self::is_value_unfrozen_raw(cx, result, ty)
}
fn is_value_unfrozen_expr<'tcx>(&self, cx: &LateContext<'tcx>, hir_id: HirId, def_id: DefId, ty: Ty<'tcx>) -> bool {
fn is_value_unfrozen_expr(cx: &LateContext<'tcx>, hir_id: HirId, def_id: DefId, ty: Ty<'tcx>) -> bool {
let args = cx.typeck_results().node_args(hir_id);
let result = Self::const_eval_resolve(cx.tcx, cx.param_env, ty::UnevaluatedConst::new(def_id, args), DUMMY_SP);
self.is_value_unfrozen_raw(cx, result, ty)
Self::is_value_unfrozen_raw(cx, result, ty)
}
pub fn const_eval_resolve<'tcx>(
pub fn const_eval_resolve(
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
ct: ty::UnevaluatedConst<'tcx>,
@ -321,26 +299,17 @@ pub fn const_eval_resolve<'tcx>(
}
}
impl<'tcx> LateLintPass<'tcx> for NonCopyConst {
impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'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::new(cx, &self.ignore_interior_mutability);
}
fn check_item(&mut self, cx: &LateContext<'tcx>, it: &'tcx Item<'_>) {
if let ItemKind::Const(.., body_id) = it.kind {
let ty = cx.tcx.type_of(it.owner_id).instantiate_identity();
if !ignored_macro(cx, it)
&& !self.is_ty_ignored(ty)
&& Self::is_unfrozen(cx, ty)
&& self.is_value_unfrozen_poly(cx, body_id, ty)
&& self.interior_mut.is_interior_mut_ty(cx, ty)
&& Self::is_value_unfrozen_poly(cx, body_id, ty)
{
lint(cx, Source::Item { item: it.span });
}
@ -354,7 +323,7 @@ fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx TraitIt
// Normalize assoc types because ones originated from generic params
// bounded other traits could have their bound.
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
if !self.is_ty_ignored(ty) && Self::is_unfrozen(cx, normalized)
if self.interior_mut.is_interior_mut_ty(cx, normalized)
// When there's no default value, lint it only according to its type;
// in other words, lint consts whose value *could* be unfrozen, not definitely is.
// This feels inconsistent with how the lint treats generic types,
@ -367,7 +336,7 @@ fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx TraitIt
// i.e. having an enum doesn't necessary mean a type has a frozen variant.
// And, implementing it isn't a trivial task; it'll probably end up
// re-implementing the trait predicate evaluation specific to `Freeze`.
&& body_id_opt.map_or(true, |body_id| self.is_value_unfrozen_poly(cx, body_id, normalized))
&& body_id_opt.map_or(true, |body_id| Self::is_value_unfrozen_poly(cx, body_id, normalized))
{
lint(cx, Source::Assoc { item: trait_item.span });
}
@ -409,8 +378,8 @@ fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<
// e.g. `layout_of(...).is_err() || has_frozen_variant(...);`
&& let ty = cx.tcx.type_of(impl_item.owner_id).instantiate_identity()
&& let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty)
&& !self.is_ty_ignored(ty) && Self::is_unfrozen(cx, normalized)
&& self.is_value_unfrozen_poly(cx, *body_id, normalized)
&& self.interior_mut.is_interior_mut_ty(cx, normalized)
&& Self::is_value_unfrozen_poly(cx, *body_id, normalized)
{
lint(cx, Source::Assoc { item: impl_item.span });
}
@ -420,9 +389,8 @@ fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<
// Normalize assoc types originated from generic params.
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
if !self.is_ty_ignored(ty)
&& Self::is_unfrozen(cx, ty)
&& self.is_value_unfrozen_poly(cx, *body_id, normalized)
if self.interior_mut.is_interior_mut_ty(cx, normalized)
&& Self::is_value_unfrozen_poly(cx, *body_id, normalized)
{
lint(cx, Source::Assoc { item: impl_item.span });
}
@ -517,9 +485,8 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
cx.typeck_results().expr_ty(dereferenced_expr)
};
if !self.is_ty_ignored(ty)
&& Self::is_unfrozen(cx, ty)
&& self.is_value_unfrozen_expr(cx, expr.hir_id, item_def_id, ty)
if self.interior_mut.is_interior_mut_ty(cx, ty)
&& Self::is_value_unfrozen_expr(cx, expr.hir_id, item_def_id, ty)
{
lint(cx, Source::Expr { expr: expr.span });
}

1
clippy_lints/src/trait_bounds.rs
View File

@ -237,6 +237,7 @@ fn cannot_combine_maybe_bound(&self, cx: &LateContext<'_>, bound: &GenericBound<
}
}
#[allow(clippy::mutable_key_type)]
fn check_type_repetition<'tcx>(&self, cx: &LateContext<'tcx>, gen: &'tcx Generics<'_>) {
struct SpanlessTy<'cx, 'tcx> {
ty: &'tcx Ty<'tcx>,

6
clippy_utils/src/lib.rs
View File

@ -2328,10 +2328,10 @@ pub fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<S
///
/// Given functions `eq` and `hash` such that `eq(a, b) == true`
/// implies `hash(a) == hash(b)`
pub fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)>
pub fn search_same<T, Hash, Eq>(exprs: &[T], mut hash: Hash, mut eq: Eq) -> Vec<(&T, &T)>
where
Hash: Fn(&T) -> u64,
Eq: Fn(&T, &T) -> bool,
Hash: FnMut(&T) -> u64,
Eq: FnMut(&T, &T) -> bool,
{
match exprs {
[a, b] if eq(a, b) => return vec![(a, b)],

124
clippy_utils/src/ty.rs
View File

@ -29,9 +29,10 @@
use rustc_trait_selection::traits::query::normalize::QueryNormalizeExt;
use rustc_trait_selection::traits::{Obligation, ObligationCause};
use std::assert_matches::debug_assert_matches;
use std::collections::hash_map::Entry;
use std::iter;
use crate::{match_def_path, path_res};
use crate::{def_path_def_ids, match_def_path, path_res};
mod type_certainty;
pub use type_certainty::expr_type_is_certain;
@ -1198,47 +1199,88 @@ fn helper<'tcx>(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, ty: AliasTy<'tcx>)
helper(tcx, param_env, make_projection(tcx, container_id, assoc_ty, args)?)
}
/// Check if given type has inner mutability such as [`std::cell::Cell`] or [`std::cell::RefCell`]
/// etc.
pub fn is_interior_mut_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
match *ty.kind() {
ty::Ref(_, inner_ty, mutbl) => mutbl == Mutability::Mut || is_interior_mut_ty(cx, inner_ty),
ty::Slice(inner_ty) => is_interior_mut_ty(cx, inner_ty),
ty::Array(inner_ty, size) => {
size.try_eval_target_usize(cx.tcx, cx.param_env)
.map_or(true, |u| u != 0)
&& is_interior_mut_ty(cx, inner_ty)
},
ty::Tuple(fields) => fields.iter().any(|ty| is_interior_mut_ty(cx, ty)),
ty::Adt(def, args) => {
// 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,
]
/// Helper to check if given type has inner mutability such as [`std::cell::Cell`] or
/// [`std::cell::RefCell`].
#[derive(Default, Debug)]
pub struct InteriorMut<'tcx> {
ignored_def_ids: FxHashSet<DefId>,
ignore_pointers: bool,
tys: FxHashMap<Ty<'tcx>, Option<bool>>,
}
impl<'tcx> InteriorMut<'tcx> {
pub fn new(cx: &LateContext<'tcx>, ignore_interior_mutability: &[String]) -> Self {
let ignored_def_ids = ignore_interior_mutability
.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 {
// The type is mutable if any of its type parameters are
args.types().any(|ty| is_interior_mut_ty(cx, ty))
} else {
!ty.has_escaping_bound_vars()
&& cx.tcx.layout_of(cx.param_env.and(ty)).is_ok()
&& !ty.is_freeze(cx.tcx, cx.param_env)
}
},
_ => false,
.flat_map(|ignored_ty| {
let path: Vec<&str> = ignored_ty.split("::").collect();
def_path_def_ids(cx, path.as_slice())
})
.collect();
Self {
ignored_def_ids,
..Self::default()
}
}
pub fn without_pointers(cx: &LateContext<'tcx>, ignore_interior_mutability: &[String]) -> Self {
Self {
ignore_pointers: true,
..Self::new(cx, ignore_interior_mutability)
}
}
/// Check if given type has inner mutability such as [`std::cell::Cell`] or
/// [`std::cell::RefCell`] etc.
pub fn is_interior_mut_ty(&mut self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
match self.tys.entry(ty) {
Entry::Occupied(o) => return *o.get() == Some(true),
// Temporarily insert a `None` to break cycles
Entry::Vacant(v) => v.insert(None),
};
let interior_mut = match *ty.kind() {
ty::RawPtr(inner_ty, _) if !self.ignore_pointers => self.is_interior_mut_ty(cx, inner_ty),
ty::Ref(_, inner_ty, _) | ty::Slice(inner_ty) => self.is_interior_mut_ty(cx, inner_ty),
ty::Array(inner_ty, size) => {
size.try_eval_target_usize(cx.tcx, cx.param_env)
.map_or(true, |u| u != 0)
&& self.is_interior_mut_ty(cx, inner_ty)
},
ty::Tuple(fields) => fields.iter().any(|ty| self.is_interior_mut_ty(cx, ty)),
ty::Adt(def, _) if def.is_unsafe_cell() => true,
ty::Adt(def, args) => {
let is_std_collection = matches!(
cx.tcx.get_diagnostic_name(def.did()),
Some(
sym::LinkedList
| sym::Vec
| sym::VecDeque
| sym::BTreeMap
| sym::BTreeSet
| sym::HashMap
| sym::HashSet
| sym::Arc
| sym::Rc
)
);
if is_std_collection || def.is_box() {
// Include the types from std collections that are behind pointers internally
args.types().any(|ty| self.is_interior_mut_ty(cx, ty))
} else if self.ignored_def_ids.contains(&def.did()) || def.is_phantom_data() {
false
} else {
def.all_fields()
.any(|f| self.is_interior_mut_ty(cx, f.ty(cx.tcx, args)))
}
},
_ => false,
};
self.tys.insert(ty, Some(interior_mut));
interior_mut
}
}

8
tests/ui-toml/mut_key/mut_key.rs
View File

@ -44,10 +44,18 @@ fn deref(&self) -> &T {
}
}
#[derive(Hash, PartialEq, Eq)]
struct ContainsCounted {
inner: Counted<String>,
}
// 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 indirect(_: HashMap<ContainsCounted, usize>) {}
fn main() {
should_not_take_this_arg(HashSet::new());
indirect(HashMap::new());
}

4
tests/ui/borrow_interior_mutable_const/traits.rs
View File

@ -158,7 +158,7 @@ trait BothOfCellAndGeneric<T> {
const INDIRECT: Cell<*const T>;
fn function() {
let _ = &Self::DIRECT;
let _ = &Self::DIRECT; //~ ERROR: interior mutability
let _ = &Self::INDIRECT; //~ ERROR: interior mutability
}
}
@ -168,7 +168,7 @@ impl<T: ConstDefault> BothOfCellAndGeneric<T> for Vec<T> {
const INDIRECT: Cell<*const T> = Cell::new(std::ptr::null());
fn function() {
let _ = &Self::DIRECT;
let _ = &Self::DIRECT; //~ ERROR: interior mutability
let _ = &Self::INDIRECT; //~ ERROR: interior mutability
}
}

18
tests/ui/borrow_interior_mutable_const/traits.stderr
View File

@ -75,6 +75,14 @@ LL | let _ = &Self::WRAPPED_SELF;
|
= help: assign this const to a local or static variable, and use the variable here
error: a `const` item with interior mutability should not be borrowed
--> tests/ui/borrow_interior_mutable_const/traits.rs:161:18
|
LL | let _ = &Self::DIRECT;
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here
error: a `const` item with interior mutability should not be borrowed
--> tests/ui/borrow_interior_mutable_const/traits.rs:162:18
|
@ -83,6 +91,14 @@ LL | let _ = &Self::INDIRECT;
|
= help: assign this const to a local or static variable, and use the variable here
error: a `const` item with interior mutability should not be borrowed
--> tests/ui/borrow_interior_mutable_const/traits.rs:171:18
|
LL | let _ = &Self::DIRECT;
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here
error: a `const` item with interior mutability should not be borrowed
--> tests/ui/borrow_interior_mutable_const/traits.rs:172:18
|
@ -123,5 +139,5 @@ LL | assert_eq!(u64::ATOMIC.load(Ordering::SeqCst), 9);
|
= help: assign this const to a local or static variable, and use the variable here
error: aborting due to 15 previous errors
error: aborting due to 17 previous errors

5
tests/ui/declare_interior_mutable_const/traits.rs
View File

@ -121,13 +121,12 @@ impl SelfType for AtomicUsize {
// Even though a constant contains a generic type, if it also have an interior mutable type,
// it should be linted at the definition site.
trait BothOfCellAndGeneric<T> {
// this is a false negative in the current implementation.
const DIRECT: Cell<T>;
const DIRECT: Cell<T>; //~ ERROR: interior mutable
const INDIRECT: Cell<*const T>; //~ ERROR: interior mutable
}
impl<T: ConstDefault> BothOfCellAndGeneric<T> for u64 {
const DIRECT: Cell<T> = Cell::new(T::DEFAULT);
const DIRECT: Cell<T> = Cell::new(T::DEFAULT); //~ ERROR: interior mutable
const INDIRECT: Cell<*const T> = Cell::new(std::ptr::null());
}

20
tests/ui/declare_interior_mutable_const/traits.stderr
View File

@ -55,22 +55,34 @@ LL | const WRAPPED_SELF: Option<Self> = Some(AtomicUsize::new(21));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: a `const` item should never be interior mutable
--> tests/ui/declare_interior_mutable_const/traits.rs:126:5
--> tests/ui/declare_interior_mutable_const/traits.rs:124:5
|
LL | const DIRECT: Cell<T>;
| ^^^^^^^^^^^^^^^^^^^^^^
error: a `const` item should never be interior mutable
--> tests/ui/declare_interior_mutable_const/traits.rs:125:5
|
LL | const INDIRECT: Cell<*const T>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: a `const` item should never be interior mutable
--> tests/ui/declare_interior_mutable_const/traits.rs:142:5
--> tests/ui/declare_interior_mutable_const/traits.rs:129:5
|
LL | const DIRECT: Cell<T> = Cell::new(T::DEFAULT);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: a `const` item should never be interior mutable
--> tests/ui/declare_interior_mutable_const/traits.rs:141:5
|
LL | const ATOMIC: AtomicUsize = AtomicUsize::new(18);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: a `const` item should never be interior mutable
--> tests/ui/declare_interior_mutable_const/traits.rs:148:5
--> tests/ui/declare_interior_mutable_const/traits.rs:147:5
|
LL | const BOUNDED_ASSOC_TYPE: T::ToBeBounded = AtomicUsize::new(19);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 11 previous errors
error: aborting due to 13 previous errors

10
tests/ui/mut_key.rs
View File

@ -5,7 +5,7 @@
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering::Relaxed;
use std::sync::Arc;
//@no-rustfix
struct Key(AtomicUsize);
impl Clone for Key {
@ -77,8 +77,6 @@ fn main() {
//~^ ERROR: mutable key type
let _map = HashMap::<&mut Cell<usize>, usize>::new();
//~^ ERROR: mutable key type
let _map = HashMap::<&mut usize, usize>::new();
//~^ ERROR: mutable key type
// Collection types from `std` who's impl of `Hash` or `Ord` delegate their type parameters
let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
//~^ ERROR: mutable key type
@ -92,8 +90,6 @@ fn main() {
//~^ ERROR: mutable key type
let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
//~^ ERROR: mutable key type
let _map = HashMap::<Result<&mut usize, ()>, usize>::new();
//~^ ERROR: mutable key type
// Smart pointers from `std` who's impl of `Hash` or `Ord` delegate their type parameters
let _map = HashMap::<Box<Cell<usize>>, usize>::new();
//~^ ERROR: mutable key type
@ -101,4 +97,8 @@ fn main() {
//~^ ERROR: mutable key type
let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
//~^ ERROR: mutable key type
// Not interior mutability
let _map = HashMap::<&mut usize, usize>::new();
let _map = HashMap::<Result<&mut usize, ()>, usize>::new();
}

32
tests/ui/mut_key.stderr
View File

@ -38,70 +38,58 @@ LL | let _map = HashMap::<&mut Cell<usize>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:80:5
|
LL | let _map = HashMap::<&mut usize, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:83:5
--> tests/ui/mut_key.rs:81:5
|
LL | let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:85:5
--> tests/ui/mut_key.rs:83:5
|
LL | let _map = HashMap::<BTreeMap<Cell<usize>, ()>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:87:5
--> tests/ui/mut_key.rs:85:5
|
LL | let _map = HashMap::<BTreeMap<(), Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:89:5
--> tests/ui/mut_key.rs:87:5
|
LL | let _map = HashMap::<BTreeSet<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:91:5
--> tests/ui/mut_key.rs:89:5
|
LL | let _map = HashMap::<Option<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:93:5
--> tests/ui/mut_key.rs:91:5
|
LL | let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:95:5
|
LL | let _map = HashMap::<Result<&mut usize, ()>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:98:5
--> tests/ui/mut_key.rs:94:5
|
LL | let _map = HashMap::<Box<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:100:5
--> tests/ui/mut_key.rs:96:5
|
LL | let _map = HashMap::<Rc<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: mutable key type
--> tests/ui/mut_key.rs:102:5
--> tests/ui/mut_key.rs:98:5
|
LL | let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 17 previous errors
error: aborting due to 15 previous errors