Rollup merge of #80723 - rylev:noop-lint-pass, r=estebank

Implement NOOP_METHOD_CALL lint

Implements the beginnings of https://github.com/rust-lang/lang-team/issues/67 - a lint for detecting noop method calls (e.g, calling `<&T as Clone>::clone()` when `T: !Clone`).

This PR does not fully realize the vision and has a few limitations that need to be addressed either before merging or in subsequent PRs:
* [ ] No UFCS support
* [ ] The warning message is pretty plain
* [ ] Doesn't work for `ToOwned`

The implementation uses [`Instance::resolve`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/instance/struct.Instance.html#method.resolve) which is normally later in the compiler. It seems that there are some invariants that this function relies on that we try our best to respect. For instance, it expects substitutions to have happened, which haven't yet performed, but we check first for `needs_subst` to ensure we're dealing with a monomorphic type.

Thank you to ```@davidtwco,``` ```@Aaron1011,``` and ```@wesleywiser``` for helping me at various points through out this PR ❤️.
This commit is contained in:
Mara 2021-03-05 10:57:14 +01:00 committed by GitHub
commit e6a6df5daa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
20 changed files with 240 additions and 22 deletions

View File

@ -24,7 +24,7 @@ pub fn get_rpath_flags(config: &mut RPathConfig<'_>) -> Vec<String> {
debug!("preparing the RPATH!");
let libs = config.used_crates.clone();
let libs = config.used_crates;
let libs = libs.iter().filter_map(|&(_, ref l)| l.option()).collect::<Vec<_>>();
let rpaths = get_rpaths(config, &libs);
let mut flags = rpaths_to_flags(&rpaths);

View File

@ -57,6 +57,7 @@ mod methods;
mod non_ascii_idents;
mod non_fmt_panic;
mod nonstandard_style;
mod noop_method_call;
mod passes;
mod redundant_semicolon;
mod traits;
@ -81,6 +82,7 @@ use methods::*;
use non_ascii_idents::*;
use non_fmt_panic::NonPanicFmt;
use nonstandard_style::*;
use noop_method_call::*;
use redundant_semicolon::*;
use traits::*;
use types::*;
@ -168,6 +170,7 @@ macro_rules! late_lint_passes {
DropTraitConstraints: DropTraitConstraints,
TemporaryCStringAsPtr: TemporaryCStringAsPtr,
NonPanicFmt: NonPanicFmt,
NoopMethodCall: NoopMethodCall,
]
);
};

View File

@ -0,0 +1,111 @@
use crate::context::LintContext;
use crate::rustc_middle::ty::TypeFoldable;
use crate::LateContext;
use crate::LateLintPass;
use rustc_hir::def::DefKind;
use rustc_hir::{Expr, ExprKind};
use rustc_middle::ty;
use rustc_span::symbol::sym;
declare_lint! {
/// The `noop_method_call` lint detects specific calls to noop methods
/// such as a calling `<&T as Clone>::clone` where `T: !Clone`.
///
/// ### Example
///
/// ```rust
/// # #![allow(unused)]
/// #![warn(noop_method_call)]
/// struct Foo;
/// let foo = &Foo;
/// let clone: &Foo = foo.clone();
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Some method calls are noops meaning that they do nothing. Usually such methods
/// are the result of blanket implementations that happen to create some method invocations
/// that end up not doing anything. For instance, `Clone` is implemented on all `&T`, but
/// 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,
Allow,
"detects the use of well-known noop methods"
}
declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]);
impl<'tcx> LateLintPass<'tcx> for NoopMethodCall {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
// We only care about method calls.
let (call, elements) = match expr.kind {
ExprKind::MethodCall(call, _, elements, _) => (call, elements),
_ => return,
};
// We only care about method calls corresponding to the `Clone`, `Deref` and `Borrow`
// traits and ignore any other method call.
let (trait_id, did) = match cx.typeck_results().type_dependent_def(expr.hir_id) {
// Verify we are dealing with a method/associated function.
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, sym::Deref, sym::Borrow]
.iter()
.any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) =>
{
(trait_id, did)
}
_ => return,
},
_ => return,
};
let substs = cx.typeck_results().node_substs(expr.hir_id);
if substs.needs_subst() {
// We can't resolve on types that require monomorphization, so we don't handle them if
// we need to perfom substitution.
return;
}
let param_env = cx.tcx.param_env(trait_id);
// Resolve the trait method instance.
let i = match ty::Instance::resolve(cx.tcx, param_env, did, substs) {
Ok(Some(i)) => i,
_ => return,
};
// (Re)check that it implements the noop diagnostic.
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 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 \
// type are the same, implying that the method call is unnecessary.
return;
}
let expr_span = expr.span;
let note = format!(
"the type `{:?}` which `{}` is being called on is the same as \
the type returned from `{}`, so the method call does not do \
anything and can be removed",
receiver_ty, method, method,
);
let span = expr_span.with_lo(receiver.span.hi());
cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| {
let method = &call.ident.name;
let message = format!(
"call to `.{}()` on a reference in this situation does nothing",
&method,
);
lint.build(&message)
.span_label(span, "unnecessary method call")
.note(&note)
.emit()
});
}
}
}
}

