Skip to content

Commit 7a55407

Browse files
ThibsGThibsG
committed
Fix suggestions when call functions involved taking by ref
1 parent 6d1ccbf commit 7a55407

7 files changed

+325
-57
lines changed

‎clippy_lints/src/methods/search_is_some.rs

Lines changed: 56 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use std::iter;
2+
13
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
24
use clippy_utils::source::{snippet, snippet_with_applicability};
35
use clippy_utils::ty::is_type_diagnostic_item;
@@ -223,6 +225,26 @@ impl DerefDelegate<'_, 'tcx> {
223225
let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability);
224226
format!("{}{}", self.suggestion_start, end_snip)
225227
}
228+
229+
fn func_takes_arg_by_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool {
230+
if_chain! {
231+
if let ExprKind::Call(func, call_args) = parent_expr.kind;
232+
let typ = self.cx.typeck_results().expr_ty(func);
233+
if let ty::FnDef(..) = typ.kind();
234+
235+
then {
236+
let mut takes_by_ref = false;
237+
for (arg, ty) in iter::zip(call_args, typ.fn_sig(self.cx.tcx).skip_binder().inputs()) {
238+
if arg.hir_id == cmt_hir_id {
239+
takes_by_ref = matches!(ty.kind(), ty::Ref(_, inner, _) if inner.is_ref());
240+
}
241+
}
242+
takes_by_ref
243+
} else {
244+
false
245+
}
246+
}
247+
}
226248
}
227249

228250
impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
@@ -252,42 +274,32 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
252274
let start_span = Span::new(self.next_pos, span.lo(), span.ctxt());
253275
let start_snip =
254276
snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
277+
278+
// suggest ampersand if call function is taking args by ref
279+
let takes_arg_by_ref = self.func_takes_arg_by_ref(parent_expr, cmt.hir_id);
280+
255281
// do not suggest ampersand if the ident is the method caller
256-
let ident_sugg = if !call_args.is_empty() && call_args[0].hir_id == cmt.hir_id {
257-
format!("{}{}", start_snip, ident_str)
258-
} else {
259-
format!("{}&{}", start_snip, ident_str)
260-
};
282+
let ident_sugg =
283+
if !call_args.is_empty() && call_args[0].hir_id == cmt.hir_id && !takes_arg_by_ref {
284+
format!("{}{}", start_snip, ident_str)
285+
} else {
286+
format!("{}&{}", start_snip, ident_str)
287+
};
261288
self.suggestion_start.push_str(&ident_sugg);
262289
self.next_pos = span.hi();
263290
return;
264-
} else {
265-
self.applicability = Applicability::Unspecified;
266291
}
292+
293+
self.applicability = Applicability::Unspecified;
267294
}
268295
}
269296

270297
// handle item projections by removing one explicit deref
271298
// i.e.: suggest `*x` instead of `**x`
272299
let mut replacement_str = ident_str;
273300

274-
// handle index projection first
275-
let index_handled = cmt.place.projections.iter().any(|proj| match proj.kind {
276-
// Index projection like `|x| foo[x]`
277-
// the index is dropped so we can't get it to build the suggestion,
278-
// so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`)
279-
// instead of `span.lo()` (i.e.: `foo`)
280-
ProjectionKind::Index => {
281-
let start_span = Span::new(self.next_pos, span.hi(), span.ctxt());
282-
start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
283-
replacement_str.clear();
284-
true
285-
},
286-
_ => false,
287-
});
288-
289-
// looking for projections other that need to be handled differently
290-
let other_projections_handled = cmt.place.projections.iter().enumerate().any(|(i, proj)| {
301+
let mut projections_handled = false;
302+
cmt.place.projections.iter().enumerate().for_each(|(i, proj)| {
291303
match proj.kind {
292304
// Field projection like `|v| v.foo`
293305
ProjectionKind::Field(idx, variant) => match cmt.place.ty_before_projection(i).kind() {
@@ -297,34 +309,41 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
297309
replacement_str,
298310
def.variants[variant].fields[idx as usize].ident.name.as_str()
299311
);
300-
true
312+
projections_handled = true;
301313
},
302314
ty::Tuple(_) => {
303315
replacement_str = format!("{}.{}", replacement_str, idx);
304-
true
316+
projections_handled = true;
305317
},
306-
_ => false,
318+
_ => (),
319+
},
320+
// Index projection like `|x| foo[x]`
321+
// the index is dropped so we can't get it to build the suggestion,
322+
// so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`)
323+
// instead of `span.lo()` (i.e.: `foo`)
324+
ProjectionKind::Index => {
325+
let start_span = Span::new(self.next_pos, span.hi(), span.ctxt());
326+
start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
327+
replacement_str.clear();
328+
projections_handled = true;
307329
},
308-
// handled previously
309-
ProjectionKind::Index |
310-
// note: unable to trigger `Subslice` kind in tests
311-
ProjectionKind::Subslice => false,
330+
// note: unable to trigger `Subslice` kind in tests
331+
ProjectionKind::Subslice => (),
312332
ProjectionKind::Deref => {
313333
// explicit deref for arrays should be avoided in the suggestion
314334
// i.e.: `|sub| *sub[1..4].len() == 3` is not expected
315-
match cmt.place.ty_before_projection(i).kind() {
335+
if let ty::Ref(_, inner, _) = cmt.place.ty_before_projection(i).kind() {
316336
// dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`)
317-
ty::Ref(_, inner, _) => {
318-
matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array())
319-
},
320-
_ => false,
337+
if matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array()) {
338+
projections_handled = true;
339+
}
321340
}
322341
},
323342
}
324343
});
325344

