Skip to content

Commit e6a6df5

Browse files
authored
Rollup merge of #80723 - rylev:noop-lint-pass, r=estebank
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 ❤️.
2 parents a0d66b5 + 25637b2 commit e6a6df5

File tree

20 files changed

+240
-22
lines changed

20 files changed

+240
-22
lines changed

compiler/rustc_codegen_ssa/src/back/rpath.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ pub fn get_rpath_flags(config: &mut RPathConfig<'_>) -> Vec<String> {
2424

2525
debug!("preparing the RPATH!");
2626

27-
let libs = config.used_crates.clone();
27+
let libs = config.used_crates;
2828
let libs = libs.iter().filter_map(|&(_, ref l)| l.option()).collect::<Vec<_>>();
2929
let rpaths = get_rpaths(config, &libs);
3030
let mut flags = rpaths_to_flags(&rpaths);

compiler/rustc_lint/src/lib.rs

+3
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ mod methods;
5757
mod non_ascii_idents;
5858
mod non_fmt_panic;
5959
mod nonstandard_style;
60+
mod noop_method_call;
6061
mod passes;
6162
mod redundant_semicolon;
6263
mod traits;
@@ -81,6 +82,7 @@ use methods::*;
8182
use non_ascii_idents::*;
8283
use non_fmt_panic::NonPanicFmt;
8384
use nonstandard_style::*;
85+
use noop_method_call::*;
8486
use redundant_semicolon::*;
8587
use traits::*;
8688
use types::*;
@@ -168,6 +170,7 @@ macro_rules! late_lint_passes {
168170
DropTraitConstraints: DropTraitConstraints,
169171
TemporaryCStringAsPtr: TemporaryCStringAsPtr,
170172
NonPanicFmt: NonPanicFmt,
173+
NoopMethodCall: NoopMethodCall,
171174
]
172175
);
173176
};
+111
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
use crate::context::LintContext;
2+
use crate::rustc_middle::ty::TypeFoldable;
3+
use crate::LateContext;
4+
use crate::LateLintPass;
5+
use rustc_hir::def::DefKind;
6+
use rustc_hir::{Expr, ExprKind};
7+
use rustc_middle::ty;
8+
use rustc_span::symbol::sym;
9+
10+
declare_lint! {
11+
/// The `noop_method_call` lint detects specific calls to noop methods
12+
/// such as a calling `<&T as Clone>::clone` where `T: !Clone`.
13+
///
14+
/// ### Example
15+
///
16+
/// ```rust
17+
/// # #![allow(unused)]
18+
/// #![warn(noop_method_call)]
19+
/// struct Foo;
20+
/// let foo = &Foo;
21+
/// let clone: &Foo = foo.clone();
22+
/// ```
23+
///
24+
/// {{produces}}
25+
///
26+
/// ### Explanation
27+
///
28+
/// Some method calls are noops meaning that they do nothing. Usually such methods
29+
/// are the result of blanket implementations that happen to create some method invocations
30+
/// that end up not doing anything. For instance, `Clone` is implemented on all `&T`, but
31+
/// calling `clone` on a `&T` where `T` does not implement clone, actually doesn't do anything
32+
/// as references are copy. This lint detects these calls and warns the user about them.
33+
pub NOOP_METHOD_CALL,
34+
Allow,
35+
"detects the use of well-known noop methods"
36+
}
37+
38+
declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]);
39+
40+
impl<'tcx> LateLintPass<'tcx> for NoopMethodCall {
41+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
42+
// We only care about method calls.
43+
let (call, elements) = match expr.kind {
44+
ExprKind::MethodCall(call, _, elements, _) => (call, elements),
45+
_ => return,
46+
};
47+
// We only care about method calls corresponding to the `Clone`, `Deref` and `Borrow`
48+
// traits and ignore any other method call.
49+
let (trait_id, did) = match cx.typeck_results().type_dependent_def(expr.hir_id) {
50+
// Verify we are dealing with a method/associated function.
51+
Some((DefKind::AssocFn, did)) => match cx.tcx.trait_of_item(did) {
52+
// Check that we're dealing with a trait method for one of the traits we care about.
53+
Some(trait_id)
54+
if [sym::Clone, sym::Deref, sym::Borrow]
55+
.iter()
56+
.any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) =>
57+
{
58+
(trait_id, did)
59+
}
60+
_ => return,
61+
},
62+
_ => return,
63+
};
64+
let substs = cx.typeck_results().node_substs(expr.hir_id);
65+
if substs.needs_subst() {
66+
// We can't resolve on types that require monomorphization, so we don't handle them if
67+
// we need to perfom substitution.
68+
return;
69+
}
70+
let param_env = cx.tcx.param_env(trait_id);
71+
// Resolve the trait method instance.
72+
let i = match ty::Instance::resolve(cx.tcx, param_env, did, substs) {
73+
Ok(Some(i)) => i,
74+
_ => return,
75+
};
76+
// (Re)check that it implements the noop diagnostic.
77+
for s in [sym::noop_method_clone, sym::noop_method_deref, sym::noop_method_borrow].iter() {
78+
if cx.tcx.is_diagnostic_item(*s, i.def_id()) {
79+
let method = &call.ident.name;
80+
let receiver = &elements[0];
81+
let receiver_ty = cx.typeck_results().expr_ty(receiver);
82+
let expr_ty = cx.typeck_results().expr_ty_adjusted(expr);
83+
if receiver_ty != expr_ty {
84+
// This lint will only trigger if the receiver type and resulting expression \
85+
// type are the same, implying that the method call is unnecessary.
86+
return;
87+
}
88+
let expr_span = expr.span;
89+
let note = format!(
90+
"the type `{:?}` which `{}` is being called on is the same as \
91+
the type returned from `{}`, so the method call does not do \
92+
anything and can be removed",
93+
receiver_ty, method, method,
94+
);
95+
96+
let span = expr_span.with_lo(receiver.span.hi());
97+
cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| {
98+
let method = &call.ident.name;
99+
let message = format!(
100+
"call to `.{}()` on a reference in this situation does nothing",
101+
&method,
102+
);
103+
lint.build(&message)
104+
.span_label(span, "unnecessary method call")
105+
.note(&note)
106+
.emit()
107+
});
108+
}
109+
}
110+
}
111+
}

