[wg-async-await] Drop `async fn` arguments in async block #59823

davidtwco · 2019-04-09T19:31:35Z

This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body.

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

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

However, the exact ordering of drops is not the same as a regular function, as visible in this playground example - I believe this to be an unrelated issue. There is a Zulip topic for this.

r? @cramertj
cc @nikomatsakis

davidtwco · 2019-04-21T01:08:17Z

I believe I've resolved the test failures with this PR.

This tests that async functions drop parameters in the same order as regular functions.

This will be used to keep track of the origin of a local in the AST. In particular, it will be used by `async fn` lowering for the locals in `let <pat>: <ty> = __arg0;` statements.

davidtwco · 2019-04-21T11:03:33Z

I've updated the test to match what Travis expects, however it isn't the same as I've been seeing locally.

This commit adds an `AsyncArgument` struct to the AST that contains the generated argument and statement that will be used in HIR lowering, name resolution and def collection.

This commit takes advantage of `AsyncArgument` type that was added in a previous commit to replace the arguments of the `async fn` in the HIR and add statements to move the bindings from the new arguments to the pattern from the old argument. For example, the async function `foo` below: async fn foo((x, _y): (T, V)) { async move { } } becomes: async fn foo(__arg0: (T, V)) { async move { let (x, _y) = __arg0; } }

This commit extends the previous commit to apply to trait methods as well as free functions.

This avoids issues with `impl_trait_in_bindings` as the type from the argument is normally used as the let binding, but `impl Trait` is unstable in binding position.

This commit introduces an `ArgSource` enum that is lowered into the HIR so that diagnostics can correctly refer to the argument pattern's original name rather than the generated pattern.

This commit changes the order that arguments and bodies of async functions are lowered so that when the body attempts to `lower_def` of a upvar then the id has already been assigned by lowering the argument first.

This commit displays the original pattern in generated documentation for async functions rather than the synthesized pattern.

src/test/run-pass/issue-54716.rs

cramertj · 2019-04-23T16:39:40Z

r=me with test fixups. This looks great, thanks so much!

This commit introduces a `assert_drop_order_after_poll` helper function to the test case for this case to reduce repetitive noise and documents what each function aims to test.

cramertj · 2019-04-23T17:53:14Z

@bors r+

bors · 2019-04-23T17:53:16Z

📌 Commit 119e67a has been approved by cramertj

@cramertj

[wg-async-await] Drop `async fn` arguments in async block Fixes rust-lang#54716. This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body. ``` async fn foo(<pattern>: <type>) { async move { } } // <-- dropped as you "exit" the fn // ...becomes... fn foo(__arg0: <ty>) { async move { let <pattern>: <ty> = __arg0; } // <-- dropped as you "exit" the async block } ``` However, the exact ordering of drops is not the same as a regular function, [as visible in this playground example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=be39af1a58e5d430be1eb3c722cb1ec3) - I believe this to be an unrelated issue. There is a [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/187312-t-compiler.2Fwg-async-await/topic/.2354716.20drop.20order) for this. r? @cramertj cc @nikomatsakis

bors · 2019-04-23T19:50:35Z

⌛ Testing commit 119e67a with merge 658f439060c837d65724c5902d1f6b3e89239214...

@cramertj

[wg-async-await] Drop `async fn` arguments in async block Fixes rust-lang#54716. This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body. ``` async fn foo(<pattern>: <type>) { async move { } } // <-- dropped as you "exit" the fn // ...becomes... fn foo(__arg0: <ty>) { async move { let <pattern>: <ty> = __arg0; } // <-- dropped as you "exit" the async block } ``` However, the exact ordering of drops is not the same as a regular function, [as visible in this playground example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=be39af1a58e5d430be1eb3c722cb1ec3) - I believe this to be an unrelated issue. There is a [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/187312-t-compiler.2Fwg-async-await/topic/.2354716.20drop.20order) for this. r? @cramertj cc @nikomatsakis

@cramertj

[wg-async-await] Drop `async fn` arguments in async block Fixes rust-lang#54716. This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body. ``` async fn foo(<pattern>: <type>) { async move { } } // <-- dropped as you "exit" the fn // ...becomes... fn foo(__arg0: <ty>) { async move { let <pattern>: <ty> = __arg0; } // <-- dropped as you "exit" the async block } ``` However, the exact ordering of drops is not the same as a regular function, [as visible in this playground example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=be39af1a58e5d430be1eb3c722cb1ec3) - I believe this to be an unrelated issue. There is a [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/187312-t-compiler.2Fwg-async-await/topic/.2354716.20drop.20order) for this. r? @cramertj cc @nikomatsakis

Centril · 2019-04-23T19:52:01Z

@bors retry

@ghost

Rollup of 6 pull requests Successful merges: - #59823 ([wg-async-await] Drop `async fn` arguments in async block ) - #59839 (Warn on unused results for operation methods on nums) - #60146 (Update fonts used by rustdoc) - #60169 (Warn when ignore-tidy-linelength is present, but no lines are too long) - #60177 (Promote rust comments to rustdoc) - #60191 (Add f16c target_feature) Failed merges: r? @ghost

