-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Merged
Merged
Lowering cleanups [1/N] #51806
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
bdcace0
Cleanup in preparation of generic extraction
oli-obk b2e2c32
Generate the `NodeId` for `existential type` in the AST
oli-obk f8e83a6
Don't generate a new NodeId for universal impl Trait
oli-obk 013fca8
Generate `DefId`s for impl Trait in the def_collector
oli-obk 1b20242
fixup
oli-obk 2ec5eab
Also place method impl trait into the surrounding module
oli-obk 9eb7561
Generate `DefId`s for the impl trait of `async` functions
oli-obk 62a5610
Don't generate `DefId`s for impl trait in trait methods
oli-obk d5efa96
Update tests
oli-obk 37cc714
Don't use `println` in the compiler
oli-obk 0e775a3
Don't visit the `impl Trait` item twice
oli-obk eb9043b
Fix rebase fallout
oli-obk a1f6a61
Undo if let -> match conversion
oli-obk a85b279
Document the `make_ret_async` argument's `NodeId`
oli-obk 5bd9eaa
Add a convenience method for getting the impl Trait `NodeId` of an `I…
oli-obk 98a48fd
Deduplicate async's impl Trait id lowering
oli-obk f21a98d
Explain the lack of item recursion
oli-obk 99575b5
Update ui tests
oli-obk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,7 @@ impl<'a> DefCollector<'a> { | |
&mut self, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice-- I'm glad to see these moved to |
||
id: NodeId, | ||
async_node_id: NodeId, | ||
return_impl_trait_id: NodeId, | ||
name: Name, | ||
span: Span, | ||
visit_fn: impl FnOnce(&mut DefCollector<'a>) | ||
|
@@ -86,6 +87,7 @@ impl<'a> DefCollector<'a> { | |
let fn_def_data = DefPathData::ValueNs(name.as_interned_str()); | ||
let fn_def = self.create_def(id, fn_def_data, ITEM_LIKE_SPACE, span); | ||
return self.with_parent(fn_def, |this| { | ||
this.create_def(return_impl_trait_id, DefPathData::ImplTrait, REGULAR_SPACE, span); | ||
let closure_def = this.create_def(async_node_id, | ||
DefPathData::ClosureExpr, | ||
REGULAR_SPACE, | ||
|
@@ -120,10 +122,14 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> { | |
ItemKind::Mod(..) if i.ident == keywords::Invalid.ident() => { | ||
return visit::walk_item(self, i); | ||
} | ||
ItemKind::Fn(_, FnHeader { asyncness: IsAsync::Async(async_node_id), .. }, ..) => { | ||
ItemKind::Fn(_, FnHeader { asyncness: IsAsync::Async { | ||
closure_id, | ||
return_impl_trait_id, | ||
}, .. }, ..) => { | ||
return self.visit_async_fn( | ||
i.id, | ||
async_node_id, | ||
closure_id, | ||
return_impl_trait_id, | ||
i.ident.name, | ||
i.span, | ||
|this| visit::walk_item(this, i) | ||
|
@@ -228,11 +234,15 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> { | |
fn visit_impl_item(&mut self, ii: &'a ImplItem) { | ||
let def_data = match ii.node { | ||
ImplItemKind::Method(MethodSig { | ||
header: FnHeader { asyncness: IsAsync::Async(async_node_id), .. }, .. | ||
header: FnHeader { asyncness: IsAsync::Async { | ||
closure_id, | ||
return_impl_trait_id, | ||
}, .. }, .. | ||
}, ..) => { | ||
return self.visit_async_fn( | ||
ii.id, | ||
async_node_id, | ||
closure_id, | ||
return_impl_trait_id, | ||
ii.ident.name, | ||
ii.span, | ||
|this| visit::walk_impl_item(this, ii) | ||
|
@@ -277,8 +287,8 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> { | |
|
||
// Async closures desugar to closures inside of closures, so | ||
// we must create two defs. | ||
if let IsAsync::Async(async_id) = asyncness { | ||
let async_def = self.create_def(async_id, | ||
if let IsAsync::Async { closure_id, .. } = asyncness { | ||
let async_def = self.create_def(closure_id, | ||
DefPathData::ClosureExpr, | ||
REGULAR_SPACE, | ||
expr.span); | ||
|
@@ -302,6 +312,9 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> { | |
fn visit_ty(&mut self, ty: &'a Ty) { | ||
match ty.node { | ||
TyKind::Mac(..) => return self.visit_macro_invoc(ty.id), | ||
TyKind::ImplTrait(node_id, _) => { | ||
self.create_def(node_id, DefPathData::ImplTrait, REGULAR_SPACE, ty.span); | ||
} | ||
_ => {} | ||
} | ||
visit::walk_ty(self, ty); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theDefId
, why do we keep both?!We only really need the
DefId
for most of the compilation, AFAIK, and the occasional need to get theNodeId
from theDefId
shouldn't be enough to keep theNodeId
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!