diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index df5dad0423c..3ad92a91ea7 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 cec5d3b2b6d..31e6ccb950a 100644 --- a/clippy_lints/src/methods/type_id_on_box.rs +++ b/clippy_lints/src/methods/type_id_on_box.rs @@ -1,5 +1,3 @@ -use std::borrow::Cow; - use crate::methods::TYPE_ID_ON_BOX; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; @@ -11,33 +9,33 @@ use rustc_middle::ty::print::with_forced_trimmed_paths; use rustc_middle::ty::{self, ExistentialPredicate, Ty}; use rustc_span::{sym, Span}; -/// Checks if a [`Ty`] is a `dyn Any` or a `dyn Trait` where `Trait: Any` -/// and returns the name of the trait object. -fn is_dyn_any(cx: &LateContext<'_>, ty: Ty<'_>) -> Option> { +/// 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().find_map(|p| match p.skip_binder() { + preds.iter().any(|p| match p.skip_binder() { ExistentialPredicate::Trait(tr) => { - if cx.tcx.is_diagnostic_item(sym::Any, tr.def_id) { - Some(Cow::Borrowed("Any")) - } else if cx - .tcx - .super_predicates_of(tr.def_id) - .predicates - .iter() - .any(|(clause, _)| { - matches!(clause.kind().skip_binder(), ty::ClauseKind::Trait(super_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())) - }) - { - Some(Cow::Owned(with_forced_trimmed_paths!(cx.tcx.def_path_str(tr.def_id)))) - } else { - None - } + }) }, - _ => None, + _ => false, }) } else { - None + false } } @@ -48,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() - && let Some(trait_path) = 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, - &format!("calling `.type_id()` on `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 \ type id of the boxed value, which is most likely not what you want", ) .note(format!( - "if this is intentional, use `TypeId::of::>()` instead, \ + "if this is intentional, use `TypeId::of::<{ty_name}>()` instead, \ which makes it more clear" - )) - .span_suggestion( - receiver.span, - "consider dereferencing first", - format!("({sugg})"), - Applicability::MaybeIncorrect, - ); + )); + + 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, + ); + } }, ); }