View File

@ -12,7 +12,6 @@ use rustc_target::spec::abi;
use std::borrow::Cow;
use std::fmt;
use std::ops::Deref;
#[derive(Clone, Copy, Debug, PartialEq, Eq, TypeFoldable)]
pub struct ExpectedFound<T> {
@ -548,7 +547,6 @@ impl<T> Trait<T> for X {
TargetFeatureCast(def_id) => {
let attrs = self.get_attrs(*def_id);
let target_spans = attrs
.deref()
.iter()
.filter(|attr| attr.has_name(sym::target_feature))
.map(|attr| attr.span);

View File

@ -165,7 +165,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
self.consume_operand(location, value);
// Invalidate all borrows of local places
let borrow_set = self.borrow_set.clone();
let borrow_set = self.borrow_set;
let resume = self.location_table.start_index(resume.start_location());
for (i, data) in borrow_set.iter_enumerated() {
if borrow_of_local_data(data.borrowed_place) {
@ -177,7 +177,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
}
TerminatorKind::Resume | TerminatorKind::Return | TerminatorKind::GeneratorDrop => {
// Invalidate all borrows of local places
let borrow_set = self.borrow_set.clone();
let borrow_set = self.borrow_set;
let start = self.location_table.start_index(location);
for (i, data) in borrow_set.iter_enumerated() {
if borrow_of_local_data(data.borrowed_place) {
@ -369,7 +369,7 @@ impl<'cx, 'tcx> InvalidationGenerator<'cx, 'tcx> {
);
let tcx = self.tcx;
let body = self.body;
let borrow_set = self.borrow_set.clone();
let borrow_set = self.borrow_set;
let indices = self.borrow_set.indices();
each_borrow_involving_path(
self,
@ -377,7 +377,7 @@ impl<'cx, 'tcx> InvalidationGenerator<'cx, 'tcx> {
body,
location,
(sd, place),
&borrow_set.clone(),
borrow_set,
indices,
|this, borrow_index, borrow| {
match (rw, borrow.kind) {

View File

@ -51,7 +51,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
PatKind::Constant { value } => Test {
span: match_pair.pattern.span,
kind: TestKind::Eq { value, ty: match_pair.pattern.ty.clone() },
kind: TestKind::Eq { value, ty: match_pair.pattern.ty },
},
PatKind::Range(range) => {

View File

@ -129,6 +129,7 @@ symbols! {
BTreeMap,
BTreeSet,
BinaryHeap,
Borrow,
C,
CString,
Center,
@ -141,6 +142,7 @@ symbols! {
Decodable,
Decoder,
Default,
Deref,
Encodable,
Encoder,
Eq,
@ -789,6 +791,9 @@ symbols! {
none_error,
nontemporal_store,
nontrapping_dash_fptoint: "nontrapping-fptoint",
noop_method_borrow,
noop_method_clone,
noop_method_deref,
noreturn,
nostack,
not,

View File

@ -819,7 +819,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
sig.decl
.inputs
.iter()
.map(|arg| match arg.clone().kind {
.map(|arg| match arg.kind {
hir::TyKind::Tup(ref tys) => ArgKind::Tuple(
Some(arg.span),
vec![("_".to_owned(), "_".to_owned()); tys.len()],

View File

@ -165,7 +165,7 @@ crate fn evaluate_goal<'tcx>(
// let's just ignore that
let sol = Canonical {
max_universe: ty::UniverseIndex::from_usize(0),
variables: obligation.variables.clone(),
variables: obligation.variables,
value: QueryResponse {
var_values: CanonicalVarValues { var_values: IndexVec::new() }
.make_identity(tcx),

View File

@ -465,7 +465,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let expected_arg_tys = self.expected_inputs_for_expected_output(
call_expr.span,
expected,
fn_sig.output().clone(),
fn_sig.output(),
fn_sig.inputs(),
);

View File

@ -711,7 +711,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
});
let ret_ty = ret_coercion.borrow().expected_ty();
let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty.clone());
let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty);
ret_coercion.borrow_mut().coerce(
self,
&self.cause(return_expr.span, ObligationCauseCode::ReturnValue(return_expr.hir_id)),

View File

@ -1801,11 +1801,11 @@ fn test_occupied_entry_key() {
let key = "hello there";
let value = "value goes here";
assert!(a.is_empty());
a.insert(key.clone(), value.clone());
a.insert(key, value);
assert_eq!(a.len(), 1);
assert_eq!(a[key], value);
match a.entry(key.clone()) {
match a.entry(key) {
Vacant(_) => panic!(),
Occupied(e) => assert_eq!(key, *e.key()),
}
@ -1821,11 +1821,11 @@ fn test_vacant_entry_key() {
let value = "value goes here";
assert!(a.is_empty());
match a.entry(key.clone()) {
match a.entry(key) {
Occupied(_) => panic!(),
Vacant(e) => {
assert_eq!(key, *e.key());
e.insert(value.clone());
e.insert(value);
}
}
assert_eq!(a.len(), 1);

View File

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

View File

@ -104,12 +104,14 @@
/// [impls]: #implementors
#[stable(feature = "rust1", since = "1.0.0")]
#[lang = "clone"]
#[rustc_diagnostic_item = "Clone"]
pub trait Clone: Sized {
/// Returns a copy of the value.
///
/// # Examples
///
/// ```
/// # #![allow(noop_method_call)]
/// let hello = "Hello"; // &str implements Clone
///
/// assert_eq!("Hello", hello.clone());
@ -221,6 +223,7 @@ mod impls {
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> Clone for &T {
#[inline]
#[rustc_diagnostic_item = "noop_method_clone"]
fn clone(&self) -> Self {
*self
}

View File

@ -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<T: ?Sized> Deref for &T {
type Target = T;
#[rustc_diagnostic_item = "noop_method_deref"]
fn deref(&self) -> &T {
*self
}

View File

@ -9,7 +9,7 @@ fn test_intersperse() {
assert_eq!(v, vec![1]);
let xs = ["a", "", "b", "c"];
let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect();
let v: Vec<&str> = xs.iter().map(|x| *x).intersperse(", ").collect();
let text: String = v.concat();
assert_eq!(text, "a, , b, c".to_string());
@ -24,7 +24,7 @@ fn test_intersperse_size_hint() {
assert_eq!(iter.size_hint(), (0, Some(0)));
let xs = ["a", "", "b", "c"];
let mut iter = xs.iter().map(|x| x.clone()).intersperse(", ");
let mut iter = xs.iter().map(|x| *x).intersperse(", ");
assert_eq!(iter.size_hint(), (7, Some(7)));
assert_eq!(iter.next(), Some("a"));

View File

@ -774,11 +774,11 @@ fn test_occupied_entry_key() {
let key = "hello there";
let value = "value goes here";
assert!(a.is_empty());
a.insert(key.clone(), value.clone());
a.insert(key, value);
assert_eq!(a.len(), 1);
assert_eq!(a[key], value);
match a.entry(key.clone()) {
match a.entry(key) {
Vacant(_) => panic!(),
Occupied(e) => assert_eq!(key, *e.key()),
}
@ -793,11 +793,11 @@ fn test_vacant_entry_key() {
let value = "value goes here";
assert!(a.is_empty());
match a.entry(key.clone()) {
match a.entry(key) {
Occupied(_) => panic!(),
Vacant(e) => {
assert_eq!(key, *e.key());
e.insert(value.clone());
e.insert(value);
}
}
assert_eq!(a.len(), 1);

View File

@ -0,0 +1,54 @@
// check-pass
#![allow(unused)]
#![warn(noop_method_call)]
use std::borrow::Borrow;
use std::ops::Deref;
struct PlainType<T>(T);
#[derive(Clone)]
struct CloneType<T>(T);
fn main() {
let non_clone_type_ref = &PlainType(1u32);
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
let clone_type_ref = &CloneType(1u32);
let clone_type_ref_clone: CloneType<u32> = clone_type_ref.clone();
// Calling clone on a double reference doesn't warn since the method call itself
// peels the outer reference off
let clone_type_ref = &&CloneType(1u32);
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 _v: Vec<&str> = xs.iter().map(|x| x.clone()).collect(); // ok, but could use `*x` instead
}
fn generic<T>(non_clone_type: &PlainType<T>) {
non_clone_type.clone();
}
fn non_generic(non_clone_type: &PlainType<u32>) {
non_clone_type.clone();
//~^ WARNING call to `.clone()` on a reference in this situation does nothing
}

View File

@ -0,0 +1,39 @@
warning: call to `.clone()` on a reference in this situation does nothing
--> $DIR/noop-method-call.rs:16:71
|
LL | let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref.clone();
| ^^^^^^^^ unnecessary method call
|
note: the lint level is defined here
--> $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
--> $DIR/noop-method-call.rs:52:19
|
LL | non_clone_type.clone();
| ^^^^^^^^ unnecessary 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: 4 warnings emitted

View File

@ -29,5 +29,6 @@ m!(y);
fn main() {
use crate::y::*;
#[allow(noop_method_call)]
(&()).deref();
}