Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-24 05:33:27 +00:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #11350 - y21:issue11349, r=xFrednet

Browse source 
[`type_id_on_box`]: lint on any `Box<dyn _>`

Closes #11349.

It now not only lints when calling `.type_id()` on the type `Box<dyn Any>`, but also on any `Box<dyn Trait>` where `Trait` is a subtrait of `Any`

changelog: FN: [`type_id_on_box`]: lint if `Any` is a sub trait
This commit is contained in:
bors 2024-03-30 21:26:36 +00:00
commit 3787a0ccb8
7 changed files with 176 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
clippy_lints/src/methods/mod.rs
View file

@ -3002,13 +3002,22 @@ declare_clippy_lint! {
declare_clippy_lint! {
/// ### What it does
/// Looks for calls to `<Box<dyn Any> as Any>::type_id`.
/// Looks for calls to `.type_id()` on a `Box<dyn _>`.
///
/// ### Why is this bad?
/// This most certainly does not do what the user expects and is very easy to miss.
/// Calling `type_id` on a `Box<dyn Any>` calls `type_id` on the `Box<..>` itself,
/// so this will return the `TypeId` of the `Box<dyn Any>` type (not the type id
/// of the value referenced by the box!).
/// This almost certainly does not do what the user expects and can lead to subtle bugs.
/// Calling `.type_id()` on a `Box<dyn Trait>` returns a fixed `TypeId` of the `Box` itself,
/// rather than returning the `TypeId` of the underlying type behind the trait object.
///
/// For `Box<dyn Any>` specifically (and trait objects that have `Any` as its supertrait),
/// this lint will provide a suggestion, which is to dereference the receiver explicitly
/// to go from `Box<dyn Any>` to `dyn Any`.
/// This makes sure that `.type_id()` resolves to a dynamic call on the trait object
/// and not on the box.
///
/// If the fixed `TypeId` of the `Box` is the intended behavior, it's better to be explicit about it
/// and write `TypeId::of::<Box<dyn Trait>>()`:
/// this makes it clear that a fixed `TypeId` is returned and not the `TypeId` of the implementor.
///
/// ### Example
/// ```rust,ignore
@ -3028,7 +3037,7 @@ declare_clippy_lint! {
#[clippy::version = "1.73.0"]
pub TYPE_ID_ON_BOX,
suspicious,
"calling `.type_id()` on `Box<dyn Any>`"
"calling `.type_id()` on a boxed trait object"
}
declare_clippy_lint! {

62
clippy_lints/src/methods/type_id_on_box.rs
View file

@ -5,13 +5,33 @@ use rustc_errors::Applicability;
use rustc_hir::Expr;
use rustc_lint::LateContext;
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
use rustc_middle::ty::print::with_forced_trimmed_paths;
use rustc_middle::ty::{self, ExistentialPredicate, Ty};
use rustc_span::{sym, Span};
fn is_dyn_any(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
/// Checks if the given type is `dyn Any`, or a trait object that has `Any` as a supertrait.
/// Only in those cases will its vtable have a `type_id` method that returns the implementor's
/// `TypeId`, and only in those cases can we give a proper suggestion to dereference the box.
///
/// If this returns false, then `.type_id()` likely (this may have FNs) will not be what the user
/// expects in any case and dereferencing it won't help either. It will likely require some
/// other changes, but it is still worth emitting a lint.
/// See <https://github.com/rust-lang/rust-clippy/pull/11350#discussion_r1544863005> for more details.
fn is_subtrait_of_any(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
if let ty::Dynamic(preds, ..) = ty.kind() {
preds.iter().any(|p| match p.skip_binder() {
ExistentialPredicate::Trait(tr) => cx.tcx.is_diagnostic_item(sym::Any, tr.def_id),
ExistentialPredicate::Trait(tr) => {
cx.tcx.is_diagnostic_item(sym::Any, tr.def_id)
|| cx
.tcx
.super_predicates_of(tr.def_id)
.predicates
.iter()
.any(|(clause, _)| {
matches!(clause.kind().skip_binder(), ty::ClauseKind::Trait(super_tr)
if cx.tcx.is_diagnostic_item(sym::Any, super_tr.def_id()))
})
},
_ => false,
})
} else {
@ -26,36 +46,42 @@ pub(super) fn check(cx: &LateContext<'_>, receiver: &Expr<'_>, call_span: Span)
&& let ty::Ref(_, ty, _) = recv_ty.kind()
&& let ty::Adt(adt, args) = ty.kind()
&& adt.is_box()
&& is_dyn_any(cx, args.type_at(0))
&& let inner_box_ty = args.type_at(0)
&& let ty::Dynamic(..) = inner_box_ty.kind()
{
let ty_name = with_forced_trimmed_paths!(ty.to_string());
span_lint_and_then(
cx,
TYPE_ID_ON_BOX,
call_span,
"calling `.type_id()` on a `Box<dyn Any>`",
&format!("calling `.type_id()` on `{ty_name}`"),
|diag| {
let derefs = recv_adjusts
.iter()
.filter(|adj| matches!(adj.kind, Adjust::Deref(None)))
.count();
let mut sugg = "*".repeat(derefs + 1);
sugg += &snippet(cx, receiver.span, "<expr>");
diag.note(
"this returns the type id of the literal type `Box<dyn Any>` instead of the \
"this returns the type id of the literal type `Box<_>` instead of the \
type id of the boxed value, which is most likely not what you want",
)
.note(
"if this is intentional, use `TypeId::of::<Box<dyn Any>>()` instead, \
which makes it more clear",
)
.span_suggestion(
receiver.span,
"consider dereferencing first",
format!("({sugg})"),
Applicability::MaybeIncorrect,
);
.note(format!(
"if this is intentional, use `TypeId::of::<{ty_name}>()` instead, \
which makes it more clear"
));
if is_subtrait_of_any(cx, inner_box_ty) {
let mut sugg = "*".repeat(derefs + 1);
sugg += &snippet(cx, receiver.span, "<expr>");
diag.span_suggestion(
receiver.span,
"consider dereferencing first",
format!("({sugg})"),
Applicability::MaybeIncorrect,
);
}
},
);
}

24
tests/ui/type_id_on_box.fixed
View file

@ -19,19 +19,37 @@ fn existential() -> impl Any {
Box::new(1) as Box<dyn Any>
}
trait AnySubTrait: Any {}
impl<T: Any> AnySubTrait for T {}
fn main() {
// Don't lint, calling `.type_id()` on a `&dyn Any` does the expected thing
let ref_dyn: &dyn Any = &42;
let _ = ref_dyn.type_id();
let any_box: Box<dyn Any> = Box::new(0usize);
let _ = (*any_box).type_id();
let _ = TypeId::of::<Box<dyn Any>>(); // Don't lint. We explicitly say "do this instead" if this is intentional
//~^ ERROR: calling `.type_id()` on
// Don't lint. We explicitly say "do this instead" if this is intentional
let _ = TypeId::of::<Box<dyn Any>>();
let _ = (*any_box).type_id();
// 2 derefs are needed here to get to the `dyn Any`
let any_box: &Box<dyn Any> = &(Box::new(0usize) as Box<dyn Any>);
let _ = (**any_box).type_id(); // 2 derefs are needed here to get to the `dyn Any`
let _ = (**any_box).type_id();
//~^ ERROR: calling `.type_id()` on
let b = existential();
let _ = b.type_id(); // Don't lint.
let _ = b.type_id(); // Don't
let b: Box<dyn AnySubTrait> = Box::new(1);
let _ = (*b).type_id();
//~^ ERROR: calling `.type_id()` on
let b: SomeBox = Box::new(0usize);
let _ = (*b).type_id();
//~^ ERROR: calling `.type_id()` on
let b = BadBox(Box::new(0usize));
let _ = b.type_id(); // Don't lint. This is a call to `<BadBox as Any>::type_id`. Not `std::boxed::Box`!

24
tests/ui/type_id_on_box.rs
View file

@ -19,19 +19,37 @@ fn existential() -> impl Any {
Box::new(1) as Box<dyn Any>
}
trait AnySubTrait: Any {}
impl<T: Any> AnySubTrait for T {}
fn main() {
// Don't lint, calling `.type_id()` on a `&dyn Any` does the expected thing
let ref_dyn: &dyn Any = &42;
let _ = ref_dyn.type_id();
let any_box: Box<dyn Any> = Box::new(0usize);
let _ = any_box.type_id();
let _ = TypeId::of::<Box<dyn Any>>(); // Don't lint. We explicitly say "do this instead" if this is intentional
//~^ ERROR: calling `.type_id()` on
// Don't lint. We explicitly say "do this instead" if this is intentional
let _ = TypeId::of::<Box<dyn Any>>();
let _ = (*any_box).type_id();
// 2 derefs are needed here to get to the `dyn Any`
let any_box: &Box<dyn Any> = &(Box::new(0usize) as Box<dyn Any>);
let _ = any_box.type_id(); // 2 derefs are needed here to get to the `dyn Any`
let _ = any_box.type_id();
//~^ ERROR: calling `.type_id()` on
let b = existential();
let _ = b.type_id(); // Don't lint.
let _ = b.type_id(); // Don't
let b: Box<dyn AnySubTrait> = Box::new(1);
let _ = b.type_id();
//~^ ERROR: calling `.type_id()` on
let b: SomeBox = Box::new(0usize);
let _ = b.type_id();
//~^ ERROR: calling `.type_id()` on
let b = BadBox(Box::new(0usize));
let _ = b.type_id(); // Don't lint. This is a call to `<BadBox as Any>::type_id`. Not `std::boxed::Box`!

33
tests/ui/type_id_on_box.stderr
View file

@ -1,37 +1,48 @@
error: calling `.type_id()` on a `Box<dyn Any>`
--> tests/ui/type_id_on_box.rs:24:13
error: calling `.type_id()` on `Box<dyn Any>`
--> tests/ui/type_id_on_box.rs:31:13
|
LL | let _ = any_box.type_id();
| -------^^^^^^^^^^
| |
| help: consider dereferencing first: `(*any_box)`
|
= note: this returns the type id of the literal type `Box<dyn Any>` instead of the type id of the boxed value, which is most likely not what you want
= note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want
= note: if this is intentional, use `TypeId::of::<Box<dyn Any>>()` instead, which makes it more clear
= note: `-D clippy::type-id-on-box` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::type_id_on_box)]`
error: calling `.type_id()` on a `Box<dyn Any>`
--> tests/ui/type_id_on_box.rs:28:13
error: calling `.type_id()` on `Box<dyn Any>`
--> tests/ui/type_id_on_box.rs:40:13
|
LL | let _ = any_box.type_id(); // 2 derefs are needed here to get to the `dyn Any`
LL | let _ = any_box.type_id();
| -------^^^^^^^^^^
| |
| help: consider dereferencing first: `(**any_box)`
|
= note: this returns the type id of the literal type `Box<dyn Any>` instead of the type id of the boxed value, which is most likely not what you want
= note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want
= note: if this is intentional, use `TypeId::of::<Box<dyn Any>>()` instead, which makes it more clear
error: calling `.type_id()` on a `Box<dyn Any>`
--> tests/ui/type_id_on_box.rs:34:13
error: calling `.type_id()` on `Box<dyn AnySubTrait>`
--> tests/ui/type_id_on_box.rs:47:13
|
LL | let _ = b.type_id();
| -^^^^^^^^^^
| |
| help: consider dereferencing first: `(*b)`
|
= note: this returns the type id of the literal type `Box<dyn Any>` instead of the type id of the boxed value, which is most likely not what you want
= note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want
= note: if this is intentional, use `TypeId::of::<Box<dyn AnySubTrait>>()` instead, which makes it more clear
error: calling `.type_id()` on `Box<dyn Any>`
--> tests/ui/type_id_on_box.rs:51:13
|
LL | let _ = b.type_id();
| -^^^^^^^^^^
| |
| help: consider dereferencing first: `(*b)`
|
= note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want
= note: if this is intentional, use `TypeId::of::<Box<dyn Any>>()` instead, which makes it more clear
error: aborting due to 3 previous errors
error: aborting due to 4 previous errors

31
tests/ui/type_id_on_box_unfixable.rs Normal file
View file

@ -0,0 +1,31 @@
#![warn(clippy::type_id_on_box)]
use std::any::{Any, TypeId};
use std::ops::Deref;
trait AnySubTrait: Any {}
impl<T: Any> AnySubTrait for T {}
// `Any` is an indirect supertrait
trait AnySubSubTrait: AnySubTrait {}
impl<T: AnySubTrait> AnySubSubTrait for T {}
// This trait mentions `Any` in its predicates, but it is not a subtrait of `Any`.
trait NormalTrait
where
i32: Any,
{
}
impl<T> NormalTrait for T {}
fn main() {
// (currently we don't look deeper than one level into the supertrait hierachy, but we probably
// could)
let b: Box<dyn AnySubSubTrait> = Box::new(1);
let _ = b.type_id();
//~^ ERROR: calling `.type_id()` on
let b: Box<dyn NormalTrait> = Box::new(1);
let _ = b.type_id();
//~^ ERROR: calling `.type_id()` on
}

22
tests/ui/type_id_on_box_unfixable.stderr Normal file
View file

@ -0,0 +1,22 @@
error: calling `.type_id()` on `Box<dyn AnySubSubTrait>`
--> tests/ui/type_id_on_box_unfixable.rs:25:13
|
LL | let _ = b.type_id();
| ^^^^^^^^^^^
|
= note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want
= note: if this is intentional, use `TypeId::of::<Box<dyn AnySubSubTrait>>()` instead, which makes it more clear
= note: `-D clippy::type-id-on-box` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::type_id_on_box)]`
error: calling `.type_id()` on `Box<dyn NormalTrait>`
--> tests/ui/type_id_on_box_unfixable.rs:29:13
|
LL | let _ = b.type_id();
| ^^^^^^^^^^^
|
= note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want
= note: if this is intentional, use `TypeId::of::<Box<dyn NormalTrait>>()` instead, which makes it more clear
error: aborting due to 2 previous errors