From 1999a3147f5ab65cd556d45e631be5c18fbaebf4 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 16 Feb 2021 22:39:05 +0100 Subject: [PATCH] Fix borrow and deref --- compiler/rustc_lint/src/noop_method_call.rs | 18 ++++------ compiler/rustc_span/src/symbol.rs | 3 ++ library/core/src/borrow.rs | 2 ++ library/core/src/ops/deref.rs | 2 ++ library/core/tests/clone.rs | 2 -- src/test/ui/issues/issue-11820.rs | 2 -- src/test/ui/lint/noop-method-call.rs | 30 +++++++++++++--- src/test/ui/lint/noop-method-call.stderr | 36 ++++++++++++++----- src/test/ui/underscore-imports/cycle.rs | 1 - .../ui/underscore-imports/macro-expanded.rs | 1 - .../clippy/tests/ui/unnecessary_clone.rs | 2 +- 11 files changed, 67 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index 335c3c575e7..3a1b9c85fd1 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -15,6 +15,7 @@ /// /// ```rust /// # #![allow(unused)] + /// #![deny(noop_method_call)] /// struct Foo; /// let foo = &Foo; /// let clone: &Foo = foo.clone(); @@ -30,7 +31,7 @@ /// calling `clone` on a `&T` where `T` does not implement clone, actually doesn't do anything /// as references are copy. This lint detects these calls and warns the user about them. pub NOOP_METHOD_CALL, - Warn, + Allow, "detects the use of well-known noop methods" } @@ -50,7 +51,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { Some((DefKind::AssocFn, did)) => match cx.tcx.trait_of_item(did) { // Check that we're dealing with a trait method for one of the traits we care about. Some(trait_id) - if [sym::Clone].iter().any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) => + if [sym::Clone, sym::Deref, sym::Borrow] + .iter() + .any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) => { (trait_id, did) } @@ -71,20 +74,11 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { _ => return, }; // (Re)check that it implements the noop diagnostic. - for (s, peel_ref) in [(sym::noop_method_clone, false)].iter() { + for s in [sym::noop_method_clone, sym::noop_method_deref, sym::noop_method_borrow].iter() { if cx.tcx.is_diagnostic_item(*s, i.def_id()) { let method = &call.ident.name; let receiver = &elements[0]; let receiver_ty = cx.typeck_results().expr_ty(receiver); - let receiver_ty = match receiver_ty.kind() { - // Remove one borrow from the receiver if appropriate to positively verify that - // the receiver `&self` type and the return type are the same, depending on the - // involved trait being checked. - ty::Ref(_, ty, _) if *peel_ref => ty, - // When it comes to `Clone` we need to check the `receiver_ty` directly. - // FIXME: we must come up with a better strategy for this. - _ => receiver_ty, - }; let expr_ty = cx.typeck_results().expr_ty_adjusted(expr); if receiver_ty != expr_ty { // This lint will only trigger if the receiver type and resulting expression \ diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index f43b180e063..61f9a080a52 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -142,6 +142,7 @@ Decodable, Decoder, Default, + Deref, Encodable, Encoder, Eq, @@ -790,7 +791,9 @@ none_error, nontemporal_store, nontrapping_dash_fptoint: "nontrapping-fptoint", + noop_method_borrow, noop_method_clone, + noop_method_deref, noreturn, nostack, not, diff --git a/library/core/src/borrow.rs b/library/core/src/borrow.rs index c9040cd0a16..f28be20aaa1 100644 --- a/library/core/src/borrow.rs +++ b/library/core/src/borrow.rs @@ -153,6 +153,7 @@ /// [`HashMap`]: ../../std/collections/struct.HashMap.html /// [`String`]: ../../std/string/struct.String.html #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_diagnostic_item = "Borrow"] pub trait Borrow { /// Immutably borrows from an owned value. /// @@ -205,6 +206,7 @@ pub trait BorrowMut: Borrow { #[stable(feature = "rust1", since = "1.0.0")] impl Borrow for T { + #[rustc_diagnostic_item = "noop_method_borrow"] fn borrow(&self) -> &T { self } diff --git a/library/core/src/ops/deref.rs b/library/core/src/ops/deref.rs index 2419771eae2..10e3ce67448 100644 --- a/library/core/src/ops/deref.rs +++ b/library/core/src/ops/deref.rs @@ -60,6 +60,7 @@ #[doc(alias = "*")] #[doc(alias = "&*")] #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_diagnostic_item = "Deref"] pub trait Deref { /// The resulting type after dereferencing. #[stable(feature = "rust1", since = "1.0.0")] @@ -78,6 +79,7 @@ pub trait Deref { impl Deref for &T { type Target = T; + #[rustc_diagnostic_item = "noop_method_deref"] fn deref(&self) -> &T { *self } diff --git a/library/core/tests/clone.rs b/library/core/tests/clone.rs index 2f2aa9a20f9..c97a87aebce 100644 --- a/library/core/tests/clone.rs +++ b/library/core/tests/clone.rs @@ -1,5 +1,3 @@ -#![cfg_attr(not(bootstrap), allow(noop_method_call))] - #[test] fn test_borrowed_clone() { let x = 5; diff --git a/src/test/ui/issues/issue-11820.rs b/src/test/ui/issues/issue-11820.rs index dc6349b10ee..7ffe9652797 100644 --- a/src/test/ui/issues/issue-11820.rs +++ b/src/test/ui/issues/issue-11820.rs @@ -1,8 +1,6 @@ // run-pass // pretty-expanded FIXME #23616 -#![allow(noop_method_call)] - struct NoClone; fn main() { diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index 8e4b5bf4d12..9870c813572 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -1,15 +1,19 @@ // check-pass #![allow(unused)] +#![warn(noop_method_call)] -struct NonCloneType(T); +use std::borrow::Borrow; +use std::ops::Deref; + +struct PlainType(T); #[derive(Clone)] struct CloneType(T); fn main() { - let non_clone_type_ref = &NonCloneType(1u32); - let non_clone_type_ref_clone: &NonCloneType = non_clone_type_ref.clone(); + let non_clone_type_ref = &PlainType(1u32); + let non_clone_type_ref_clone: &PlainType = non_clone_type_ref.clone(); //~^ WARNING call to `.clone()` on a reference in this situation does nothing let clone_type_ref = &CloneType(1u32); @@ -20,15 +24,31 @@ fn main() { let clone_type_ref = &&CloneType(1u32); let clone_type_ref_clone: &CloneType = clone_type_ref.clone(); + let non_deref_type = &PlainType(1u32); + let non_deref_type_deref: &PlainType = non_deref_type.deref(); + //~^ WARNING call to `.deref()` on a reference in this situation does nothing + + // Dereferencing a &&T does not warn since it has collapsed the double reference + let non_deref_type = &&PlainType(1u32); + let non_deref_type_deref: &PlainType = non_deref_type.deref(); + + let non_borrow_type = &PlainType(1u32); + let non_borrow_type_borrow: &PlainType = non_borrow_type.borrow(); + //~^ WARNING call to `.borrow()` on a reference in this situation does nothing + + // Borrowing a &&T does not warn since it has collapsed the double reference + let non_borrow_type = &&PlainType(1u32); + let non_borrow_type_borrow: &PlainType = non_borrow_type.borrow(); + let xs = ["a", "b", "c"]; let _v: Vec<&str> = xs.iter().map(|x| x.clone()).collect(); // ok, but could use `*x` instead } -fn generic(non_clone_type: &NonCloneType) { +fn generic(non_clone_type: &PlainType) { non_clone_type.clone(); } -fn non_generic(non_clone_type: &NonCloneType) { +fn non_generic(non_clone_type: &PlainType) { non_clone_type.clone(); //~^ WARNING call to `.clone()` on a reference in this situation does nothing } diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index 85a67f53840..7f6f96bf1d1 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -1,19 +1,39 @@ warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:12:74 + --> $DIR/noop-method-call.rs:16:71 | -LL | let non_clone_type_ref_clone: &NonCloneType = non_clone_type_ref.clone(); - | ^^^^^^^^ unnecessary method call +LL | let non_clone_type_ref_clone: &PlainType = non_clone_type_ref.clone(); + | ^^^^^^^^ unnecessary method call | - = note: `#[warn(noop_method_call)]` on by default - = note: the type `&NonCloneType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed +note: the lint level is defined here + --> $DIR/noop-method-call.rs:4:9 + | +LL | #![warn(noop_method_call)] + | ^^^^^^^^^^^^^^^^ + = note: the type `&PlainType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed + +warning: call to `.deref()` on a reference in this situation does nothing + --> $DIR/noop-method-call.rs:28:63 + | +LL | let non_deref_type_deref: &PlainType = non_deref_type.deref(); + | ^^^^^^^^ unnecessary method call + | + = note: the type `&PlainType` which `deref` is being called on is the same as the type returned from `deref`, so the method call does not do anything and can be removed + +warning: call to `.borrow()` on a reference in this situation does nothing + --> $DIR/noop-method-call.rs:36:66 + | +LL | let non_borrow_type_borrow: &PlainType = non_borrow_type.borrow(); + | ^^^^^^^^^ unnecessary method call + | + = note: the type `&PlainType` which `borrow` is being called on is the same as the type returned from `borrow`, so the method call does not do anything and can be removed warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:32:19 + --> $DIR/noop-method-call.rs:52:19 | LL | non_clone_type.clone(); | ^^^^^^^^ unnecessary method call | - = note: the type `&NonCloneType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed + = note: the type `&PlainType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed -warning: 2 warnings emitted +warning: 4 warnings emitted diff --git a/src/test/ui/underscore-imports/cycle.rs b/src/test/ui/underscore-imports/cycle.rs index 987410fa84b..bacf9b2d5a9 100644 --- a/src/test/ui/underscore-imports/cycle.rs +++ b/src/test/ui/underscore-imports/cycle.rs @@ -14,6 +14,5 @@ mod y { pub fn main() { use x::*; - #[allow(noop_method_call)] (&0).deref(); } diff --git a/src/test/ui/underscore-imports/macro-expanded.rs b/src/test/ui/underscore-imports/macro-expanded.rs index 55e86e84855..43f527bc9a4 100644 --- a/src/test/ui/underscore-imports/macro-expanded.rs +++ b/src/test/ui/underscore-imports/macro-expanded.rs @@ -3,7 +3,6 @@ // check-pass #![feature(decl_macro, rustc_attrs)] -#![allow(noop_method_call)] mod x { pub use std::ops::Not as _; diff --git a/src/tools/clippy/tests/ui/unnecessary_clone.rs b/src/tools/clippy/tests/ui/unnecessary_clone.rs index ce26634a995..6770a7fac90 100644 --- a/src/tools/clippy/tests/ui/unnecessary_clone.rs +++ b/src/tools/clippy/tests/ui/unnecessary_clone.rs @@ -1,7 +1,7 @@ // does not test any rustfixable lints #![warn(clippy::clone_on_ref_ptr)] -#![allow(unused, noop_method_call, clippy::redundant_clone, clippy::unnecessary_wraps)] +#![allow(unused, clippy::redundant_clone, clippy::unnecessary_wraps)] use std::cell::RefCell; use std::rc::{self, Rc};