Skip to content

Commit 5ab23fa

Browse files
profetiaprofetia
committed
Expand mutable capture check for is_iter_with_side_effects() 
1 parent c6d76bb commit 5ab23fa

12 files changed

+191
-122
lines changed

‎clippy_lints/src/methods/double_ended_iterator_last.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Exp
3232
{
3333
let mut sugg = vec![(call_span, String::from("next_back()"))];
3434
let mut dont_apply = false;
35+
3536
// if `self_expr` is a reference, it is mutable because it is used for `.last()`
37+
// TODO: Change this to lint only when the referred iterator is not used later. If it is used later,
38+
// changing to `next_back()` may change its behavior.
3639
if !(is_mutable(cx, self_expr) || self_type.is_ref()) {
3740
if let Some(hir_id) = path_to_local(self_expr)
3841
&& let Node::Pat(pat) = cx.tcx.hir_node(hir_id)

‎clippy_utils/src/ty/mod.rs

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1377,33 +1377,43 @@ pub fn option_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'t
13771377
}
13781378
}
13791379

1380-
/// Check if `ty` is an `Iterator` and has side effects when iterated over. Currently, this only
1381-
/// checks if the `ty` contains mutable captures, and thus may be imcomplete.
1380+
/// Check if a Ty<'_> of `Iterator` has side effects when iterated over by checking if it
1381+
/// captures any mutable references or equivalents.
13821382
pub fn is_iter_with_side_effects<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>) -> bool {
1383-
let Some(iter_trait) = cx.tcx.lang_items().iterator_trait() else {
1384-
return false;
1385-
};
1386-
1387-
is_iter_with_side_effects_impl(cx, iter_ty, iter_trait)
1383+
let mut phantoms = FxHashSet::default();
1384+
has_non_owning_mutable_access(cx, &mut phantoms, iter_ty)
13881385
}
13891386

1390-
fn is_iter_with_side_effects_impl<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>, iter_trait: DefId) -> bool {
1391-
if implements_trait(cx, iter_ty, iter_trait, &[])
1392-
&& let ty::Adt(_, args) = iter_ty.kind()
1393-
{
1394-
return args.types().any(|arg_ty| {
1395-
if let ty::Closure(_, closure_args) = arg_ty.kind()
1396-
&& let Some(captures) = closure_args.types().next_back()
1397-
{
1398-
captures
1399-
.tuple_fields()
1400-
.iter()
1401-
.any(|capture_ty| matches!(capture_ty.ref_mutability(), Some(Mutability::Mut)))
1402-
} else {
1403-
is_iter_with_side_effects_impl(cx, arg_ty, iter_trait)
1404-
}
1405-
});
1406-
}
1387+
/// Check if `ty` contains mutable references or equivalent, which includes:
1388+
/// - A mutable reference/pointer.
1389+
/// - A reference/pointer to a non-`Freeze` type.
1390+
/// - A `PhantomData` type containing any of the previous.
1391+
fn has_non_owning_mutable_access<'tcx>(
1392+
cx: &LateContext<'tcx>,
1393+
phantoms: &mut FxHashSet<Ty<'tcx>>,
1394+
ty: Ty<'tcx>,
1395+
) -> bool {
1396+
match ty.kind() {
1397+
ty::Adt(adt_def, args) if adt_def.is_phantom_data() => {
1398+
phantoms.insert(ty)
1399+
&& args
1400+
.types()
1401+
.any(|arg_ty| has_non_owning_mutable_access(cx, phantoms, arg_ty))
1402+
},
1403+
ty::Adt(adt_def, args) => adt_def
1404+
.all_fields()
1405+
.any(|field| has_non_owning_mutable_access(cx, phantoms, field.ty(cx.tcx, args))),
1406+
ty::Array(elem_ty, _) | ty::Slice(elem_ty) => has_non_owning_mutable_access(cx, phantoms, *elem_ty),
14071407

1408-
false
1408+
ty::RawPtr(pointee_ty, mutability) | ty::Ref(_, pointee_ty, mutability) => {
1409+
mutability.is_mut() || !pointee_ty.is_freeze(cx.tcx, cx.typing_env())
1410+
},
1411+
ty::Closure(_, closure_args) => {
1412+
matches!(closure_args.types().next_back(), Some(captures) if has_non_owning_mutable_access(cx, phantoms, captures))
1413+
},
1414+
ty::Tuple(tuple_args) => tuple_args
1415+
.iter()
1416+
.any(|arg_ty| has_non_owning_mutable_access(cx, phantoms, arg_ty)),
1417+
_ => false,
1418+
}
14091419
}

