From c5a9adc2bef3afcaf8bcdb260e218a82911a034e Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 18 Jun 2023 22:20:10 +0200 Subject: [PATCH 1/5] new lint: `type_id_on_box` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/mod.rs | 35 ++++++++++++ clippy_lints/src/methods/type_id_on_box.rs | 64 ++++++++++++++++++++++ tests/ui/type_id_on_box.fixed | 20 +++++++ tests/ui/type_id_on_box.rs | 20 +++++++ tests/ui/type_id_on_box.stderr | 25 +++++++++ 7 files changed, 166 insertions(+) create mode 100644 clippy_lints/src/methods/type_id_on_box.rs create mode 100644 tests/ui/type_id_on_box.fixed create mode 100644 tests/ui/type_id_on_box.rs create mode 100644 tests/ui/type_id_on_box.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 269be545b0a..f82a3739259 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5230,6 +5230,7 @@ Released 2018-09-13 [`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref [`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err [`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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7690e8f7247..0a2db21ca95 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -403,6 +403,7 @@ 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, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 99c984ba65a..e98fe99eac7 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -87,6 +87,7 @@ 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; @@ -2922,6 +2923,37 @@ "use of sort() when sort_unstable() is equivalent" } +declare_clippy_lint! { + /// ### What it does + /// Looks for calls to ` 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` 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!). + /// + /// ### Example + /// ```rust,ignore + /// use std::any::{Any, TypeId}; + /// + /// let any_box: Box = Box::new(42_i32); + /// assert_eq!(any_box.type_id(), TypeId::of::()); // ⚠️ this fails! + /// ``` + /// Use instead: + /// ```rust + /// use std::any::{Any, TypeId}; + /// + /// let any_box: Box = Box::new(42_i32); + /// assert_eq!((*any_box).type_id(), TypeId::of::()); + /// // ^ dereference first, to call `type_id` on `dyn Any` + /// ``` + #[clippy::version = "1.47.0"] + pub TYPE_ID_ON_BOX, + suspicious, + "calling `.type_id()` on `Box`" +} + declare_clippy_lint! { /// ### What it does /// Detects `().hash(_)`. @@ -3878,6 +3910,9 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { ("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], _, _)) => { diff --git a/clippy_lints/src/methods/type_id_on_box.rs b/clippy_lints/src/methods/type_id_on_box.rs new file mode 100644 index 00000000000..8ccbd0d8d1b --- /dev/null +++ b/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_trait(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_trait(cx, substs.type_at(0)) + { + span_lint_and_then( + cx, + TYPE_ID_ON_BOX, + call_span, + "calling `.type_id()` on a `Box`", + |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( + "if this is intentional, use `TypeId::of::>()` instead, \ + which makes it more clear" + ) + .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 new file mode 100644 index 00000000000..fc177afacd4 --- /dev/null +++ b/tests/ui/type_id_on_box.fixed @@ -0,0 +1,20 @@ +//@run-rustfix + +#![warn(clippy::type_id_on_box)] + +use std::any::{Any, TypeId}; + +fn existential() -> impl Any { + Box::new(1) as Box +} + +fn main() { + let any_box: Box = Box::new(0usize); + let _ = (*any_box).type_id(); + let _ = TypeId::of::>(); // don't lint, user probably explicitly wants to do this + let _ = (*any_box).type_id(); + let any_box: &Box = &(Box::new(0usize) as Box); + let _ = (**any_box).type_id(); // 2 derefs are needed here + let b = existential(); + let _ = b.type_id(); // don't lint +} diff --git a/tests/ui/type_id_on_box.rs b/tests/ui/type_id_on_box.rs new file mode 100644 index 00000000000..36cf631a092 --- /dev/null +++ b/tests/ui/type_id_on_box.rs @@ -0,0 +1,20 @@ +//@run-rustfix + +#![warn(clippy::type_id_on_box)] + +use std::any::{Any, TypeId}; + +fn existential() -> impl Any { + Box::new(1) as Box +} + +fn main() { + let any_box: Box = Box::new(0usize); + let _ = any_box.type_id(); + let _ = TypeId::of::>(); // don't lint, user probably explicitly wants to do this + let _ = (*any_box).type_id(); + let any_box: &Box = &(Box::new(0usize) as Box); + let _ = any_box.type_id(); // 2 derefs are needed here + let b = existential(); + let _ = b.type_id(); // don't lint +} diff --git a/tests/ui/type_id_on_box.stderr b/tests/ui/type_id_on_box.stderr new file mode 100644 index 00000000000..47e0e814daf --- /dev/null +++ b/tests/ui/type_id_on_box.stderr @@ -0,0 +1,25 @@ +error: calling `.type_id()` on a `Box` + --> $DIR/type_id_on_box.rs:13: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: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear + = note: `-D clippy::type-id-on-box` implied by `-D warnings` + +error: calling `.type_id()` on a `Box` + --> $DIR/type_id_on_box.rs:17:13 + | +LL | let _ = any_box.type_id(); // 2 derefs are needed here + | -------^^^^^^^^^^ + | | + | 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: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear + +error: aborting due to 2 previous errors + From 9ab1e8e95c45cbe9cdf657bded08a7d874e48fbd Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 18 Jun 2023 22:27:20 +0200 Subject: [PATCH 2/5] that was definitely not the right version --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index e98fe99eac7..255a8cc0a95 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2948,7 +2948,7 @@ /// assert_eq!((*any_box).type_id(), TypeId::of::()); /// // ^ dereference first, to call `type_id` on `dyn Any` /// ``` - #[clippy::version = "1.47.0"] + #[clippy::version = "1.72.0"] pub TYPE_ID_ON_BOX, suspicious, "calling `.type_id()` on `Box`" From 26ac76c15f4a12b2debde692dfff02a08d516f6e Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 18 Jun 2023 22:36:08 +0200 Subject: [PATCH 3/5] add it to the methods lint pass --- clippy_lints/src/methods/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 255a8cc0a95..6c0e15d16d7 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3389,6 +3389,7 @@ pub fn new( STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, ITER_WITH_DRAIN, + TYPE_ID_ON_BOX, USELESS_ASREF, UNNECESSARY_FOLD, UNNECESSARY_FILTER_MAP, From b0dfecd8c13aaf51946ce43db8a6ed398b9a5bf5 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 19 Jun 2023 00:36:16 +0200 Subject: [PATCH 4/5] add a few more test cases --- tests/ui/type_id_on_box.fixed | 26 +++++++++++++++++++++++--- tests/ui/type_id_on_box.rs | 26 +++++++++++++++++++++++--- tests/ui/type_id_on_box.stderr | 19 +++++++++++++++---- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/tests/ui/type_id_on_box.fixed b/tests/ui/type_id_on_box.fixed index fc177afacd4..615d809c897 100644 --- a/tests/ui/type_id_on_box.fixed +++ b/tests/ui/type_id_on_box.fixed @@ -3,6 +3,19 @@ #![warn(clippy::type_id_on_box)] use std::any::{Any, TypeId}; +use std::ops::Deref; + +type SomeBox = Box; + +struct BadBox(Box); + +impl Deref for BadBox { + type Target = Box; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} fn existential() -> impl Any { Box::new(1) as Box @@ -11,10 +24,17 @@ fn existential() -> impl Any { fn main() { let any_box: Box = Box::new(0usize); let _ = (*any_box).type_id(); - let _ = TypeId::of::>(); // don't lint, user probably explicitly wants to do this + let _ = TypeId::of::>(); // Don't lint. We explicitly say "do this instead" if this is intentional let _ = (*any_box).type_id(); let any_box: &Box = &(Box::new(0usize) as Box); - let _ = (**any_box).type_id(); // 2 derefs are needed here + 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.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 `::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 36cf631a092..74b6c74ae5f 100644 --- a/tests/ui/type_id_on_box.rs +++ b/tests/ui/type_id_on_box.rs @@ -3,6 +3,19 @@ #![warn(clippy::type_id_on_box)] use std::any::{Any, TypeId}; +use std::ops::Deref; + +type SomeBox = Box; + +struct BadBox(Box); + +impl Deref for BadBox { + type Target = Box; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} fn existential() -> impl Any { Box::new(1) as Box @@ -11,10 +24,17 @@ fn existential() -> impl Any { fn main() { let any_box: Box = Box::new(0usize); let _ = any_box.type_id(); - let _ = TypeId::of::>(); // don't lint, user probably explicitly wants to do this + let _ = TypeId::of::>(); // Don't lint. We explicitly say "do this instead" if this is intentional let _ = (*any_box).type_id(); let any_box: &Box = &(Box::new(0usize) as Box); - let _ = any_box.type_id(); // 2 derefs are needed here + 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.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 `::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 47e0e814daf..1525328c0d0 100644 --- a/tests/ui/type_id_on_box.stderr +++ b/tests/ui/type_id_on_box.stderr @@ -1,5 +1,5 @@ error: calling `.type_id()` on a `Box` - --> $DIR/type_id_on_box.rs:13:13 + --> $DIR/type_id_on_box.rs:26:13 | LL | let _ = any_box.type_id(); | -------^^^^^^^^^^ @@ -11,9 +11,9 @@ LL | let _ = any_box.type_id(); = note: `-D clippy::type-id-on-box` implied by `-D warnings` error: calling `.type_id()` on a `Box` - --> $DIR/type_id_on_box.rs:17:13 + --> $DIR/type_id_on_box.rs:30:13 | -LL | let _ = any_box.type_id(); // 2 derefs are needed here +LL | let _ = any_box.type_id(); // 2 derefs are needed here to get to the `dyn Any` | -------^^^^^^^^^^ | | | help: consider dereferencing first: `(**any_box)` @@ -21,5 +21,16 @@ LL | let _ = any_box.type_id(); // 2 derefs are needed here = 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 +error: calling `.type_id()` on a `Box` + --> $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` 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 From 1b6738ba39bf5ecd4d32f95a53966662baabc1e0 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Tue, 20 Jun 2023 17:33:32 +0200 Subject: [PATCH 5/5] s/is_dyn_trait/is_dyn_any --- clippy_lints/src/methods/type_id_on_box.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/type_id_on_box.rs b/clippy_lints/src/methods/type_id_on_box.rs index 8ccbd0d8d1b..88adba504aa 100644 --- a/clippy_lints/src/methods/type_id_on_box.rs +++ b/clippy_lints/src/methods/type_id_on_box.rs @@ -10,7 +10,7 @@ use rustc_middle::ty::{self, ExistentialPredicate}; use rustc_span::{sym, Span}; -fn is_dyn_trait(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { +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), @@ -28,7 +28,7 @@ pub(super) fn check(cx: &LateContext<'_>, receiver: &Expr<'_>, call_span: Span) && let ty::Ref(_, ty, _) = recv_ty.kind() && let ty::Adt(adt, substs) = ty.kind() && adt.is_box() - && is_dyn_trait(cx, substs.type_at(0)) + && is_dyn_any(cx, substs.type_at(0)) { span_lint_and_then( cx,