make arc-likes for mutable-key configurable

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"]`
This commit is contained in:
Andre Bogus 2022-10-20 23:40:10 +02:00
parent 191c9839f0
commit eba36e6d95
7 changed files with 202 additions and 69 deletions

View File

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

View File

@ -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,26 +80,44 @@
"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);
}
}
@ -105,71 +125,83 @@ 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()));
}
// 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");
}
}
}
/// 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,
}
}
}
/// 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)
}
},
_ => false,
}
}

View File

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

View File

@ -598,7 +598,13 @@ fn find_crate(tcx: TyCtxt<'_>, name: &str) -> Option<DefId> {
[primitive] => {
return PrimTy::from_name(Symbol::intern(primitive)).map_or(Res::Err, Res::PrimTy);
},
[base, ref path @ ..] => (base, path),
[base, ref path @ ..] => {
let crate_name = cx.sess().opts.crate_name.as_deref();
if Some(base) == crate_name {
return def_path_res_local(cx, path);
}
(base, path)
},
_ => return Res::Err,
};
let tcx = cx.tcx;
@ -642,7 +648,41 @@ fn find_crate(tcx: TyCtxt<'_>, name: &str) -> Option<DefId> {
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..];
if path.is_empty() {
let def_id = item_def_id.to_def_id();
return Res::Def(cx.tcx.def_kind(def_id), def_id);
}
if let ItemKind::Mod(m) = kind {
next_ids = Some(m.item_ids);
};
break;
}
}
}
if let Some(next_ids) = next_ids {
ids = next_ids;
} else {
break;
}
}
Res::Err
}

View File

@ -0,0 +1 @@
ignore-interior-mutability = ["mut_key::Counted"]

View File

@ -0,0 +1,53 @@
// compile-flags: --crate-name mut_key
#![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());
}

View File

@ -20,6 +20,7 @@ error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown fie
enforced-import-renames
enum-variant-name-threshold
enum-variant-size-threshold
ignore-interior-mutability
large-error-threshold
literal-representation-threshold
max-fn-params-bools