-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[WIP] HirId-ification #57578
[WIP] HirId-ification #57578
Conversation
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 |
☔ The latest upstream changes (presumably #57381) made this pull request unmergeable. Please resolve the merge conflicts. |
da1d2c2
to
7ea60e0
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 |
@varkor: I think I've made plenty of progress and further changes will require a bit more aggressive approach; could you help me with indentifying the cause of the error (not the tidy one ^^) that causes breakage? I would be a lot more confident in continuing if we could get rid of it. |
7ea60e0
to
eb1fca5
Compare
Thanks for working on this super old tech debt! |
@ljedrz: I'll take a look as soon as I can! (It's been busy this week, so I haven't had a chance yet.) |
@varkor: thanks! I'm still trying to crack it myself, but I could definitely use some guidance ^^. Most of the changes are substitutions ( |
68d64eb
to
3d47b7e
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 |
3d47b7e
to
e814235
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 |
☔ The latest upstream changes (presumably #57747) made this pull request unmergeable. Please resolve the merge conflicts. |
e814235
to
17988ee
Compare
HirIdify some lints Unblocks rust-lang/rust#58561 (a part of [rust-lang/rust#57578](rust-lang/rust#57578)). Can we branch it like with #3790? I can rebase on a different commit if need be. Haven't had time to run tests yet, so I'd wait for Travis 🙈.
Remove NodeId from more HIR nodes The next iteration of HirIdification (#57578). Removes `NodeId` from: - [x] `Stmt` - [x] `Local` - [x] `Field` - [x] `AnonConst` - [x] `TraitItem` - [x] `ImplItem` - [x] `TypeBinding` - [x] `Arg` - [x] `TraitRef` - [x] `VisibilityKind` It will most probably break clippy again; I'd appreciate a **delegate** again if/when it is good to go so I can attach a clippy fix later. r? @Zoxc
Remove NodeId from even more HIR nodes The next iteration of HirIdification (#57578). Removes `NodeId` from: - [x] `StructField` - [x] `ForeignItem` - [x] `Item` - [x] `Pat` - [x] `FieldPat` - [x] `VariantData` - [x] `ImplItemId` (replaces it with `HirId`) - [x] `TraitItemId` (replaces it with `HirId`)
HirIdification: almost there The next iteration of HirIdification (#57578). Replaces a bunch of `NodeId` method calls (mostly `as_local_node_id`) with `HirId` ones. Removes `NodeId` from: - [x] `PathSegment` - [x] `PatKind` - [x] `Destination` (replaces it with `HirId`) In addition this PR also removes `Visitor::visit_def_mention`, which doesn't seem to be doing anything.
HirIdification: rework Map The next iteration of HirIdification (#57578). - remove `NodeId` from `Entry` - change `Map::map` to an `FxHashMap<HirId, Entry>` - base the `NodeId` `Map` methods on `HirId` ones (reverses the current state) - HirIdify `librustdoc` a little bit (some `NodeId` `Map` methods were converted to work on `HirId`s) The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the `Map` searches are cached (which is now possible thanks to using `HirId`s). r? @Zoxc
HirIdification: rework Map The next iteration of HirIdification (#57578). - remove `NodeId` from `Entry` - change `Map::map` to an `FxHashMap<HirId, Entry>` - base the `NodeId` `Map` methods on `HirId` ones (reverses the current state) - HirIdify `librustdoc` a little bit (some `NodeId` `Map` methods were converted to work on `HirId`s) The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the `Map` searches are cached (which is now possible thanks to using `HirId`s). r? @Zoxc
middle: replace NodeId with HirId in AccessLevels Pushing the limits of HirIdification (rust-lang#57578). Replaces `NodeId` with `HirId` in `middle::privacy::AccessLevels`. Actually this time I was more successful and cracked it; I probably tried to HirIdify too much at once when I attempted it last time ^^. r? @Zoxc
…r=Zoxc HirIdification: kill off NodeId stragglers The final stages of HirIdification (rust-lang#57578). This PR, along with rust-lang#59042, should finalize the HirIdification process (at least the more straightforward bits). - replace `NodeId` with `HirId` in `trait_impls` - remove all `NodeId`s from `borrowck` - remove all `NodeId`s from `typeck` - remove all `NodeId`s from `mir` - remove `trait_auto_impl` (unused) I would be cool to also remove `NodeId` from `hir::def::Def`, `middle::privacy::AccessLevel` and `hir::ItemId`, but I don't know if this is feasible. I'll be happy to do more if I've missed anything.
middle: replace NodeId with HirId in AccessLevels Pushing the limits of HirIdification (rust-lang#57578). Replaces `NodeId` with `HirId` in `middle::privacy::AccessLevels`. Actually this time I was more successful and cracked it; I probably tried to HirIdify too much at once when I attempted it last time ^^. r? @Zoxc
…r=Zoxc HirIdification: kill off NodeId stragglers The final stages of HirIdification (rust-lang#57578). This PR, along with rust-lang#59042, should finalize the HirIdification process (at least the more straightforward bits). - replace `NodeId` with `HirId` in `trait_impls` - remove all `NodeId`s from `borrowck` - remove all `NodeId`s from `typeck` - remove all `NodeId`s from `mir` - remove `trait_auto_impl` (unused) I would be cool to also remove `NodeId` from `hir::def::Def`, `middle::privacy::AccessLevel` and `hir::ItemId`, but I don't know if this is feasible. I'll be happy to do more if I've missed anything.
middle: replace NodeId with HirId in AccessLevels Pushing the limits of HirIdification (rust-lang#57578). Replaces `NodeId` with `HirId` in `middle::privacy::AccessLevels`. Actually this time I was more successful and cracked it; I probably tried to HirIdify too much at once when I attempted it last time ^^. r? @Zoxc
middle: replace NodeId with HirId in AccessLevels Pushing the limits of HirIdification (#57578). Replaces `NodeId` with `HirId` in `middle::privacy::AccessLevels`. Actually this time I was more successful and cracked it; I probably tried to HirIdify too much at once when I attempted it last time ^^. r? @Zoxc
HirIdification: kill off NodeId stragglers The final stages of HirIdification (#57578). This PR, along with #59042, should finalize the HirIdification process (at least the more straightforward bits). - replace `NodeId` with `HirId` in `trait_impls` - remove all `NodeId`s from `borrowck` - remove all `NodeId`s from `typeck` - remove all `NodeId`s from `mir` - remove `trait_auto_impl` (unused) I would be cool to also remove `NodeId` from `hir::def::Def`, `middle::privacy::AccessLevel` and `hir::ItemId`, but I don't know if this is feasible. I'll be happy to do more if I've missed anything.
HirIdification: rework Map The next iteration of HirIdification (#57578). - remove `NodeId` from `Entry` - change `Map::map` to an `FxHashMap<HirId, Entry>` - base the `NodeId` `Map` methods on `HirId` ones (reverses the current state) - HirIdify `librustdoc` a little bit (some `NodeId` `Map` methods were converted to work on `HirId`s) The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the `Map` searches are cached (which is now possible thanks to using `HirId`s). r? @Zoxc
This is a WIP attempt at complete
HirId
-ification, i.e. deprecatingast::NodeId
s after the lowering process.Sadly this can't be done one-by-one, because everything is connected to each other; I added
HirId
toHir
nodes, introduced methods working onHirId
instead ofNodeId
and I'm gradually removingNodeId
s.This PR currently fails to build
stage1
due to some error on my part, probably inhir::lowering
orhir::intravisit
; I would like to get some help with that, and also to know if I have made any "design flaws" (e.g. removal ofNodeId
where it will definitely be needed, e.g. by some tool) that haven't revealed themselves just yet.Current progress on the main hit list (checked ones only have
HirId
now):hir::AnonConst
hir::Arg
hir::Block
hir::BodyId
hir::Destination
- depends onhir::Def::Label
hir::Expr
hir::Field
hir::FieldPat
hir::ForeignItem
hir::GenericParam
hir::ImplItem
hir::ImplItemId
hir::Item
hir::ItemId
-NodeId
used bysyntax::visit::Visitor
hir::Lifetime
hir::Local
hir::MacroDef
hir::Pat
hir::PathSegment
- used insave-analysis
hir::StmtKind
hir::StructField
hir::TraitItem
hir::TraitItemId
hir::TraitRef
hir::Ty
hir::TypeBinding
hir::VariantData
hir::WhereClause
hir::WhereEqPredicate
hir::def::Def::Label
hir::def::Def::Local
hir::def::Def::Upvar
hir::map::Entry
hir::map::blocks::ItemFnParts
hir::map::blocks::ClosureParts
hir::map::collector::NodeCollector
hir::map::hir_id_validator::HirIdValidator
Until the HirId-ification is 100% complete the overall code delta will be in plus - this is mostly due to the duplication of some methods working on
NodeId
.Fixes #50928.
r? @varkor