Fix borrow and deref

This commit is contained in:
Ryan Levick 2021-02-16 22:39:05 +01:00
parent da3995f0ec
commit 1999a3147f
11 changed files with 67 additions and 32 deletions

View File

@ -15,6 +15,7 @@
/// ///
/// ```rust /// ```rust
/// # #![allow(unused)] /// # #![allow(unused)]
/// #![deny(noop_method_call)]
/// struct Foo; /// struct Foo;
/// let foo = &Foo; /// let foo = &Foo;
/// let clone: &Foo = foo.clone(); /// 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 /// 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. /// as references are copy. This lint detects these calls and warns the user about them.
pub NOOP_METHOD_CALL, pub NOOP_METHOD_CALL,
Warn, Allow,
"detects the use of well-known noop methods" "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) { 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. // Check that we're dealing with a trait method for one of the traits we care about.
Some(trait_id) 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) (trait_id, did)
} }
@ -71,20 +74,11 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
_ => return, _ => return,
}; };
// (Re)check that it implements the noop diagnostic. // (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()) { if cx.tcx.is_diagnostic_item(*s, i.def_id()) {
let method = &call.ident.name; let method = &call.ident.name;
let receiver = &elements[0]; let receiver = &elements[0];
let receiver_ty = cx.typeck_results().expr_ty(receiver); 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); let expr_ty = cx.typeck_results().expr_ty_adjusted(expr);
if receiver_ty != expr_ty { if receiver_ty != expr_ty {
// This lint will only trigger if the receiver type and resulting expression \ // This lint will only trigger if the receiver type and resulting expression \

View File

@ -142,6 +142,7 @@
Decodable, Decodable,
Decoder, Decoder,
Default, Default,
Deref,
Encodable, Encodable,
Encoder, Encoder,
Eq, Eq,
@ -790,7 +791,9 @@
none_error, none_error,
nontemporal_store, nontemporal_store,
nontrapping_dash_fptoint: "nontrapping-fptoint", nontrapping_dash_fptoint: "nontrapping-fptoint",
noop_method_borrow,
noop_method_clone, noop_method_clone,
noop_method_deref,
noreturn, noreturn,
nostack, nostack,
not, not,

View File

@ -153,6 +153,7 @@
/// [`HashMap<K, V>`]: ../../std/collections/struct.HashMap.html /// [`HashMap<K, V>`]: ../../std/collections/struct.HashMap.html
/// [`String`]: ../../std/string/struct.String.html /// [`String`]: ../../std/string/struct.String.html
#[stable(feature = "rust1", since = "1.0.0")] #[stable(feature = "rust1", since = "1.0.0")]
#[rustc_diagnostic_item = "Borrow"]
pub trait Borrow<Borrowed: ?Sized> { pub trait Borrow<Borrowed: ?Sized> {
/// Immutably borrows from an owned value. /// Immutably borrows from an owned value.
/// ///
@ -205,6 +206,7 @@ pub trait BorrowMut<Borrowed: ?Sized>: Borrow<Borrowed> {
#[stable(feature = "rust1", since = "1.0.0")] #[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> Borrow<T> for T { impl<T: ?Sized> Borrow<T> for T {
#[rustc_diagnostic_item = "noop_method_borrow"]
fn borrow(&self) -> &T { fn borrow(&self) -> &T {
self self
} }

View File

@ -60,6 +60,7 @@
#[doc(alias = "*")] #[doc(alias = "*")]
#[doc(alias = "&*")] #[doc(alias = "&*")]
#[stable(feature = "rust1", since = "1.0.0")] #[stable(feature = "rust1", since = "1.0.0")]
#[rustc_diagnostic_item = "Deref"]
pub trait Deref { pub trait Deref {
/// The resulting type after dereferencing. /// The resulting type after dereferencing.
#[stable(feature = "rust1", since = "1.0.0")] #[stable(feature = "rust1", since = "1.0.0")]
@ -78,6 +79,7 @@ pub trait Deref {
impl<T: ?Sized> Deref for &T { impl<T: ?Sized> Deref for &T {
type Target = T; type Target = T;
#[rustc_diagnostic_item = "noop_method_deref"]
fn deref(&self) -> &T { fn deref(&self) -> &T {
*self *self
} }

View File

@ -1,5 +1,3 @@
#![cfg_attr(not(bootstrap), allow(noop_method_call))]
#[test] #[test]
fn test_borrowed_clone() { fn test_borrowed_clone() {
let x = 5; let x = 5;

View File

@ -1,8 +1,6 @@
// run-pass // run-pass
// pretty-expanded FIXME #23616 // pretty-expanded FIXME #23616
#![allow(noop_method_call)]
struct NoClone; struct NoClone;
fn main() { fn main() {

View File

@ -1,15 +1,19 @@
// check-pass // check-pass
#![allow(unused)] #![allow(unused)]
#![warn(noop_method_call)]
struct NonCloneType<T>(T); use std::borrow::Borrow;
use std::ops::Deref;
struct PlainType<T>(T);
#[derive(Clone)] #[derive(Clone)]
struct CloneType<T>(T); struct CloneType<T>(T);
fn main() { fn main() {
let non_clone_type_ref = &NonCloneType(1u32); let non_clone_type_ref = &PlainType(1u32);
let non_clone_type_ref_clone: &NonCloneType<u32> = non_clone_type_ref.clone(); let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref.clone();
//~^ WARNING call to `.clone()` on a reference in this situation does nothing //~^ WARNING call to `.clone()` on a reference in this situation does nothing
let clone_type_ref = &CloneType(1u32); let clone_type_ref = &CloneType(1u32);
@ -20,15 +24,31 @@ fn main() {
let clone_type_ref = &&CloneType(1u32); let clone_type_ref = &&CloneType(1u32);
let clone_type_ref_clone: &CloneType<u32> = clone_type_ref.clone(); let clone_type_ref_clone: &CloneType<u32> = clone_type_ref.clone();
let non_deref_type = &PlainType(1u32);
let non_deref_type_deref: &PlainType<u32> = 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<u32> = non_deref_type.deref();
let non_borrow_type = &PlainType(1u32);
let non_borrow_type_borrow: &PlainType<u32> = 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<u32> = non_borrow_type.borrow();
let xs = ["a", "b", "c"]; let xs = ["a", "b", "c"];
let _v: Vec<&str> = xs.iter().map(|x| x.clone()).collect(); // ok, but could use `*x` instead let _v: Vec<&str> = xs.iter().map(|x| x.clone()).collect(); // ok, but could use `*x` instead
} }
fn generic<T>(non_clone_type: &NonCloneType<T>) { fn generic<T>(non_clone_type: &PlainType<T>) {
non_clone_type.clone(); non_clone_type.clone();
} }
fn non_generic(non_clone_type: &NonCloneType<u32>) { fn non_generic(non_clone_type: &PlainType<u32>) {
non_clone_type.clone(); non_clone_type.clone();
//~^ WARNING call to `.clone()` on a reference in this situation does nothing //~^ WARNING call to `.clone()` on a reference in this situation does nothing
} }

View File

@ -1,19 +1,39 @@
warning: call to `.clone()` on a reference in this situation does nothing 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<u32> = non_clone_type_ref.clone(); LL | let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref.clone();
| ^^^^^^^^ unnecessary method call | ^^^^^^^^ unnecessary method call
| |
= note: `#[warn(noop_method_call)]` on by default note: the lint level is defined here
= note: the type `&NonCloneType<u32>` 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 --> $DIR/noop-method-call.rs:4:9
|
LL | #![warn(noop_method_call)]
| ^^^^^^^^^^^^^^^^
= note: the type `&PlainType<u32>` 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<u32> = non_deref_type.deref();
| ^^^^^^^^ unnecessary method call
|
= note: the type `&PlainType<u32>` 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<u32> = non_borrow_type.borrow();
| ^^^^^^^^^ unnecessary method call
|
= note: the type `&PlainType<u32>` 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 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(); LL | non_clone_type.clone();
| ^^^^^^^^ unnecessary method call | ^^^^^^^^ unnecessary method call
| |
= note: the type `&NonCloneType<u32>` 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<u32>` 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

View File

@ -14,6 +14,5 @@ mod y {
pub fn main() { pub fn main() {
use x::*; use x::*;
#[allow(noop_method_call)]
(&0).deref(); (&0).deref();
} }

View File

@ -3,7 +3,6 @@
// check-pass // check-pass
#![feature(decl_macro, rustc_attrs)] #![feature(decl_macro, rustc_attrs)]
#![allow(noop_method_call)]
mod x { mod x {
pub use std::ops::Not as _; pub use std::ops::Not as _;

View File

@ -1,7 +1,7 @@
// does not test any rustfixable lints // does not test any rustfixable lints
#![warn(clippy::clone_on_ref_ptr)] #![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::cell::RefCell;
use std::rc::{self, Rc}; use std::rc::{self, Rc};