lint on any Box<dyn _>, but provide a suggestion for subtypes of dyn Any

Author: y21

clippy_lints/src/methods/mod.rs

@@ -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! {

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<Cow<'static, str>> {
+/// 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().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<dyn {trait_path}>`"),
+            &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<_>` 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::<Box<dyn {trait_path}>>()` 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, "<expr>");
+
+                    diag.span_suggestion(
+                        receiver.span,
+                        "consider dereferencing first",
+                        format!("({sugg})"),
+                        Applicability::MaybeIncorrect,
+                    );
+                }
             },
         );
     }