From 84e03b621522799a72f55d3e728d63aebb561139 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 10 Jul 2022 15:04:29 -0400 Subject: [PATCH] Don't lint `explicit_auto_deref` on `dyn Trait` return --- clippy_lints/src/dereference.rs | 122 ++++++++++++++-------------- clippy_utils/src/ty.rs | 51 +++++++----- tests/ui/explicit_auto_deref.fixed | 9 ++ tests/ui/explicit_auto_deref.rs | 9 ++ tests/ui/explicit_auto_deref.stderr | 8 +- 5 files changed, 119 insertions(+), 80 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 8495d2c6df7..40e62dbd586 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -668,14 +668,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .. }) if span.ctxt() == ctxt => { let ty = cx.tcx.type_of(def_id); - Some(if let ty::Ref(_, ty, _) = *ty.kind() { - Position::DerefStable( - precedence, - ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), - ) - } else { - Position::Other(precedence) - }) + Some(ty_auto_deref_stability(cx, ty, precedence).position_for_result(cx)) }, Node::Item(&Item { @@ -699,18 +692,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & let output = cx .tcx .erase_late_bound_regions(cx.tcx.fn_sig(def_id.to_def_id()).output()); - Some(if let ty::Ref(_, ty, _) = *output.kind() { - if ty.has_placeholders() || ty.has_opaque_types() { - Position::ReborrowStable(precedence) - } else { - Position::DerefStable( - precedence, - ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), - ) - } - } else { - Position::Other(precedence) - }) + Some(ty_auto_deref_stability(cx, output, precedence).position_for_result(cx)) }, Node::Expr(parent) if parent.span.ctxt() == ctxt => match parent.kind { @@ -730,18 +712,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & let output = cx .tcx .erase_late_bound_regions(cx.tcx.fn_sig(cx.tcx.hir().local_def_id(owner_id)).output()); - if let ty::Ref(_, ty, _) = *output.kind() { - if ty.has_placeholders() || ty.has_opaque_types() { - Position::ReborrowStable(precedence) - } else { - Position::DerefStable( - precedence, - ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), - ) - } - } else { - Position::Other(precedence) - } + ty_auto_deref_stability(cx, output, precedence).position_for_result(cx) }, ) }, @@ -757,7 +728,8 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & // Type inference for closures can depend on how they're called. Only go by the explicit // types here. Some(ty) => binding_ty_auto_deref_stability(cx, ty, precedence), - None => param_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence), + None => ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence) + .position_for_arg(), }), ExprKind::MethodCall(_, args, _) => { let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(); @@ -798,11 +770,12 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & Position::MethodReceiver } } else { - param_auto_deref_stability( + ty_auto_deref_stability( cx, cx.tcx.erase_late_bound_regions(cx.tcx.fn_sig(id).input(i)), precedence, ) + .position_for_arg() } }) }, @@ -813,7 +786,9 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .find(|f| f.expr.hir_id == child_id) .zip(variant) .and_then(|(field, variant)| variant.fields.iter().find(|f| f.name == field.ident.name)) - .map(|field| param_auto_deref_stability(cx, cx.tcx.type_of(field.did), precedence)) + .map(|field| { + ty_auto_deref_stability(cx, cx.tcx.type_of(field.did), precedence).position_for_arg() + }) }, ExprKind::Field(child, name) if child.hir_id == e.hir_id => Some(Position::FieldAccess(name.name)), ExprKind::Unary(UnOp::Deref, child) if child.hir_id == e.hir_id => Some(Position::Deref), @@ -890,17 +865,17 @@ fn binding_ty_auto_deref_stability(cx: &LateContext<'_>, ty: &hir::Ty<'_>, prece | TyKind::Never | TyKind::Tup(_) | TyKind::Ptr(_) - | TyKind::TraitObject(..) | TyKind::Path(_) => Position::DerefStable( precedence, cx - .typeck_results() - .node_type(ty.ty.hir_id) - .is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), + .typeck_results() + .node_type(ty.ty.hir_id) + .is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), ), TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) + | TyKind::TraitObject(..) | TyKind::Err => Position::ReborrowStable(precedence), }; } @@ -937,10 +912,39 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { v.0 } +struct TyPosition<'tcx> { + position: Position, + ty: Option>, +} +impl From for TyPosition<'_> { + fn from(position: Position) -> Self { + Self { position, ty: None } + } +} +impl<'tcx> TyPosition<'tcx> { + fn new_deref_stable_for_result(precedence: i8, ty: Ty<'tcx>) -> Self { + Self { + position: Position::ReborrowStable(precedence), + ty: Some(ty), + } + } + fn position_for_result(self, cx: &LateContext<'tcx>) -> Position { + match (self.position, self.ty) { + (Position::ReborrowStable(precedence), Some(ty)) => { + Position::DerefStable(precedence, ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env)) + }, + (position, _) => position, + } + } + fn position_for_arg(self) -> Position { + self.position + } +} + // Checks whether a type is stable when switching to auto dereferencing, -fn param_auto_deref_stability<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, precedence: i8) -> Position { +fn ty_auto_deref_stability<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, precedence: i8) -> TyPosition<'tcx> { let ty::Ref(_, mut ty, _) = *ty.kind() else { - return Position::Other(precedence); + return Position::Other(precedence).into(); }; loop { @@ -949,38 +953,38 @@ fn param_auto_deref_stability<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, preced ty = ref_ty; continue; }, - ty::Infer(_) - | ty::Error(_) - | ty::Param(_) - | ty::Bound(..) - | ty::Opaque(..) - | ty::Placeholder(_) - | ty::Dynamic(..) => Position::ReborrowStable(precedence), - ty::Adt(..) if ty.has_placeholders() || ty.has_param_types_or_consts() => { - Position::ReborrowStable(precedence) + ty::Param(_) => TyPosition::new_deref_stable_for_result(precedence, ty), + ty::Infer(_) | ty::Error(_) | ty::Bound(..) | ty::Opaque(..) | ty::Placeholder(_) | ty::Dynamic(..) => { + Position::ReborrowStable(precedence).into() }, - ty::Adt(..) - | ty::Bool + ty::Adt(..) if ty.has_placeholders() || ty.has_opaque_types() => { + Position::ReborrowStable(precedence).into() + }, + ty::Adt(_, substs) if substs.has_param_types_or_consts() => { + TyPosition::new_deref_stable_for_result(precedence, ty) + }, + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) - | ty::Float(_) - | ty::Foreign(_) - | ty::Str | ty::Array(..) - | ty::Slice(..) + | ty::Float(_) | ty::RawPtr(..) + | ty::FnPtr(_) => Position::DerefStable(precedence, true).into(), + ty::Str | ty::Slice(..) => Position::DerefStable(precedence, false).into(), + ty::Adt(..) + | ty::Foreign(_) | ty::FnDef(..) - | ty::FnPtr(_) - | ty::Closure(..) | ty::Generator(..) | ty::GeneratorWitness(..) + | ty::Closure(..) | ty::Never | ty::Tuple(_) | ty::Projection(_) => Position::DerefStable( precedence, ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), - ), + ) + .into(), }; } } diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index a05d633d980..d394360fc67 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -503,7 +503,7 @@ pub fn all_predicates_of(tcx: TyCtxt<'_>, id: DefId) -> impl Iterator { Sig(Binder<'tcx, FnSig<'tcx>>, Option), Closure(Option<&'tcx FnDecl<'tcx>>, Binder<'tcx, FnSig<'tcx>>), - Trait(Binder<'tcx, Ty<'tcx>>, Option>>), + Trait(Binder<'tcx, Ty<'tcx>>, Option>>, Option), } impl<'tcx> ExprFnSig<'tcx> { /// Gets the argument type at the given offset. This will return `None` when the index is out of @@ -518,7 +518,7 @@ impl<'tcx> ExprFnSig<'tcx> { } }, Self::Closure(_, sig) => Some(sig.input(0).map_bound(|ty| ty.tuple_fields()[i])), - Self::Trait(inputs, _) => Some(inputs.map_bound(|ty| ty.tuple_fields()[i])), + Self::Trait(inputs, _, _) => Some(inputs.map_bound(|ty| ty.tuple_fields()[i])), } } @@ -541,7 +541,7 @@ impl<'tcx> ExprFnSig<'tcx> { decl.and_then(|decl| decl.inputs.get(i)), sig.input(0).map_bound(|ty| ty.tuple_fields()[i]), )), - Self::Trait(inputs, _) => Some((None, inputs.map_bound(|ty| ty.tuple_fields()[i]))), + Self::Trait(inputs, _, _) => Some((None, inputs.map_bound(|ty| ty.tuple_fields()[i]))), } } @@ -550,12 +550,16 @@ impl<'tcx> ExprFnSig<'tcx> { pub fn output(self) -> Option>> { match self { Self::Sig(sig, _) | Self::Closure(_, sig) => Some(sig.output()), - Self::Trait(_, output) => output, + Self::Trait(_, output, _) => output, } } pub fn predicates_id(&self) -> Option { - if let ExprFnSig::Sig(_, id) = *self { id } else { None } + if let ExprFnSig::Sig(_, id) | ExprFnSig::Trait(_, _, id) = *self { + id + } else { + None + } } } @@ -580,7 +584,7 @@ fn ty_sig<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> Some(ExprFnSig::Closure(decl, subs.as_closure().sig())) }, ty::FnDef(id, subs) => Some(ExprFnSig::Sig(cx.tcx.bound_fn_sig(id).subst(cx.tcx, subs), Some(id))), - ty::Opaque(id, _) => ty_sig(cx, cx.tcx.type_of(id)), + ty::Opaque(id, _) => sig_from_bounds(cx, ty, cx.tcx.item_bounds(id), cx.tcx.opt_parent(id)), ty::FnPtr(sig) => Some(ExprFnSig::Sig(sig, None)), ty::Dynamic(bounds, _) => { let lang_items = cx.tcx.lang_items(); @@ -594,26 +598,31 @@ fn ty_sig<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> .projection_bounds() .find(|p| lang_items.fn_once_output().map_or(false, |id| id == p.item_def_id())) .map(|p| p.map_bound(|p| p.term.ty().unwrap())); - Some(ExprFnSig::Trait(bound.map_bound(|b| b.substs.type_at(0)), output)) + Some(ExprFnSig::Trait(bound.map_bound(|b| b.substs.type_at(0)), output, None)) }, _ => None, } }, ty::Projection(proj) => match cx.tcx.try_normalize_erasing_regions(cx.param_env, ty) { Ok(normalized_ty) if normalized_ty != ty => ty_sig(cx, normalized_ty), - _ => sig_for_projection(cx, proj).or_else(|| sig_from_bounds(cx, ty)), + _ => sig_for_projection(cx, proj).or_else(|| sig_from_bounds(cx, ty, cx.param_env.caller_bounds(), None)), }, - ty::Param(_) => sig_from_bounds(cx, ty), + ty::Param(_) => sig_from_bounds(cx, ty, cx.param_env.caller_bounds(), None), _ => None, } } -fn sig_from_bounds<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { +fn sig_from_bounds<'tcx>( + cx: &LateContext<'tcx>, + ty: Ty<'tcx>, + predicates: &'tcx [Predicate<'tcx>], + predicates_id: Option, +) -> Option> { let mut inputs = None; let mut output = None; let lang_items = cx.tcx.lang_items(); - for (pred, _) in all_predicates_of(cx.tcx, cx.typeck_results().hir_owner.to_def_id()) { + for pred in predicates { match pred.kind().skip_binder() { PredicateKind::Trait(p) if (lang_items.fn_trait() == Some(p.def_id()) @@ -621,11 +630,12 @@ fn sig_from_bounds<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option { - if inputs.is_some() { + let i = pred.kind().rebind(p.trait_ref.substs.type_at(1)); + if inputs.map_or(false, |inputs| i != inputs) { // Multiple different fn trait impls. Is this even allowed? return None; } - inputs = Some(pred.kind().rebind(p.trait_ref.substs.type_at(1))); + inputs = Some(i); }, PredicateKind::Projection(p) if Some(p.projection_ty.item_def_id) == lang_items.fn_once_output() @@ -641,7 +651,7 @@ fn sig_from_bounds<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option(cx: &LateContext<'tcx>, ty: ProjectionTy<'tcx>) -> Option> { @@ -661,14 +671,15 @@ fn sig_for_projection<'tcx>(cx: &LateContext<'tcx>, ty: ProjectionTy<'tcx>) -> O || lang_items.fn_mut_trait() == Some(p.def_id()) || lang_items.fn_once_trait() == Some(p.def_id())) => { - if inputs.is_some() { + let i = pred + .map_bound(|pred| pred.kind().rebind(p.trait_ref.substs.type_at(1))) + .subst(cx.tcx, ty.substs); + + if inputs.map_or(false, |inputs| inputs != i) { // Multiple different fn trait impls. Is this even allowed? return None; } - inputs = Some( - pred.map_bound(|pred| pred.kind().rebind(p.trait_ref.substs.type_at(1))) - .subst(cx.tcx, ty.substs), - ); + inputs = Some(i); }, PredicateKind::Projection(p) if Some(p.projection_ty.item_def_id) == lang_items.fn_once_output() => { if output.is_some() { @@ -684,7 +695,7 @@ fn sig_for_projection<'tcx>(cx: &LateContext<'tcx>, ty: ProjectionTy<'tcx>) -> O } } - inputs.map(|ty| ExprFnSig::Trait(ty, output)) + inputs.map(|ty| ExprFnSig::Trait(ty, output, None)) } #[derive(Clone, Copy)] diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index d3efc299414..464dd2dc0da 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -233,4 +233,13 @@ fn main() { _ => panic!(), }; let _: &X = &*if true { Y(X) } else { panic!() }; + + fn deref_to_u>(x: &T) -> &U { + x + } + + let _ = |x: &'static Box>| -> &'static dyn Iterator { &**x }; + fn ret_any(x: &Box) -> &dyn std::any::Any { + &**x + } } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index e25ccbe78ce..453b90f09b3 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -233,4 +233,13 @@ fn main() { _ => panic!(), }; let _: &X = &*if true { Y(X) } else { panic!() }; + + fn deref_to_u>(x: &T) -> &U { + &**x + } + + let _ = |x: &'static Box>| -> &'static dyn Iterator { &**x }; + fn ret_any(x: &Box) -> &dyn std::any::Any { + &**x + } } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index 0d7a9564a90..f2933390fb9 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -210,5 +210,11 @@ error: deref which would be done by auto-deref LL | let _ = || -> &'static str { return *s }; | ^^ help: try this: `s` -error: aborting due to 35 previous errors +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:238:9 + | +LL | &**x + | ^^^^ help: try this: `x` + +error: aborting due to 36 previous errors