Small fixes and improvements before next release

[novacrazy]

I removed Send/Sync implementations on GenericArrayIter, because it was a mistake to add them. Rust will automatically implement Send where possible, and GenericArrayIter shouldn't be Sync because the indices are not atomic variables.

I've improved the generics tests to include most of the common use scenarios, and noted in functional.rs that tests/generics.rs has those as examples.

Finally, while writing the previous tests/examples, I noticed I did actually find the SequenceItem accessor alias type useful in keeping the generic conditions cleaner, so I added that back in.

[novacrazy]

I just noticed this here: rust-lang/rust#49000, and I think we should model GenericArrayIter like that. In fact, it looks like it borrowed from GenericArrayIter in a few places, and I like a few of the changes there, so there is no harm to making ours on-par with the future standard library iterator.

I'll go ahead and push that to this pull request, since it doesn't actually change that much, just a few ergonomics.

[cuviper]

FWIW, I didn't really borrow from GenericArrayIter. Rather, I've been working on variations in std for a while (see rust-lang/rust#32871), and I brought those ideas here in #13. I'm not concerned, but I don't want there to be any confusion about the origin of code, especially regarding copyright and licenses.

[novacrazy]

That’s fair. This crate is MIT licensed anyway, so I wasn’t judging.

EDIT: I see now you're the original author of GenericArrayIter in this crate, too. Sorry, been a long day already.

While you’re here, there is a significant problem with both yours and this crates consuming iterators, and that is it is impossible for Rust to fully optimize them.

You can look through #57 for my previous work on it, but I’ve spent a lot of time trying to get Rust/LLVM to auto-vectorize certain operations, and we’ve basically just had to roll our own system for it, described in that PR. You might want to be aware of that.

[cuviper]

I'll be happy to get IntoIterator for arrays in std at all, and then we can worry about optimizing... 😉

[cuviper]

I removed Send/Sync implementations on GenericArrayIter, because it was a mistake to add them. Rust will automatically implement Send where possible, and GenericArrayIter shouldn't be Sync because the indices are not atomic variables.

I agree you should leave them auto-implemented, but I don't see why Sync is a problem. Sync is only concerned with things you can do with &self, and the index changes are only in &mut self methods.

[novacrazy]

That... is a good point.

So I suppose Send can be left auto-implemented, but Sync can be manually implemented so T: Sync can be accessed across threads when the iterator isn't being modified.

[cuviper]

I don't think you need a manual implementation. Using a nightly rustdoc which shows auto impls, this PR documents both auto Send and Sync already, where T: Send and T: Sync respectively.

[novacrazy]

Ah, I hadn't seen that since I try to target stable for public crates. Well, cool, that makes things easier.

[novacrazy]

How were you able to prove lifetime invariance for the std implementation? That test doesn't seem to play well with GenericArrayIter, and I think it might be because of ArrayLength.

[cuviper]

How were you able to prove lifetime invariance for the std implementation?

I just copied that test from the vec:IntoIter tests, and it worked. 😄

Does a variance test like this work for even just GenericArray?

[novacrazy]

No, it does not.

However, adding where 'new: 'static to those test functions seems to convince it, so it knows they're related, but just can't prove the invariance automatically.

[cuviper]

You're saying "invariance", but AIUI covariance is the opposite. We want this to allow the reduction to a shorter lifetime. I think 'new: 'static is backwards, basically implying that 'new is exactly 'static, so with that the test passes even if the type is invariant. We're essentially testing 'static: 'new, which is true for all lifetimes, but requires the type to be covariant.

(I'm not sure though -- variance is confusing!)

[novacrazy]

Yeah, it is. Invariance is same/constant/equivalance, Covariance is usually one is less than the other, and the relationships that implies, or something like that.

I think you're right, though, since the test does say "covariance". I must have mixed up the words somewhere, and inverting the where statement does make it fail again. Maybe I'll take another look at it again after some sleep and reviewing all the lifetime rules, but for now I guess it doesn't matter much.

[cuviper]

It's interesting. This isn't allowed:

fn array<'new>(a: GenericArray<&'static str, U10>) -> GenericArray<&'new str, U10> {
    a
}

error[E0308]: mismatched types
  --> src/lib.rs:85:9
   |
85 |         a
   |         ^ lifetime mismatch
   |
   = note: expected type `GenericArray<&'new str, _>`
              found type `GenericArray<&'static str, _>`
note: the lifetime 'new as defined on the function body at 84:5...
  --> src/lib.rs:84:5
   |
84 | /     fn array<'new>(a: GenericArray<&'static str, U10>) -> GenericArray<&'new str, U10> {
85 | |         a
86 | |     }
   | |_____^
   = note: ...does not necessarily outlive the static lifetime

I don't know what about the array itself is making it invariant.

But the underlying data does exhibit covariance -- this is fine:

fn array_rebuild<'new>(a: GenericArray<&'static str, U10>) -> GenericArray<&'new str, U10> {
    GenericArray { data: a.data }
}

[novacrazy]

It's definitely something with ArrayLength::ArrayType.

This works:

struct Test<T, N: ArrayLength<T>>(PhantomData<(T, N)>);

fn test_covariance<'new>(i: Test<&'static str, U10>) -> Test<&'new str, U10> {
    i
}

but this fails:

struct Test<T, N: ArrayLength<T>>(PhantomData<(T, N::ArrayType)>);

fn test_covariance<'new>(i: Test<&'static str, U10>) -> Test<&'new str, U10> {
    i
}