Skip to content
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

Drop order of async fn arguments doesn't exactly match the equivalent regular fn. #60236

Closed
davidtwco opened this issue Apr 24, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@davidtwco
Copy link
Member

commented Apr 24, 2019

While #59823 resolved async fn arguments being dropped before the function was polled, the resulting drop order still does not exactly match the equivalent fn.

This is visible in this playground example which compares the drop order of four functions with and without fn and async fn. This is a behavior of closures which can be seen in this playground example which approximates the desugaring of async fns.

@Centril

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

So in my view, this should be considered blocking for the same reason as the other drop issue. However, if it comes to it and we cannot fix it in time for stabilizing, we can feature gate this part and move on.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

The only argument I can come up with in favor of this drop order is basically:

  • the existing drop order of fn is not what we would have wanted (I believe this, but I also doubt its worth trying to change it)
  • having a simple desugaring for async fn is important

You might make a case that, because people sometimes have to manually desugar into a fn that returns an async block for various reasons, having a simple transformation is important.

But I think this whole case sounds a bit weak to me -- in contrast, matching fn and async fn as closely as possible feels like what would people would expect

Of course I also think that the drop order between fn parameters, particularly when intermingled with complex patterns, is highly unlikely to matter, but who's to say what people ultimately rely on.

Still, this implies that most of the time, people can desugar any which way to use an async move block and it will be fine.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

I'm going to mark this as blocking for now.

@davidtwco davidtwco self-assigned this Apr 30, 2019

Centril added a commit to Centril/rust that referenced this issue May 1, 2019

Rollup merge of rust-lang#60437 - davidtwco:issue-60236, r=nikomatsakis
Ensure that drop order of `async fn` matches `fn` and that users cannot refer to generated arguments.

Fixes rust-lang#60236 and fixes rust-lang#60438.

This PR modifies the lowering of `async fn` arguments so that the
drop order matches the equivalent `fn`.

Previously, async function arguments were lowered as shown below:

    async fn foo(<pattern>: <ty>) {
      async move {
      }
    } // <-- dropped as you "exit" the fn

    // ...becomes...
    fn foo(__arg0: <ty>) {
      async move {
        let <pattern> = __arg0;
      } // <-- dropped as you "exit" the async block
    }

After this PR, async function arguments will be lowered as:

    async fn foo(<pattern>: <ty>, <pattern>: <ty>, <pattern>: <ty>) {
      async move {
      }
    } // <-- dropped as you "exit" the fn

    // ...becomes...
    fn foo(__arg0: <ty>, __arg1: <ty>, __arg2: <ty>) {
      async move {
        let __arg2 = __arg2;
        let <pattern> = __arg2;
        let __arg1 = __arg1;
        let <pattern> = __arg1;
        let __arg0 = __arg0;
        let <pattern> = __arg0;
      } // <-- dropped as you "exit" the async block
    }

If `<pattern>` is a simple ident, then it is lowered to a single
`let <pattern> = <pattern>;` statement as an optimization.

This PR also stops users from referring to the generated `__argN`
identifiers.

r? @nikomatsakis

@bors bors closed this in #60437 May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.