Here follows the main reverts applied in order to make this commit: Revert "Rollup merge of rust-lang#60676 - davidtwco:issue-60674, r=cramertj" This reverts commit 45b0945, reversing changes made to f6df1f6. Revert "Rollup merge of rust-lang#60437 - davidtwco:issue-60236, r=nikomatsakis" This reverts commit 16939a5, reversing changes made to 12bf981. Revert "Rollup merge of rust-lang#59823 - davidtwco:issue-54716, r=cramertj" This reverts commit 62d1574, reversing changes made to 4eff852.

rust-highfive assigned cramertj Apr 9, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2019

davidtwco mentioned this pull request Apr 9, 2019

[wg-async-await] Drop async fn arguments in async block #59135

Closed

This comment has been minimized.

Sign in to view

davidtwco force-pushed the issue-54716 branch from 6307521 to 414e269 Compare April 21, 2019 01:08

This comment has been minimized.

Sign in to view

davidtwco added 2 commits April 21, 2019 09:48

Add test for drop order in async functions.

8b57be1

This tests that async functions drop parameters in the same order as regular functions.

Introduce LocalSource into the AST.

41c6bb1

This will be used to keep track of the origin of a local in the AST. In particular, it will be used by `async fn` lowering for the locals in `let <pat>: <ty> = __arg0;` statements.

davidtwco force-pushed the issue-54716 branch from 414e269 to ec85d21 Compare April 21, 2019 09:25

This comment has been minimized.

Sign in to view

davidtwco force-pushed the issue-54716 branch from ec85d21 to 785fe9f Compare April 21, 2019 11:01

This comment has been minimized.

Sign in to view

davidtwco added 7 commits April 21, 2019 16:46

Add AsyncArgument to AST.

879abb1

This commit adds an `AsyncArgument` struct to the AST that contains the generated argument and statement that will be used in HIR lowering, name resolution and def collection.

Enforce consistent drop order w/ async methods.

6134655

This commit extends the previous commit to apply to trait methods as well as free functions.

Do not specify type in generated let bindings.

92e72df

This avoids issues with `impl_trait_in_bindings` as the type from the argument is normally used as the let binding, but `impl Trait` is unstable in binding position.

Introduce ArgSource for diagnostics.

9d7da82

This commit introduces an `ArgSource` enum that is lowered into the HIR so that diagnostics can correctly refer to the argument pattern's original name rather than the generated pattern.

Correct lowering order to avoid ICE after rebase.

44ddbc5

This commit changes the order that arguments and bodies of async functions are lowered so that when the body attempts to `lower_def` of a upvar then the id has already been assigned by lowering the argument first.

Display original pattern in rustdoc.

09c707f

This commit displays the original pattern in generated documentation for async functions rather than the synthesized pattern.

davidtwco force-pushed the issue-54716 branch from ae4ee36 to 09c707f Compare April 21, 2019 15:46

cramertj reviewed Apr 23, 2019

View reviewed changes

src/test/run-pass/issue-54716.rs Outdated Show resolved Hide resolved

cramertj reviewed Apr 23, 2019

View reviewed changes

src/test/run-pass/issue-54716.rs Outdated Show resolved Hide resolved

Reduce noise and document test case.

119e67a

This commit introduces a `assert_drop_order_after_poll` helper function to the test case for this case to reduce repetitive noise and documents what each function aims to test.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2019

Centril mentioned this pull request Apr 23, 2019

Rollup of 7 pull requests #60206

Closed

Centril mentioned this pull request Apr 23, 2019

Rollup of 6 pull requests #60211

Merged

Centril mentioned this pull request Apr 23, 2019

Rollup of 6 pull requests #60212

Closed

rust-lang deleted a comment from rust-highfive Apr 23, 2019

bors merged commit 119e67a into rust-lang:master Apr 23, 2019

davidtwco mentioned this pull request Apr 24, 2019

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

Closed

davidtwco deleted the issue-54716 branch April 24, 2019 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wg-async-await] Drop `async fn` arguments in async block #59823

[wg-async-await] Drop `async fn` arguments in async block #59823

davidtwco commented Apr 9, 2019

This comment has been minimized.

davidtwco commented Apr 21, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

davidtwco commented Apr 21, 2019

This comment has been minimized.

This comment has been minimized.

cramertj commented Apr 23, 2019

cramertj commented Apr 23, 2019

bors commented Apr 23, 2019

bors commented Apr 23, 2019

Centril commented Apr 23, 2019

[wg-async-await] Drop async fn arguments in async block #59823

[wg-async-await] Drop async fn arguments in async block #59823

Conversation

davidtwco commented Apr 9, 2019

This comment has been minimized.

davidtwco commented Apr 21, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

davidtwco commented Apr 21, 2019

This comment has been minimized.

This comment has been minimized.

cramertj commented Apr 23, 2019

cramertj commented Apr 23, 2019

bors commented Apr 23, 2019

bors commented Apr 23, 2019

Centril commented Apr 23, 2019

[wg-async-await] Drop `async fn` arguments in async block #59823

[wg-async-await] Drop `async fn` arguments in async block #59823