Skip to content

Fix pinned boxed async functions #916

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

Merged
merged 4 commits into from
Apr 12, 2018

Conversation

raviqqe
Copy link
Contributor

@raviqqe raviqqe commented Mar 28, 2018

This change fixes pinned boxed async functions which were unable to compile.

@raviqqe raviqqe force-pushed the pinned-box-async branch 12 times, most recently from 42f3f4e to 9244ed7 Compare March 29, 2018 08:04
@raviqqe
Copy link
Contributor Author

raviqqe commented Mar 29, 2018

I made the Attribute enum to pass information of if futures should be Send or not and reduce arguments of the async_inner() function. However, I don't know whether it's a correct approach.

@ngg
Copy link
Contributor

ngg commented Apr 3, 2018

It's working for me pretty well, can you please merge this?

Copy link

@withoutboats withoutboats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just want to clarify why the additional re-exports.

@@ -264,6 +264,15 @@ pub mod prelude {
#[cfg(feature = "std")]
pub use futures_core::executor::Executor;

#[cfg(feature = "nightly")]
pub use futures_stable::{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these already exported through the stable submodule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to put them into futures::prelude as normal Future and Stream so that users don't need to use them every time in modules which use #[async] stuff otherwise they'll see some error messages about missing methods of pin() or pin_local().

@ngg
Copy link
Contributor

ngg commented Apr 3, 2018

This code doesn't work for me (it works if one of the parameters is i32 instead of &i32)

#[async(pinned)]
fn bar2(x: &i32, y: &i32) -> Result<i32, i32> {
    Ok(x+y)
}

It fails with the following error:

error[E0226]: only a single explicit lifetime bound is permitted
  --> futures/tests/async_await/pinned.rs:16:45
   |
16 | fn bar2(x: &i32, y: &i32) -> Result<i32, i32> {
   |                                             ^

error[E0623]: lifetime mismatch
  --> futures/tests/async_await/pinned.rs:16:30
   |
16 | fn bar2(x: &i32, y: &i32) -> Result<i32, i32> {
   |                     ----     ^^^^^^^^^^^^^^^^
   |                     |        |
   |                     |        ...but data from `y` is returned here
   |                     this parameter and the return type are declared with different lifetimes...

EDIT:
I just realized it's not working with #[async], it might be independent from this.
When using #[async] instead of #[async(pinned)] the first error disappears but the second stays.

EDIT 2:
If I explicitly specify the same lifetime for both arguments then both the pinned and normal version seem to work.

@withoutboats
Copy link

@ngg's first error is because Box<Trait + 'a + 'b> isn't valid syntax. In contrast, impl Trait + 'a + 'b is valid syntax, but it doesn't work. (This seems pretty bad). The second error is the result of neither working, even if syntactically its allowed.

The expansion of lifetimes needs to be changed to instead of just adding + 'a + 'b ... to the return type, instead adding an additional lifetime and bounding every lifetime by that. So the expansion changes:

#[async] fn foo(x: &i32, y: &i32) -> Result<i32, !> { ... }

// currently:
fn foo<'_async0, '_async1>(x: &'_async0 i32, y: &'_async1 i32)
    -> impl Future<Item = i32, Error = !> + '_async0 + '_async1 { ... }

// changed to:
fn foo<'_ret, '_async0: '_ret, '_async1: '_ret>(x: &'_async0, y: &'_async1 i32)
    -> impl Future<Item = i32, Error = !> + '_ret { ... }

@withoutboats
Copy link

Its a separate issue from this PR though!

@Nemo157
Copy link
Member

Nemo157 commented Apr 3, 2018

See rust-lang/rust#49431 for a rust issue about impl Trait + 'a + 'b.

@ngg
Copy link
Contributor

ngg commented Apr 3, 2018

Yeah I realized it's different but only after writing this here. (I wrongly remembered that it was working with normal #[async], and I tried it only with #[async(pinned)]).
Even if rust-lang/rust#49431 will be fixed it won't work with pinned as currently Box<T + 'a + 'b> and PinBox<T + 'a + 'b> are not permitted either.

Copy link
Contributor Author

@raviqqe raviqqe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did any collaborators review @ngg's #932 which looks cleaner than mine?

@@ -264,6 +264,15 @@ pub mod prelude {
#[cfg(feature = "std")]
pub use futures_core::executor::Executor;

#[cfg(feature = "nightly")]
pub use futures_stable::{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to put them into futures::prelude as normal Future and Stream so that users don't need to use them every time in modules which use #[async] stuff otherwise they'll see some error messages about missing methods of pin() or pin_local().

@raviqqe raviqqe force-pushed the pinned-box-async branch 3 times, most recently from 0258ca7 to 9f5acf4 Compare April 4, 2018 01:25
@ngg
Copy link
Contributor

ngg commented Apr 4, 2018

@raviqqe I reverted it pretty fast when I realized you already did it and that Nemo157@5c32c05 is based on your version. I actually prefer your solution with not creating gen_pinbox and gen_pinbox_local, but calling pin() and pin_local() inline, the other differences are quite irrelevant after #918.

@raviqqe raviqqe force-pushed the pinned-box-async branch from 9f5acf4 to 7d5a4c6 Compare April 5, 2018 19:45
@withoutboats
Copy link

Will merge if tests pass, thanks for this!

@withoutboats
Copy link

one failure, missing spawn_pinned method

@withoutboats withoutboats merged commit 6c0f224 into rust-lang:master Apr 12, 2018
@withoutboats
Copy link

Merged, thanks so much for doing this! 🎉

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