326345
// handle `ProjectionKind::Deref` if no special case detected
327-
if !index_handled && !other_projections_handled {
346+
if !projections_handled {
328347
let last_deref = cmt
329348
.place
330349
.projections

‎tests/ui/search_is_some_fixable_none.fixed

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,17 +102,63 @@ mod issue7392 {
102102
*x == 9
103103
}
104104

105-
fn simple_fn(x: u32) -> bool {
105+
fn deref_enough(x: u32) -> bool {
106106
x == 78
107107
}
108108

109+
fn arg_no_deref(x: &&u32) -> bool {
110+
**x == 78
111+
}
112+
109113
fn more_projections() {
110114
let x = 19;
111115
let ppx: &u32 = &x;
112116
let _ = ![ppx].iter().any(|ppp_x: &&u32| please(ppp_x));
113117
let _ = ![String::from("Hey hey")].iter().any(|s| s.len() == 2);
114118

115119
let v = vec![3, 2, 1, 0];
116-
let _ = !v.iter().any(|x| simple_fn(*x));
120+
let _ = !v.iter().any(|x| deref_enough(*x));
121+
122+
#[allow(clippy::redundant_closure)]
123+
let _ = !v.iter().any(|x| arg_no_deref(&x));
124+
}
125+
126+
fn field_index_projection() {
127+
struct FooDouble {
128+
bar: Vec<Vec<i32>>,
129+
}
130+
struct Foo {
131+
bar: Vec<i32>,
132+
}
133+
struct FooOuter {
134+
inner: Foo,
135+
inner_double: FooDouble,
136+
}
137+
let vfoo = vec![FooOuter {
138+
inner: Foo { bar: vec![0, 1, 2, 3] },
139+
inner_double: FooDouble {
140+
bar: vec![vec![0, 1, 2, 3]],
141+
},
142+
}];
143+
let _ = !vfoo
144+
.iter().any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2);
145+
}
146+
147+
fn index_field_projection() {
148+
struct Foo {
149+
bar: i32,
150+
}
151+
struct FooOuter {
152+
inner: Vec<Foo>,
153+
}
154+
let vfoo = vec![FooOuter {
155+
inner: vec![Foo { bar: 0 }],
156+
}];
157+
let _ = !vfoo.iter().any(|v| v.inner[0].bar == 2);
158+
}
159+
160+
fn double_deref_index_projection() {
161+
let vfoo = vec![&&[0, 1, 2, 3]];
162+
let _ = !vfoo.iter().any(|x| (**x)[0] == 9);
117163
}
118164
}

‎tests/ui/search_is_some_fixable_none.rs

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,17 +106,65 @@ mod issue7392 {
106106
*x == 9
107107
}
108108

109-
fn simple_fn(x: u32) -> bool {
109+
fn deref_enough(x: u32) -> bool {
110110
x == 78
111111
}
112112

113+
fn arg_no_deref(x: &&u32) -> bool {
114+
**x == 78
115+
}
116+
113117
fn more_projections() {
114118
let x = 19;
115119
let ppx: &u32 = &x;
116120
let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_none();
117121
let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none();
118122

119123
let v = vec![3, 2, 1, 0];
120-
let _ = v.iter().find(|x| simple_fn(**x)).is_none();
124+
let _ = v.iter().find(|x| deref_enough(**x)).is_none();
125+
126+
#[allow(clippy::redundant_closure)]
127+
let _ = v.iter().find(|x| arg_no_deref(x)).is_none();
128+
}
129+
130+
fn field_index_projection() {
131+
struct FooDouble {
132+
bar: Vec<Vec<i32>>,
133+
}
134+
struct Foo {
135+
bar: Vec<i32>,
136+
}
137+
struct FooOuter {
138+
inner: Foo,
139+
inner_double: FooDouble,
140+
}
141+
let vfoo = vec![FooOuter {
142+
inner: Foo { bar: vec![0, 1, 2, 3] },
143+
inner_double: FooDouble {
144+
bar: vec![vec![0, 1, 2, 3]],
145+
},
146+
}];
147+
let _ = vfoo
148+
.iter()
149+
.find(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2)
150+
.is_none();
151+
}
152+
153+
fn index_field_projection() {
154+
struct Foo {
155+
bar: i32,
156+
}
157+
struct FooOuter {
158+
inner: Vec<Foo>,
159+
}
160+
let vfoo = vec![FooOuter {
161+
inner: vec![Foo { bar: 0 }],
162+
}];
163+
let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_none();
164+
}
165+
166+
fn double_deref_index_projection() {
167+
let vfoo = vec![&&[0, 1, 2, 3]];
168+
let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none();
121169
}
122170
}

‎tests/ui/search_is_some_fixable_none.stderr

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,22 +170,56 @@ LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_none();
170170
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|sub| sub[1..4].len() == 3)`
171171

172172
error: called `is_none()` after searching an `Iterator` with `find`
173-
--> $DIR/search_is_some_fixable_none.rs:116:17
173+
--> $DIR/search_is_some_fixable_none.rs:120:17
174174
|
175175
LL | let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_none();
176176
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![ppx].iter().any(|ppp_x: &&u32| please(ppp_x))`
177177

