Added the ability to collect a stream of results #207
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sunjay
force-pushed
the
fromstream-collections
branch
from
81a2fc5 to
ad05101
Compare
yoshuawuyts
approved these changes
Sep 17, 2019
|
Thanks for your review! Looks like everything has passed. Should this be merged now? |
|
Usually I'd like to give @stjepang a chance to review first (especially since we have a 1.0 coming up), but given bors r+ |
bors bot
added a commit
that referenced
this pull request
Sep 17, 2019
207: Added the ability to collect a stream of results r=yoshuawuyts a=sunjay As requested here: https://twitter.com/yoshuawuyts/status/1174026374316773377 The standard library has a very useful implementation of `FromIterator` that takes an iterator of `Result<T, E>` values and is able to produce a value of type `Result<Vec<T>, E>`. I asked for this in `async-std` and @yoshuawuyts recommended that I contribute the impl. It turns out that the implementation in the standard library is even more general than I initially thought. It allows any collection that implements `FromIterator` to be collected from an iterator of `Result<T, E>` values. That means that you can collect into `Result<Vec<T>, E>`, `Result<HashSet<T>, E>`, etc. I wanted to add a similarly generic impl for this crate so we can also support collecting into any collection that implements `FromStream`. The implementation for this is based heavily on [what exists in `std`](https://github.com/rust-lang/rust/blob/9150f844e2624eb013ec78ca08c1d416e6644026/src/libcore/result.rs#L1379-L1429). I made a new `result` module since that's where this impl is in `std`. I still wanted to maintain the conventions of this repo, so I copied the `vec` module that @yoshuawuyts created in #125. Much like in that PR, the new `result` module is private. There is a doctest in the documentation for `collect` that both teaches that this feature exists and tests that it works in some simple cases. ## Documentation Screenshot  Co-authored-by: Sunjay Varma <[email protected]>
Build succeeded
|
This was referenced Sep 30, 2019
bors bot
added a commit
that referenced
this pull request
Oct 4, 2019
266: Changes Extend trait in order to allow streams that yield references r=yoshuawuyts a=sunjay This is not ready to merge yet. I am mainly opening it so we can discuss a change I had to make to the `Extend` trait. cc @yoshuawuyts @stjepang (and anyone else interested) ## Before this can be merged - [x] Discuss/Approve changes to `Extend` trait - [x] Change to using `for_each` after #264 is merged - [ ] (optional) Wait until a `copied()` method is added to `StreamExt` so that the `&char` impl can be finished. - We can also just comment out or remove the impl that uses `copied` until that is added ## Changes To The Extend Trait While writing the impls of the `Extend` trait for the `String` type, I noticed that certain impls weren't possible because there is no bound on `Extend` that guarantees that the type `A` being yielded from the stream actually lives long enough. We probably didn't run into this earlier because this usually isn't a problem for owned values since the compiler doesn't have to worry about whether they will out live the stream that they come from. I ran into this because of the `Extend` impls that consume streams of references. The difference between the async `Extend` and the standard library `Extend` is that the async `Extend` returns a value that still references the input stream. That means that if `A` is any reference type, the compiler needs to be able to guarantee that `A` will be around as long as the `Future` returned from the trait method is around. To fix this, I had to add the bound shown below: ```patch pub trait Extend<A> { /// Extends a collection with the contents of a stream. fn stream_extend<'a, T: IntoStream<Item = A> + 'a>( &'a mut self, stream: T, - ) -> Pin<Box<dyn Future<Output = ()> + 'a>>; + ) -> Pin<Box<dyn Future<Output = ()> + 'a>> where A: 'a; } ``` This guarantees that each value of type `A` will last at least as long as our boxed future does. The bound had to be in a where clause on the method (and not on the declaration of `A` because the lifetime `'a` isn't in scope at the trait level. I don't think there are any negative consequences of using a where clause like this, but that's why I wanted to bring it up for discussion. In addition to this, I had to ensure that when writing the `Extend` impls for `String` I appropriately bounded the lifetime of the references from the stream. You can see this in the code below with `where 'b: 'a`. ```rust impl<'b> Extend<&'b str> for String { fn stream_extend<'a, S: IntoStream<Item = &'b str> + 'a>( &'a mut self, stream: S, ) -> Pin<Box<dyn Future<Output = ()> + 'a>> where 'b: 'a { //TODO: This can just be: stream.into_stream().for_each(move |s| self.push_str(s)) Box::pin(stream.into_stream().fold((), move |(), s| self.push_str(s))) } } ``` I should note that initially I tried to make it work with just the impl shown above, without modifying the `Extend` trait. This doesn't work because it would be a stricter bound than what is found in the trait itself. Rust does not allow stricter bounds like that because it could potentially cause unsoundness when dealing with generics. Of course, I am totally open to being completely wrong in my understanding of how to resolve this issue. I tried to solve the problem with as minimal of a change as possible. Please let me know if you have some better ideas or other suggestions. ## `FromStream` impls for String The purpose of adding these `Extend` impls is to continue my work from #129 in adding the rest of the `FromStream` impls. The `Extend` impls are used directly to add all of the `FromStream` impls for `String`. Just like with #207 and #265, this adds a new `string` module that is unstable just like the other modules added for `FromStream`. Co-authored-by: Sunjay Varma <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As requested here: https://twitter.com/yoshuawuyts/status/1174026374316773377
The standard library has a very useful implementation of
FromIteratorthat takes an iterator ofResult<T, E>values and is able to produce a value of typeResult<Vec<T>, E>. I asked for this inasync-stdand @yoshuawuyts recommended that I contribute the impl. It turns out that the implementation in the standard library is even more general than I initially thought. It allows any collection that implementsFromIteratorto be collected from an iterator ofResult<T, E>values. That means that you can collect intoResult<Vec<T>, E>,Result<HashSet<T>, E>, etc.I wanted to add a similarly generic impl for this crate so we can also support collecting into any collection that implements
FromStream.The implementation for this is based heavily on what exists in
std. I made a newresultmodule since that's where this impl is instd. I still wanted to maintain the conventions of this repo, so I copied thevecmodule that @yoshuawuyts created in #125. Much like in that PR, the newresultmodule is private.There is a doctest in the documentation for
collectthat both teaches that this feature exists and tests that it works in some simple cases.Documentation Screenshot