Skip to content

Document NodeId #80860

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 11, 2021
Merged

Document NodeId #80860

merged 2 commits into from
Feb 11, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Jan 10, 2021

No description provided.

@camelid camelid added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Jan 10, 2021
@rust-highfive
Copy link
Contributor

r? @ecstatic-morse

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2021
@camelid
Copy link
Member Author

camelid commented Jan 10, 2021

r? @Aaron1011

Aaron1011
Aaron1011 reviewed Jan 11, 2021
View reviewed changes
compiler/rustc_ast/src/node_id.rs Outdated
/// Identifies an AST node.
///
/// This identifies top-level definitions, expressions, and everything in between.
/// This is later split into [`DefId`] and `HirId` for the HIR.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you say "converted" instead of "split"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"converted" didn't sound quite right to me, so I did "turned into" instead.

camelid
camelid commented Jan 12, 2021
View reviewed changes
compiler/rustc_ast/src/node_id.rs Outdated
Comment on lines 21 to 22
/// When parsing and doing expansions, we initially give all AST nodes this AST
/// node value. Then later, during expansion, we renumber them to have small,
/// positive ids.
/// [`NodeId`]. Then later, during expansion, we renumber them to have small,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is confusing to me: it says that "when parsing and doing expansions" we use a dummy ID, but "then later, during expansion" we create real IDs. Does it mean that at the beginning of expansions we use dummy IDs but a later phase of it we create real ones?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct - the creation of real NodeIds is done here:

rust/compiler/rustc_expand/src/expand.rs

Lines 1702 to 1707 in 9bc8b00

fn visit_id(&mut self, id: &mut ast::NodeId) {
if self.monotonic {
debug_assert_eq!(*id, ast::DUMMY_NODE_ID);
*id = self.cx.resolver.next_node_id()
}
}

@JohnCSimon
Copy link
Member

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2021
@sanxiyn
Copy link
Member

sanxiyn commented Feb 1, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 1, 2021

📌 Commit 2af4a01 has been approved by sanxiyn

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2021
@camelid
Copy link
Member Author

camelid commented Feb 1, 2021

I was planning on clarifying the part about the dummy IDs a bit, and it might make sense for Aaron1011 to take another look too once I do that. @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 1, 2021
@camelid
Copy link
Member Author

camelid commented Feb 8, 2021

Ready for a final review!

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2021
@sanxiyn
Copy link
Member

sanxiyn commented Feb 10, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 10, 2021

📌 Commit 0f3e2f6 has been approved by sanxiyn

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 10, 2021
@bors
Copy link
Collaborator

bors commented Feb 11, 2021

⌛ Testing commit 0f3e2f6 with merge 9ce7268...

@bors
Copy link
Collaborator

bors commented Feb 11, 2021

☀️ Test successful - checks-actions
Approved by: sanxiyn
Pushing 9ce7268 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 11, 2021
@bors bors merged commit 9ce7268 into rust-lang:master Feb 11, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 11, 2021
@camelid camelid deleted the nodeid-docs branch February 11, 2021 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011 Aaron1011 left review comments

Assignees

@Aaron1011 Aaron1011

Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
8 participants
@camelid @rust-highfive @JohnCSimon @sanxiyn @bors @Aaron1011 @ecstatic-morse @rustbot