178178
error: called `is_none()` after searching an `Iterator` with `find`
179-
--> $DIR/search_is_some_fixable_none.rs:117:17
179+
--> $DIR/search_is_some_fixable_none.rs:121:17
180180
|
181181
LL | let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none();
182182
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![String::from("Hey hey")].iter().any(|s| s.len() == 2)`
183183

184184
error: called `is_none()` after searching an `Iterator` with `find`
185-
--> $DIR/search_is_some_fixable_none.rs:120:17
185+
--> $DIR/search_is_some_fixable_none.rs:124:17
186+
|
187+
LL | let _ = v.iter().find(|x| deref_enough(**x)).is_none();
188+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| deref_enough(*x))`
189+
190+
error: called `is_none()` after searching an `Iterator` with `find`
191+
--> $DIR/search_is_some_fixable_none.rs:127:17
192+
|
193+
LL | let _ = v.iter().find(|x| arg_no_deref(x)).is_none();
194+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| arg_no_deref(&x))`
195+
196+
error: called `is_none()` after searching an `Iterator` with `find`
197+
--> $DIR/search_is_some_fixable_none.rs:147:17
198+
|
199+
LL | let _ = vfoo
200+
| _________________^
201+
LL | | .iter()
202+
LL | | .find(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2)
203+
LL | | .is_none();
204+
| |______________________^
205+
|
206+
help: use `!_.any()` instead
207+
|
208+
LL ~ let _ = !vfoo
209+
LL ~ .iter().any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2);
210+
|
211+
212+
error: called `is_none()` after searching an `Iterator` with `find`
213+
--> $DIR/search_is_some_fixable_none.rs:163:17
214+
|
215+
LL | let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_none();
216+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.inner[0].bar == 2)`
217+
218+
error: called `is_none()` after searching an `Iterator` with `find`
219+
--> $DIR/search_is_some_fixable_none.rs:168:17
186220
|
187-
LL | let _ = v.iter().find(|x| simple_fn(**x)).is_none();
188-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| simple_fn(*x))`
221+
LL | let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none();
222+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|x| (**x)[0] == 9)`
189223

190-
error: aborting due to 29 previous errors
224+
error: aborting due to 33 previous errors
191225

0 commit comments

Comments
 (0)