From 9ce0f82b8ab44869777d441b51aecfb9d8ebdef9 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 5 Jul 2022 22:30:36 -0400 Subject: [PATCH] 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