Add `Iterator::array_chunks` (take N+1) #100026

WaffleLapkin · 2022-08-01T15:25:37Z

A revival of #92393.

r? @Mark-Simulacrum
cc @rossmacarthur @scottmcm @the8472

I've tried to address most of the review comments on the previous attempt. The only thing I didn't address is try_fold implementation, I've left the "custom" one for now, not sure what exactly should it use.

…ce used to create an `IntoIter` from its parts

(suggested in the review of the previous attempt to add `ArrayChunks`)

As explained in the review of the previous attempt to add `ArrayChunks`, adapters that shrink the length can't implement `TrustedLen`.

It doesn't seem to be used at all.

rustbot · 2022-08-01T15:25:40Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

the8472 · 2022-08-01T18:44:03Z

Note that iter.next_chunk() exists now (#98326). And I expect these things to optimize poorly by default, so we'll need specialized implementations (as in #98553) for all sources that can yield chunks more efficiently and then plumb that through adapters that don't alter lengths like Map, Copied, Take etc. otherwise one won't get the autovectorized code one might expect.

I'm not sure if this the right approach and some design work is needed (#81615).

scottmcm · 2022-08-01T20:00:13Z

+1 that this should just use https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.next_chunk -- conveniently, that should mean that this PR gets substantially simpler, since the unsafe can live over there instead of here.

the8472 · 2022-08-02T08:44:10Z

library/core/src/iter/adapters/array_chunks.rs

+ let mut acc = init;
+ let mut iter = self.iter.by_ref().rev();
+
+ // NB remainder is handled by `next_back_remainder`, so
+ // `next_chunk` can't return `Err` with non-empty remainder
+ // (assuming correct `I as ExactSizeIterator` impl).
+ while let Ok(mut chunk) = iter.next_chunk() {
+ chunk.reverse();
+ acc = f(acc, chunk)?
+ }


by_ref AND a double-reverse? 😬 that must be horrendously slow.

Imo we either need a next_chunk_back or only limit it to forward iteration. The former could be done in a separate PR, but we need to decide on an approach.

I'm sorry you had to see this impl 😅

WaffleLapkin · 2022-08-05T00:02:47Z

@rustbot ready

scottmcm · 2022-08-05T00:07:48Z

library/core/src/iter/adapters/array_chunks.rs

+ self.next_back_remainder();
+
+ let mut acc = init;
+ let mut iter = self.iter.by_ref().rev();


suggestion: You have a Sized iterator here (since it's a field), so you can avoid some of the by_ref optimization penalty by using https://github.com/rust-lang/rust/blob/master/library/core/src/iter/adapters/by_ref_sized.rs instead of by_ref().

Mark-Simulacrum · 2022-08-06T20:10:17Z

r? @scottmcm

library/core/src/iter/traits/iterator.rs

scottmcm · 2022-08-10T01:07:24Z

library/core/src/iter/adapters/array_chunks.rs

+ Self: Sized,
+ F: FnMut(B, Self::Item) -> B,
+ {
+ self.try_fold(init, |acc, x| NeverShortCircuit(f(acc, x))).0


To avoid making a closure type generic over I too:

Suggested change

self.try_fold(init, |acc, x| NeverShortCircuit(f(acc, x))).0

self.try_fold(init, NeverShortCircuit::wrap_mut_2(f)).0

(and the same in rfold)

scottmcm

I have a few comments, but overall I think this is essentially fine for nightly. (Lots of open questions for stabilization, but that's ok.)

Can you please open a tracking issue, add it to the unstable attributes, and address my other comments?

scottmcm · 2022-08-13T06:04:07Z

library/core/src/iter/adapters/array_chunks.rs

+ // Take the last `rem` elements out of `self.iter`.
+ let mut remainder =
+ // SAFETY: `unwrap_err` always succeeds because x % N < N for all x.
+ unsafe { self.iter.by_ref().rev().take(rem).next_chunk().unwrap_err_unchecked() };


I don't think I'll block the PR on this, but unwrap_err_unchecked here scares me -- I added a note to the tracking issue.

My instinct for now is that %N is definitely trustworthy, so that combined with take is sufficient to say this is ok. But I'm still afraid, since it's not TrustedLen, and wonder if someone can come up with an evil example here where it somehow gets a full chunk and thus is UB in safe code.

(At least the obvious things, like a len() that always says usize, is protected against because of the modulo.)

As already noted in another comment I don't like this entire method. Either we should add next_chunk_back or remove the DEI impl.

So, an evil impl can return less than rem elements, but actual <= rem < N still holds (I don't think there is any way to trick Take into returning more than rem elements) so the chunk will be an error no matter what.

An evil impl can just return a wrong length, so after this iterator returns a number of elements not divisible by N. But this is fine too, an Err will just be ignored:

rust/library/core/src/iter/adapters/array_chunks.rs

Lines 116 to 119 in 5fbcde1

// NB remainder is handled by `next_back_remainder`, so

// `next_chunk` can't return `Err` with non-empty remainder

// (assuming correct `I as ExactSizeIterator` impl).

while let Ok(mut chunk) = iter.next_chunk() {

That being said, as @the8472 already said, we should probably rewrite the DEI impl anyway (I'll try to do this, after this lands).

Yeah, the % is certainly fine. What I'm not yet convinced of is that there's no way to make take misbehave somehow -- for example, Take::try_fold uses the inner try_fold, so maybe there'd be a way for that to be implemented wrongly-but-not-UB that could result in too many things getting put in the array somehow.

But I agree that just making a nicer implementation without the unsafe{} is the right plan.

Actually you are right 😨

Overriding try_fold with some nasty unsafe to skip over Break(_) allows you to trick Take into returning more elements that it should (playground).

n can underflow, if a bad iterator impl skips over Break(_):

rust/library/core/src/iter/adapters/take.rs

Lines 87 to 89 in 2fbc08e

*n -= 1;

let r = fold(acc, x);

if *n == 0 { ControlFlow::Break(r) } else { ControlFlow::from_try(r) }

The fact that unsafe code can't trust Take is scary.

I think the problem there is still in the unsafe in Misbehave -- it protects against double-drop, but duplicating arbitrary values can violate safety invariants in general. (For example, if the type is !Clone + !Default, I can use you moving it into me to track resource consumption, like a ZST tracker for a global semaphore.) So that specific example is just "well unsound unsafe-using code breaks everything".

But yeah, even though I've not been able to come up with a safe trick that would do it, things along those lines are what make me worried about it. (And certainly if specialization existed then it would be possible to do particularly weird things in safe code because it can violate parametricity _{even worse than in normal code}.)

The example could check, that type_id::<B>() == type_id::<usize>() or something similar, which would IMO make the code sound.

Yeah, I thought about that one, but TypeId::of::<B>() needs B: 'static, which we don't have here. Regardless, it's yet another chink in the armour that makes be scared we'll break through eventually.

scottmcm · 2022-08-13T06:07:09Z

Running over this again I noticed one thing I'm wondering about, but I don't think it needs to block going to nightly. Thanks for pushing this through!

@bors r+

bors · 2022-08-13T06:07:11Z

📌 Commit 5fbcde1 has been approved by scottmcm

It is now in the queue for this repository.

the8472 · 2022-08-13T06:22:15Z

(take N+1)

Clearly a bug! :D

Add `Iterator::array_chunks` (take N+1) A revival of rust-lang#92393. r? `@Mark-Simulacrum` cc `@rossmacarthur` `@scottmcm` `@the8472` I've tried to address most of the review comments on the previous attempt. The only thing I didn't address is `try_fold` implementation, I've left the "custom" one for now, not sure what exactly should it use.

Rollup of 6 pull requests Successful merges: - rust-lang#99582 (Delay a span bug if we see ty/const generic params during writeback) - rust-lang#99861 (orphan check: rationalize our handling of constants) - rust-lang#100026 (Add `Iterator::array_chunks` (take N+1)) - rust-lang#100115 (Suggest removing `let` if `const let` or `let const` is used) - rust-lang#100126 (rustc_target: Update some old naming around self contained linking) - rust-lang#100487 (`assert_{inhabited,zero_valid,uninit_valid}` intrinsics are safe) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rossmacarthur and others added 9 commits August 1, 2022 16:39

Add Iterator::array_chunks()

ca3d101

Use array::IntoIter for the ArrayChunks remainder

f548518

Remove array::IntoIter::with_partial -- an artifact of the past, on…

ef72349

…ce used to create an `IntoIter` from its parts

Forward ArrayChunks::next{,_back} to try_{for_each,rfold}

b8b1486

(suggested in the review of the previous attempt to add `ArrayChunks`)

Remove incorrect impl TrustedLen for ArrayChunks

4db628a

As explained in the review of the previous attempt to add `ArrayChunks`, adapters that shrink the length can't implement `TrustedLen`.

Use #[track_caller] to make panic in Iterator::array_chunks nicer

3102b39

Remove Fuse from ArrayChunks implementation

37dfb04

It doesn't seem to be used at all.

Simplify ArrayChunks::is_empty

4c0292c

Simplify ArrayChunks::{,r}fold impls

475e4ba

rust-highfive assigned Mark-Simulacrum Aug 1, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 1, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2022

This comment has been minimized.

Sign in to view

scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2022

Use next_chunk in ArrayChunks impl

756bd6e

the8472 reviewed Aug 2, 2022

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2022

scottmcm reviewed Aug 5, 2022

View reviewed changes

rust-highfive assigned scottmcm and unassigned Mark-Simulacrum Aug 6, 2022

scottmcm reviewed Aug 10, 2022

View reviewed changes

library/core/src/iter/traits/iterator.rs Show resolved Hide resolved

scottmcm reviewed Aug 10, 2022

View reviewed changes

scottmcm requested changes Aug 10, 2022

View reviewed changes

scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2022

WaffleLapkin mentioned this pull request Aug 12, 2022

Tracking Issue for Iterator::array_chunks #100450

Open

7 tasks

WaffleLapkin added 2 commits August 12, 2022 14:57

address review comments

eb6b729

fill-in tracking issue for feature(iter_array_chunks)

5fbcde1

scottmcm reviewed Aug 13, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 13, 2022

Dylan-DPC mentioned this pull request Aug 13, 2022

Rollup of 7 pull requests #100481

Closed

Dylan-DPC mentioned this pull request Aug 14, 2022

Rollup of 6 pull requests #100525

Merged

bors merged commit 482a6ea into rust-lang:master Aug 14, 2022

rustbot added this to the 1.65.0 milestone Aug 14, 2022

WaffleLapkin deleted the array-chunks branch August 14, 2022 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Iterator::array_chunks` (take N+1) #100026

Add `Iterator::array_chunks` (take N+1) #100026

WaffleLapkin commented Aug 1, 2022

rustbot commented Aug 1, 2022

This comment has been minimized.

the8472 commented Aug 1, 2022 •

edited

Loading

scottmcm commented Aug 1, 2022

the8472 Aug 2, 2022

WaffleLapkin Aug 2, 2022

WaffleLapkin commented Aug 5, 2022

scottmcm Aug 5, 2022

Mark-Simulacrum commented Aug 6, 2022

scottmcm Aug 10, 2022

scottmcm left a comment

scottmcm Aug 13, 2022

the8472 Aug 13, 2022 •

edited

Loading

WaffleLapkin Aug 14, 2022 •

edited

Loading

WaffleLapkin Aug 14, 2022

scottmcm Aug 14, 2022

WaffleLapkin Aug 14, 2022

scottmcm Aug 14, 2022

WaffleLapkin Aug 14, 2022

scottmcm Aug 14, 2022

scottmcm commented Aug 13, 2022

bors commented Aug 13, 2022

the8472 commented Aug 13, 2022

	self.try_fold(init, \|acc, x\| NeverShortCircuit(f(acc, x))).0
	self.try_fold(init, NeverShortCircuit::wrap_mut_2(f)).0

	// NB remainder is handled by `next_back_remainder`, so
	// `next_chunk` can't return `Err` with non-empty remainder
	// (assuming correct `I as ExactSizeIterator` impl).
	while let Ok(mut chunk) = iter.next_chunk() {

	*n -= 1;
	let r = fold(acc, x);
	if *n == 0 { ControlFlow::Break(r) } else { ControlFlow::from_try(r) }

Add Iterator::array_chunks (take N+1) #100026

Add Iterator::array_chunks (take N+1) #100026

Conversation

WaffleLapkin commented Aug 1, 2022

rustbot commented Aug 1, 2022

This comment has been minimized.

the8472 commented Aug 1, 2022 • edited Loading

scottmcm commented Aug 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaffleLapkin commented Aug 5, 2022

Choose a reason for hiding this comment

Mark-Simulacrum commented Aug 6, 2022

Choose a reason for hiding this comment

scottmcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the8472 Aug 13, 2022 • edited Loading

Choose a reason for hiding this comment

WaffleLapkin Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Aug 13, 2022

bors commented Aug 13, 2022

the8472 commented Aug 13, 2022

Add `Iterator::array_chunks` (take N+1) #100026

Add `Iterator::array_chunks` (take N+1) #100026

the8472 commented Aug 1, 2022 •

edited

Loading

the8472 Aug 13, 2022 •

edited

Loading

WaffleLapkin Aug 14, 2022 •

edited

Loading