From 2ba192695527b1633f63396b3e12bbbdf7b19fef Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 14 Jun 2023 16:52:02 +0200 Subject: [PATCH 1/4] [`derivable_impls`]: don't lint if expr unsize-coerces --- clippy_lints/src/derivable_impls.rs | 38 +++++++++++++++++++++++++---- tests/ui/derivable_impls.fixed | 21 ++++++++++++++++ tests/ui/derivable_impls.rs | 21 ++++++++++++++++ 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/derivable_impls.rs b/clippy_lints/src/derivable_impls.rs index 8f68f90a2a130..618214f4aa25a 100644 --- a/clippy_lints/src/derivable_impls.rs +++ b/clippy_lints/src/derivable_impls.rs @@ -8,7 +8,8 @@ use rustc_hir::{ Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, PathSegment, QPath, Ty, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{Adt, AdtDef, SubstsRef}; +use rustc_middle::ty::adjustment::{Adjust, PointerCast}; +use rustc_middle::ty::{self, Adt, AdtDef, SubstsRef, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::sym; @@ -75,6 +76,19 @@ fn is_path_self(e: &Expr<'_>) -> bool { } } +fn contains_trait_object(cx: &LateContext<'_>, ty: ty::Ty<'_>) -> bool { + match ty.kind() { + ty::TyKind::Ref(_, ty, _) => { + contains_trait_object(cx, *ty) + }, + ty::TyKind::Adt(def, substs) => { + def.is_box() && substs[0].as_type().map_or(false, |ty| contains_trait_object(cx, ty)) + } + ty::TyKind::Dynamic(..) => true, + _ => false, + } +} + fn check_struct<'tcx>( cx: &LateContext<'tcx>, item: &'tcx Item<'_>, @@ -82,6 +96,7 @@ fn check_struct<'tcx>( func_expr: &Expr<'_>, adt_def: AdtDef<'_>, substs: SubstsRef<'_>, + typeck_results: &'tcx TypeckResults<'tcx>, ) { if let TyKind::Path(QPath::Resolved(_, p)) = self_ty.kind { if let Some(PathSegment { args, .. }) = p.segments.last() { @@ -96,10 +111,23 @@ fn check_struct<'tcx>( } } } + + // the default() call might unsize coerce to a trait object (e.g. Box<T> to Box<dyn Trait>), + // which would not be the same if derived (see #10158). + // this closure checks both if the expr is equivalent to a `default()` call and does not + // have such coercions. + let is_default_without_adjusts = |expr| { + is_default_equivalent(cx, expr) + && typeck_results.expr_adjustments(expr).iter().all(|adj| { + !matches!(adj.kind, Adjust::Pointer(PointerCast::Unsize) + if contains_trait_object(cx, adj.target)) + }) + }; + let should_emit = match peel_blocks(func_expr).kind { - ExprKind::Tup(fields) => fields.iter().all(|e| is_default_equivalent(cx, e)), - ExprKind::Call(callee, args) if is_path_self(callee) => args.iter().all(|e| is_default_equivalent(cx, e)), - ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_equivalent(cx, ef.expr)), + ExprKind::Tup(fields) => fields.iter().all(|e| is_default_without_adjusts(e)), + ExprKind::Call(callee, args) if is_path_self(callee) => args.iter().all(|e| is_default_without_adjusts(e)), + ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_without_adjusts(ef.expr)), _ => false, }; @@ -197,7 +225,7 @@ impl<'tcx> LateLintPass<'tcx> for DerivableImpls { then { if adt_def.is_struct() { - check_struct(cx, item, self_ty, func_expr, adt_def, substs); + check_struct(cx, item, self_ty, func_expr, adt_def, substs, cx.tcx.typeck_body(*b)); } else if adt_def.is_enum() && self.msrv.meets(msrvs::DEFAULT_ENUM_ATTRIBUTE) { check_enum(cx, item, func_expr, adt_def); } diff --git a/tests/ui/derivable_impls.fixed b/tests/ui/derivable_impls.fixed index aa0efb85c297a..a10f3d0107076 100644 --- a/tests/ui/derivable_impls.fixed +++ b/tests/ui/derivable_impls.fixed @@ -268,4 +268,25 @@ impl Default for OtherGenericType { } } +mod issue10158 { + pub trait T {} + + #[derive(Default)] + pub struct S {} + impl T for S {} + + pub struct Outer { + pub inner: Box<dyn T>, + } + + impl Default for Outer { + fn default() -> Self { + Outer { + // Box::<S>::default() adjusts to Box<dyn T> + inner: Box::<S>::default(), + } + } + } +} + fn main() {} diff --git a/tests/ui/derivable_impls.rs b/tests/ui/derivable_impls.rs index 8dc999ad58602..18cef1c5be895 100644 --- a/tests/ui/derivable_impls.rs +++ b/tests/ui/derivable_impls.rs @@ -304,4 +304,25 @@ impl Default for OtherGenericType { } } +mod issue10158 { + pub trait T {} + + #[derive(Default)] + pub struct S {} + impl T for S {} + + pub struct Outer { + pub inner: Box<dyn T>, + } + + impl Default for Outer { + fn default() -> Self { + Outer { + // Box::<S>::default() adjusts to Box<dyn T> + inner: Box::<S>::default(), + } + } + } +} + fn main() {} From 5b6ba204a7de1d5c694a7b25269a25ee68c98830 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 14 Jun 2023 17:01:57 +0200 Subject: [PATCH 2/4] fmt --- clippy_lints/src/derivable_impls.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/derivable_impls.rs b/clippy_lints/src/derivable_impls.rs index 618214f4aa25a..4ea58c4770963 100644 --- a/clippy_lints/src/derivable_impls.rs +++ b/clippy_lints/src/derivable_impls.rs @@ -78,12 +78,10 @@ fn is_path_self(e: &Expr<'_>) -> bool { fn contains_trait_object(cx: &LateContext<'_>, ty: ty::Ty<'_>) -> bool { match ty.kind() { - ty::TyKind::Ref(_, ty, _) => { - contains_trait_object(cx, *ty) - }, + ty::TyKind::Ref(_, ty, _) => contains_trait_object(cx, *ty), ty::TyKind::Adt(def, substs) => { def.is_box() && substs[0].as_type().map_or(false, |ty| contains_trait_object(cx, ty)) - } + }, ty::TyKind::Dynamic(..) => true, _ => false, } @@ -119,7 +117,7 @@ fn check_struct<'tcx>( let is_default_without_adjusts = |expr| { is_default_equivalent(cx, expr) && typeck_results.expr_adjustments(expr).iter().all(|adj| { - !matches!(adj.kind, Adjust::Pointer(PointerCast::Unsize) + !matches!(adj.kind, Adjust::Pointer(PointerCast::Unsize) if contains_trait_object(cx, adj.target)) }) }; From fcb9a382f70125209e3ac4b82ef0e5e67decae65 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 14 Jun 2023 17:10:26 +0200 Subject: [PATCH 3/4] dogfood --- clippy_lints/src/derivable_impls.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/derivable_impls.rs b/clippy_lints/src/derivable_impls.rs index 4ea58c4770963..55cfe0d6b8b8c 100644 --- a/clippy_lints/src/derivable_impls.rs +++ b/clippy_lints/src/derivable_impls.rs @@ -76,12 +76,10 @@ fn is_path_self(e: &Expr<'_>) -> bool { } } -fn contains_trait_object(cx: &LateContext<'_>, ty: ty::Ty<'_>) -> bool { +fn contains_trait_object(ty: ty::Ty<'_>) -> bool { match ty.kind() { - ty::TyKind::Ref(_, ty, _) => contains_trait_object(cx, *ty), - ty::TyKind::Adt(def, substs) => { - def.is_box() && substs[0].as_type().map_or(false, |ty| contains_trait_object(cx, ty)) - }, + ty::TyKind::Ref(_, ty, _) => contains_trait_object(*ty), + ty::TyKind::Adt(def, substs) => def.is_box() && substs[0].as_type().map_or(false, contains_trait_object), ty::TyKind::Dynamic(..) => true, _ => false, } @@ -118,13 +116,13 @@ fn check_struct<'tcx>( is_default_equivalent(cx, expr) && typeck_results.expr_adjustments(expr).iter().all(|adj| { !matches!(adj.kind, Adjust::Pointer(PointerCast::Unsize) - if contains_trait_object(cx, adj.target)) + if contains_trait_object(adj.target)) }) }; let should_emit = match peel_blocks(func_expr).kind { - ExprKind::Tup(fields) => fields.iter().all(|e| is_default_without_adjusts(e)), - ExprKind::Call(callee, args) if is_path_self(callee) => args.iter().all(|e| is_default_without_adjusts(e)), + ExprKind::Tup(fields) => fields.iter().all(is_default_without_adjusts), + ExprKind::Call(callee, args) if is_path_self(callee) => args.iter().all(is_default_without_adjusts), ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_without_adjusts(ef.expr)), _ => false, }; From 4795c919399610d9105fae67492d3eb65c0c1c01 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 14 Jun 2023 17:15:57 +0200 Subject: [PATCH 4/4] fix internal lints --- clippy_lints/src/derivable_impls.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/derivable_impls.rs b/clippy_lints/src/derivable_impls.rs index 55cfe0d6b8b8c..ec0ca50cfec70 100644 --- a/clippy_lints/src/derivable_impls.rs +++ b/clippy_lints/src/derivable_impls.rs @@ -4,12 +4,13 @@ use clippy_utils::source::indent_of; use clippy_utils::{is_default_equivalent, peel_blocks}; use rustc_errors::Applicability; use rustc_hir::{ + self as hir, def::{CtorKind, CtorOf, DefKind, Res}, - Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, PathSegment, QPath, Ty, TyKind, + Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, PathSegment, QPath, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, PointerCast}; -use rustc_middle::ty::{self, Adt, AdtDef, SubstsRef, TypeckResults}; +use rustc_middle::ty::{self, Adt, AdtDef, SubstsRef, Ty, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::sym; @@ -76,11 +77,11 @@ fn is_path_self(e: &Expr<'_>) -> bool { } } -fn contains_trait_object(ty: ty::Ty<'_>) -> bool { +fn contains_trait_object(ty: Ty<'_>) -> bool { match ty.kind() { - ty::TyKind::Ref(_, ty, _) => contains_trait_object(*ty), - ty::TyKind::Adt(def, substs) => def.is_box() && substs[0].as_type().map_or(false, contains_trait_object), - ty::TyKind::Dynamic(..) => true, + ty::Ref(_, ty, _) => contains_trait_object(*ty), + ty::Adt(def, substs) => def.is_box() && substs[0].as_type().map_or(false, contains_trait_object), + ty::Dynamic(..) => true, _ => false, } } @@ -88,7 +89,7 @@ fn contains_trait_object(ty: ty::Ty<'_>) -> bool { fn check_struct<'tcx>( cx: &LateContext<'tcx>, item: &'tcx Item<'_>, - self_ty: &Ty<'_>, + self_ty: &hir::Ty<'_>, func_expr: &Expr<'_>, adt_def: AdtDef<'_>, substs: SubstsRef<'_>,