‎tests/ui/crashes/ice-11230.fixed

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ fn main() {
1212
// needless_collect
1313
trait Helper<'a>: Iterator<Item = fn()> {}
1414

15+
// Should not be linted because we have no idea whether the iterator has side effects
1516
fn x(w: &mut dyn for<'a> Helper<'a>) {
16-
w.next().is_none();
17-
//~^ needless_collect
17+
w.collect::<Vec<_>>().is_empty();
1818
}

‎tests/ui/crashes/ice-11230.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ fn main() {
1212
// needless_collect
1313
trait Helper<'a>: Iterator<Item = fn()> {}
1414

15+
// Should not be linted because we have no idea whether the iterator has side effects
1516
fn x(w: &mut dyn for<'a> Helper<'a>) {
1617
w.collect::<Vec<_>>().is_empty();
17-
//~^ needless_collect
1818
}

‎tests/ui/crashes/ice-11230.stderr

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,5 @@ LL | for v in A.iter() {}
77
= note: `-D clippy::explicit-iter-loop` implied by `-D warnings`
88
= help: to override `-D warnings` add `#[allow(clippy::explicit_iter_loop)]`
99

10-
error: avoid using `collect()` when not needed
11-
--> tests/ui/crashes/ice-11230.rs:16:7
12-
|
13-
LL | w.collect::<Vec<_>>().is_empty();
14-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()`
15-
|
16-
= note: `-D clippy::needless-collect` implied by `-D warnings`
17-
= help: to override `-D warnings` add `#[allow(clippy::needless_collect)]`
18-
19-
error: aborting due to 2 previous errors
10+
error: aborting due to 1 previous error
2011

‎tests/ui/double_ended_iterator_last.fixed

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,28 +52,35 @@ fn main() {
5252
let _ = CustomLast.last();
5353
}
5454

55+
// Should not be linted because applying the lint would move the original iterator. This can only be
56+
// linted if the iterator is used thereafter.
5557
fn issue_14139() {
5658
let mut index = [true, true, false, false, false, true].iter();
57-
let mut subindex = index.by_ref().take(3);
58-
let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
59+
let subindex = index.by_ref().take(3);
60+
let _ = subindex.last();
61+
let _ = index.next();
5962

6063
let mut index = [true, true, false, false, false, true].iter();
6164
let mut subindex = index.by_ref().take(3);
62-
let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
65+
let _ = subindex.last();
66+
let _ = index.next();
6367

6468
let mut index = [true, true, false, false, false, true].iter();
6569
let mut subindex = index.by_ref().take(3);
6670
let subindex = &mut subindex;
67-
let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
71+
let _ = subindex.last();
72+
let _ = index.next();
6873

6974
let mut index = [true, true, false, false, false, true].iter();
7075
let mut subindex = index.by_ref().take(3);
7176
let subindex = &mut subindex;
72-
let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
77+
let _ = subindex.last();
78+
let _ = index.next();
7379

7480
let mut index = [true, true, false, false, false, true].iter();
75-
let (mut subindex, _) = (index.by_ref().take(3), 42);
76-
let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
81+
let (subindex, _) = (index.by_ref().take(3), 42);
82+
let _ = subindex.last();
83+
let _ = index.next();
7784
}
7885

7986
fn drop_order() {

‎tests/ui/double_ended_iterator_last.rs

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,28 +52,35 @@ fn main() {
5252
let _ = CustomLast.last();
5353
}
5454

55+
// Should not be linted because applying the lint would move the original iterator. This can only be
56+
// linted if the iterator is used thereafter.
5557
fn issue_14139() {
5658
let mut index = [true, true, false, false, false, true].iter();
5759
let subindex = index.by_ref().take(3);
58-
let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
60+
let _ = subindex.last();
61+
let _ = index.next();
5962

6063
let mut index = [true, true, false, false, false, true].iter();
6164
let mut subindex = index.by_ref().take(3);
62-
let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
65+
let _ = subindex.last();
66+
let _ = index.next();
6367

6468
let mut index = [true, true, false, false, false, true].iter();
6569
let mut subindex = index.by_ref().take(3);
6670
let subindex = &mut subindex;
67-
let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
71+
let _ = subindex.last();
72+
let _ = index.next();
6873

6974
let mut index = [true, true, false, false, false, true].iter();
7075
let mut subindex = index.by_ref().take(3);
7176
let subindex = &mut subindex;
72-
let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
77+
let _ = subindex.last();
78+
let _ = index.next();
7379

7480
let mut index = [true, true, false, false, false, true].iter();
7581
let (subindex, _) = (index.by_ref().take(3), 42);
76-
let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
82+
let _ = subindex.last();
83+
let _ = index.next();
7784
}
7885

7986
fn drop_order() {

‎tests/ui/double_ended_iterator_last.stderr

Lines changed: 2 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -18,55 +18,7 @@ LL | let _ = DeIterator.last();
1818
| help: try: `next_back()`
1919

2020
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
21-
--> tests/ui/double_ended_iterator_last.rs:58:13
22-
|
23-
LL | let _ = subindex.last();
24-
| ^^^^^^^^^^^^^^^
25-
|
26-
help: try
27-
|
28-
LL ~ let mut subindex = index.by_ref().take(3);
29-
LL ~ let _ = subindex.next_back();
30-
|
31-
32-
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
33-
--> tests/ui/double_ended_iterator_last.rs:62:13
34-
|
35-
LL | let _ = subindex.last();
36-
| ^^^^^^^^^------
37-
| |
38-
| help: try: `next_back()`
39-
40-
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
41-
--> tests/ui/double_ended_iterator_last.rs:67:13
42-
|
43-
LL | let _ = subindex.last();
44-
| ^^^^^^^^^------
45-
| |
46-
| help: try: `next_back()`
47-
48-
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
49-
--> tests/ui/double_ended_iterator_last.rs:72:13
50-
|
51-
LL | let _ = subindex.last();
52-
| ^^^^^^^^^------
53-
| |
54-
| help: try: `next_back()`
55-
56-
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
57-
--> tests/ui/double_ended_iterator_last.rs:76:13
58-
|
59-
LL | let _ = subindex.last();
60-
| ^^^^^^^^^^^^^^^
61-
|
62-
help: try
63-
|
64-
LL ~ let (mut subindex, _) = (index.by_ref().take(3), 42);
65-
LL ~ let _ = subindex.next_back();
66-
|
67-
68-
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
69-
--> tests/ui/double_ended_iterator_last.rs:89:36
21+
--> tests/ui/double_ended_iterator_last.rs:96:36
7022
|
7123
LL | println!("Last element is {}", v.last().unwrap().0);
7224
| ^^^^^^^^
@@ -78,5 +30,5 @@ LL ~ let mut v = v.into_iter();
7830
LL ~ println!("Last element is {}", v.next_back().unwrap().0);
7931
|
8032

81-
error: aborting due to 8 previous errors
33+
error: aborting due to 3 previous errors
8234

‎tests/ui/double_ended_iterator_last_unfixable.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
//@no-rustfix
22
#![warn(clippy::double_ended_iterator_last)]
33

4+
// Should not be linted because applying the lint would move the original iterator. This can only be
5+
// linted if the iterator is used thereafter.
46
fn main() {
57
let mut index = [true, true, false, false, false, true].iter();
68
let subindex = (index.by_ref().take(3), 42);
7-
let _ = subindex.0.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
9+
let _ = subindex.0.last();
10+
let _ = index.next();
811
}
912

1013
fn drop_order() {

‎tests/ui/double_ended_iterator_last_unfixable.stderr

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,5 @@
11
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
2-
--> tests/ui/double_ended_iterator_last_unfixable.rs:7:13
3-
|
4-
LL | let _ = subindex.0.last();
5-
| ^^^^^^^^^^^------
6-
| |
7-
| help: try: `next_back()`
8-
|
9-
note: this must be made mutable to use `.next_back()`
10-
--> tests/ui/double_ended_iterator_last_unfixable.rs:7:13
11-
|
12-
LL | let _ = subindex.0.last();
13-
| ^^^^^^^^^^
14-
= note: `-D clippy::double-ended-iterator-last` implied by `-D warnings`
15-
= help: to override `-D warnings` add `#[allow(clippy::double_ended_iterator_last)]`
16-
17-
error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
18-
--> tests/ui/double_ended_iterator_last_unfixable.rs:20:36
2+
--> tests/ui/double_ended_iterator_last_unfixable.rs:23:36
193
|
204
LL | println!("Last element is {}", v.0.last().unwrap().0);
215
| ^^^^------
@@ -24,10 +8,12 @@ LL | println!("Last element is {}", v.0.last().unwrap().0);
248
|
259
= note: this change will alter drop order which may be undesirable
2610
note: this must be made mutable to use `.next_back()`
27-
--> tests/ui/double_ended_iterator_last_unfixable.rs:20:36
11+
--> tests/ui/double_ended_iterator_last_unfixable.rs:23:36
2812
|
2913
LL | println!("Last element is {}", v.0.last().unwrap().0);
3014
| ^^^
15+
= note: `-D clippy::double-ended-iterator-last` implied by `-D warnings`
16+
= help: to override `-D warnings` add `#[allow(clippy::double_ended_iterator_last)]`
3117

32-
error: aborting due to 2 previous errors
18+
error: aborting due to 1 previous error
3319

‎tests/ui/needless_collect.fixed

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,13 +128,45 @@ fn bar<I: IntoIterator<Item = usize>>(_: Vec<usize>, _: I) {}
128128
fn baz<I: IntoIterator<Item = usize>>(_: I, _: (), _: impl IntoIterator<Item = char>) {}
129129

130130
mod issue9191 {
131+
use std::cell::Cell;
131132
use std::collections::HashSet;
133+
use std::hash::Hash;
134+
use std::marker::PhantomData;
135+
use std::ops::Deref;
132136

133-
fn foo(xs: Vec<i32>, mut ys: HashSet<i32>) {
137+
fn captures_ref_mut(xs: Vec<i32>, mut ys: HashSet<i32>) {
134138
if xs.iter().map(|x| ys.remove(x)).collect::<Vec<_>>().contains(&true) {
135139
todo!()
136140
}
137141
}
142+
143+
#[derive(Debug, Clone)]
144+
struct MyRef<'a>(PhantomData<&'a mut Cell<HashSet<i32>>>, *mut Cell<HashSet<i32>>);
145+
146+
impl MyRef<'_> {
147+
fn new(target: &mut Cell<HashSet<i32>>) -> Self {
148+
MyRef(PhantomData, target)
149+
}
150+
151+
fn get(&mut self) -> &mut Cell<HashSet<i32>> {
152+
unsafe { &mut *self.1 }
153+
}
154+
}
155+
156+
fn captures_phantom(xs: Vec<i32>, mut ys: Cell<HashSet<i32>>) {
157+
let mut ys_ref = MyRef::new(&mut ys);
158+
if xs
159+
.iter()
160+
.map({
161+
let mut ys_ref = ys_ref.clone();
162+
move |x| ys_ref.get().get_mut().remove(x)
163+
})
164+
.collect::<Vec<_>>()
165+
.contains(&true)
166+
{
167+
todo!()
168+
}
169+
}
138170
}
139171

140172
pub fn issue8055(v: impl IntoIterator<Item = i32>) -> Result<impl Iterator<Item = i32>, usize> {
@@ -155,3 +187,26 @@ pub fn issue8055(v: impl IntoIterator<Item = i32>) -> Result<impl Iterator<Item
155187
}
156188
Ok(res.into_iter())
157189
}
190+
191+
mod issue8055_regression {
192+
struct Foo<T> {
193+
inner: T,
194+
marker: core::marker::PhantomData<Self>,
195+
}
196+
197+
impl<T: Iterator> Iterator for Foo<T> {
198+
type Item = T::Item;
199+
fn next(&mut self) -> Option<Self::Item> {
200+
self.inner.next()
201+
}
202+
}
203+
204+
fn foo() {
205+
Foo {
206+
inner: [].iter(),
207+
marker: core::marker::PhantomData,
208+
}
209+
.collect::<Vec<&i32>>()
210+
.len();
211+
}
212+
}

0 commit comments

Comments
 (0)