Skip to content
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

Panic if NodeIds are used for incremental compilation #68997

Merged
merged 2 commits into from
Feb 16, 2020

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Feb 9, 2020

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2020
@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 9, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Feb 9, 2020

⌛ Trying commit e517ee9 with merge 4e727aa...

bors added a commit that referenced this pull request Feb 9, 2020
[WIP] Treat NodeIs as pure values for incremental compilation

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented Feb 9, 2020

☀️ Try build successful - checks-azure
Build commit: 4e727aa (4e727aa494e3766fe1f66d9a5c87078a58eabcf2)

@rust-timer
Copy link
Collaborator

Queued 4e727aa with parent 6dff769, future comparison URL.

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 12, 2020

This makes the query system treat node ids as pure integers (which is what they are) instead of HIR ids.

cc @rust-lang/compiler Does anyone know of any scenarios this would break?

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 12, 2020

In order to do this, we'd have to track node_to_hir_id as that would reveal the HirId now. There's just 1 call of it left in the compiler proper though (some more in save-analysis).

@eddyb
Copy link
Member

eddyb commented Feb 15, 2020

Do we deal with NodeIds anymore though? Can we remove these impls?

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 15, 2020

They exist in macro defs and attributes (in token trees). I'm not sure if they are actually used for anything though.

@eddyb
Copy link
Member

eddyb commented Feb 15, 2020

cc @petrochenkov (sounds like the problem is interpolated Nonterminals?)

@petrochenkov
Copy link
Contributor

@eddyb, do you remember these PRs - #56492 ("syntax: use MetaPath (a simpler version of Path) for Attribute and MetaItem"), #56480 (Use hir::Path instead of ast::Path in HIR Attribute's)?
The goal was to avoid AST types being embedded into HIR types due to attributes.

@petrochenkov
Copy link
Contributor

Nonterminal tokens don't exist in HIR, they are converted into primitive tokens during AST -> HIR lowering, so NodeIds shouldn't exist in HIR either. HIR hashing can insert some kind of runtime panic for them.

@eddyb
Copy link
Member

eddyb commented Feb 15, 2020

@eddyb, do you remember these PRs - #56492 ("syntax: use MetaPath (a simpler version of Path) for Attribute and MetaItem"), #56480 (Use hir::Path instead of ast::Path in HIR Attribute's)?

Oh right, it's... been a while. I plan to eventually go over unfinished work like that as soon as it's possible, but things keep coming up and most of my plans end up in the trash.
All I can do for the time being is apologize, I suppose.

HIR hashing can insert some kind of runtime panic for them.

I wanted to suggest panicking but I wasn't sure if we're expecting leftover NodeIds, thanks!

@Zoxc Zoxc changed the title [WIP] Treat NodeIs as pure values for incremental compilation Panic if NodeIds are used for incremental compilation Feb 16, 2020
@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 16, 2020

I made this panic instead.

r? @petrochenkov

@petrochenkov
Copy link
Contributor

@bors r+
(If it passes through CI.)

@bors
Copy link
Collaborator

bors commented Feb 16, 2020

📌 Commit 8a37811 has been approved by petrochenkov

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2020
@bors
Copy link
Collaborator

bors commented Feb 16, 2020

⌛ Testing commit 8a37811 with merge 116dff9...

bors added a commit that referenced this pull request Feb 16, 2020
Panic if NodeIds are used for incremental compilation

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented Feb 16, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 116dff9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 16, 2020
@bors bors merged commit 8a37811 into rust-lang:master Feb 16, 2020
@Zoxc Zoxc deleted the pure-node-id branch February 16, 2020 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants