-
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
Lowering cleanups [1/N] #51806
Lowering cleanups [1/N] #51806
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
r? @cramertj I touched a lot of the async lowering code |
This comment has been minimized.
This comment has been minimized.
src/librustc/hir/map/collector.rs
Outdated
let node_str = match self.definitions.opt_def_index(id) { | ||
Some(def_index) => { | ||
self.definitions.def_path(def_index).to_string_no_crate() | ||
} | ||
None => format!("{:?}", node) | ||
}; | ||
|
||
if hir_id == ::hir::DUMMY_HIR_ID { | ||
println!("Maybe you forgot to lower the node id {:?}?", id); |
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.
Should this be bug!
?
☔ The latest upstream changes (presumably #51805) made this pull request unmergeable. Please resolve the merge conflicts. |
1ebe798
to
f68b1e7
Compare
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 |
src/librustc/hir/lowering.rs
Outdated
self.collect_elided_lifetimes = old_collect_elided_lifetimes; | ||
} else { | ||
hir::intravisit::walk_ty(self, t); | ||
match t.node { |
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.
Nit: why switch this to match
?
src/librustc/hir/lowering.rs
Outdated
// impl_trait_return_allow: determines whether impl Trait can be used in return position. | ||
// This guards against trait declarations and implementations where impl Trait is | ||
// disallowed. | ||
// make_ret_async: if enabled, converts `-> T` into `-> impl Future<Output = T>` in the | ||
// make_ret_async: if `Some`, converts `-> T` into `-> impl Future<Output = T>` in the |
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.
Can you leave a comment about what the NodeId
is used for? At first glance I expected it to be the closure_node_id that is stored in IsAsync
, and was surprised that it was actually for the impl Trait
.
src/librustc/hir/lowering.rs
Outdated
@@ -3021,6 +3016,11 @@ impl<'a> LoweringContext<'a> { | |||
}); | |||
let impl_trait_return_allow = !self.is_in_trait_impl; | |||
|
|||
let asyncness = match sig.header.asyncness { |
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 see this copy-pasted a few places-- can you break this out into a return_impl_trait_id
method that returns an Option<NodeId>
?
src/librustc/hir/lowering.rs
Outdated
ItemKind::MacroDef(..) => SmallVector::new(), | ||
ItemKind::Fn(ref decl, ref header, ..) => { | ||
let mut ids = SmallVector::one(hir::ItemId { id: i.id }); | ||
if let IsAsync::Async { return_impl_trait_id, .. } = header.asyncness { |
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.
Nit: this async impl trait id addition is copy-pasted below-- I feel like it belongs as part of lower_impl_trait_ids
for the function in question. It's unfortunate that async
means that the FnDecl
on its own isn't enough to see all of the impl Trait
instances-- perhaps have lower_impl_trait_ids
also accept the FnHeader
, or maybe just the IsAsync
?
@@ -77,6 +77,7 @@ impl<'a> DefCollector<'a> { | |||
&mut self, |
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.
Nice-- I'm glad to see these moved to def_collector
.
src/librustc/hir/intravisit.rs
Outdated
visitor.visit_def_mention(Def::Existential(def_id)); | ||
visitor.visit_nested_item(item_id); |
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.
Can you leave a comment here explaining that we don't visit the impl Trait
ID here because it's collected by lower_item_id
for the function itself? (here)
This is great! r=me with nits addressed. |
☔ The latest upstream changes (presumably #51773) made this pull request unmergeable. Please resolve the merge conflicts. |
1fc4518
to
f21a98d
Compare
@bors r=cramertj |
📌 Commit f21a98d has been approved by |
@bors r=cramertj |
📌 Commit 99575b5 has been approved by |
⌛ Testing commit 99575b5 with merge 8edb2c121e01c78b5a8d52fb411b108cdf964e24... |
💔 Test failed - status-travis |
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 |
1 similar comment
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 |
visitor.visit_def_mention(Def::Existential(def_id)); | ||
visitor.visit_nested_item(item_id); |
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.
Hang on, if the NodeId
is pointing to the same thing as the DefId
, why do we keep both?!
We only really need the DefId
for most of the compilation, AFAIK, and the occasional need to get the NodeId
from the DefId
shouldn't be enough to keep the NodeId
in the HIR.
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 have further refactorings planned, this entire variant is getting removed ASAP, so I didn't feel very motivated to make it idiomatic
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.
Oh, huh, are you going with a fake path?
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.
Yea, not very fake though. The items are already real and in the parent of the function.
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.
Yeah, I mean just not part of the original AST. Seems good!
☀️ Test successful - status-appveyor, status-travis |
Lowering cleanups [3/N] Needs #51806 to be merged first
No description provided.