-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rustc: remove Res::Upvar. #61276
rustc: remove Res::Upvar. #61276
Conversation
@bors try |
⌛ Trying commit 5934fcc73a9c9950978089c185877fbc85ea9b4b with merge 65978d81bc091c03518ff224ee97411cb6091b4d... |
☀️ Try build successful - checks-travis |
@rust-timer build 65978d81bc091c03518ff224ee97411cb6091b4d |
Success: Queued 65978d81bc091c03518ff224ee97411cb6091b4d with parent 7da1185, comparison URL. |
Finished benchmarking try commit 65978d81bc091c03518ff224ee97411cb6091b4d, comparison URL. |
Looks good! |
Looks like noise to me. Also, I've pushed the HIR-based upvar collection. |
☔ The latest upstream changes (presumably #61305) made this pull request unmergeable. Please resolve the merge conflicts. |
(You can ignore test failures, I just wanted to push my progress in moving the |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@@ -1183,6 +1183,11 @@ dependencies = [ | |||
"typenum 1.10.0 (registry+https://github.com/rust-lang/crates.io-index)", | |||
] | |||
|
|||
[[package]] | |||
name = "indexmap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this data structure have any "classic" name?
Search by "IndexMap" only leads to the Rust version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know (for the record, this crate used to be called ordermap
). cc @bluss
src/libsyntax/parse/parser.rs
Outdated
/// | ||
/// 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: &mut FnDecl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this removed.
I somehow missed the moment when all this out-of-place stuff was added to the parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replacement in HIR lowering is not done yet, so if you want I can back out the async changes and do that in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me for the part before async then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've submitted #61413 that builds upon this and implements the async part.
I didn't look at the async parts very carefully, perhaps some other reviewer would be more suitable for those. |
This comment has been minimized.
This comment has been minimized.
@bors retry collection-tests... |
⌛ Testing commit f7a4c9d with merge 044fb846ee76c0a864e75d5ae20b6ca398b119be... |
💔 Test failed - status-appveyor |
@bors retry (spurious Windows link failure?) |
rustc: remove Res::Upvar. By keeping track of the current "`body_owner`" (the `DefId` of the current fn/closure/const/etc.) in several passes, `Res::Upvar` and `hir::Upvar` don't need to contain contextual information about the closure. By leveraging [`indexmap`](https://docs.rs/indexmap), the list of upvars for a given closure can now also be queried, to check whether a local variable is a closure capture, and so `Res::Upvar` can be merged with `Res::Local`. And finally, the `tcx.upvars(...)` query now collects upvars from HIR, without relying on `rustc_resolve`. r? @petrochenkov cc @varkor @davidtwco
☀️ Test successful - checks-travis, status-appveyor |
📣 Toolstate changed by #61276! Tested on commit 4c7bb8b. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). |
Tested on commit rust-lang/rust@4c7bb8b. Direct link to PR: <rust-lang/rust#61276> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @Xanewok, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @Xanewok, @rust-lang/infra).
Rustup for rust-lang/rust#61276 changelog: none
Rustup for rust-lang/rust#61276 changelog: none
…-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
…-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
…-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
By keeping track of the current "
body_owner
" (theDefId
of the current fn/closure/const/etc.) in several passes,Res::Upvar
andhir::Upvar
don't need to contain contextual information about the closure.By leveraging
indexmap
, the list of upvars for a given closure can now also be queried, to check whether a local variable is a closure capture, and soRes::Upvar
can be merged withRes::Local
.And finally, the
tcx.upvars(...)
query now collects upvars from HIR, without relying onrustc_resolve
.r? @petrochenkov cc @varkor @davidtwco