From c990e2922ac7ff27e9a7516e9ac92cac5989c9e0 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 5 Jul 2022 16:03:51 -0400 Subject: [PATCH 1/6] Don't suggest using auto deref for block expressions --- clippy_lints/src/dereference.rs | 127 ++++++++++++++++++++-------- tests/ui/explicit_auto_deref.fixed | 17 ++++ tests/ui/explicit_auto_deref.rs | 17 ++++ tests/ui/explicit_auto_deref.stderr | 72 ++++++++-------- 4 files changed, 167 insertions(+), 66 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 8c7cf7748be1..b87d41455d9b 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -17,7 +17,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{symbol::sym, Span, Symbol}; +use rustc_span::{symbol::sym, Span, Symbol, DUMMY_SP}; use rustc_trait_selection::infer::InferCtxtExt; declare_clippy_lint! { @@ -609,18 +609,21 @@ enum Position { Postfix, Deref, /// Any other location which will trigger auto-deref to a specific time. - DerefStable(i8), + /// Contains the precedence of the parent expression and whether the target type is sized. + DerefStable(i8, bool), /// Any other location which will trigger auto-reborrowing. + /// Contains the precedence of the parent expression. ReborrowStable(i8), + /// Contains the precedence of the parent expression. Other(i8), } impl Position { fn is_deref_stable(self) -> bool { - matches!(self, Self::DerefStable(_)) + matches!(self, Self::DerefStable(..)) } fn is_reborrow_stable(self) -> bool { - matches!(self, Self::DerefStable(_) | Self::ReborrowStable(_)) + matches!(self, Self::DerefStable(..) | Self::ReborrowStable(_)) } fn can_auto_borrow(self) -> bool { @@ -628,7 +631,7 @@ impl Position { } fn lint_explicit_deref(self) -> bool { - matches!(self, Self::Other(_) | Self::DerefStable(_) | Self::ReborrowStable(_)) + matches!(self, Self::Other(_) | Self::DerefStable(..) | Self::ReborrowStable(_)) } fn precedence(self) -> i8 { @@ -639,7 +642,7 @@ impl Position { | Self::FieldAccess(_) | Self::Postfix => PREC_POSTFIX, Self::Deref => PREC_PREFIX, - Self::DerefStable(p) | Self::ReborrowStable(p) | Self::Other(p) => p, + Self::DerefStable(p, _) | Self::ReborrowStable(p) | Self::Other(p) => p, } } } @@ -659,7 +662,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & } match parent { Node::Local(Local { ty: Some(ty), span, .. }) if span.ctxt() == ctxt => { - Some(binding_ty_auto_deref_stability(ty, precedence)) + Some(binding_ty_auto_deref_stability(cx, ty, precedence)) }, Node::Item(&Item { kind: ItemKind::Static(..) | ItemKind::Const(..), @@ -680,8 +683,11 @@ 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 ty.is_ref() { - Position::DerefStable(precedence) + 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) }) @@ -705,13 +711,20 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & span, .. }) if span.ctxt() == ctxt => { - let output = cx.tcx.fn_sig(def_id.to_def_id()).skip_binder().output(); - Some(if !output.is_ref() { - Position::Other(precedence) - } else if output.has_placeholders() || output.has_opaque_types() { - Position::ReborrowStable(precedence) + 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::DerefStable(precedence) + Position::Other(precedence) }) }, @@ -725,21 +738,24 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & }) = cx.tcx.hir().get(owner_id) { match fn_decl.output { - FnRetTy::Return(ty) => binding_ty_auto_deref_stability(ty, precedence), + FnRetTy::Return(ty) => binding_ty_auto_deref_stability(cx, ty, precedence), FnRetTy::DefaultReturn(_) => Position::Other(precedence), } } else { let output = cx .tcx - .fn_sig(cx.tcx.hir().local_def_id(owner_id)) - .skip_binder() - .output(); - if !output.is_ref() { - Position::Other(precedence) - } else if output.has_placeholders() || output.has_opaque_types() { - Position::ReborrowStable(precedence) + .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::DerefStable(precedence) + Position::Other(precedence) } }, ) @@ -755,8 +771,8 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .map(|(hir_ty, ty)| match hir_ty { // 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(ty, precedence), - None => param_auto_deref_stability(ty.skip_binder(), precedence), + Some(ty) => binding_ty_auto_deref_stability(cx, ty, precedence), + None => param_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence), }), ExprKind::MethodCall(_, args, _) => { let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(); @@ -797,7 +813,11 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & Position::MethodReceiver } } else { - param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i], precedence) + param_auto_deref_stability( + cx, + cx.tcx.erase_late_bound_regions(cx.tcx.fn_sig(id).input(i)), + precedence, + ) } }) }, @@ -808,7 +828,7 @@ 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.tcx.type_of(field.did), precedence)) + .map(|field| param_auto_deref_stability(cx, cx.tcx.type_of(field.did), precedence)) }, 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), @@ -840,7 +860,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & // // Here `y1` and `y2` would resolve to different types, so the type `&Box<_>` is not stable when // switching to auto-dereferencing. -fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>, precedence: i8) -> Position { +fn binding_ty_auto_deref_stability(cx: &LateContext<'_>, ty: &hir::Ty<'_>, precedence: i8) -> Position { let TyKind::Rptr(_, ty) = &ty.kind else { return Position::Other(precedence); }; @@ -870,7 +890,13 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>, precedence: i8) -> Position { Position::ReborrowStable(precedence) } else { - Position::DerefStable(precedence) + 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()), + ) } }, TyKind::Slice(_) @@ -880,7 +906,13 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>, precedence: i8) -> Position | TyKind::Tup(_) | TyKind::Ptr(_) | TyKind::TraitObject(..) - | TyKind::Path(_) => Position::DerefStable(precedence), + | 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()), + ), TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) @@ -921,7 +953,7 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { } // Checks whether a type is stable when switching to auto dereferencing, -fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { +fn param_auto_deref_stability<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, precedence: i8) -> Position { let ty::Ref(_, mut ty, _) = *ty.kind() else { return Position::Other(precedence); }; @@ -960,7 +992,10 @@ fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { | ty::GeneratorWitness(..) | ty::Never | ty::Tuple(_) - | ty::Projection(_) => Position::DerefStable(precedence), + | ty::Projection(_) => Position::DerefStable( + precedence, + ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), + ), }; } } @@ -1040,6 +1075,19 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data }); }, State::ExplicitDeref { deref_span_id } => { + if matches!( + expr.kind, + ExprKind::Block(..) + | ExprKind::ConstBlock(_) + | ExprKind::If(..) + | ExprKind::Loop(..) + | ExprKind::Match(..) + ) && matches!(data.position, Position::DerefStable(_, true)) + { + // Rustc bug: auto deref doesn't work on block expression when targeting sized types. + return; + } + let (span, hir_id, precedence) = if let Some((span, hir_id)) = deref_span_id && !cx.typeck_results().expr_ty(expr).is_ref() { @@ -1067,6 +1115,19 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data ); }, State::ExplicitDerefField { .. } => { + if matches!( + expr.kind, + ExprKind::Block(..) + | ExprKind::ConstBlock(_) + | ExprKind::If(..) + | ExprKind::Loop(..) + | ExprKind::Match(..) + ) && matches!(data.position, Position::DerefStable(_, true)) + { + // Rustc bug: auto deref doesn't work on block expression when targeting sized types. + return; + } + span_lint_hir_and_then( cx, EXPLICIT_AUTO_DEREF, diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index a650fdc1f897..327cfbaf3242 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -67,6 +67,7 @@ fn main() { let s = String::new(); let _: &str = &s; + let _: &str = &{ String::new() }; let _ = &*s; // Don't lint. Inferred type would change. let _: &_ = &*s; // Don't lint. Inferred type would change. @@ -215,4 +216,20 @@ fn main() { let s = &"str"; let _ = || return *s; let _ = || -> &'static str { return s }; + + struct X; + struct Y(X); + impl core::ops::Deref for Y { + type Target = X; + fn deref(&self) -> &Self::Target { + &self.0 + } + } + let _: &X = &*{ Y(X) }; + let _: &X = &*match 0 { + #[rustfmt::skip] + 0 => { Y(X) }, + _ => panic!(), + }; + let _: &X = &*if true { Y(X) } else { panic!() }; } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 8f4f352576a7..471a03d60a96 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -67,6 +67,7 @@ fn main() { let s = String::new(); let _: &str = &*s; + let _: &str = &*{ String::new() }; let _ = &*s; // Don't lint. Inferred type would change. let _: &_ = &*s; // Don't lint. Inferred type would change. @@ -215,4 +216,20 @@ fn main() { let s = &"str"; let _ = || return *s; let _ = || -> &'static str { return *s }; + + struct X; + struct Y(X); + impl core::ops::Deref for Y { + type Target = X; + fn deref(&self) -> &Self::Target { + &self.0 + } + } + let _: &X = &*{ Y(X) }; + let _: &X = &*match 0 { + #[rustfmt::skip] + 0 => { Y(X) }, + _ => panic!(), + }; + let _: &X = &*if true { Y(X) } else { panic!() }; } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index 92765307ea73..d1bc51f5bddf 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -7,196 +7,202 @@ LL | let _: &str = &*s; = note: `-D clippy::explicit-auto-deref` implied by `-D warnings` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:73:12 + --> $DIR/explicit_auto_deref.rs:70:20 + | +LL | let _: &str = &*{ String::new() }; + | ^^^^^^^^^^^^^^^^^^ help: try this: `{ String::new() }` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:74:12 | LL | f_str(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:77:14 + --> $DIR/explicit_auto_deref.rs:78:14 | LL | f_str_t(&*s, &*s); // Don't lint second param. | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:80:25 + --> $DIR/explicit_auto_deref.rs:81:25 | LL | let _: &Box = &**b; | ^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:86:8 + --> $DIR/explicit_auto_deref.rs:87:8 | LL | c(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:92:9 + --> $DIR/explicit_auto_deref.rs:93:9 | LL | &**x | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:96:11 + --> $DIR/explicit_auto_deref.rs:97:11 | LL | { &**x } | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:100:9 + --> $DIR/explicit_auto_deref.rs:101:9 | LL | &**{ x } | ^^^^^^^^ help: try this: `{ x }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:104:9 + --> $DIR/explicit_auto_deref.rs:105:9 | LL | &***x | ^^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:121:13 + --> $DIR/explicit_auto_deref.rs:122:13 | LL | f1(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:122:13 + --> $DIR/explicit_auto_deref.rs:123:13 | LL | f2(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:123:13 + --> $DIR/explicit_auto_deref.rs:124:13 | LL | f3(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:124:28 + --> $DIR/explicit_auto_deref.rs:125:28 | LL | f4.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:125:13 + --> $DIR/explicit_auto_deref.rs:126:13 | LL | f5(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:126:13 + --> $DIR/explicit_auto_deref.rs:127:13 | LL | f6(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:127:28 + --> $DIR/explicit_auto_deref.rs:128:28 | LL | f7.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:128:26 + --> $DIR/explicit_auto_deref.rs:129:26 | LL | f8.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:129:13 + --> $DIR/explicit_auto_deref.rs:130:13 | LL | f9(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:130:14 + --> $DIR/explicit_auto_deref.rs:131:14 | LL | f10(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:131:27 + --> $DIR/explicit_auto_deref.rs:132:27 | LL | f11.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:135:17 + --> $DIR/explicit_auto_deref.rs:136:17 | LL | let _ = S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:140:22 + --> $DIR/explicit_auto_deref.rs:141:22 | LL | let _ = S2 { s: &*s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:156:30 + --> $DIR/explicit_auto_deref.rs:157:30 | LL | let _ = Self::S1(&**s); | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:157:35 + --> $DIR/explicit_auto_deref.rs:158:35 | LL | let _ = Self::S2 { s: &**s }; | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:160:21 + --> $DIR/explicit_auto_deref.rs:161:21 | LL | let _ = E1::S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:161:26 + --> $DIR/explicit_auto_deref.rs:162:26 | LL | let _ = E1::S2 { s: &*s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:179:13 + --> $DIR/explicit_auto_deref.rs:180:13 | LL | let _ = (*b).foo; | ^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:180:13 + --> $DIR/explicit_auto_deref.rs:181:13 | LL | let _ = (**b).foo; | ^^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:195:19 + --> $DIR/explicit_auto_deref.rs:196:19 | LL | let _ = f_str(*ref_str); | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:197:19 + --> $DIR/explicit_auto_deref.rs:198:19 | LL | let _ = f_str(**ref_ref_str); | ^^^^^^^^^^^^^ help: try this: `ref_ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:207:13 + --> $DIR/explicit_auto_deref.rs:208:13 | LL | f_str(&&*ref_str); // `needless_borrow` will suggest removing both references | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:208:12 + --> $DIR/explicit_auto_deref.rs:209:12 | LL | f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference | ^^^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:217:41 + --> $DIR/explicit_auto_deref.rs:218:41 | LL | let _ = || -> &'static str { return *s }; | ^^ help: try this: `s` -error: aborting due to 33 previous errors +error: aborting due to 34 previous errors From 9ce0f82b8ab44869777d441b51aecfb9d8ebdef9 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 5 Jul 2022 22:30:36 -0400 Subject: [PATCH 2/6] Include the borrow in the suggestion for `explicit_auto_deref` --- clippy_lints/src/dereference.rs | 79 ++++++++----------- tests/ui/explicit_auto_deref.fixed | 1 + tests/ui/explicit_auto_deref.rs | 1 + tests/ui/explicit_auto_deref.stderr | 118 +++++++++++++++------------- 4 files changed, 98 insertions(+), 101 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index b87d41455d9b..8495d2c6df71 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -183,24 +183,24 @@ enum State { }, DerefedBorrow(DerefedBorrow), ExplicitDeref { - // Span and id of the top-level deref expression if the parent expression is a borrow. - deref_span_id: Option<(Span, HirId)>, + mutability: Option, }, ExplicitDerefField { name: Symbol, }, Reborrow { - deref_span: Span, - deref_hir_id: HirId, + mutability: Mutability, + }, + Borrow { + mutability: Mutability, }, - Borrow, } // A reference operation considered by this lint pass enum RefOp { Method(Mutability), Deref, - AddrOf, + AddrOf(Mutability), } struct RefPat { @@ -263,7 +263,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); } else if position.is_deref_stable() { self.state = Some(( - State::ExplicitDeref { deref_span_id: None }, + State::ExplicitDeref { mutability: None }, StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } @@ -289,7 +289,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { }, )); }, - RefOp::AddrOf => { + RefOp::AddrOf(mutability) => { // Find the number of times the borrow is auto-derefed. let mut iter = adjustments.iter(); let mut deref_count = 0usize; @@ -359,7 +359,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); } else if position.is_deref_stable() { self.state = Some(( - State::Borrow, + State::Borrow { mutability }, StateData { span: expr.span, hir_id: expr.hir_id, @@ -395,7 +395,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { data, )); }, - (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) if state.count != 0 => { + (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf(_)) if state.count != 0 => { self.state = Some(( State::DerefedBorrow(DerefedBorrow { count: state.count - 1, @@ -404,12 +404,12 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { data, )); }, - (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) => { + (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf(mutability)) => { let position = data.position; report(cx, expr, State::DerefedBorrow(state), data); if position.is_deref_stable() { self.state = Some(( - State::Borrow, + State::Borrow { mutability }, StateData { span: expr.span, hir_id: expr.hir_id, @@ -430,43 +430,28 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); } else if position.is_deref_stable() { self.state = Some(( - State::ExplicitDeref { deref_span_id: None }, + State::ExplicitDeref { mutability: None }, StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } }, - (Some((State::Borrow, data)), RefOp::Deref) => { + (Some((State::Borrow { mutability }, data)), RefOp::Deref) => { if typeck.expr_ty(sub_expr).is_ref() { - self.state = Some(( - State::Reborrow { - deref_span: expr.span, - deref_hir_id: expr.hir_id, - }, - data, - )); + self.state = Some((State::Reborrow { mutability }, data)); } else { self.state = Some(( State::ExplicitDeref { - deref_span_id: Some((expr.span, expr.hir_id)), + mutability: Some(mutability), }, data, )); } }, - ( - Some(( - State::Reborrow { - deref_span, - deref_hir_id, - }, - data, - )), - RefOp::Deref, - ) => { + (Some((State::Reborrow { mutability }, data)), RefOp::Deref) => { self.state = Some(( State::ExplicitDeref { - deref_span_id: Some((deref_span, deref_hir_id)), + mutability: Some(mutability), }, data, )); @@ -573,7 +558,7 @@ fn try_parse_ref_op<'tcx>( ExprKind::Unary(UnOp::Deref, sub_expr) if !typeck.expr_ty(sub_expr).is_unsafe_ptr() => { return Some((RefOp::Deref, sub_expr)); }, - ExprKind::AddrOf(BorrowKind::Ref, _, sub_expr) => return Some((RefOp::AddrOf, sub_expr)), + ExprKind::AddrOf(BorrowKind::Ref, mutability, sub_expr) => return Some((RefOp::AddrOf(mutability), sub_expr)), _ => return None, }; if tcx.is_diagnostic_item(sym::deref_method, def_id) { @@ -1074,7 +1059,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data diag.span_suggestion(data.span, "change this to", sugg, app); }); }, - State::ExplicitDeref { deref_span_id } => { + State::ExplicitDeref { mutability } => { if matches!( expr.kind, ExprKind::Block(..) @@ -1088,29 +1073,33 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data return; } - let (span, hir_id, precedence) = if let Some((span, hir_id)) = deref_span_id + let (prefix, precedence) = if let Some(mutability) = mutability && !cx.typeck_results().expr_ty(expr).is_ref() { - (span, hir_id, PREC_PREFIX) + let prefix = match mutability { + Mutability::Not => "&", + Mutability::Mut => "&mut ", + }; + (prefix, 0) } else { - (data.span, data.hir_id, data.position.precedence()) + ("", data.position.precedence()) }; span_lint_hir_and_then( cx, EXPLICIT_AUTO_DEREF, - hir_id, - span, + data.hir_id, + data.span, "deref which would be done by auto-deref", |diag| { let mut app = Applicability::MachineApplicable; - let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, span.ctxt(), "..", &mut app); + let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app); let sugg = if !snip_is_macro && expr.precedence().order() < precedence && !has_enclosing_paren(&snip) { - format!("({})", snip) + format!("{}({})", prefix, snip) } else { - snip.into() + format!("{}{}", prefix, snip) }; - diag.span_suggestion(span, "try this", sugg, app); + diag.span_suggestion(data.span, "try this", sugg, app); }, ); }, @@ -1141,7 +1130,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data }, ); }, - State::Borrow | State::Reborrow { .. } => (), + State::Borrow { .. } | State::Reborrow { .. } => (), } } diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 327cfbaf3242..d3efc2994145 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -68,6 +68,7 @@ fn main() { let _: &str = &s; let _: &str = &{ String::new() }; + let _: &str = &mut { String::new() }; let _ = &*s; // Don't lint. Inferred type would change. let _: &_ = &*s; // Don't lint. Inferred type would change. diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 471a03d60a96..e25ccbe78cef 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -68,6 +68,7 @@ fn main() { let _: &str = &*s; let _: &str = &*{ String::new() }; + let _: &str = &mut *{ String::new() }; let _ = &*s; // Don't lint. Inferred type would change. let _: &_ = &*s; // Don't lint. Inferred type would change. diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index d1bc51f5bddf..0d7a9564a900 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -1,208 +1,214 @@ error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:69:20 + --> $DIR/explicit_auto_deref.rs:69:19 | LL | let _: &str = &*s; - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` | = note: `-D clippy::explicit-auto-deref` implied by `-D warnings` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:70:20 + --> $DIR/explicit_auto_deref.rs:70:19 | LL | let _: &str = &*{ String::new() }; - | ^^^^^^^^^^^^^^^^^^ help: try this: `{ String::new() }` + | ^^^^^^^^^^^^^^^^^^^ help: try this: `&{ String::new() }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:74:12 + --> $DIR/explicit_auto_deref.rs:71:19 + | +LL | let _: &str = &mut *{ String::new() }; + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut { String::new() }` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:75:11 | LL | f_str(&*s); - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:78:14 + --> $DIR/explicit_auto_deref.rs:79:13 | LL | f_str_t(&*s, &*s); // Don't lint second param. - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:81:25 + --> $DIR/explicit_auto_deref.rs:82:24 | LL | let _: &Box = &**b; - | ^^^ help: try this: `b` + | ^^^^ help: try this: `&b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:87:8 + --> $DIR/explicit_auto_deref.rs:88:7 | LL | c(&*s); - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:93:9 + --> $DIR/explicit_auto_deref.rs:94:9 | LL | &**x | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:97:11 + --> $DIR/explicit_auto_deref.rs:98:11 | LL | { &**x } | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:101:9 + --> $DIR/explicit_auto_deref.rs:102:9 | LL | &**{ x } | ^^^^^^^^ help: try this: `{ x }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:105:9 + --> $DIR/explicit_auto_deref.rs:106:9 | LL | &***x | ^^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:122:13 + --> $DIR/explicit_auto_deref.rs:123:12 | LL | f1(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:123:13 + --> $DIR/explicit_auto_deref.rs:124:12 | LL | f2(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:124:13 + --> $DIR/explicit_auto_deref.rs:125:12 | LL | f3(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:125:28 + --> $DIR/explicit_auto_deref.rs:126:27 | LL | f4.callable_str()(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:126:13 + --> $DIR/explicit_auto_deref.rs:127:12 | LL | f5(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:127:13 + --> $DIR/explicit_auto_deref.rs:128:12 | LL | f6(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:128:28 + --> $DIR/explicit_auto_deref.rs:129:27 | LL | f7.callable_str()(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:129:26 + --> $DIR/explicit_auto_deref.rs:130:25 | LL | f8.callable_t()(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:130:13 + --> $DIR/explicit_auto_deref.rs:131:12 | LL | f9(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:131:14 + --> $DIR/explicit_auto_deref.rs:132:13 | LL | f10(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:132:27 + --> $DIR/explicit_auto_deref.rs:133:26 | LL | f11.callable_t()(&*x); - | ^^ help: try this: `x` + | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:136:17 + --> $DIR/explicit_auto_deref.rs:137:16 | LL | let _ = S1(&*s); - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:141:22 + --> $DIR/explicit_auto_deref.rs:142:21 | LL | let _ = S2 { s: &*s }; - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:157:30 + --> $DIR/explicit_auto_deref.rs:158:30 | LL | let _ = Self::S1(&**s); | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:158:35 + --> $DIR/explicit_auto_deref.rs:159:35 | LL | let _ = Self::S2 { s: &**s }; | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:161:21 + --> $DIR/explicit_auto_deref.rs:162:20 | LL | let _ = E1::S1(&*s); - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:162:26 + --> $DIR/explicit_auto_deref.rs:163:25 | LL | let _ = E1::S2 { s: &*s }; - | ^^ help: try this: `s` + | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:180:13 + --> $DIR/explicit_auto_deref.rs:181:13 | LL | let _ = (*b).foo; | ^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:181:13 + --> $DIR/explicit_auto_deref.rs:182:13 | LL | let _ = (**b).foo; | ^^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:196:19 + --> $DIR/explicit_auto_deref.rs:197:19 | LL | let _ = f_str(*ref_str); | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:198:19 + --> $DIR/explicit_auto_deref.rs:199:19 | LL | let _ = f_str(**ref_ref_str); | ^^^^^^^^^^^^^ help: try this: `ref_ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:208:13 + --> $DIR/explicit_auto_deref.rs:209:13 | LL | f_str(&&*ref_str); // `needless_borrow` will suggest removing both references | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:209:12 + --> $DIR/explicit_auto_deref.rs:210:12 | LL | f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference | ^^^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:218:41 + --> $DIR/explicit_auto_deref.rs:219:41 | LL | let _ = || -> &'static str { return *s }; | ^^ help: try this: `s` -error: aborting due to 34 previous errors +error: aborting due to 35 previous errors From 84e03b621522799a72f55d3e728d63aebb561139 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 10 Jul 2022 15:04:29 -0400 Subject: [PATCH 3/6] 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 8495d2c6df71..40e62dbd5864 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 a05d633d980c..d394360fc678 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 d3efc2994145..464dd2dc0da6 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 e25ccbe78cef..453b90f09b33 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 0d7a9564a900..f2933390fb95 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 From d602ab1445a7c1982583df9297e21e137b450eb2 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 17 Jul 2022 11:12:25 -0400 Subject: [PATCH 4/6] Don't lint `exlipicit_auto_deref` when other adjustments are needed --- clippy_lints/src/dereference.rs | 6 +++++- tests/ui/explicit_auto_deref.fixed | 13 +++++++++++++ tests/ui/explicit_auto_deref.rs | 13 +++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 40e62dbd5864..05dbc7434855 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -357,7 +357,11 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { }), StateData { span: expr.span, hir_id: expr.hir_id, position }, )); - } else if position.is_deref_stable() { + } else if position.is_deref_stable() + // Auto-deref doesn't combine with other adjustments + && next_adjust.map_or(true, |a| matches!(a.kind, Adjust::Deref(_) | Adjust::Borrow(_))) + && iter.all(|a| matches!(a.kind, Adjust::Deref(_) | Adjust::Borrow(_))) + { self.state = Some(( State::Borrow { mutability }, StateData { diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 464dd2dc0da6..1e868af87cb8 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -242,4 +242,17 @@ fn main() { fn ret_any(x: &Box) -> &dyn std::any::Any { &**x } + + let x = String::new(); + let _: *const str = &*x; + + struct S7([u32; 1]); + impl core::ops::Deref for S7 { + type Target = [u32; 1]; + fn deref(&self) -> &Self::Target { + &self.0 + } + } + let x = S7([0]); + let _: &[u32] = &*x; } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 453b90f09b33..a2157d9b4f05 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -242,4 +242,17 @@ fn main() { fn ret_any(x: &Box) -> &dyn std::any::Any { &**x } + + let x = String::new(); + let _: *const str = &*x; + + struct S7([u32; 1]); + impl core::ops::Deref for S7 { + type Target = [u32; 1]; + fn deref(&self) -> &Self::Target { + &self.0 + } + } + let x = S7([0]); + let _: &[u32] = &*x; } From 5285928bc0190efb54495d8126b68778873884b9 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 8 Aug 2022 10:09:05 -0400 Subject: [PATCH 5/6] Fix ICE when checking the HIR ty of closure args. --- clippy_lints/src/dereference.rs | 67 +++++++++++++--------- tests/ui/explicit_auto_deref.fixed | 11 ++++ tests/ui/explicit_auto_deref.rs | 11 ++++ tests/ui/explicit_auto_deref.stderr | 86 ++++++++++++++++------------- 4 files changed, 112 insertions(+), 63 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 05dbc7434855..821528d7ab94 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -15,7 +15,7 @@ use rustc_hir::{ use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; -use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitable, TypeckResults}; +use rustc_middle::ty::{self, Binder, BoundVariableKind, List, Ty, TyCtxt, TypeVisitable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{symbol::sym, Span, Symbol, DUMMY_SP}; use rustc_trait_selection::infer::InferCtxtExt; @@ -651,7 +651,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & } match parent { Node::Local(Local { ty: Some(ty), span, .. }) if span.ctxt() == ctxt => { - Some(binding_ty_auto_deref_stability(cx, ty, precedence)) + Some(binding_ty_auto_deref_stability(cx, ty, precedence, List::empty())) }, Node::Item(&Item { kind: ItemKind::Static(..) | ItemKind::Const(..), @@ -703,13 +703,23 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & ExprKind::Ret(_) => { let owner_id = cx.tcx.hir().body_owner(cx.enclosing_body.unwrap()); Some( - if let Node::Expr(Expr { - kind: ExprKind::Closure(&Closure { fn_decl, .. }), - .. - }) = cx.tcx.hir().get(owner_id) + if let Node::Expr( + closure @ Expr { + kind: ExprKind::Closure(&Closure { fn_decl, .. }), + .. + }, + ) = cx.tcx.hir().get(owner_id) { match fn_decl.output { - FnRetTy::Return(ty) => binding_ty_auto_deref_stability(cx, ty, precedence), + FnRetTy::Return(ty) => { + if let Some(sig) = expr_sig(cx, closure) + && let Some(output) = sig.output() + { + binding_ty_auto_deref_stability(cx, ty, precedence, output.bound_vars()) + } else { + Position::Other(precedence) + } + }, FnRetTy::DefaultReturn(_) => Position::Other(precedence), } } else { @@ -731,7 +741,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .map(|(hir_ty, ty)| match hir_ty { // 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), + Some(hir_ty) => binding_ty_auto_deref_stability(cx, hir_ty, precedence, ty.bound_vars()), None => ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence) .position_for_arg(), }), @@ -824,7 +834,12 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & // // Here `y1` and `y2` would resolve to different types, so the type `&Box<_>` is not stable when // switching to auto-dereferencing. -fn binding_ty_auto_deref_stability(cx: &LateContext<'_>, ty: &hir::Ty<'_>, precedence: i8) -> Position { +fn binding_ty_auto_deref_stability<'tcx>( + cx: &LateContext<'tcx>, + ty: &'tcx hir::Ty<'_>, + precedence: i8, + binder_args: &'tcx List, +) -> Position { let TyKind::Rptr(_, ty) = &ty.kind else { return Position::Other(precedence); }; @@ -856,31 +871,31 @@ fn binding_ty_auto_deref_stability(cx: &LateContext<'_>, ty: &hir::Ty<'_>, prece } else { Position::DerefStable( precedence, - cx - .typeck_results() - .node_type(ty.ty.hir_id) + cx.tcx + .erase_late_bound_regions(Binder::bind_with_vars( + cx.typeck_results().node_type(ty.ty.hir_id), + binder_args, + )) .is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), ) } }, - TyKind::Slice(_) - | TyKind::Array(..) - | TyKind::BareFn(_) - | TyKind::Never + TyKind::Slice(_) => Position::DerefStable(precedence, false), + TyKind::Array(..) | TyKind::Ptr(_) | TyKind::BareFn(_) => Position::DerefStable(precedence, true), + TyKind::Never | TyKind::Tup(_) - | TyKind::Ptr(_) | 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()), + cx.tcx + .erase_late_bound_regions(Binder::bind_with_vars( + cx.typeck_results().node_type(ty.ty.hir_id), + binder_args, + )) + .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), + TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::TraitObject(..) | TyKind::Err => { + Position::ReborrowStable(precedence) + }, }; } } diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 1e868af87cb8..27bc7fbfae3c 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -1,5 +1,6 @@ // run-rustfix +#![feature(closure_lifetime_binder)] #![warn(clippy::explicit_auto_deref)] #![allow( dead_code, @@ -255,4 +256,14 @@ fn main() { } let x = S7([0]); let _: &[u32] = &*x; + + let c1 = |x: &Vec<&u32>| {}; + let x = &&vec![&1u32]; + c1(x); + let _ = for<'a, 'b> |x: &'a &'a Vec<&'b u32>, b: bool| -> &'a Vec<&'b u32> { + if b { + return x; + } + *x + }; } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index a2157d9b4f05..64aea9f464e4 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -1,5 +1,6 @@ // run-rustfix +#![feature(closure_lifetime_binder)] #![warn(clippy::explicit_auto_deref)] #![allow( dead_code, @@ -255,4 +256,14 @@ fn main() { } let x = S7([0]); let _: &[u32] = &*x; + + let c1 = |x: &Vec<&u32>| {}; + let x = &&vec![&1u32]; + c1(*x); + let _ = for<'a, 'b> |x: &'a &'a Vec<&'b u32>, b: bool| -> &'a Vec<&'b u32> { + if b { + return *x; + } + *x + }; } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index f2933390fb95..12db0c6f87fc 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -1,5 +1,5 @@ error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:69:19 + --> $DIR/explicit_auto_deref.rs:70:19 | LL | let _: &str = &*s; | ^^^ help: try this: `&s` @@ -7,214 +7,226 @@ LL | let _: &str = &*s; = note: `-D clippy::explicit-auto-deref` implied by `-D warnings` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:70:19 + --> $DIR/explicit_auto_deref.rs:71:19 | LL | let _: &str = &*{ String::new() }; | ^^^^^^^^^^^^^^^^^^^ help: try this: `&{ String::new() }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:71:19 + --> $DIR/explicit_auto_deref.rs:72:19 | LL | let _: &str = &mut *{ String::new() }; | ^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut { String::new() }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:75:11 + --> $DIR/explicit_auto_deref.rs:76:11 | LL | f_str(&*s); | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:79:13 + --> $DIR/explicit_auto_deref.rs:80:13 | LL | f_str_t(&*s, &*s); // Don't lint second param. | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:82:24 + --> $DIR/explicit_auto_deref.rs:83:24 | LL | let _: &Box = &**b; | ^^^^ help: try this: `&b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:88:7 + --> $DIR/explicit_auto_deref.rs:89:7 | LL | c(&*s); | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:94:9 + --> $DIR/explicit_auto_deref.rs:95:9 | LL | &**x | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:98:11 + --> $DIR/explicit_auto_deref.rs:99:11 | LL | { &**x } | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:102:9 + --> $DIR/explicit_auto_deref.rs:103:9 | LL | &**{ x } | ^^^^^^^^ help: try this: `{ x }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:106:9 + --> $DIR/explicit_auto_deref.rs:107:9 | LL | &***x | ^^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:123:12 + --> $DIR/explicit_auto_deref.rs:124:12 | LL | f1(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:124:12 + --> $DIR/explicit_auto_deref.rs:125:12 | LL | f2(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:125:12 + --> $DIR/explicit_auto_deref.rs:126:12 | LL | f3(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:126:27 + --> $DIR/explicit_auto_deref.rs:127:27 | LL | f4.callable_str()(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:127:12 + --> $DIR/explicit_auto_deref.rs:128:12 | LL | f5(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:128:12 + --> $DIR/explicit_auto_deref.rs:129:12 | LL | f6(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:129:27 + --> $DIR/explicit_auto_deref.rs:130:27 | LL | f7.callable_str()(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:130:25 + --> $DIR/explicit_auto_deref.rs:131:25 | LL | f8.callable_t()(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:131:12 + --> $DIR/explicit_auto_deref.rs:132:12 | LL | f9(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:132:13 + --> $DIR/explicit_auto_deref.rs:133:13 | LL | f10(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:133:26 + --> $DIR/explicit_auto_deref.rs:134:26 | LL | f11.callable_t()(&*x); | ^^^ help: try this: `&x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:137:16 + --> $DIR/explicit_auto_deref.rs:138:16 | LL | let _ = S1(&*s); | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:142:21 + --> $DIR/explicit_auto_deref.rs:143:21 | LL | let _ = S2 { s: &*s }; | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:158:30 + --> $DIR/explicit_auto_deref.rs:159:30 | LL | let _ = Self::S1(&**s); | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:159:35 + --> $DIR/explicit_auto_deref.rs:160:35 | LL | let _ = Self::S2 { s: &**s }; | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:162:20 + --> $DIR/explicit_auto_deref.rs:163:20 | LL | let _ = E1::S1(&*s); | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:163:25 + --> $DIR/explicit_auto_deref.rs:164:25 | LL | let _ = E1::S2 { s: &*s }; | ^^^ help: try this: `&s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:181:13 + --> $DIR/explicit_auto_deref.rs:182:13 | LL | let _ = (*b).foo; | ^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:182:13 + --> $DIR/explicit_auto_deref.rs:183:13 | LL | let _ = (**b).foo; | ^^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:197:19 + --> $DIR/explicit_auto_deref.rs:198:19 | LL | let _ = f_str(*ref_str); | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:199:19 + --> $DIR/explicit_auto_deref.rs:200:19 | LL | let _ = f_str(**ref_ref_str); | ^^^^^^^^^^^^^ help: try this: `ref_ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:209:13 + --> $DIR/explicit_auto_deref.rs:210:13 | LL | f_str(&&*ref_str); // `needless_borrow` will suggest removing both references | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:210:12 + --> $DIR/explicit_auto_deref.rs:211:12 | LL | f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference | ^^^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:219:41 + --> $DIR/explicit_auto_deref.rs:220:41 | LL | let _ = || -> &'static str { return *s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:238:9 + --> $DIR/explicit_auto_deref.rs:239:9 | LL | &**x | ^^^^ help: try this: `x` -error: aborting due to 36 previous errors +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:262:8 + | +LL | c1(*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:265:20 + | +LL | return *x; + | ^^ help: try this: `x` + +error: aborting due to 38 previous errors From ecb51fe6a55f975f2d3bfd65d3568c263c248460 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 8 Aug 2022 10:25:05 -0400 Subject: [PATCH 6/6] Lint `explicit_auto_deref` in implicit return positions for closures --- clippy_lints/src/dereference.rs | 45 +++++++++++++++++++---------- clippy_utils/src/ty.rs | 3 +- tests/ui/explicit_auto_deref.fixed | 4 +-- tests/ui/explicit_auto_deref.rs | 2 +- tests/ui/explicit_auto_deref.stderr | 8 ++++- 5 files changed, 42 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 821528d7ab94..1c19a9e0e60e 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; use clippy_utils::sugg::has_enclosing_paren; -use clippy_utils::ty::{expr_sig, peel_mid_ty_refs, variant_of_res}; +use clippy_utils::ty::{expr_sig, peel_mid_ty_refs, ty_sig, variant_of_res}; use clippy_utils::{get_parent_expr, is_lint_allowed, path_to_local, walk_to_expr_usage}; use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX}; use rustc_data_structures::fx::FxIndexMap; @@ -704,24 +704,13 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & let owner_id = cx.tcx.hir().body_owner(cx.enclosing_body.unwrap()); Some( if let Node::Expr( - closure @ Expr { - kind: ExprKind::Closure(&Closure { fn_decl, .. }), + closure_expr @ Expr { + kind: ExprKind::Closure(closure), .. }, ) = cx.tcx.hir().get(owner_id) { - match fn_decl.output { - FnRetTy::Return(ty) => { - if let Some(sig) = expr_sig(cx, closure) - && let Some(output) = sig.output() - { - binding_ty_auto_deref_stability(cx, ty, precedence, output.bound_vars()) - } else { - Position::Other(precedence) - } - }, - FnRetTy::DefaultReturn(_) => Position::Other(precedence), - } + closure_result_position(cx, closure, cx.typeck_results().expr_ty(closure_expr), precedence) } else { let output = cx .tcx @@ -730,6 +719,12 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & }, ) }, + ExprKind::Closure(closure) => Some(closure_result_position( + cx, + closure, + cx.typeck_results().expr_ty(parent), + precedence, + )), ExprKind::Call(func, _) if func.hir_id == child_id => { (child_id == e.hir_id).then_some(Position::Callee) }, @@ -825,6 +820,26 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & (position, adjustments) } +fn closure_result_position<'tcx>( + cx: &LateContext<'tcx>, + closure: &'tcx Closure<'_>, + ty: Ty<'tcx>, + precedence: i8, +) -> Position { + match closure.fn_decl.output { + FnRetTy::Return(hir_ty) => { + if let Some(sig) = ty_sig(cx, ty) + && let Some(output) = sig.output() + { + binding_ty_auto_deref_stability(cx, hir_ty, precedence, output.bound_vars()) + } else { + Position::Other(precedence) + } + }, + FnRetTy::DefaultReturn(_) => Position::Other(precedence), + } +} + // Checks the stability of auto-deref when assigned to a binding with the given explicit type. // // e.g. diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index d394360fc678..e7d670766a05 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -572,7 +572,8 @@ pub fn expr_sig<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { +/// If the type is function like, get the signature for it. +pub fn ty_sig<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { if ty.is_box() { return ty_sig(cx, ty.boxed_ty()); } diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 27bc7fbfae3c..d1d35e5c0eb4 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -257,13 +257,13 @@ fn main() { let x = S7([0]); let _: &[u32] = &*x; - let c1 = |x: &Vec<&u32>| {}; + let c1 = |_: &Vec<&u32>| {}; let x = &&vec![&1u32]; c1(x); let _ = for<'a, 'b> |x: &'a &'a Vec<&'b u32>, b: bool| -> &'a Vec<&'b u32> { if b { return x; } - *x + x }; } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 64aea9f464e4..deedafad153b 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -257,7 +257,7 @@ fn main() { let x = S7([0]); let _: &[u32] = &*x; - let c1 = |x: &Vec<&u32>| {}; + let c1 = |_: &Vec<&u32>| {}; let x = &&vec![&1u32]; c1(*x); let _ = for<'a, 'b> |x: &'a &'a Vec<&'b u32>, b: bool| -> &'a Vec<&'b u32> { diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index 12db0c6f87fc..91863abcc5d2 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -228,5 +228,11 @@ error: deref which would be done by auto-deref LL | return *x; | ^^ help: try this: `x` -error: aborting due to 38 previous errors +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:267:9 + | +LL | *x + | ^^ help: try this: `x` + +error: aborting due to 39 previous errors