Skip to content

Commit

Permalink
Rollup merge of rust-lang#80723 - rylev:noop-lint-pass, r=estebank
Browse files Browse the repository at this point in the history
Implement NOOP_METHOD_CALL lint

Implements the beginnings of rust-lang/lang-team#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 ❤️.
  • Loading branch information
Dylan-DPC authored Mar 4, 2021
2 parents ea7c3e7 + 25637b2 commit 72e5b5a
Show file tree
Hide file tree
Showing 20 changed files with 240 additions and 22 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/back/rpath.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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::*;
Expand Down Expand Up @@ -168,6 +170,7 @@ macro_rules! late_lint_passes {
DropTraitConstraints: DropTraitConstraints,
TemporaryCStringAsPtr: TemporaryCStringAsPtr,
NonPanicFmt: NonPanicFmt,
NoopMethodCall: NoopMethodCall,
]
);
};
Expand Down
111 changes: 111 additions & 0 deletions compiler/rustc_lint/src/noop_method_call.rs
Original file line number Diff line number Diff line change
@@ -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()
});
}
}
}
}
2 changes: 0 additions & 2 deletions compiler/rustc_middle/src/ty/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_mir/src/borrow_check/invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -369,15 +369,15 @@ 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,
tcx,
body,
location,
(sd, place),
&borrow_set.clone(),
borrow_set,
indices,
|this, borrow_index, borrow| {
match (rw, borrow.kind) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/matches/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ symbols! {
BTreeMap,
BTreeSet,
BinaryHeap,
Borrow,
C,
CString,
Center,
Expand All @@ -141,6 +142,7 @@ symbols! {
Decodable,
Decoder,
Default,
Deref,
Encodable,
Encoder,
Eq,
Expand Down Expand Up @@ -790,6 +792,9 @@ symbols! {
none_error,
nontemporal_store,
nontrapping_dash_fptoint: "nontrapping-fptoint",
noop_method_borrow,
noop_method_clone,
noop_method_deref,
noreturn,
nostack,
not,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()],
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_traits/src/chalk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_typeck/src/check/callee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
);

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_typeck/src/check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
8 changes: 4 additions & 4 deletions library/alloc/src/collections/btree/map/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
}
Expand All @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/borrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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
}
Expand Down
3 changes: 3 additions & 0 deletions library/core/src/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/ops/deref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand All @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions library/core/tests/iter/adapters/intersperse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand All @@ -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"));
Expand Down
8 changes: 4 additions & 4 deletions library/std/src/collections/hash/map/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
}
Expand All @@ -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);
Expand Down
Loading

0 comments on commit 72e5b5a

Please sign in to comment.