diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index df5dad0423cf..3ad92a91ea75 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3002,13 +3002,22 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Looks for calls to ` as Any>::type_id`. + /// Looks for calls to `.type_id()` on a `Box`. /// /// ### 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` calls `type_id` on the `Box<..>` itself, - /// so this will return the `TypeId` of the `Box` 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` returns a fixed `TypeId` of the `Box` itself, + /// rather than returning the `TypeId` of the underlying type behind the trait object. + /// + /// For `Box` 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` 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::>()`: + /// 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`" + "calling `.type_id()` on a boxed trait object" } declare_clippy_lint! { diff --git a/clippy_lints/src/methods/type_id_on_box.rs b/clippy_lints/src/methods/type_id_on_box.rs index 4917936a9322..31e6ccb950ad 100644 --- a/clippy_lints/src/methods/type_id_on_box.rs +++ b/clippy_lints/src/methods/type_id_on_box.rs @@ -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 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`", + &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, ""); - diag.note( - "this returns the type id of the literal type `Box` 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::>()` 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, ""); + + diag.span_suggestion( + receiver.span, + "consider dereferencing first", + format!("({sugg})"), + Applicability::MaybeIncorrect, + ); + } }, ); } diff --git a/tests/ui/type_id_on_box.fixed b/tests/ui/type_id_on_box.fixed index 538c38b70e6b..3656043700fa 100644 --- a/tests/ui/type_id_on_box.fixed +++ b/tests/ui/type_id_on_box.fixed @@ -19,19 +19,37 @@ fn existential() -> impl Any { Box::new(1) as Box } +trait AnySubTrait: Any {} +impl 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 = Box::new(0usize); let _ = (*any_box).type_id(); - let _ = TypeId::of::>(); // 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::>(); let _ = (*any_box).type_id(); + + // 2 derefs are needed here to get to the `dyn Any` let any_box: &Box = &(Box::new(0usize) as Box); - 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 = 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 `::type_id`. Not `std::boxed::Box`! diff --git a/tests/ui/type_id_on_box.rs b/tests/ui/type_id_on_box.rs index f224d273bc23..4bd9e73f2da0 100644 --- a/tests/ui/type_id_on_box.rs +++ b/tests/ui/type_id_on_box.rs @@ -19,19 +19,37 @@ fn existential() -> impl Any { Box::new(1) as Box } +trait AnySubTrait: Any {} +impl 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 = Box::new(0usize); let _ = any_box.type_id(); - let _ = TypeId::of::>(); // 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::>(); let _ = (*any_box).type_id(); + + // 2 derefs are needed here to get to the `dyn Any` let any_box: &Box = &(Box::new(0usize) as Box); - 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 = 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 `::type_id`. Not `std::boxed::Box`! diff --git a/tests/ui/type_id_on_box.stderr b/tests/ui/type_id_on_box.stderr index 0fce6a37c004..4528195607da 100644 --- a/tests/ui/type_id_on_box.stderr +++ b/tests/ui/type_id_on_box.stderr @@ -1,37 +1,48 @@ -error: calling `.type_id()` on a `Box` - --> tests/ui/type_id_on_box.rs:24:13 +error: calling `.type_id()` on `Box` + --> 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` 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::>()` 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` - --> tests/ui/type_id_on_box.rs:28:13 +error: calling `.type_id()` on `Box` + --> 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` 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::>()` instead, which makes it more clear -error: calling `.type_id()` on a `Box` - --> tests/ui/type_id_on_box.rs:34:13 +error: calling `.type_id()` on `Box` + --> 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` 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::>()` instead, which makes it more clear + +error: calling `.type_id()` on `Box` + --> 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::>()` instead, which makes it more clear -error: aborting due to 3 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/type_id_on_box_unfixable.rs b/tests/ui/type_id_on_box_unfixable.rs new file mode 100644 index 000000000000..f6d09834adb1 --- /dev/null +++ b/tests/ui/type_id_on_box_unfixable.rs @@ -0,0 +1,31 @@ +#![warn(clippy::type_id_on_box)] + +use std::any::{Any, TypeId}; +use std::ops::Deref; + +trait AnySubTrait: Any {} +impl AnySubTrait for T {} + +// `Any` is an indirect supertrait +trait AnySubSubTrait: AnySubTrait {} +impl AnySubSubTrait for T {} + +// This trait mentions `Any` in its predicates, but it is not a subtrait of `Any`. +trait NormalTrait +where + i32: Any, +{ +} +impl 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 = Box::new(1); + let _ = b.type_id(); + //~^ ERROR: calling `.type_id()` on + + let b: Box = Box::new(1); + let _ = b.type_id(); + //~^ ERROR: calling `.type_id()` on +} diff --git a/tests/ui/type_id_on_box_unfixable.stderr b/tests/ui/type_id_on_box_unfixable.stderr new file mode 100644 index 000000000000..539ed481ec10 --- /dev/null +++ b/tests/ui/type_id_on_box_unfixable.stderr @@ -0,0 +1,22 @@ +error: calling `.type_id()` on `Box` + --> 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::>()` 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` + --> 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::>()` instead, which makes it more clear + +error: aborting due to 2 previous errors +