Auto merge of #10987 - y21:type_id_on_box, r=llogiq

new lint: `type_id_on_box`

Closes #7687.

A new lint that detects calling `.type_id()` on `Box<dyn Any>` (and not on the underlying `dyn Any`), which can make up for some pretty confusing bugs!

changelog: new lint: [`type_id_on_box`]

Author: bors

CHANGELOG.md

@@ -5266,6 +5266,7 @@ Released 2018-09-13
 [`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err
 [`tuple_array_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#tuple_array_conversions
 [`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
+[`type_id_on_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_id_on_box
 [`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
 [`unchecked_duration_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction
 [`undocumented_unsafe_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks

clippy_lints/src/declared_lints.rs

@@ -404,6 +404,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::SUSPICIOUS_MAP_INFO,
     crate::methods::SUSPICIOUS_SPLITN_INFO,
     crate::methods::SUSPICIOUS_TO_OWNED_INFO,
+    crate::methods::TYPE_ID_ON_BOX_INFO,
     crate::methods::UNINIT_ASSUMED_INIT_INFO,
     crate::methods::UNIT_HASH_INFO,
     crate::methods::UNNECESSARY_FILTER_MAP_INFO,

clippy_lints/src/methods/mod.rs

@@ -88,6 +88,7 @@ mod suspicious_command_arg_space;
 mod suspicious_map;
 mod suspicious_splitn;
 mod suspicious_to_owned;
+mod type_id_on_box;
 mod uninit_assumed_init;
 mod unit_hash;
 mod unnecessary_filter_map;
@@ -2925,6 +2926,37 @@ declare_clippy_lint! {
     "use of sort() when sort_unstable() is equivalent"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Looks for calls to `<Box<dyn Any> as Any>::type_id`.
+    ///
+    /// ### 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!).
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// use std::any::{Any, TypeId};
+    ///
+    /// let any_box: Box<dyn Any> = Box::new(42_i32);
+    /// assert_eq!(any_box.type_id(), TypeId::of::<i32>()); // ⚠️ this fails!
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::any::{Any, TypeId};
+    ///
+    /// let any_box: Box<dyn Any> = Box::new(42_i32);
+    /// assert_eq!((*any_box).type_id(), TypeId::of::<i32>());
+    /// //          ^ dereference first, to call `type_id` on `dyn Any`
+    /// ```
+    #[clippy::version = "1.72.0"]
+    pub TYPE_ID_ON_BOX,
+    suspicious,
+    "calling `.type_id()` on `Box<dyn Any>`"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Detects `().hash(_)`.
@@ -3389,6 +3421,7 @@ impl_lint_pass!(Methods => [
     STRING_EXTEND_CHARS,
     ITER_CLONED_COLLECT,
     ITER_WITH_DRAIN,
+    TYPE_ID_ON_BOX,
     USELESS_ASREF,
     UNNECESSARY_FOLD,
     UNNECESSARY_FILTER_MAP,
@@ -3914,6 +3947,9 @@ impl Methods {
                 ("to_os_string" | "to_path_buf" | "to_vec", []) => {
                     implicit_clone::check(cx, name, expr, recv);
                 },
+                ("type_id", []) => {
+                    type_id_on_box::check(cx, recv, expr.span);
+                }
                 ("unwrap", []) => {
                     match method_call(recv) {
                         Some(("get", recv, [get_arg], _, _)) => {

clippy_lints/src/methods/type_id_on_box.rs

@@ -0,0 +1,64 @@
+use crate::methods::TYPE_ID_ON_BOX;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::source::snippet;
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_lint::LateContext;
+use rustc_middle::ty::adjustment::Adjust;
+use rustc_middle::ty::adjustment::Adjustment;
+use rustc_middle::ty::Ty;
+use rustc_middle::ty::{self, ExistentialPredicate};
+use rustc_span::{sym, Span};
+
+fn is_dyn_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),
+            _ => false,
+        })
+    } else {
+        false
+    }
+}
+
+pub(super) fn check(cx: &LateContext<'_>, receiver: &Expr<'_>, call_span: Span) {
+    let recv_adjusts = cx.typeck_results().expr_adjustments(receiver);
+
+    if let Some(Adjustment { target: recv_ty, .. }) = recv_adjusts.last()
+        && let ty::Ref(_, ty, _) = recv_ty.kind()
+        && let ty::Adt(adt, substs) = ty.kind()
+        && adt.is_box()
+        && is_dyn_any(cx, substs.type_at(0))
+    {
+        span_lint_and_then(
+            cx,
+            TYPE_ID_ON_BOX,
+            call_span,
+            "calling `.type_id()` on a `Box<dyn Any>`",
+            |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 \
+                    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,
+                );
+            },
+        );
+    }
+}

tests/ui/type_id_on_box.fixed

@@ -0,0 +1,40 @@
+//@run-rustfix
+
+#![warn(clippy::type_id_on_box)]
+
+use std::any::{Any, TypeId};
+use std::ops::Deref;
+
+type SomeBox = Box<dyn Any>;
+
+struct BadBox(Box<dyn Any>);
+
+impl Deref for BadBox {
+    type Target = Box<dyn Any>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+fn existential() -> impl Any {
+    Box::new(1) as Box<dyn Any>
+}
+
+fn main() {
+    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
+    let _ = (*any_box).type_id();
+    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 b = existential();
+    let _ = b.type_id(); // Don't lint.
+
+    let b: SomeBox = Box::new(0usize);
+    let _ = (*b).type_id();
+
+    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`!
+}

tests/ui/type_id_on_box.rs

@@ -0,0 +1,40 @@
+//@run-rustfix
+
+#![warn(clippy::type_id_on_box)]
+
+use std::any::{Any, TypeId};
+use std::ops::Deref;
+
+type SomeBox = Box<dyn Any>;
+
+struct BadBox(Box<dyn Any>);
+
+impl Deref for BadBox {
+    type Target = Box<dyn Any>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+fn existential() -> impl Any {
+    Box::new(1) as Box<dyn Any>
+}
+
+fn main() {
+    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
+    let _ = (*any_box).type_id();
+    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 b = existential();
+    let _ = b.type_id(); // Don't lint.
+
+    let b: SomeBox = Box::new(0usize);
+    let _ = b.type_id();
+
+    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`!
+}

tests/ui/type_id_on_box.stderr

@@ -0,0 +1,36 @@
+error: calling `.type_id()` on a `Box<dyn Any>`
+  --> $DIR/type_id_on_box.rs:26: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: 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`
+
+error: calling `.type_id()` on a `Box<dyn Any>`
+  --> $DIR/type_id_on_box.rs:30:13
+   |
+LL |     let _ = any_box.type_id(); // 2 derefs are needed here to get to the `dyn Any`
+   |             -------^^^^^^^^^^
+   |             |
+   |             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: 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>`
+  --> $DIR/type_id_on_box.rs:36: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: if this is intentional, use `TypeId::of::<Box<dyn Any>>()` instead, which makes it more clear
+
+error: aborting due to 3 previous errors
+