From 6d21b79be90553ef45f58b682b40432cac840b10 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 29 Jan 2022 20:29:43 -0500 Subject: [PATCH] Fix `needless_borrow` suggestion when calling a trait method taking `self` --- clippy_lints/src/dereference.rs | 55 ++++++++++++++++++++++++++------- tests/ui/needless_borrow.fixed | 30 ++++++++++++++++++ tests/ui/needless_borrow.rs | 30 ++++++++++++++++++ tests/ui/needless_borrow.stderr | 14 ++++++++- 4 files changed, 117 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 3821beaea53..9418c4a66b6 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -11,11 +11,13 @@ ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, TraitItemKind, TyKind, UnOp, }; +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, TypeFoldable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{symbol::sym, Span, Symbol}; +use rustc_trait_selection::infer::InferCtxtExt; declare_clippy_lint! { /// ### What it does @@ -165,7 +167,6 @@ struct StateData { struct DerefedBorrow { count: usize, - required_precedence: i8, msg: &'static str, position: Position, } @@ -329,19 +330,19 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { "this expression creates a reference which is immediately dereferenced by the compiler"; let borrow_msg = "this expression borrows a value the compiler would automatically borrow"; - let (required_refs, required_precedence, msg) = if position.can_auto_borrow() { - (1, PREC_POSTFIX, if deref_count == 1 { borrow_msg } else { deref_msg }) + let (required_refs, msg) = if position.can_auto_borrow() { + (1, if deref_count == 1 { borrow_msg } else { deref_msg }) } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) = next_adjust.map(|a| &a.kind) { if matches!(mutability, AutoBorrowMutability::Mut { .. }) && !position.is_reborrow_stable() { - (3, 0, deref_msg) + (3, deref_msg) } else { - (2, 0, deref_msg) + (2, deref_msg) } } else { - (2, 0, deref_msg) + (2, deref_msg) }; if deref_count >= required_refs { @@ -350,7 +351,6 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // One of the required refs is for the current borrow expression, the remaining ones // can't be removed without breaking the code. See earlier comment. count: deref_count - required_refs, - required_precedence, msg, position, }), @@ -601,6 +601,8 @@ fn deref_method_same_type<'tcx>(result_ty: Ty<'tcx>, arg_ty: Ty<'tcx>) -> bool { #[derive(Clone, Copy)] enum Position { MethodReceiver, + /// The method is defined on a reference type. e.g. `impl Foo for &T` + MethodReceiverRefImpl, Callee, FieldAccess(Symbol), Postfix, @@ -627,6 +629,13 @@ fn can_auto_borrow(self) -> bool { fn lint_explicit_deref(self) -> bool { matches!(self, Self::Other | Self::DerefStable | Self::ReborrowStable) } + + fn needs_parens(self, precedence: i8) -> bool { + matches!( + self, + Self::MethodReceiver | Self::MethodReceiverRefImpl | Self::Callee | Self::FieldAccess(_) | Self::Postfix + ) && precedence < PREC_POSTFIX + } } /// Walks up the parent expressions attempting to determine both how stable the auto-deref result @@ -730,10 +739,34 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(); args.iter().position(|arg| arg.hir_id == child_id).map(|i| { if i == 0 { - if e.hir_id == child_id { - Position::MethodReceiver - } else { + // Check for calls to trait methods where the trait is implemented on a reference. + // Two cases need to be handled: + // * `self` methods on `&T` will never have auto-borrow + // * `&self` methods on `&T` can have auto-borrow, but `&self` methods on `T` will take + // priority. + if e.hir_id != child_id { Position::ReborrowStable + } else if let Some(trait_id) = cx.tcx.trait_of_item(id) + && let arg_ty = cx.tcx.erase_regions(cx.typeck_results().expr_ty_adjusted(e)) + && let ty::Ref(_, sub_ty, _) = *arg_ty.kind() + && let subs = cx.typeck_results().node_substs_opt(child_id).unwrap_or_else( + || cx.tcx.mk_substs([].iter()) + ) && let impl_ty = if cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref() { + // Trait methods taking `&self` + sub_ty + } else { + // Trait methods taking `self` + arg_ty + } && impl_ty.is_ref() + && cx.tcx.infer_ctxt().enter(|infcx| + infcx + .type_implements_trait(trait_id, impl_ty, subs, cx.param_env) + .must_apply_modulo_regions() + ) + { + Position::MethodReceiverRefImpl + } else { + Position::MethodReceiver } } else { param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i]) @@ -964,7 +997,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data let mut app = Applicability::MachineApplicable; let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0; span_lint_hir_and_then(cx, NEEDLESS_BORROW, data.hir_id, data.span, state.msg, |diag| { - let sugg = if state.required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) { + let sugg = if state.position.needs_parens(expr.precedence().order()) && !has_enclosing_paren(&snip) { format!("({})", snip) } else { snip.into() diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index f48f2ae58dc..cb005122436 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -85,6 +85,36 @@ fn main() { let _ = x.0; let x = &x as *const (i32, i32); let _ = unsafe { (*x).0 }; + + // Issue #8367 + trait Foo { + fn foo(self); + } + impl Foo for &'_ () { + fn foo(self) {} + } + (&()).foo(); // Don't lint. `()` doesn't implement `Foo` + (&()).foo(); + + impl Foo for i32 { + fn foo(self) {} + } + impl Foo for &'_ i32 { + fn foo(self) {} + } + (&5).foo(); // Don't lint. `5` will call `::foo` + (&5).foo(); + + trait FooRef { + fn foo_ref(&self); + } + impl FooRef for () { + fn foo_ref(&self) {} + } + impl FooRef for &'_ () { + fn foo_ref(&self) {} + } + (&&()).foo_ref(); // Don't lint. `&()` will call `<() as FooRef>::foo_ref` } #[allow(clippy::needless_borrowed_reference)] diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs index 63515a82158..d636a401003 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -85,6 +85,36 @@ fn ref_mut_i32(_: &mut i32) {} let _ = (&x).0; let x = &x as *const (i32, i32); let _ = unsafe { (&*x).0 }; + + // Issue #8367 + trait Foo { + fn foo(self); + } + impl Foo for &'_ () { + fn foo(self) {} + } + (&()).foo(); // Don't lint. `()` doesn't implement `Foo` + (&&()).foo(); + + impl Foo for i32 { + fn foo(self) {} + } + impl Foo for &'_ i32 { + fn foo(self) {} + } + (&5).foo(); // Don't lint. `5` will call `::foo` + (&&5).foo(); + + trait FooRef { + fn foo_ref(&self); + } + impl FooRef for () { + fn foo_ref(&self) {} + } + impl FooRef for &'_ () { + fn foo_ref(&self) {} + } + (&&()).foo_ref(); // Don't lint. `&()` will call `<() as FooRef>::foo_ref` } #[allow(clippy::needless_borrowed_reference)] diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr index cd23d9fd072..8a2e2b98959 100644 --- a/tests/ui/needless_borrow.stderr +++ b/tests/ui/needless_borrow.stderr @@ -108,5 +108,17 @@ error: this expression borrows a value the compiler would automatically borrow LL | let _ = unsafe { (&*x).0 }; | ^^^^^ help: change this to: `(*x)` -error: aborting due to 18 previous errors +error: this expression creates a reference which is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:97:5 + | +LL | (&&()).foo(); + | ^^^^^^ help: change this to: `(&())` + +error: this expression creates a reference which is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:106:5 + | +LL | (&&5).foo(); + | ^^^^^ help: change this to: `(&5)` + +error: aborting due to 20 previous errors