Skip to content

Commit 52f52fd

Browse files
smoeliussmoelius
committed
Enhance needless_borrow to consider trait implementations
1 parent 7c8e1bf commit 52f52fd

File tree

7 files changed

+553
-47
lines changed

7 files changed

+553
-47
lines changed

‎clippy_lints/src/dereference.rs

Lines changed: 270 additions & 25 deletions
Large diffs are not rendered by default.

‎clippy_lints/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -807,7 +807,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
807807
store.register_late_pass(|| Box::new(verbose_file_reads::VerboseFileReads));
808808
store.register_late_pass(|| Box::new(redundant_pub_crate::RedundantPubCrate::default()));
809809
store.register_late_pass(|| Box::new(unnamed_address::UnnamedAddress));
810-
store.register_late_pass(|| Box::new(dereference::Dereferencing::default()));
810+
store.register_late_pass(move || Box::new(dereference::Dereferencing::new(msrv)));
811811
store.register_late_pass(|| Box::new(option_if_let_else::OptionIfLetElse));
812812
store.register_late_pass(|| Box::new(future_not_send::FutureNotSend));
813813
store.register_late_pass(|| Box::new(if_let_mutex::IfLetMutex));

‎clippy_lints/src/methods/unnecessary_to_owned.rs

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,8 @@ use clippy_utils::source::snippet_opt;
55
use clippy_utils::ty::{
66
contains_ty, get_associated_type, get_iterator_item_ty, implements_trait, is_copy, peel_mid_ty_refs,
77
};
8-
use clippy_utils::{meets_msrv, msrvs};
9-
108
use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item};
9+
use clippy_utils::{meets_msrv, msrvs};
1110
use rustc_errors::Applicability;
1211
use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind};
1312
use rustc_lint::LateContext;
@@ -360,25 +359,15 @@ fn get_input_traits_and_projections<'tcx>(
360359
) -> (Vec<TraitPredicate<'tcx>>, Vec<ProjectionPredicate<'tcx>>) {
361360
let mut trait_predicates = Vec::new();
362361
let mut projection_predicates = Vec::new();
363-
for (predicate, _) in cx.tcx.predicates_of(callee_def_id).predicates.iter() {
364-
// `substs` should have 1 + n elements. The first is the type on the left hand side of an
365-
// `as`. The remaining n are trait parameters.
366-
let is_input_substs = |substs: SubstsRef<'tcx>| {
367-
if_chain! {
368-
if let Some(arg) = substs.iter().next();
369-
if let GenericArgKind::Type(arg_ty) = arg.unpack();
370-
if arg_ty == input;
371-
then { true } else { false }
372-
}
373-
};
362+
for predicate in cx.tcx.param_env(callee_def_id).caller_bounds() {
374363
match predicate.kind().skip_binder() {
375364
PredicateKind::Trait(trait_predicate) => {
376-
if is_input_substs(trait_predicate.trait_ref.substs) {
365+
if trait_predicate.trait_ref.self_ty() == input {
377366
trait_predicates.push(trait_predicate);
378367
}
379368
},
380369
PredicateKind::Projection(projection_predicate) => {
381-
if is_input_substs(projection_predicate.projection_ty.substs) {
370+
if projection_predicate.projection_ty.self_ty() == input {
382371
projection_predicates.push(projection_predicate);
383372
}
384373
},

‎clippy_utils/src/msrvs.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ macro_rules! msrv_aliases {
1313
// names may refer to stabilized feature flags or library items
1414
msrv_aliases! {
1515
1,62,0 { BOOL_THEN_SOME }
16-
1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN }
16+
1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR }
1717
1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
1818
1,51,0 { BORROW_AS_PTR, UNSIGNED_ABS }
1919
1,50,0 { BOOL_THEN }

‎tests/ui/needless_borrow.fixed

Lines changed: 116 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// run-rustfix
22

3-
#![feature(lint_reasons)]
3+
#![feature(custom_inner_attributes, lint_reasons)]
44

55
#[warn(clippy::all, clippy::needless_borrow)]
66
#[allow(unused_variables, clippy::unnecessary_mut_passed)]
@@ -127,6 +127,20 @@ fn main() {
127127
0
128128
}
129129
}
130+
131+
let _ = std::process::Command::new("ls").args(["-a", "-l"]).status().unwrap();
132+
let _ = std::path::Path::new(".").join(".");
133+
deref_target_is_x(X);
134+
multiple_constraints([[""]]);
135+
multiple_constraints_normalizes_to_same(X, X);
136+
let _ = Some("").unwrap_or("");
137+
138+
only_sized(&""); // Don't lint. `Sized` is only bound
139+
let _ = std::any::Any::type_id(&""); // Don't lint. `Any` is only bound
140+
let _ = Box::new(&""); // Don't lint. Type parameter appears in return type
141+
ref_as_ref_path(&""); // Don't lint. Argument type is not a type parameter
142+
refs_only(&()); // Don't lint. `&T` implements trait, but `T` doesn't
143+
multiple_constraints_normalizes_to_different(&[[""]], &[""]); // Don't lint. Projected type appears in arguments
130144
}
131145

132146
#[allow(clippy::needless_borrowed_reference)]
@@ -183,3 +197,104 @@ mod issue9160 {
183197
}
184198
}
185199
}
200+
201+
#[derive(Clone, Copy)]
202+
struct X;
203+
204+
impl std::ops::Deref for X {
205+
type Target = X;
206+
fn deref(&self) -> &Self::Target {
207+
self
208+
}
209+
}
210+
211+
fn deref_target_is_x<T>(_: T)
212+
where
213+
T: std::ops::Deref<Target = X>,
214+
{
215+
}
216+
217+
fn multiple_constraints<T, U, V, X, Y>(_: T)
218+
where
219+
T: IntoIterator<Item = U> + IntoIterator<Item = X>,
220+
U: IntoIterator<Item = V>,
221+
V: AsRef<str>,
222+
X: IntoIterator<Item = Y>,
223+
Y: AsRef<std::ffi::OsStr>,
224+
{
225+
}
226+
227+
fn multiple_constraints_normalizes_to_same<T, U, V>(_: T, _: V)
228+
where
229+
T: std::ops::Deref<Target = U>,
230+
U: std::ops::Deref<Target = V>,
231+
{
232+
}
233+
234+
fn only_sized<T>(_: T) {}
235+
236+
fn ref_as_ref_path<T: 'static>(_: &'static T)
237+
where
238+
&'static T: AsRef<std::path::Path>,
239+
{
240+
}
241+
242+
trait RefsOnly {
243+
type Referent;
244+
}
245+
246+
impl<T> RefsOnly for &T {
247+
type Referent = T;
248+
}
249+
250+
fn refs_only<T, U>(_: T)
251+
where
252+
T: RefsOnly<Referent = U>,
253+
{
254+
}
255+
256+
fn multiple_constraints_normalizes_to_different<T, U, V>(_: T, _: U)
257+
where
258+
T: IntoIterator<Item = U>,
259+
U: IntoIterator<Item = V>,
260+
V: AsRef<str>,
261+
{
262+
}
263+
264+
// https://github.com/rust-lang/rust-clippy/pull/9136#pullrequestreview-1037379321
265+
#[allow(dead_code)]
266+
mod copyable_iterator {
267+
#[derive(Clone, Copy)]
268+
struct Iter;
269+
impl Iterator for Iter {
270+
type Item = ();
271+
fn next(&mut self) -> Option<Self::Item> {
272+
None
273+
}
274+
}
275+
fn takes_iter(_: impl Iterator) {}
276+
fn dont_warn(mut x: Iter) {
277+
takes_iter(&mut x);
278+
}
279+
fn warn(mut x: &mut Iter) {
280+
takes_iter(&mut x)
281+
}
282+
}
283+
284+
mod under_msrv {
285+
#![allow(dead_code)]
286+
#![clippy::msrv = "1.52.0"]
287+
288+
fn foo() {
289+
let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
290+
}
291+
}
292+
293+
mod meets_msrv {
294+
#![allow(dead_code)]
295+
#![clippy::msrv = "1.53.0"]
296+
297+
fn foo() {
298+
let _ = std::process::Command::new("ls").args(["-a", "-l"]).status().unwrap();
299+
}
300+
}

