Consider side effects when rewriting iterator behaviors #14490
Conversation
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
I don't see why this needs to be limited to checking only closure captures. Any iterator that contains a mutable reference has the same issue.
I think any iterator type containing one of the following would need to not lint:
- A mutable reference/pointer
- A reference/pointer to a non-
Freezetype - A
PhantomDatatype containing any of the previous.
profetia
force-pushed
the
issue14444
branch
2 times, most recently
from
92e35bf to
4a54775
Compare
|
Updated. Now these will also be covered. |
|
This CI failure does not seem to be my problem. Any ideas? |
I've restarted it to see if it's intermittent (we'll have to fix it anyway) or not. For reference, here is the failing version. Edit: this does not look like it is intermittent. |
|
Thanks to #14514, the CI is working now. |
|
r? clippy |
|
samueltardieu is on vacation. Please choose another assignee. |
|
r? Jarcho sending it back, then 😄 |
| /// Check if a Ty<'_> of `Iterator` has side effects when iterated over by checking if it | ||
| /// captures any mutable references or equivalents. | ||
| pub fn is_iter_with_side_effects<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>) -> bool { | ||
| let Some(iter_trait) = cx.tcx.lang_items().iterator_trait() else { | ||
| return false; | ||
| }; | ||
|
|
||
| is_iter_with_side_effects_impl(cx, iter_ty, iter_trait) | ||
| } | ||
|
|
||
| fn is_iter_with_side_effects_impl<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>, iter_trait: DefId) -> bool { | ||
| if let ty::Adt(adt_def, args) = iter_ty.kind() { | ||
| return adt_def | ||
| .all_fields() | ||
| .any(|field| is_ty_with_side_effects(cx, field.ty(cx.tcx, args), iter_trait)); | ||
| } | ||
|
|
||
| false | ||
| } |
There was a problem hiding this comment.
This part still doesn't need to exist. There's no need for checking if the type is an iterator here; it's already known by all the lints.
| fn is_ty_with_side_effects<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, iter_trait: DefId) -> bool { | ||
| match ty.kind() { | ||
| ty::RawPtr(..) | ty::Ref(..) | ty::Adt(..) => is_mutable_reference_or_equivalent_and(cx, ty, |inner_ty| { | ||
| !implements_trait(cx, inner_ty, iter_trait, &[]) || is_iter_with_side_effects_impl(cx, inner_ty, iter_trait) | ||
| }), | ||
| ty::Closure(_, closure_args) => { | ||
| matches!(closure_args.types().next_back(), Some(captures) if captures.tuple_fields().iter().any(|capture_ty| is_ty_with_side_effects(cx, capture_ty, iter_trait))) | ||
| }, | ||
| ty::Array(elem_ty, _) | ty::Slice(elem_ty) => is_ty_with_side_effects(cx, *elem_ty, iter_trait), | ||
| ty::Tuple(field_tys) => field_tys | ||
| .iter() | ||
| .any(|field_ty| is_ty_with_side_effects(cx, field_ty, iter_trait)), | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| /// Check if `ty` is a mutable reference or equivalent. This includes: | ||
| /// - A mutable reference/pointer. | ||
| /// - A reference/pointer to a non-`Freeze` type. | ||
| /// - A `PhantomData` type containing any of the previous. | ||
| pub fn is_mutable_reference_or_equivalent<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { | ||
| is_mutable_reference_or_equivalent_and(cx, ty, |_| true) | ||
| } | ||
|
|
||
| fn is_mutable_reference_or_equivalent_and<'tcx, F>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, pred: F) -> bool | ||
| where | ||
| F: Fn(Ty<'tcx>) -> bool, | ||
| { | ||
| match ty.kind() { | ||
| ty::RawPtr(ty, mutability) | ty::Ref(_, ty, mutability) => { | ||
| (mutability.is_mut() || !ty.is_freeze(cx.tcx, cx.typing_env())) && pred(*ty) | ||
| }, | ||
| ty::Adt(adt_def, args) => adt_def.all_fields().any(|field| { | ||
| matches!(field.ty(cx.tcx, args).kind(), ty::Adt(adt_def, args) if adt_def.is_phantom_data() && args.types().any(|arg_ty| is_mutable_reference_or_equivalent(cx, arg_ty))) | ||
| }), | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
These need to be merged. ADTs, closures, tuples etc. need to be treated the same at every level of nesting.
| ty::Adt(adt_def, args) => adt_def.all_fields().any(|field| { | ||
| matches!(field.ty(cx.tcx, args).kind(), ty::Adt(adt_def, args) if adt_def.is_phantom_data() && args.types().any(|arg_ty| is_mutable_reference_or_equivalent(cx, arg_ty))) | ||
| }), |
There was a problem hiding this comment.
This should go back to being two arms, one to handle PhantomData specially, and one to handle every other ADT.
rustbot
added
S-waiting-on-author
|
For the util name something like |
Closes #9191
Closes #14444
Closes #8055
Adds a new helper to partly check for side effects by recursively checking if the iterator type contains closures with mutable captures.
changelog: [
double_ended_iterator_last] fix FP when iter has side effectschangelog: [
needless_collect] fix lint not consider side effects