From eef8a8c17374e160d7e8681e7d397a4814b4a515 Mon Sep 17 00:00:00 2001 From: Benjamin Saunders Date: Thu, 15 May 2025 10:22:43 +0200 Subject: [PATCH] Introduce coerce_any_ref_to_any --- CHANGELOG.md | 1 + clippy_lints/src/coerce_any_ref_to_any.rs | 108 ++++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + rust-toolchain.toml | 2 +- tests/ui/coerce_any_ref_to_any.fixed | 25 +++++ tests/ui/coerce_any_ref_to_any.rs | 25 +++++ tests/ui/coerce_any_ref_to_any.stderr | 23 +++++ 8 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/coerce_any_ref_to_any.rs create mode 100644 tests/ui/coerce_any_ref_to_any.fixed create mode 100644 tests/ui/coerce_any_ref_to_any.rs create mode 100644 tests/ui/coerce_any_ref_to_any.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 28147dfbea3e..b6f21c2811ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5685,6 +5685,7 @@ Released 2018-09-13 [`cmp_nan`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_nan [`cmp_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_null [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned +[`coerce_any_ref_to_any`]: https://rust-lang.github.io/rust-clippy/master/index.html#coerce_any_ref_to_any [`cognitive_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity [`collapsible_else_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if diff --git a/clippy_lints/src/coerce_any_ref_to_any.rs b/clippy_lints/src/coerce_any_ref_to_any.rs new file mode 100644 index 000000000000..f877f34571b7 --- /dev/null +++ b/clippy_lints/src/coerce_any_ref_to_any.rs @@ -0,0 +1,108 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet; +use clippy_utils::sym; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, ExistentialPredicate, Ty, TyCtxt}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// + /// Protects against unintended coercion of references to container types to `&dyn Any` + /// when the container type dereferences to a `dyn Any` which could be reborrowed instead. + /// + /// ### Why is this bad? + /// + /// The intention is usually to get a reference to the `dyn Any` the value derefernces to, + /// rather than getting a `&dyn Any` which is actually referring to the outer container type. + /// + /// ### Example + /// + /// Here we see that because `Box` itself implements `Any`, `&Box` + /// gets coerced to an `&dyn Any` which refers to *the `Box` itself`*, rather than the + /// inner `dyn Any`. + /// ```no_run + /// # use std::any::Any; + /// let x: Box = Box::new(0u32); + /// let dyn_any_of_box: &dyn Any = &x; + /// + /// // Fails as we have a &dyn Any to the Box, not the u32 + /// assert_eq!(dyn_any_of_box.downcast_ref::(), None); + /// ``` + /// Use instead: + /// ```no_run + /// # use std::any::Any; + /// let x: Box = Box::new(0u32); + /// let dyn_any_of_u32: &dyn Any = &*x; + /// + /// // Succeeds since we have a &dyn Any to the inner u32! + /// assert_eq!(dyn_any_of_u32.downcast_ref::(), Some(&0u32)); + /// ``` + #[clippy::version = "1.88.0"] + pub COERCE_ANY_REF_TO_ANY, + nursery, + "coercing to `&dyn Any` when dereferencing could produce a `dyn Any` without coercion is usually not intended" +} +declare_lint_pass!(CoerceAnyRefToAny => [COERCE_ANY_REF_TO_ANY]); + +impl<'tcx> LateLintPass<'tcx> for CoerceAnyRefToAny { + fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { + // If this expression has an effective type of `&dyn Any` ... + { + let coerced_ty = cx.typeck_results().expr_ty_adjusted(e); + + let ty::Ref(_, coerced_ref_ty, _) = *coerced_ty.kind() else { + return; + }; + if !is_dyn_any(cx.tcx, coerced_ref_ty) { + return; + } + } + + let expr_ty = cx.typeck_results().expr_ty(e); + let ty::Ref(_, expr_ref_ty, _) = *expr_ty.kind() else { + return; + }; + // ... but only due to coercion ... + if is_dyn_any(cx.tcx, expr_ref_ty) { + return; + } + // ... and it also *derefs* to `dyn Any` ... + let Some((depth, target)) = clippy_utils::ty::deref_chain(cx, expr_ref_ty).enumerate().last() else { + return; + }; + if !is_dyn_any(cx.tcx, target) { + return; + } + + // ... that's probably not intended. + let (span, deref_count) = match e.kind { + // If `e` was already an `&` expression, skip `*&` in the suggestion + ExprKind::AddrOf(_, _, referent) => (referent.span, depth), + _ => (e.span, depth + 1), + }; + span_lint_and_sugg( + cx, + COERCE_ANY_REF_TO_ANY, + e.span, + format!("coercing `{expr_ty}` to `&dyn Any` rather than dereferencing to the `dyn Any` inside"), + "consider dereferencing", + format!("&{}{}", str::repeat("*", deref_count), snippet(cx, span, "x")), + Applicability::MaybeIncorrect, + ); + } +} + +fn is_dyn_any(tcx: TyCtxt<'_>, ty: Ty<'_>) -> bool { + let ty::Dynamic(traits, ..) = ty.kind() else { + return false; + }; + traits.iter().any(|binder| { + let Some(ExistentialPredicate::Trait(t)) = binder.no_bound_vars() else { + return false; + }; + tcx.is_diagnostic_item(sym::Any, t.def_id) + }) +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index bb825c7655f8..27167f99475a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -77,6 +77,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::cfg_not_test::CFG_NOT_TEST_INFO, crate::checked_conversions::CHECKED_CONVERSIONS_INFO, crate::cloned_ref_to_slice_refs::CLONED_REF_TO_SLICE_REFS_INFO, + crate::coerce_any_ref_to_any::COERCE_ANY_REF_TO_ANY_INFO, crate::cognitive_complexity::COGNITIVE_COMPLEXITY_INFO, crate::collapsible_if::COLLAPSIBLE_ELSE_IF_INFO, crate::collapsible_if::COLLAPSIBLE_IF_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 006145cc623c..9435e7afa999 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -95,6 +95,7 @@ mod casts; mod cfg_not_test; mod checked_conversions; mod cloned_ref_to_slice_refs; +mod coerce_any_ref_to_any; mod cognitive_complexity; mod collapsible_if; mod collection_is_never_read; @@ -944,5 +945,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap)); store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix)); store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf))); + store.register_late_pass(|_| Box::new(coerce_any_ref_to_any::CoerceAnyRefToAny)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 39c7f0e4ad5a..6d59b4228078 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -2,5 +2,5 @@ # begin autogenerated nightly channel = "nightly-2025-05-01" # end autogenerated nightly -components = ["cargo", "llvm-tools", "rust-src", "rust-std", "rustc", "rustc-dev", "rustfmt"] +components = ["cargo", "llvm-tools", "rust-src", "rust-std", "rustc", "rustc-dev", "rustfmt", "rust-analyzer"] profile = "minimal" diff --git a/tests/ui/coerce_any_ref_to_any.fixed b/tests/ui/coerce_any_ref_to_any.fixed new file mode 100644 index 000000000000..31d9bd48fa21 --- /dev/null +++ b/tests/ui/coerce_any_ref_to_any.fixed @@ -0,0 +1,25 @@ +#![warn(clippy::coerce_any_ref_to_any)] + +use std::any::Any; + +fn main() { + let x: Box = Box::new(()); + let ref_x = &x; + + f(&*x); + //~^ coerce_any_ref_to_any + + f(&**ref_x); + //~^ coerce_any_ref_to_any + + let _: &dyn Any = &*x; + //~^ coerce_any_ref_to_any + + f(&42); + f(&Box::new(())); + f(&**ref_x); + f(&*x); + let _: &dyn Any = &*x; +} + +fn f(_: &dyn Any) {} diff --git a/tests/ui/coerce_any_ref_to_any.rs b/tests/ui/coerce_any_ref_to_any.rs new file mode 100644 index 000000000000..c93fe0e69ccd --- /dev/null +++ b/tests/ui/coerce_any_ref_to_any.rs @@ -0,0 +1,25 @@ +#![warn(clippy::coerce_any_ref_to_any)] + +use std::any::Any; + +fn main() { + let x: Box = Box::new(()); + let ref_x = &x; + + f(&x); + //~^ coerce_any_ref_to_any + + f(ref_x); + //~^ coerce_any_ref_to_any + + let _: &dyn Any = &x; + //~^ coerce_any_ref_to_any + + f(&42); + f(&Box::new(())); + f(&**ref_x); + f(&*x); + let _: &dyn Any = &*x; +} + +fn f(_: &dyn Any) {} diff --git a/tests/ui/coerce_any_ref_to_any.stderr b/tests/ui/coerce_any_ref_to_any.stderr new file mode 100644 index 000000000000..fe5884fa3828 --- /dev/null +++ b/tests/ui/coerce_any_ref_to_any.stderr @@ -0,0 +1,23 @@ +error: coercing `&std::boxed::Box` to `&dyn Any` rather than dereferencing to the `dyn Any` inside + --> tests/ui/coerce_any_ref_to_any.rs:9:7 + | +LL | f(&x); + | ^^ help: consider dereferencing: `&*x` + | + = note: `-D clippy::coerce-any-ref-to-any` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::coerce_any_ref_to_any)]` + +error: coercing `&std::boxed::Box` to `&dyn Any` rather than dereferencing to the `dyn Any` inside + --> tests/ui/coerce_any_ref_to_any.rs:12:7 + | +LL | f(ref_x); + | ^^^^^ help: consider dereferencing: `&**ref_x` + +error: coercing `&std::boxed::Box` to `&dyn Any` rather than dereferencing to the `dyn Any` inside + --> tests/ui/coerce_any_ref_to_any.rs:15:23 + | +LL | let _: &dyn Any = &x; + | ^^ help: consider dereferencing: `&*x` + +error: aborting due to 3 previous errors +