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

Re-implement async fn drop order lowering #61413

Merged
merged 7 commits into from Jun 4, 2019

Conversation

Projects
None yet
6 participants
@davidtwco
Copy link
Member

commented May 31, 2019

This PR re-implements the async fn drop order lowering changes so
that it all takes place in HIR lowering, building atop the work done by
@eddyb to refactor Res::Upvar.

Previously, this types involved in the lowering were constructed in
libsyntax as they had to be used during name resolution and HIR
lowering. This was awful because none of that logic should have existed
in libsyntax.

This commit also changes ArgSource to keep a HirId to the original
argument pattern rather than a cloned copy of the pattern.

Only b7aa4ed and 71fb8fa should be reviewed, any other commits
are from #61276 (though 447e336 might end up staying in this PR).

As a nice side effect, it also fixes #61187 (cc #61192).

r? @eddyb
cc @cramertj

@davidtwco davidtwco force-pushed the davidtwco:async-argument-order-in-a-sane-way branch from 5b87188 to b91cfd9 May 31, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

☔️ The latest upstream changes (presumably #61394) made this pull request unmergeable. Please resolve the merge conflicts.

@davidtwco

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

I'll hold off on rebasing this until #61276 lands or makes significant changes.

Show resolved Hide resolved src/librustc_mir/build/mod.rs Outdated
Show resolved Hide resolved src/librustdoc/clean/mod.rs Outdated
Show resolved Hide resolved src/librustc/hir/lowering.rs Outdated

@davidtwco davidtwco force-pushed the davidtwco:async-argument-order-in-a-sane-way branch 2 times, most recently from abf5e4a to c45b54b Jun 2, 2019

@@ -30,7 +30,7 @@ error[E0106]: missing lifetime specifier
LL | fn foo2(_: &'_ u8, y: &'_ u8) -> &'_ u8 { y }
| ^^ expected lifetime parameter
|
= help: this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from `_` or `y`
= help: this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from argument 1 or `y`

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 2, 2019

Member

@estebank Just to check with you, this is an improvement, right?

This comment has been minimized.

Copy link
@estebank

estebank Jun 3, 2019

Contributor

@eddyb It'd might be even better if we added spans to it, like below, but this is an improvement in my eyes

error[E0106]: missing lifetime specifier
  --> $DIR/underscore-lifetime-binders.rs:16:35
   |
LL | fn foo2(_: &'_ u8, y: &'_ u8) -> &'_ u8 { y }
   |                                   ^^ expected lifetime parameter
   |
help: this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from argument 1 or `y`
   |
LL | fn foo2(_: &'_ u8, y: &'_ u8) -> &'_ u8 { y }
   |         ^          ^

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 3, 2019

Member

It would be even better if it pointed at the lifetimes in the signature (which is information we could collect).

This comment has been minimized.

Copy link
@davidtwco

davidtwco Jun 3, 2019

Author Member

I think it might be best if we left this as a follow-up (it's a bit out of scope for this PR). I'd be happy to do it after this though.

@eddyb

eddyb approved these changes Jun 2, 2019

Copy link
Member

left a comment

r=me with MIR building changes backed out, or after someone else checks off on them

Show resolved Hide resolved src/librustc/hir/lowering.rs Outdated
Show resolved Hide resolved src/librustc/hir/lowering.rs Outdated
@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jun 3, 2019

☔️ The latest upstream changes (presumably #61331) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb and others added some commits May 29, 2019

syntax: revert `ast::AsyncArgument` and associated changes.
Here follows the main reverts applied in order to make this commit:

Revert "Rollup merge of #60676 - davidtwco:issue-60674, r=cramertj"

This reverts commit 45b0945, reversing
changes made to f6df1f6.

Revert "Rollup merge of #60437 - davidtwco:issue-60236, r=nikomatsakis"

This reverts commit 16939a5, reversing
changes made to 12bf981.

Revert "Rollup merge of #59823 - davidtwco:issue-54716, r=cramertj"

This reverts commit 62d1574, reversing
changes made to 4eff852.
rustc: async fn drop order lowering in HIR
This commit re-implements the async fn drop order lowering changes so
that it all takes place in HIR lowering, building atop the work done by
`@eddyb` to refactor `Res::Upvar`.

Previously, this types involved in the lowering were constructed in
libsyntax as they had to be used during name resolution and HIR
lowering. This was awful because none of that logic should have existed
in libsyntax.

This commit also changes `ArgSource` to keep a `HirId` to the original
argument pattern rather than a cloned copy of the pattern.

@davidtwco davidtwco force-pushed the davidtwco:async-argument-order-in-a-sane-way branch from c45b54b to cbcba1c Jun 3, 2019

davidtwco added some commits Jun 2, 2019

rustc: remove `HirId` from `ArgSource::AsyncFn`
This commit removes the `HirId` from `ArgSource::AsyncFn`, relying on
the fact that only `simple_ident` is used in each of the locations that
previously took the original pattern from the `ArgSource::AsyncFn`.
rustc: use lowering helpers
This commit changes the lowering to stop creating HIR statements,
expressions and patterns directly and instead uses the pre-existing
helper functions.
rustc: construct statement vector directly
This commit simplifies the previous logic to construct the statement
vector directly rather than constructing a `Vec` of
`(hir::Stmt, Option<hir::Stmt>)` first.
rustc: remove `ArgSource`
`ArgSource` is no longer used anywhere, so it can be removed.

@davidtwco davidtwco force-pushed the davidtwco:async-argument-order-in-a-sane-way branch from cbcba1c to 2bb92aa Jun 3, 2019

@eddyb

eddyb approved these changes Jun 3, 2019

@eddyb

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

📌 Commit 2bb92aa has been approved by eddyb

Centril added a commit to Centril/rust that referenced this pull request Jun 4, 2019

Rollup merge of rust-lang#61413 - davidtwco:async-argument-order-in-a…
…-sane-way, r=eddyb

Re-implement async fn drop order lowering

This PR re-implements the async fn drop order lowering changes so
that it all takes place in HIR lowering, building atop the work done by
@eddyb to refactor `Res::Upvar`.

Previously, this types involved in the lowering were constructed in
libsyntax as they had to be used during name resolution and HIR
lowering. This was awful because none of that logic should have existed
in libsyntax.

This commit also changes `ArgSource` to keep a `HirId` to the original
argument pattern rather than a cloned copy of the pattern.

Only b7aa4ed and 71fb8fa should be reviewed, any other commits
are from rust-lang#61276 (though 447e336 might end up staying in this PR).

As a nice side effect, it also fixes rust-lang#61187 (cc rust-lang#61192).

r? @eddyb
cc @cramertj

bors added a commit that referenced this pull request Jun 4, 2019

Auto merge of #61507 - Centril:rollup-hdm781j, r=Centril
Rollup of 13 pull requests

Successful merges:

 - #61373 (Emit StorageDead along unwind paths for generators)
 - #61404 (miri unsizing: fix projecting into a field of an operand)
 - #61409 (Fix an ICE with a const argument in a trait)
 - #61413 (Re-implement async fn drop order lowering )
 - #61419 (Add an unusual-conversion example to to_uppercase)
 - #61420 (Succinctify splice docs)
 - #61444 (Suggest using `as_ref` on `*const T`)
 - #61446 (On TerminatorKind::DropAndReplace still handle unused_mut correctly)
 - #61485 (azure: retry s3 upload if it fails)
 - #61489 (ci: Reenable step timings on AppVeyor)
 - #61496 (Do not panic in tidy on unbalanced parentheses in cfg's)
 - #61497 (Treat 0 as special value for codegen-units-std)
 - #61499 (Add regression test for existential type ICE #53457)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 4, 2019

Rollup merge of rust-lang#61413 - davidtwco:async-argument-order-in-a…
…-sane-way, r=eddyb

Re-implement async fn drop order lowering

This PR re-implements the async fn drop order lowering changes so
that it all takes place in HIR lowering, building atop the work done by
@eddyb to refactor `Res::Upvar`.

Previously, this types involved in the lowering were constructed in
libsyntax as they had to be used during name resolution and HIR
lowering. This was awful because none of that logic should have existed
in libsyntax.

This commit also changes `ArgSource` to keep a `HirId` to the original
argument pattern rather than a cloned copy of the pattern.

Only b7aa4ed and 71fb8fa should be reviewed, any other commits
are from rust-lang#61276 (though 447e336 might end up staying in this PR).

As a nice side effect, it also fixes rust-lang#61187 (cc rust-lang#61192).

r? @eddyb
cc @cramertj

bors added a commit that referenced this pull request Jun 4, 2019

Auto merge of #61509 - Centril:rollup-s26bbh0, r=Centril
Rollup of 14 pull requests

Successful merges:

 - #61135 (Fix documentation of `Rc::make_mut` regarding `rc::Weak`.)
 - #61404 (miri unsizing: fix projecting into a field of an operand)
 - #61409 (Fix an ICE with a const argument in a trait)
 - #61413 (Re-implement async fn drop order lowering )
 - #61419 (Add an unusual-conversion example to to_uppercase)
 - #61420 (Succinctify splice docs)
 - #61444 (Suggest using `as_ref` on `*const T`)
 - #61446 (On TerminatorKind::DropAndReplace still handle unused_mut correctly)
 - #61485 (azure: retry s3 upload if it fails)
 - #61489 (ci: Reenable step timings on AppVeyor)
 - #61494 (Utilize cfg(bootstrap) over cfg(stage0))
 - #61496 (Do not panic in tidy on unbalanced parentheses in cfg's)
 - #61497 (Treat 0 as special value for codegen-units-std)
 - #61499 (Add regression test for existential type ICE #53457)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 4, 2019

Rollup merge of rust-lang#61413 - davidtwco:async-argument-order-in-a…
…-sane-way, r=eddyb

Re-implement async fn drop order lowering

This PR re-implements the async fn drop order lowering changes so
that it all takes place in HIR lowering, building atop the work done by
@eddyb to refactor `Res::Upvar`.

Previously, this types involved in the lowering were constructed in
libsyntax as they had to be used during name resolution and HIR
lowering. This was awful because none of that logic should have existed
in libsyntax.

This commit also changes `ArgSource` to keep a `HirId` to the original
argument pattern rather than a cloned copy of the pattern.

Only b7aa4ed and 71fb8fa should be reviewed, any other commits
are from rust-lang#61276 (though 447e336 might end up staying in this PR).

As a nice side effect, it also fixes rust-lang#61187 (cc rust-lang#61192).

r? @eddyb
cc @cramertj

bors added a commit that referenced this pull request Jun 4, 2019

Auto merge of #61510 - Centril:rollup-bvi95y2, r=Centril
Rollup of 13 pull requests

Successful merges:

 - #61135 (Fix documentation of `Rc::make_mut` regarding `rc::Weak`.)
 - #61404 (miri unsizing: fix projecting into a field of an operand)
 - #61409 (Fix an ICE with a const argument in a trait)
 - #61413 (Re-implement async fn drop order lowering )
 - #61419 (Add an unusual-conversion example to to_uppercase)
 - #61420 (Succinctify splice docs)
 - #61444 (Suggest using `as_ref` on `*const T`)
 - #61446 (On TerminatorKind::DropAndReplace still handle unused_mut correctly)
 - #61485 (azure: retry s3 upload if it fails)
 - #61489 (ci: Reenable step timings on AppVeyor)
 - #61496 (Do not panic in tidy on unbalanced parentheses in cfg's)
 - #61497 (Treat 0 as special value for codegen-units-std)
 - #61499 (Add regression test for existential type ICE #53457)

Failed merges:

r? @ghost

@bors bors merged commit 2bb92aa into rust-lang:master Jun 4, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@davidtwco davidtwco deleted the davidtwco:async-argument-order-in-a-sane-way branch Jun 4, 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.