‎tests/ui/needless_borrow.rs

Lines changed: 116 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// run-rustfix
22

3-
#![feature(lint_reasons)]
3+
#![feature(custom_inner_attributes, lint_reasons)]
44

55
#[warn(clippy::all, clippy::needless_borrow)]
66
#[allow(unused_variables, clippy::unnecessary_mut_passed)]
@@ -127,6 +127,20 @@ fn main() {
127127
0
128128
}
129129
}
130+
131+
let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
132+
let _ = std::path::Path::new(".").join(&&".");
133+
deref_target_is_x(&X);
134+
multiple_constraints(&[[""]]);
135+
multiple_constraints_normalizes_to_same(&X, X);
136+
let _ = Some("").unwrap_or(&"");
137+
138+
only_sized(&""); // Don't lint. `Sized` is only bound
139+
let _ = std::any::Any::type_id(&""); // Don't lint. `Any` is only bound
140+
let _ = Box::new(&""); // Don't lint. Type parameter appears in return type
141+
ref_as_ref_path(&""); // Don't lint. Argument type is not a type parameter
142+
refs_only(&()); // Don't lint. `&T` implements trait, but `T` doesn't
143+
multiple_constraints_normalizes_to_different(&[[""]], &[""]); // Don't lint. Projected type appears in arguments
130144
}
131145

