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

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().

To avoid making a closure type generic over I too:

(and the same in rfold)

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:

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(_):

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.