compiler/rustc_middle/src/ty/error.rs

-2
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ use rustc_target::spec::abi;
1212

1313
use std::borrow::Cow;
1414
use std::fmt;
15-
use std::ops::Deref;
1615

1716
#[derive(Clone, Copy, Debug, PartialEq, Eq, TypeFoldable)]
1817
pub struct ExpectedFound<T> {
@@ -548,7 +547,6 @@ impl<T> Trait<T> for X {
548547
TargetFeatureCast(def_id) => {
549548
let attrs = self.get_attrs(*def_id);
550549
let target_spans = attrs
551-
.deref()
552550
.iter()
553551
.filter(|attr| attr.has_name(sym::target_feature))
554552
.map(|attr| attr.span);

compiler/rustc_mir/src/borrow_check/invalidation.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
165165
self.consume_operand(location, value);
166166

167167
// Invalidate all borrows of local places
168-
let borrow_set = self.borrow_set.clone();
168+
let borrow_set = self.borrow_set;
169169
let resume = self.location_table.start_index(resume.start_location());
170170
for (i, data) in borrow_set.iter_enumerated() {
171171
if borrow_of_local_data(data.borrowed_place) {
@@ -177,7 +177,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
177177
}
178178
TerminatorKind::Resume | TerminatorKind::Return | TerminatorKind::GeneratorDrop => {
179179
// Invalidate all borrows of local places
180-
let borrow_set = self.borrow_set.clone();
180+
let borrow_set = self.borrow_set;
181181
let start = self.location_table.start_index(location);
182182
for (i, data) in borrow_set.iter_enumerated() {
183183
if borrow_of_local_data(data.borrowed_place) {
@@ -369,15 +369,15 @@ impl<'cx, 'tcx> InvalidationGenerator<'cx, 'tcx> {
369369
);
370370
let tcx = self.tcx;
371371
let body = self.body;
372-
let borrow_set = self.borrow_set.clone();
372+
let borrow_set = self.borrow_set;
373373
let indices = self.borrow_set.indices();
374374
each_borrow_involving_path(
375375
self,
376376
tcx,
377377
body,
378378
location,
379379
(sd, place),
380-
&borrow_set.clone(),
380+
borrow_set,
381381
indices,
382382
|this, borrow_index, borrow| {
383383
match (rw, borrow.kind) {

compiler/rustc_mir_build/src/build/matches/test.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
5151

5252
PatKind::Constant { value } => Test {
5353
span: match_pair.pattern.span,
54-
kind: TestKind::Eq { value, ty: match_pair.pattern.ty.clone() },
54+
kind: TestKind::Eq { value, ty: match_pair.pattern.ty },
5555
},
5656

5757
PatKind::Range(range) => {

compiler/rustc_span/src/symbol.rs

+5
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ symbols! {
129129
BTreeMap,
130130
BTreeSet,
131131
BinaryHeap,
132+
Borrow,
132133
C,
133134
CString,
134135
Center,
@@ -141,6 +142,7 @@ symbols! {
141142
Decodable,
142143
Decoder,
143144
Default,
145+
Deref,
144146
Encodable,
145147
Encoder,
146148
Eq,
@@ -789,6 +791,9 @@ symbols! {
789791
none_error,
790792
nontemporal_store,
791793
nontrapping_dash_fptoint: "nontrapping-fptoint",
794+
noop_method_borrow,
795+
noop_method_clone,
796+
noop_method_deref,
792797
noreturn,
793798
nostack,
794799
not,

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -819,7 +819,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
819819
sig.decl
820820
.inputs
821821
.iter()
822-
.map(|arg| match arg.clone().kind {
822+
.map(|arg| match arg.kind {
823823
hir::TyKind::Tup(ref tys) => ArgKind::Tuple(
824824
Some(arg.span),
825825
vec![("_".to_owned(), "_".to_owned()); tys.len()],

compiler/rustc_traits/src/chalk/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ crate fn evaluate_goal<'tcx>(
165165
// let's just ignore that
166166
let sol = Canonical {
167167
max_universe: ty::UniverseIndex::from_usize(0),
168-
variables: obligation.variables.clone(),
168+
variables: obligation.variables,
169169
value: QueryResponse {
170170
var_values: CanonicalVarValues { var_values: IndexVec::new() }
171171
.make_identity(tcx),

compiler/rustc_typeck/src/check/callee.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
465465
let expected_arg_tys = self.expected_inputs_for_expected_output(
466466
call_expr.span,
467467
expected,
468-
fn_sig.output().clone(),
468+
fn_sig.output(),
469469
fn_sig.inputs(),
470470
);
471471

compiler/rustc_typeck/src/check/expr.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -711,7 +711,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
711711
});
712712

713713
let ret_ty = ret_coercion.borrow().expected_ty();
714-
let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty.clone());
714+
let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty);
715715
ret_coercion.borrow_mut().coerce(
716716
self,
717717
&self.cause(return_expr.span, ObligationCauseCode::ReturnValue(return_expr.hir_id)),

library/alloc/src/collections/btree/map/tests.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -1801,11 +1801,11 @@ fn test_occupied_entry_key() {
18011801
let key = "hello there";
18021802
let value = "value goes here";
18031803
assert!(a.is_empty());
1804-
a.insert(key.clone(), value.clone());
1804+
a.insert(key, value);
18051805
assert_eq!(a.len(), 1);
18061806
assert_eq!(a[key], value);
18071807

1808-
match a.entry(key.clone()) {
1808+
match a.entry(key) {
18091809
Vacant(_) => panic!(),
18101810
Occupied(e) => assert_eq!(key, *e.key()),
18111811
}
@@ -1821,11 +1821,11 @@ fn test_vacant_entry_key() {
18211821
let value = "value goes here";
18221822

18231823
assert!(a.is_empty());
1824-
match a.entry(key.clone()) {
1824+
match a.entry(key) {
18251825
Occupied(_) => panic!(),
18261826
Vacant(e) => {
18271827
assert_eq!(key, *e.key());
1828-
e.insert(value.clone());
1828+
e.insert(value);
18291829
}
18301830
}
18311831
assert_eq!(a.len(), 1);

library/core/src/borrow.rs

+2
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@
153153
/// [`HashMap<K, V>`]: ../../std/collections/struct.HashMap.html
154154
/// [`String`]: ../../std/string/struct.String.html
155155
#[stable(feature = "rust1", since = "1.0.0")]
156+
#[rustc_diagnostic_item = "Borrow"]
156157
pub trait Borrow<Borrowed: ?Sized> {
157158
/// Immutably borrows from an owned value.
158159
///
@@ -205,6 +206,7 @@ pub trait BorrowMut<Borrowed: ?Sized>: Borrow<Borrowed> {
205206

206207
#[stable(feature = "rust1", since = "1.0.0")]
207208
impl<T: ?Sized> Borrow<T> for T {
209+
#[rustc_diagnostic_item = "noop_method_borrow"]
208210
fn borrow(&self) -> &T {
209211
self
210212
}

library/core/src/clone.rs

+3
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,14 @@
104104
/// [impls]: #implementors
105105
#[stable(feature = "rust1", since = "1.0.0")]
106106
#[lang = "clone"]
107+
#[rustc_diagnostic_item = "Clone"]
107108
pub trait Clone: Sized {
108109
/// Returns a copy of the value.
109110
///
110111
/// # Examples
111112
///
112113
/// ```
114+
/// # #![allow(noop_method_call)]
113115
/// let hello = "Hello"; // &str implements Clone
114116
///
115117
/// assert_eq!("Hello", hello.clone());
@@ -221,6 +223,7 @@ mod impls {
221223
#[stable(feature = "rust1", since = "1.0.0")]
222224
impl<T: ?Sized> Clone for &T {
223225
#[inline]
226+
#[rustc_diagnostic_item = "noop_method_clone"]
224227
fn clone(&self) -> Self {
225228
*self
226229
}

library/core/src/ops/deref.rs

+2
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
#[doc(alias = "*")]
6161
#[doc(alias = "&*")]
6262
#[stable(feature = "rust1", since = "1.0.0")]
63+
#[rustc_diagnostic_item = "Deref"]
6364
pub trait Deref {
6465
/// The resulting type after dereferencing.
6566
#[stable(feature = "rust1", since = "1.0.0")]
@@ -78,6 +79,7 @@ pub trait Deref {
7879
impl<T: ?Sized> Deref for &T {
7980
type Target = T;
8081

82+
#[rustc_diagnostic_item = "noop_method_deref"]
8183
fn deref(&self) -> &T {
8284
*self
8385
}

library/core/tests/iter/adapters/intersperse.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ fn test_intersperse() {
99
assert_eq!(v, vec![1]);
1010

1111
let xs = ["a", "", "b", "c"];
12-
let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect();
12+
let v: Vec<&str> = xs.iter().map(|x| *x).intersperse(", ").collect();
1313
let text: String = v.concat();
1414
assert_eq!(text, "a, , b, c".to_string());
1515

@@ -24,7 +24,7 @@ fn test_intersperse_size_hint() {
2424
assert_eq!(iter.size_hint(), (0, Some(0)));
2525

2626
let xs = ["a", "", "b", "c"];
27-
let mut iter = xs.iter().map(|x| x.clone()).intersperse(", ");
27+
let mut iter = xs.iter().map(|x| *x).intersperse(", ");
2828
assert_eq!(iter.size_hint(), (7, Some(7)));
2929

3030
assert_eq!(iter.next(), Some("a"));

library/std/src/collections/hash/map/tests.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -774,11 +774,11 @@ fn test_occupied_entry_key() {
774774
let key = "hello there";
775775
let value = "value goes here";
776776
assert!(a.is_empty());
777-
a.insert(key.clone(), value.clone());
777+
a.insert(key, value);
778778
assert_eq!(a.len(), 1);
779779
assert_eq!(a[key], value);
780780

781-
match a.entry(key.clone()) {
781+
match a.entry(key) {
782782
Vacant(_) => panic!(),
783783
Occupied(e) => assert_eq!(key, *e.key()),
784784
}
@@ -793,11 +793,11 @@ fn test_vacant_entry_key() {
793793
let value = "value goes here";
794794

795795
assert!(a.is_empty());
796-
match a.entry(key.clone()) {
796+
match a.entry(key) {
797797
Occupied(_) => panic!(),
798798
Vacant(e) => {
799799
assert_eq!(key, *e.key());
800-
e.insert(value.clone());
800+
e.insert(value);
801801
}
802802
}
803803
assert_eq!(a.len(), 1);

0 commit comments

Comments
 (0)