Skip to content

Update async_block* macros to support Stable & Unpin #936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Apr 3, 2018

I'm not sold on the name, especially since it uses a completely different meaning to the pinned argument to #[async]. I could do the same as has happened to #[async] and change async_block to return a StableFuture along with a new async_block_move macro for Future.

My preferred way to do this would be to keep a single async_block macro and pass an argument to it as posited in #918 for #[async(move)] (along with the change to StableFuture by default); I've been trying to come up with a way to nicely pass attributes to the async_block macro, but I can't think of anything I'm really attached to. The best I came up with was something like:

async_block!(move, {
    await!(bar(&x));
});

which I believe is unambiguous because $($ident,)+ { $($tt)+ } cannot be parsed as an expr.

@cramertj
Copy link
Member

cramertj commented Apr 3, 2018

Perhaps the naming here should be inverted to async_block and movable_async_block?

cc @withoutboats

@awestlake87
Copy link

async_block! is essentially an #[async] closure with empty params. Maybe the answer is to keep the attribute the same and apply it to a || { } expression instead.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 12, 2018

@awestlake87 unfortunately the async/await RFC has reserved that syntax for async closures (which I'm slightly tempted to try implementing for #[async] at some point). It does mention having an async_block! { ... } macro which may become something like async { ... } in the future, but presumably relies on the eventual automatic impl !Unpin for <unnamed self referential Generator> to distinguish between Unpin and immovable futures returned from that macro.

I think changing async_block! to be immovable by default is probably the best way to go, if the argument naming in #964 is accepted then I'm leaning toward async_block_unpin! for the Unpin variant, otherwise I guess async_block_move!. I think having a suffix rather than a prefix keeps this more consistent with the async macro taking arguments, it's sort of like an argument built into the macro name itself. I'll rebase and update to that now, but feel free to disagree here or there about the naming.

@awestlake87
Copy link

@Nemo157 I see, I forgot it needed to be called in order to act as a future. The macro does look cleaner, although I'd definitely prefer the first class syntax in that RFC in the longterm. I'm glad there's talk about adding futures to core and stabilizing async/await separately from proc_macro2 and generators.

As for the naming, I agree with @Nemo157 when it comes to changing async_block! to immovable by default because that's how closures act in rust. I think I'm leaning more towards async_block_move! than async_block_unpin! though, as it's more consistent with rust's concept of move for closures. Pin/Unpin is a concept that users will need to learn in order to understand why yield points make borrowing unsafe, but I don't think it conveys enough information about how it's treating the environment around the block to someone who's naively using async/await.

Nemo157 and others added 3 commits April 13, 2018 21:31
Changes `async_block` and `async_stream_block` to return `StableFuture`
and `StableStream` respectively. Adds new `async_block_unpin` and
`async_stream_block_unpin` to create `Future`s and `Stream`s from blocks
instead.
@Nemo157 Nemo157 changed the title Add async_block_pinned macro Update async_block* macros to support Stable & Unpin Apr 13, 2018
@withoutboats
Copy link

I'm inclined to do just remove the current async_block and only support the unpinned version, per my comment on #964.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 16, 2018

Subsumed by #964

@Nemo157 Nemo157 closed this Apr 16, 2018
@Nemo157 Nemo157 deleted the pinned-block branch February 16, 2019 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants