From c990e2922ac7ff27e9a7516e9ac92cac5989c9e0 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 5 Jul 2022 16:03:51 -0400 Subject: [PATCH] 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