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.

Sign up for GitHub

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

Jump to bottom

Show interior mutability chain in mutable_key_type #13496

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Show interior mutability chain in mutable_key_type #13496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions clippy_lints/src/mut_key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use clippy_config::Conf;
use clippy_utils::diagnostics::span_lint;
use clippy_utils::diagnostics::span_lint_and_then;
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::print::with_forced_trimmed_paths;
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_session::impl_lint_pass;
use rustc_span::Span;
Expand Down Expand Up @@ -132,8 +133,14 @@ impl<'tcx> MutableKeyType<'tcx> {
)
{
let subst_ty = args.type_at(0);
if self.interior_mut.is_interior_mut_ty(cx, subst_ty) {
span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type");
if let Some(chain) = self.interior_mut.interior_mut_ty_chain(cx, subst_ty) {
span_lint_and_then(cx, MUTABLE_KEY_TYPE, span, "mutable key type", |diag| {
for ty in chain.iter().rev() {
diag.note(with_forced_trimmed_paths!(format!(
"... because it contains `{ty}`, which has interior mutability"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since type names can be quite long sometimes, wouldn't it be best to use … instead of ... here? In monospace font the difference would be two characters wide:

... because it contains `UnsafeCell<usize>`, which has interior mutability
… because it contains `UnsafeCell<usize>`, which has interior mutability

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with the three dots because I think it's much more common in diagnostics (in fact, I think I've never actually seen a non-ASCII character like that in a diagnostic - rust-lang/rust#126597 adds unicode block drawing but it requires an extra --error-format=human-unicode).

Some rustc examples
Some cargo examples

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a "…" in a diagnostic issued by clippy_lints/src/methods/or_then_unwrap.rs:

clippy_lints/src/methods/or_then_unwrap.rs
25:        title = "found `.or(Some(…)).unwrap()`";
32:        title = "found `.or(Ok(…)).unwrap()`";

)));
}
});
}
}
}
Expand Down
48 changes: 28 additions & 20 deletions clippy_utils/src/ty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,7 @@ pub fn make_normalized_projection<'tcx>(
pub struct InteriorMut<'tcx> {
ignored_def_ids: FxHashSet<DefId>,
ignore_pointers: bool,
tys: FxHashMap<Ty<'tcx>, Option<bool>>,
tys: FxHashMap<Ty<'tcx>, Option<&'tcx ty::List<Ty<'tcx>>>>,
}

impl<'tcx> InteriorMut<'tcx> {
Expand All @@ -1194,25 +1194,24 @@ impl<'tcx> InteriorMut<'tcx> {
}
}

/// 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 {
/// Check if given type has interior mutability such as [`std::cell::Cell`] or
/// [`std::cell::RefCell`] etc. and if it does, returns a chain of types that causes
/// this type to be interior mutable
pub fn interior_mut_ty_chain(&mut self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'tcx ty::List<Ty<'tcx>>> {
match self.tys.entry(ty) {
Entry::Occupied(o) => return *o.get() == Some(true),
Entry::Occupied(o) => return *o.get(),
// 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)
let chain = match *ty.kind() {
ty::RawPtr(inner_ty, _) if !self.ignore_pointers => self.interior_mut_ty_chain(cx, inner_ty),
ty::Ref(_, inner_ty, _) | ty::Slice(inner_ty) => self.interior_mut_ty_chain(cx, inner_ty),
ty::Array(inner_ty, size) if size.try_eval_target_usize(cx.tcx, cx.param_env) != Some(0) => {
self.interior_mut_ty_chain(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::Tuple(fields) => fields.iter().find_map(|ty| self.interior_mut_ty_chain(cx, ty)),
ty::Adt(def, _) if def.is_unsafe_cell() => Some(ty::List::empty()),
ty::Adt(def, args) => {
let is_std_collection = matches!(
cx.tcx.get_diagnostic_name(def.did()),
Expand All @@ -1231,19 +1230,28 @@ impl<'tcx> InteriorMut<'tcx> {

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))
args.types().find_map(|ty| self.interior_mut_ty_chain(cx, ty))
} else if self.ignored_def_ids.contains(&def.did()) || def.is_phantom_data() {
false
None
} else {
def.all_fields()
.any(|f| self.is_interior_mut_ty(cx, f.ty(cx.tcx, args)))
.find_map(|f| self.interior_mut_ty_chain(cx, f.ty(cx.tcx, args)))
}
},
_ => false,
_ => None,
};

self.tys.insert(ty, Some(interior_mut));
interior_mut
chain.map(|chain| {
let list = cx.tcx.mk_type_list_from_iter(chain.iter().chain([ty]));
self.tys.insert(ty, Some(list));
list
})
}

/// Check if given type has interior 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 {
self.interior_mut_ty_chain(cx, ty).is_some()
}
}

Expand Down
60 changes: 60 additions & 0 deletions tests/ui/mut_key.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ error: mutable key type
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Key`, which has interior mutability
= note: ... because it contains `AtomicUsize`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability
= note: `-D clippy::mutable-key-type` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::mutable_key_type)]`

Expand All @@ -12,84 +15,141 @@ error: mutable key type
|
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
| ^^^^^^^^^^^^
|
= note: ... because it contains `Key`, which has interior mutability
= note: ... because it contains `AtomicUsize`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:35:5
|
LL | let _other: HashMap<Key, bool> = HashMap::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Key`, which has interior mutability
= note: ... because it contains `AtomicUsize`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:63:22
|
LL | fn tuples_bad<U>(_m: &mut HashMap<(Key, U), bool>) {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `(Key, U)`, which has interior mutability
= note: ... because it contains `Key`, which has interior mutability
= note: ... because it contains `AtomicUsize`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:76:5
|
LL | let _map = HashMap::<Cell<usize>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:78:5
|
LL | let _map = HashMap::<&mut Cell<usize>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `&mut Cell<usize>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:81:5
|
LL | let _map = HashMap::<Vec<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Vec<Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:83:5
|
LL | let _map = HashMap::<BTreeMap<Cell<usize>, ()>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `BTreeMap<Cell<usize>, ()>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:85:5
|
LL | let _map = HashMap::<BTreeMap<(), Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `BTreeMap<(), Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:87:5
|
LL | let _map = HashMap::<BTreeSet<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `BTreeSet<Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:89:5
|
LL | let _map = HashMap::<Option<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Option<Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:91:5
|
LL | let _map = HashMap::<Option<Vec<Cell<usize>>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Option<Vec<Cell<usize>>>`, which has interior mutability
= note: ... because it contains `Vec<Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:94:5
|
LL | let _map = HashMap::<Box<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Box<Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:96:5
|
LL | let _map = HashMap::<Rc<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Rc<Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: mutable key type
--> tests/ui/mut_key.rs:98:5
|
LL | let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ... because it contains `Arc<Cell<usize>>`, which has interior mutability
= note: ... because it contains `Cell<usize>`, which has interior mutability
= note: ... because it contains `UnsafeCell<usize>`, which has interior mutability

error: aborting due to 15 previous errors