Re-implement async fn drop order lowering #61413

davidtwco · 2019-05-31T21:01:23Z

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

src/librustc/hir/lowering.rs

bors · 2019-06-01T04:01:33Z

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

davidtwco · 2019-06-01T10:44:57Z

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

src/librustc/infer/error_reporting/nice_region_error/different_lifetimes.rs

src/librustc/infer/error_reporting/nice_region_error/named_anon_conflict.rs

src/librustc/middle/resolve_lifetime.rs

src/librustc_mir/build/mod.rs

src/librustdoc/clean/mod.rs

src/librustc/hir/lowering.rs

src/librustc_mir/build/mod.rs

eddyb · 2019-06-02T20:22:23Z

src/test/ui/underscore-lifetime/underscore-lifetime-binders.stderr

@@ -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`


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

@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 } | ^ ^

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

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

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

src/librustc/hir/lowering.rs

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.

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.

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`.

This commit changes the lowering to stop creating HIR statements, expressions and patterns directly and instead uses the pre-existing helper functions.

This commit simplifies the previous logic to construct the statement vector directly rather than constructing a `Vec` of `(hir::Stmt, Option<hir::Stmt>)` first.

`ArgSource` is no longer used anywhere, so it can be removed.

eddyb · 2019-06-03T20:27:35Z

@bors r+

bors · 2019-06-03T20:27:36Z

📌 Commit 2bb92aa has been approved by eddyb

@eddyb

…-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

@eddyb

…-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

@eddyb

…-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

@ghost

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

rust-highfive assigned eddyb May 31, 2019

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

This was referenced May 31, 2019

rustc: remove Res::Upvar. #61276

Merged

Do not ICE on missing access place description during mutability error reporting #61192

Closed

matthewjasper reviewed May 31, 2019

View reviewed changes

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

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