(Rust implementation) Add idiomatic rust iterator for Vector

[jaynus]

Implemented for T: Follow (which provides get accessor).

In rust, it is more idiomatic to use iterators and combinators on 'collections' of data instead of a len()/get() set. The Flatbuffers implementation currently requires:

}```

This PR allows more rust-ish:
```for obj in vector.iter() {
}

let transformed = vector.iter.map(|i| i.id()).collect::Vec<u32>(); // psuedo-example

Immutable iterator that respects lifetimes. This allows for more idiomatic combinator/iterator access of vector types in flatbuffers. Iterator respects inner vector lifetimes, and allocates single iterator object for tracking (reference + iterator location track).

[rw]

@jaynus LGTM! Would you add a no-op commit to get CI to run again? Looks like Appveyor had an error.

[jaynus]

Done and passed.

[rw]

@jaynus Would you please add a test for this? Perhaps as a sub-mod under the roundtrip_vectors module in the test suite? Link: https://github.com/google/flatbuffers/blob/master/tests/rust_usage_test/tests/integration_test.rs#L774

[rw]

@jaynus Or, minimally, by adding a vector iterator assert to the serialized_example_is_accessible_and_correct function. Link: https://github.com/google/flatbuffers/blob/master/tests/rust_usage_test/tests/integration_test.rs#L185

[tymcauley]

This would be a great change! In this commit that was just merged, I had to add some code in a few spots to tests/rust_usage_test/tests/integration_test.rs that looks like this:

let mut rust_vec: Vec<T> = Vec::with_capacity(flatbuffer_vector.len());
for i in flatbuffer_vector.len() {
    rust_vec.push(flatbuffer_vector.get(i));
}

Those might be good candidates to replace with code that tests out this new functionality. Not exactly sure, but maybe something like this:

let rust_vec = flatbuffer_vector.iter().collect::Vec<T>();

[jaynus]

As per the commit note, added a small test addition to roundtrip_vectors, which is implemented in the prop function which creates and tests a vector of a given type. this means the iterator is tested for every prop generation test cycle (every time used in roundtrip vectors)

[rw]

@jaynus Cool! Let's add your vector collect check to some more tests, so we can test more situations:

vector_of_string_store_helper_build
vector_of_string_store_manual_build
vector_of_f64_store
vector_of_table_store
vector_of_bool_store
vector_of_struct_store

[TethysSvensson]

I don't think this works as expected with regards to lifetimes.

Specifically, the items returned by the iterator has a lifetime limited by the borrowing of the vector, instead of by the lifetime argument in the vector itself.

I would expect all four of these functions to compile (and give the same result):

https://gist.github.com/TethysSvensson/2615a8a1aff7f243fda78c3c1e1ced51

In the current implementation, only the first and last one compiles.

I think part of it is that Vector does not implement Copy and Clone even though it easily could.

[TethysSvensson]

I think this PR should include something along the lines of this to work as expected:

TethysSvensson@cb7d088

(This commit comes from a branch we use at my work. It contains a cherry-pick of your commit, but also adds additional derives to the generated enums that we don't expect to get upstreamed.)

[rw]

@TethysSvensson Thanks for thinking hard about the lifetimes! @jaynus WDYT?

[jaynus]

I've updated this PR with multiple cases.

I apologize for the delay, I just got wrapped up in other work for a while.

• took the recommended commit of correct lifetimes and implementations of clone+copy, as these make sense as they are just referential structures.
• Added all the requested integration tests

[jaynus]

I've updated for all these requested changes.

[jean-airoldie]

Ping

[jean-airoldie]

@rw Deriving Clone an Copy is not possible in this case since it puts a Copy / Clone bound on T. In this case we only need to be able to copy the marker to T (PhantomData in this case), not the actual T type.

[rw]

@jean-airoldie @jaynus Is this ready to go in? I know there was the other PR @jean-airoldie worked on, so I want to make sure nothing is missed. Otherwise this LGTM.

[jean-airoldie]

@rw Yes its pretty much ready. The only thing I found is that some bounds could be less strict. But if you merge this PR I'll make another one to fix that.

[jaynus]

I am here. This has just fallen through the cracks of my github alerts. What is needed to get this finalized and merged?

[jean-airoldie]

LGTM
edit: Tested against some of my code and I found an issue.

[jean-airoldie]

Should be impl<'a, T: 'a + Follow<'a>> Vector<'a, T> {

[jaynus]

Woops What I got for editing on the road :)

[TethysSvensson]

You can use the Copy instance to implement the Clone instance. This makes it more clear that Copy == Clone for this type.

Specifically you can replace the body with *self.

However it might be late enough in the process that this should be changed in a later commit.

[jean-airoldie]

@jaynus We should provide a size_hint using vector's len method.

[jean-airoldie]

@jaynus Ping.

[rw]

@jaynus @jean-airoldie @TethysSvensson Looks like both PRs went stagnant. What do you think about merging one of them?

[TethysSvensson]

@rw: I have been managing a fork of this repo for my work and my fork includes a more efficient version of this PR.

I would be happy if I could get some or all of it upstreamed.

So far I have created #5577, #5579 and #5580 which contains most of the changes in my fork.

My version of this PR is in #5579.

[rw]

Implemented by #5579. Thanks @jaynus @jean-airoldie @TethysSvensson for working on this!