diff --git a/CHANGELOG.md b/CHANGELOG.md index 28147dfbea3e..17ddcde527ed 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_container_to_any`]: https://rust-lang.github.io/rust-clippy/master/index.html#coerce_container_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_container_to_any.rs b/clippy_lints/src/coerce_container_to_any.rs new file mode 100644 index 000000000000..2a59f98a1b60 --- /dev/null +++ b/clippy_lints/src/coerce_container_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 directly referenced instead. + /// + /// ### Why is this bad? + /// + /// The intention is usually to get a reference to the `dyn Any` the value dereferences to, + /// rather than coercing a reference to the container itself to `&dyn Any`. + /// + /// ### Example + /// + /// Because `Box` itself implements `Any`, `&Box` + /// can be 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_CONTAINER_TO_ANY, + nursery, + "coercing to `&dyn Any` when dereferencing could produce a `dyn Any` without coercion is usually not intended" +} +declare_lint_pass!(CoerceContainerToAny => [COERCE_CONTAINER_TO_ANY]); + +impl<'tcx> LateLintPass<'tcx> for CoerceContainerToAny { + 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_CONTAINER_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 ExistentialPredicate::Trait(t) = binder.skip_binder() 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..64b7edaf67ad 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_container_to_any::COERCE_CONTAINER_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..139afe2317b6 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_container_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_container_to_any::CoerceContainerToAny)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/coerce_container_to_any.fixed b/tests/ui/coerce_container_to_any.fixed new file mode 100644 index 000000000000..a1596cdaf126 --- /dev/null +++ b/tests/ui/coerce_container_to_any.fixed @@ -0,0 +1,25 @@ +#![warn(clippy::coerce_container_to_any)] + +use std::any::Any; + +fn main() { + let x: Box = Box::new(()); + let ref_x = &x; + + f(&*x); + //~^ coerce_container_to_any + + f(&**ref_x); + //~^ coerce_container_to_any + + let _: &dyn Any = &*x; + //~^ coerce_container_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_container_to_any.rs b/tests/ui/coerce_container_to_any.rs new file mode 100644 index 000000000000..6f542e641cad --- /dev/null +++ b/tests/ui/coerce_container_to_any.rs @@ -0,0 +1,25 @@ +#![warn(clippy::coerce_container_to_any)] + +use std::any::Any; + +fn main() { + let x: Box = Box::new(()); + let ref_x = &x; + + f(&x); + //~^ coerce_container_to_any + + f(ref_x); + //~^ coerce_container_to_any + + let _: &dyn Any = &x; + //~^ coerce_container_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_container_to_any.stderr b/tests/ui/coerce_container_to_any.stderr new file mode 100644 index 000000000000..9f50025b03ee --- /dev/null +++ b/tests/ui/coerce_container_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_container_to_any.rs:9:7 + | +LL | f(&x); + | ^^ help: consider dereferencing: `&*x` + | + = note: `-D clippy::coerce-container-to-any` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::coerce_container_to_any)]` + +error: coercing `&std::boxed::Box` to `&dyn Any` rather than dereferencing to the `dyn Any` inside + --> tests/ui/coerce_container_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_container_to_any.rs:15:23 + | +LL | let _: &dyn Any = &x; + | ^^ help: consider dereferencing: `&*x` + +error: aborting due to 3 previous errors +