Skip to content

Commit fb6e491

Browse files
authored
Rollup merge of rust-lang#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 e0ef0c2 + af88b73 commit fb6e491

File tree

25 files changed

+257
-25
lines changed

25 files changed

+257
-25
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
@@ -55,6 +55,7 @@ mod levels;
5555
mod methods;
5656
mod non_ascii_idents;
5757
mod nonstandard_style;
58+
mod noop_method_call;
5859
mod panic_fmt;
5960
mod passes;
6061
mod redundant_semicolon;
@@ -81,6 +82,7 @@ use internal::*;
8182
use methods::*;
8283
use non_ascii_idents::*;
8384
use nonstandard_style::*;
85+
use noop_method_call::*;
8486
use panic_fmt::PanicFmt;
8587
use redundant_semicolon::*;
8688
use traits::*;
@@ -168,6 +170,7 @@ macro_rules! late_lint_passes {
168170
ClashingExternDeclarations: ClashingExternDeclarations::new(),
169171
DropTraitConstraints: DropTraitConstraints,
170172
TemporaryCStringAsPtr: TemporaryCStringAsPtr,
173+
NoopMethodCall: NoopMethodCall,
171174
PanicFmt: PanicFmt,
172175
]
173176
);
+125
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
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+
/// struct Foo;
19+
/// let foo = &Foo;
20+
/// let clone: &Foo = foo.clone();
21+
/// ```
22+
///
23+
/// {{produces}}
24+
///
25+
/// ### Explanation
26+
///
27+
/// Some method calls are noops meaning that they do nothing. Usually such methods
28+
/// are the result of blanket implementations that happen to create some method invocations
29+
/// that end up not doing anything. For instance, `Clone` is implemented on all `&T`, but
30+
/// calling `clone` on a `&T` where `T` does not implement clone, actually doesn't do anything
31+
/// as references are copy. This lint detects these calls and warns the user about them.
32+
pub NOOP_METHOD_CALL,
33+
Warn,
34+
"detects the use of well-known noop methods"
35+
}
36+
37+
declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]);
38+
39+
impl<'tcx> LateLintPass<'tcx> for NoopMethodCall {
40+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
41+
// We only care about method calls.
42+
let (call, elements) = match expr.kind {
43+
ExprKind::MethodCall(call, _, elements, _) => (call, elements),
44+
_ => return,
45+
};
46+
// We only care about method calls corresponding to the `Clone`, `Deref` and `Borrow`
47+
// traits and ignore any other method call.
48+
let (trait_id, did) = match cx.typeck_results().type_dependent_def(expr.hir_id) {
49+
// Verify we are dealing with a method/associated function.
50+
Some((DefKind::AssocFn, did)) => match cx.tcx.trait_of_item(did) {
51+
// Check that we're dealing with a trait method for one of the traits we care about.
52+
Some(trait_id)
53+
if [sym::Clone, sym::Deref, sym::Borrow]
54+
.iter()
55+
.any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) =>
56+
{
57+
(trait_id, did)
58+
}
59+
_ => return,
60+
},
61+
_ => return,
62+
};
63+
let substs = cx.typeck_results().node_substs(expr.hir_id);
64+
if substs.needs_subst() {
65+
// We can't resolve on types that require monomorphization, so we don't handle them if
66+
// we need to perfom substitution.
67+
return;
68+
}
69+
let param_env = cx.tcx.param_env(trait_id);
70+
// Resolve the trait method instance.
71+
let i = match ty::Instance::resolve(cx.tcx, param_env, did, substs) {
72+
Ok(Some(i)) => i,
73+
_ => return,
74+
};
75+
// (Re)check that it implements the noop diagnostic.
76+
for (s, peel_ref) in [
77+
(sym::noop_method_borrow, true),
78+
(sym::noop_method_clone, false),
79+
(sym::noop_method_deref, true),
80+
]
81+
.iter()
82+
{
83+
if cx.tcx.is_diagnostic_item(*s, i.def_id()) {
84+
let method = &call.ident.name;
85+
let receiver = &elements[0];
86+
let receiver_ty = cx.typeck_results().expr_ty(receiver);
87+
let receiver_ty = match receiver_ty.kind() {
88+
// Remove one borrow from the receiver if appropriate to positively verify that
89+
// the receiver `&self` type and the return type are the same, depending on the
90+
// involved trait being checked.
91+
ty::Ref(_, ty, _) if *peel_ref => ty,
92+
// When it comes to `Clone` we need to check the `receiver_ty` directly.
93+
// FIXME: we must come up with a better strategy for this.
94+
_ => receiver_ty,
95+
};
96+
let expr_ty = cx.typeck_results().expr_ty_adjusted(expr);
97+
if receiver_ty != expr_ty {
98+
// This lint will only trigger if the receiver type and resulting expression \
99+
// type are the same, implying that the method call is unnecessary.
100+
return;
101+
}
102+
let expr_span = expr.span;
103+
let note = format!(
104+
"the type `{:?}` which `{}` is being called on is the same as \
105+
the type returned from `{}`, so the method call does not do \
106+
anything and can be removed",
107+
receiver_ty, method, method,
108+
);
109+
110+
let span = expr_span.with_lo(receiver.span.hi());
111+
cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| {
112+
let method = &call.ident.name;
113+
let message = format!(
114+
"call to `.{}()` on a reference in this situation does nothing",
115+
&method,
116+
);
117+
lint.build(&message)
118+
.span_label(span, "unnecessary method call")
119+
.note(&note)
120+
.emit()
121+
});
122+
}
123+
}
124+
}
125+
}

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> {
@@ -535,7 +534,6 @@ impl<T> Trait<T> for X {
535534
TargetFeatureCast(def_id) => {
536535
let attrs = self.get_attrs(*def_id);
537536
let target_spans = attrs
538-
.deref()
539537
.iter()
540538
.filter(|attr| attr.has_name(sym::target_feature))
541539
.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
@@ -126,6 +126,7 @@ symbols! {
126126
Argument,
127127
ArgumentV1,
128128
Arguments,
129+
Borrow,
129130
C,
130131
CString,
131132
Center,
@@ -136,6 +137,7 @@ symbols! {
136137
Decodable,
137138
Decoder,
138139
Default,
140+
Deref,
139141
Encodable,
140142
Encoder,
141143
Eq,
@@ -765,6 +767,9 @@ symbols! {
765767
none_error,
766768
nontemporal_store,
767769
nontrapping_dash_fptoint: "nontrapping-fptoint",
770+
noop_method_borrow,
771+
noop_method_clone,
772+
noop_method_deref,
768773
noreturn,
769774
nostack,
770775
not,

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

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

compiler/rustc_traits/src/chalk/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ crate fn evaluate_goal<'tcx>(
112112
});
113113
let sol = Canonical {
114114
max_universe: ty::UniverseIndex::from_usize(0),
115-
variables: obligation.variables.clone(),
115+
variables: obligation.variables,
116116
value: QueryResponse {
117117
var_values: CanonicalVarValues { var_values },
118118
region_constraints: QueryRegionConstraints::default(),
@@ -137,7 +137,7 @@ crate fn evaluate_goal<'tcx>(
137137
// let's just ignore that
138138
let sol = Canonical {
139139
max_universe: ty::UniverseIndex::from_usize(0),
140-
variables: obligation.variables.clone(),
140+
variables: obligation.variables,
141141
value: QueryResponse {
142142
var_values: CanonicalVarValues { var_values: IndexVec::new() }
143143
.make_identity(tcx),

compiler/rustc_typeck/src/check/callee.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
446446
let expected_arg_tys = self.expected_inputs_for_expected_output(
447447
call_expr.span,
448448
expected,
449-
fn_sig.output().clone(),
449+
fn_sig.output(),
450450
fn_sig.inputs(),
451451
);
452452

compiler/rustc_typeck/src/check/expr.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -718,7 +718,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
718718
});
719719

720720
let ret_ty = ret_coercion.borrow().expected_ty();
721-
let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty.clone());
721+
let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty);
722722
ret_coercion.borrow_mut().coerce(
723723
self,
724724
&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
@@ -1633,11 +1633,11 @@ fn test_occupied_entry_key() {
16331633
let key = "hello there";
16341634
let value = "value goes here";
16351635
assert!(a.is_empty());
1636-
a.insert(key.clone(), value.clone());
1636+
a.insert(key, value);
16371637
assert_eq!(a.len(), 1);
16381638
assert_eq!(a[key], value);
16391639

1640-
match a.entry(key.clone()) {
1640+
match a.entry(key) {
16411641
Vacant(_) => panic!(),
16421642
Occupied(e) => assert_eq!(key, *e.key()),
16431643
}
@@ -1653,11 +1653,11 @@ fn test_vacant_entry_key() {
16531653
let value = "value goes here";
16541654

16551655
assert!(a.is_empty());
1656-
match a.entry(key.clone()) {
1656+
match a.entry(key) {
16571657
Occupied(_) => panic!(),
16581658
Vacant(e) => {
16591659
assert_eq!(key, *e.key());
1660-
e.insert(value.clone());
1660+
e.insert(value);
16611661
}
16621662
}
16631663
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
///
@@ -219,6 +220,7 @@ impl<T: ?Sized> BorrowMut<T> for T {
219220

220221
#[stable(feature = "rust1", since = "1.0.0")]
221222
impl<T: ?Sized> Borrow<T> for &T {
223+
#[rustc_diagnostic_item = "noop_method_borrow"]
222224
fn borrow(&self) -> &T {
223225
&**self
224226
}

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")]
@@ -77,6 +78,7 @@ pub trait Deref {
7778
impl<T: ?Sized> Deref for &T {
7879
type Target = T;
7980

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

library/core/tests/clone.rs

+2
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#![cfg_attr(not(bootstrap), allow(noop_method_call))]
2+
13
#[test]
24
fn test_borrowed_clone() {
35
let x = 5;

library/core/tests/iter.rs

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

35173517
let xs = ["a", "", "b", "c"];
3518-
let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect();
3518+
let v: Vec<&str> = xs.iter().map(|x| *x).intersperse(", ").collect();
35193519
let text: String = v.concat();
35203520
assert_eq!(text, "a, , b, c".to_string());
35213521

@@ -3530,7 +3530,7 @@ fn test_intersperse_size_hint() {
35303530
assert_eq!(iter.size_hint(), (0, Some(0)));
35313531

35323532
let xs = ["a", "", "b", "c"];
3533-
let mut iter = xs.iter().map(|x| x.clone()).intersperse(", ");
3533+
let mut iter = xs.iter().map(|x| *x).intersperse(", ");
35343534
assert_eq!(iter.size_hint(), (7, Some(7)));
35353535

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

0 commit comments

Comments
 (0)