132146
#[allow(clippy::needless_borrowed_reference)]
@@ -183,3 +197,104 @@ mod issue9160 {
183197
}
184198
}
185199
}
200+
201+
#[derive(Clone, Copy)]
202+
struct X;
203+
204+
impl std::ops::Deref for X {
205+
type Target = X;
206+
fn deref(&self) -> &Self::Target {
207+
self
208+
}
209+
}
210+
211+
fn deref_target_is_x<T>(_: T)
212+
where
213+
T: std::ops::Deref<Target = X>,
214+
{
215+
}
216+
217+
fn multiple_constraints<T, U, V, X, Y>(_: T)
218+
where
219+
T: IntoIterator<Item = U> + IntoIterator<Item = X>,
220+
U: IntoIterator<Item = V>,
221+
V: AsRef<str>,
222+
X: IntoIterator<Item = Y>,
223+
Y: AsRef<std::ffi::OsStr>,
224+
{
225+
}
226+
227+
fn multiple_constraints_normalizes_to_same<T, U, V>(_: T, _: V)
228+
where
229+
T: std::ops::Deref<Target = U>,
230+
U: std::ops::Deref<Target = V>,
231+
{
232+
}
233+
234+
fn only_sized<T>(_: T) {}
235+
236+
fn ref_as_ref_path<T: 'static>(_: &'static T)
237+
where
238+
&'static T: AsRef<std::path::Path>,
239+
{
240+
}
241+
242+
trait RefsOnly {
243+
type Referent;
244+
}
245+
246+
impl<T> RefsOnly for &T {
247+
type Referent = T;
248+
}
249+
250+
fn refs_only<T, U>(_: T)
251+
where
252+
T: RefsOnly<Referent = U>,
253+
{
254+
}
255+
256+
fn multiple_constraints_normalizes_to_different<T, U, V>(_: T, _: U)
257+
where
258+
T: IntoIterator<Item = U>,
259+
U: IntoIterator<Item = V>,
260+
V: AsRef<str>,
261+
{
262+
}
263+
264+
// https://github.com/rust-lang/rust-clippy/pull/9136#pullrequestreview-1037379321
265+
#[allow(dead_code)]
266+
mod copyable_iterator {
267+
#[derive(Clone, Copy)]
268+
struct Iter;
269+
impl Iterator for Iter {
270+
type Item = ();
271+
fn next(&mut self) -> Option<Self::Item> {
272+
None
273+
}
274+
}
275+
fn takes_iter(_: impl Iterator) {}
276+
fn dont_warn(mut x: Iter) {
277+
takes_iter(&mut x);
278+
}
279+
fn warn(mut x: &mut Iter) {
280+
takes_iter(&mut x)
281+
}
282+
}
283+
284+
mod under_msrv {
285+
#![allow(dead_code)]
286+
#![clippy::msrv = "1.52.0"]
287+
288+
fn foo() {
289+
let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
290+
}
291+
}
292+
293+
mod meets_msrv {
294+
#![allow(dead_code)]
295+
#![clippy::msrv = "1.53.0"]
296+
297+
fn foo() {
298+
let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
299+
}
300+
}

