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

Correct handling of arguments in async fn #60535

Merged
merged 2 commits into from
May 7, 2019

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented May 4, 2019

Fixes #60509
Fixes #60566

r? @cramertj or @davidtwco

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2019
Centril
Centril previously requested changes May 4, 2019
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented May 4, 2019

cc @matthewjasper @eddyb -- Can you please double check this?

@@ -8712,22 +8713,43 @@ impl<'a> Parser<'a> {
///
/// The arguments of the function are replaced in HIR lowering with the arguments created by
/// this function and the statements created here are inserted at the top of the closure body.
fn construct_async_arguments(&mut self, asyncness: &mut Spanned<IsAsync>, decl: &FnDecl) {
fn construct_async_arguments(&mut self, asyncness: &mut Spanned<IsAsync>, decl: &mut FnDecl) {
Copy link
Member

Choose a reason for hiding this comment

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

@davidtwco This is the hack that we'd want to move to HIR lowering but we can't because of rustc_resolve interacting with upvars, right?

Copy link
Member

Choose a reason for hiding this comment

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

@eddyb Yeah, we'll presumably want to lower to the same HIR in the end. Because of the interaction with rustc_resolve, it's easier to insert the statements/replace the arguments in the AST during name resolution so it can make the correct locals resolve to Def::Upvar. If we didn't construct them here, then these same types would need to be constructed in rustc_resolve and in HIR lowering.

Other places, like the def collector, that use these constructed types would be able to get away with just using NodeIds that we'd create here (much like the IsAsync type worked previously), and we'd only construct the actual statements and arguments during HIR lowering.

@taiki-e
Copy link
Member Author

taiki-e commented May 6, 2019

by-ref binding:

async fn foo(ref mut x: A) { ... }

// desugaring (current master)
fn foo(ref x: A) -> ... { //~ ERROR temporary value dropped while borrowed [E0716]
    async move {
        let ref x = x;
        ...
    }
}

// desugaring (this PR)
fn foo(__arg0: A) -> ... {
    async move {
        let mut __arg0 = __arg0; // here always mutable binding
        let ref x = __arg0;
        ...
    }
}

by-value binding:

async fn foo(mut x: A) { ... }

// desugaring (current master)
fn foo(mut x: A) -> ... { //~ WARNING variable does not need to be mutable
    async move {
        let mut x = x;
        ...
    }
}

// desugaring (this PR)
fn foo(x: A) -> ... { // removed mutability
    async move {
        let mut x = x;
        ...
    }
}

other patterns:

async fn foo((mut x, y): (A, A)) { ... }

// desugaring (current master)
fn foo(__arg0: (A, A)) -> ... {
    async move {
        let __arg0 = __arg0;
        let (mut x, y) = __arg0; //~ ERROR cannot borrow `__arg0.0` as mutable, as `__arg0` is not declared as mutable [E0596]
        ...
    }
}

// desugaring (this PR), it is handled in the same way as by-ref binding.
fn foo(__arg0: A) -> ... {
    async move {
        let mut __arg0 = __arg0; // here always mutable binding
        let (mut x, y) = __arg0;
        ...
    }
}

@taiki-e
Copy link
Member Author

taiki-e commented May 6, 2019

I somehow forgot to write this, sorry.

@taiki-e
Copy link
Member Author

taiki-e commented May 6, 2019

The reason by-ref binding isn't be desugared like the following is that it breaks the drop order (57ec63c is the test used when this was checked locally. Also this is the branch used when I checked it: taiki-e@2cb3031).

fn foo(x: A) -> ... {
    async move {
        let ref x = x;
        ...
    }
}

@cramertj
Copy link
Member

cramertj commented May 7, 2019

@bors r+

@bors
Copy link
Contributor

bors commented May 7, 2019

📌 Commit 57ec63c has been approved by cramertj

@bors 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 May 7, 2019
Centril added a commit to Centril/rust that referenced this pull request May 7, 2019
bors added a commit that referenced this pull request May 7, 2019
Rollup of 5 pull requests

Successful merges:

 - #60489 (Remove hamburger button from source code page)
 - #60535 (Correct handling of arguments in async fn)
 - #60579 (Rename `ParamTy::idx` to `ParamTy::index`)
 - #60583 (Fix parsing issue with negative literals as const generic arguments)
 - #60609 (Be a bit more explicit asserting over the vec rather than the len)

Failed merges:

r? @ghost
@bors bors merged commit 57ec63c into rust-lang:master May 7, 2019
@taiki-e taiki-e deleted the async-fn-arguments branch May 7, 2019 22:34
Centril added a commit to Centril/rust that referenced this pull request May 9, 2019
Fix async desugaring providing wrong input to procedural macros.

Fixes rust-lang#60674.

This PR fixes a minor oversight introduced by rust-lang#60535 where unused `mut` binding modes were removed from the arguments to an `async fn` (as they were added to the statement that we insert into the closure body). However, this meant that the input to procedural macros was incorrect. This removes that and instead fixes the `unused_mut` error that it avoided.

r? @cramertj
cc @taiki-e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
8 participants