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

[`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

7 files changed, 176 insertions, 41 deletions

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 `<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! {
diff --git a/clippy_lints/src/methods/type_id_on_box.rs b/clippy_lints/src/methods/type_id_on_box.rs
index 4917936a932..31e6ccb950a 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 <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,
+                    );
+                }
             },
         );
     }
diff --git a/tests/ui/type_id_on_box.fixed b/tests/ui/type_id_on_box.fixed
index 538c38b70e6..3656043700f 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<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`!
diff --git a/tests/ui/type_id_on_box.rs b/tests/ui/type_id_on_box.rs
index f224d273bc2..4bd9e73f2da 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<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`!
diff --git a/tests/ui/type_id_on_box.stderr b/tests/ui/type_id_on_box.stderr
index 0fce6a37c00..4528195607d 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<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<_>` 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<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: 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 00000000000..f6d09834adb
--- /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<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
+}
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 00000000000..539ed481ec1
--- /dev/null
+++ b/tests/ui/type_id_on_box_unfixable.stderr
@@ -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
+