‎tests/ui/needless_borrow.stderr

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,17 +120,59 @@ error: this expression creates a reference which is immediately dereferenced by
120120
LL | (&&5).foo();
121121
| ^^^^^ help: change this to: `(&5)`
122122

123+
error: the borrowed expression implements the required traits
124+
--> $DIR/needless_borrow.rs:131:51
125+
|
126+
LL | let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
127+
| ^^^^^^^^^^^^^ help: change this to: `["-a", "-l"]`
128+
129+
error: the borrowed expression implements the required traits
130+
--> $DIR/needless_borrow.rs:132:44
131+
|
132+
LL | let _ = std::path::Path::new(".").join(&&".");
133+
| ^^^^^ help: change this to: `"."`
134+
135+
error: the borrowed expression implements the required traits
136+
--> $DIR/needless_borrow.rs:133:23
137+
|
138+
LL | deref_target_is_x(&X);
139+
| ^^ help: change this to: `X`
140+
141+
error: the borrowed expression implements the required traits
142+
--> $DIR/needless_borrow.rs:134:26
143+
|
144+
LL | multiple_constraints(&[[""]]);
145+
| ^^^^^^^ help: change this to: `[[""]]`
146+
147+
error: the borrowed expression implements the required traits
148+
--> $DIR/needless_borrow.rs:135:45
149+
|
150+
LL | multiple_constraints_normalizes_to_same(&X, X);
151+
| ^^ help: change this to: `X`
152+
153+
error: this expression creates a reference which is immediately dereferenced by the compiler
154+
--> $DIR/needless_borrow.rs:136:32
155+
|
156+
LL | let _ = Some("").unwrap_or(&"");
157+
| ^^^ help: change this to: `""`
158+
123159
error: this expression borrows a value the compiler would automatically borrow
124-
--> $DIR/needless_borrow.rs:173:13
160+
--> $DIR/needless_borrow.rs:187:13
125161
|
126162
LL | (&self.f)()
127163
| ^^^^^^^^^ help: change this to: `(self.f)`
128164

129165
error: this expression borrows a value the compiler would automatically borrow
130-
--> $DIR/needless_borrow.rs:182:13
166+
--> $DIR/needless_borrow.rs:196:13
131167
|
132168
LL | (&mut self.f)()
133169
| ^^^^^^^^^^^^^ help: change this to: `(self.f)`
134170

135-
error: aborting due to 22 previous errors
171+
error: the borrowed expression implements the required traits
172+
--> $DIR/needless_borrow.rs:298:55
173+
|
174+
LL | let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
175+
| ^^^^^^^^^^^^^ help: change this to: `["-a", "-l"]`
176+
177+
error: aborting due to 29 previous errors
136178

0 commit comments

Comments
 (0)