-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
|
Why is the #[async_stream(item = "T")]
fn foo() -> Result<(), U> Wouldn't returning #[async_stream]
fn foo() -> Result<T, U>
// Becomes:
fn foo() -> impl Stream<Item=T, Error=U> To me that makes a lot more sense. |
That would be possible, and would support streams that can continue returning values even after they return an error (still not something I have ever seen used though). It doesn't match nicely with First thought on how the second option would look: #[async_stream(item = "u32", error = "&'static str")]
fn blast_off(abort: bool) {
stream_yield!(Ok(2));
if abort {
stream_yield!(Err("blast off aborted"));
return;
}
stream_yield!(Ok(1));
}
Oh, yes, that's gonna need some extra state to keep track of whether it's done that I was really hoping to avoid. The second option above would avoid this, but I don't think we want to drop |
That was my original implementation, I changed it based on #7 (comment), so that the declared return type matches the values actually passed to |
@Nemo157 Looking through the comments quickly I haven't found a good reason why returning I would push for returning an |
Yes, two ways of yielding an error value is also what I chose for tiny-async-await. In my opinion it isn't a problem. |
It's not that it's not possible, I even still have a branch implementing it, it's that you then hit one of two issues
After reading through that linked thread I think the start of rust-lang/futures-rs#206 (comment) really makes being able to both yield and return an error make sense to me, they are two distinct error cases that just happen to be constrained to the same type by the trait design. I'll try and push an update tonight changing this and fixing the end state to allow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, thanks!
Could you also update the README as well?
examples/echo.rs
Outdated
@@ -43,19 +42,17 @@ fn main() { | |||
core.run(server).unwrap(); | |||
} | |||
|
|||
#[async] | |||
fn handle_client(socket: TcpStream) -> io::Result<u64> { | |||
#[async_stream(item = "u64")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to get better error messages we may want to avoid the usage of a string here and instead use raw tokens, perhaps something like:
#[async_stream(item = u64)]
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that Rust allows that now, I thought I had tried something similar when custom derives first came out and had it denied by rustc
before calling into the proc macro.
syn
fails to parse it since it's not really a valid attribute, I guess I can just switch from using syn
to parse to working on the token tree directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syn
fails to parse it since it's not really a valid attribute, I guess I can just switch from usingsyn
to parse to working on the token tree directly.
It is a valid attribute when #[feature(proc_macro)]
is enabled in the source. Also, the git version of syn
that this crate uses will parse it since it has support for arbitrary tts in attributes. It's not a valid MetaItem
so Attribute::meta_item()
will return None
, and you have to operate on Attribute::tts
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
futures-async-macro/src/lib.rs
Outdated
} | ||
}; | ||
|
||
// We've got to get a bit creative with our handling of arguments. For a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this avoid duplication with #[async]
and be refactored to a common implementation?
macro_rules! stream_yield { | ||
($e:expr) => ({ | ||
let e = $e; | ||
yield ::futures::Async::Ready(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this perhaps just be yield e
? I think it'd be fine to use two different conversions internally for the two attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean by two different conversions here, could you explain a bit more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sure yeah, basically I just mean that yielding in #[async]
should be just a literal yield
, and yielding in #[async_stream]
would be yield e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of having the stream_yield!
macro, just do a rewrite from yield e;
to yield ::futures::Async::Ready(e);
as part of the #[async_stream]
macro? That does have the downside of meaning you can't write macros that yield values, but I assume those would be rare enough to not matter. I'll try and get this and the attribute parsing done tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no I was thinking we'd keep the stream_yield!
macro, but it would be implemented with yield e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, how would that fit with await!
and #[async] for
yielding Async::NotReady
then, just have the user do something like stream_yield!(Async::Ready(e))
? I thought part of the purpose of async/await syntax was to allow composing futures/streams together without having to work with some of the more verbose underlying types like Async
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, that makes sense! In that case let's leave as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how much magic (tm) is acceptable, #[async]
and #[async_stream]
could themselves expand await!
in different ways that make sense for each of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah I'm fine using the strategy here, seems fine to me!
src/lib.rs
Outdated
@@ -15,6 +15,7 @@ | |||
#![feature(conservative_impl_trait)] | |||
#![feature(generator_trait)] | |||
#![feature(use_extern_macros)] | |||
#![feature(never_type)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally prefer to not block on this stabilizing, could we avoid this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to a custom uninhabited type.
I've changed the argument to be a path instead of a string and added some basic details to the readme. Probably should have a full technical details section in the readme as well, I'll try and get to that by the end of the week. |
@@ -311,7 +406,7 @@ impl Folder for ExpandAsyncFor { | |||
} | |||
} | |||
futures_await::Async::NotReady => { | |||
yield; | |||
yield futures_await::Async::NotReady; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be best using ::futures::...
.
Could a test be added for this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an extern crate futures_await;
line just above. This whole #[async] for
expander uses that instead of via ::futures
.
quote! { | ||
Box<::futures::Stream< | ||
Item = !, | ||
Error = <! as ::futures::__rt::IsResult>::Err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it may not work? Maybe a test could be added for #[async_stream(boxed, item = ...)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha nevermind, missed the new replace_bangs
function
src/lib.rs
Outdated
GeneratorState::Yielded(Async::NotReady) | ||
=> Ok(Async::NotReady), | ||
GeneratorState::Yielded(Async::Ready(_)) | ||
=> unreachable!("Mu doesn't exist"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I believe you can do match mu {}
instead of an unreachable!
src/lib.rs
Outdated
type Error = <<T::Yield as IsAsync>::Item as IsResult>::Err; | ||
|
||
fn poll(&mut self) -> Poll<Option<Self::Item>, Self::Error> { | ||
if self.done { return Ok(Async::Ready(None)) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to elide the boolean here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's related to @Arnavion's comment: #32 (comment)
I tried the test in e9d53a5 on the changes before 44fd004 and had it fail. Unless your conclusion from rust-lang/futures-rs#206 (comment) has changed then there needs to be the ability to track one more state change after the generator completes if it returns an error (and to support ?
it needs to retain the ability to return an error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right yeah, makes sense
src/lib.rs
Outdated
|
||
impl<T> Stream for GenStream<T> | ||
where T: Generator, | ||
T::Yield: IsAsync, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the IsAsync
trait needed here? Or can this just use T: Generator<Yield = Async<U>>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's required to allow yielding results and have the equality constraint <<T::Yield as IsAsync>::Item as IsResult>::Err = <T::Return as IsResult>::Err
. If there's some way to encode that equality without it then it can be dropped, I'll take a look at all these bounds and see if there's some sort of simplification that can happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified it to
where
T: Generator,
T::Yield: IsAsync<Item = Result<U, <T::Return as IsResult>::Err>>,
T::Return: IsResult<Ok = ()>,
I attempted
where
T: Generator<Yield = Async<Result<U, <T::Return as IsResult>::Err>>>,
T::Return: IsResult<Ok = ()>,
but that fails because of error[E0391]: unsupported cyclic reference between types/traits detected
Looking great! I'm pretty ok merging this as-is basically. One last thing we may want to test is a few error messages? Just basic scenarios where |
README.md
Outdated
fn fetch_all(client: hyper::Client, urls: Vec<&'static str>) -> io::Result<()> { | ||
for url in urls { | ||
let s = await!(fetch(client, url))?; | ||
stream_yield!(Ok(s)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this intended to be stream_yield!(s)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so it looks like this was changed during discussion?
I'd personally prefer to only yield the item type from stream_yield!
. Our eventual desired semantics for Stream
is that an Err
fuses (ends) the stream as well as returning None
, so I'm hoping we can get a jump start on that here by only allowing yielding items and using return Err(...)
for errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, that's a change I'm very much in favour of. For some reason I got the impression from the linked issue that that wouldn't be changing any time soon, if it is a concrete plan then I can definitely switch this back.
examples/echo.rs
Outdated
@@ -43,19 +42,17 @@ fn main() { | |||
core.run(server).unwrap(); | |||
} | |||
|
|||
#[async] | |||
fn handle_client(socket: TcpStream) -> io::Result<u64> { | |||
#[async_stream(item = u64)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this stay as it was before? This was mostly intended to showcase the similarity to an otherwise synchronous echo server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
} | ||
AsyncStreamArg(term, Some(ty)) => { | ||
if term == "item" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also panic if item_ty
is Some
already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for arg in args.0 { | ||
match arg { | ||
AsyncStreamArg(term, None) => { | ||
if term == "boxed" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to below, could this panic if boxed
is already true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
--> $DIR/bad-return-type.rs:17:36 | ||
| | ||
17 | #[async_stream(item = Option<i32>)] | ||
| ____________________________________^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a particularly confusing error message, especially with respect to span information. Is there anything we can do to improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The horribleness of this error message comes from stream_yield!
expanding to
let e = $e;
yield ::futures::Async::Ready(e)
the span which is in error is the e
in the second line of the expansion (although, why it then highlights what it does I don't know). Changing it to simply
yield ::futures::Async::Ready($e)
changes the error message to what it should be:
error[E0308]: mismatched types
--> $DIR/bad-return-type.rs:25:19
|
25 | stream_yield!(val);
| ^^^
| |
| expected enum `std::option::Option`, found integral variable
| help: try using a variant of the expected type: `Some(val)`
|
= note: expected type `std::option::Option<_>`
found type `{integer}`
but causes test failures
error[E0597]: `t` does not live long enough
--> tests/smoke.rs:98:19
|
98 | stream_yield!(t.clone());
| ^ does not live long enough
...
101 | }
| - borrowed value only lives until here
|
= note: borrowed value must be valid for the static lifetime...
I believe those are related to rust-lang/rust#44197, once that is resolved then the stream_yield!
expansion can be updated and this should be fixed.
I'd prefer to merge with the current error message and I'll open an issue to fix it once the upstream issue is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is to change the expansion now and change the test to
let c = t.clone();
stream_yield!(c);
Doing this instead would increase the churn once rust-lang/rust#44197 is fixed, every user of this library that had to make an extra temporary would have to fix their code at some point. As long as that issue doesn't take too long to fix I think we can live with the worse error message for a short while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yeah this sounds like a fine tradeoff to me, mind leaving a comment in the definition of the stream_yield!
macro pointing to the upstream issue?
Thanks so much again! I left some comments, but it looks like the |
Yep, with not having to keep the error equality |
Thanks so much @Nemo157! With that last comment I'm good to merge |
👍 |
Current usage is similar to
where the signature
is transformed into
#[async_stream(boxed, item = T)]
is also supportedMajor stuff that needs to happen before merging:
Decide if(EDIT: replaced with a local uninhabited type)never_type
feature is ok, or come up with an alternative method to makeawait!
compatible with both#[async]
and#[async_stream]
Rebase/merge latest changes(EDIT: rebased)Add tests(EDIT: smoke tests added)Pull out some helper methods to reduce duplication between(EDIT: Replaced with a single helper implementation)#[async]
and#[async_stream]
Fixes #7