Remove strict invariant node_type on hir_type during ty privacy visit#157883
Remove strict invariant node_type on hir_type during ty privacy visit#157883Shourya742 wants to merge 3 commits into
Conversation
rustbot
added
S-waiting-on-review
| if let Some(ty) = self | ||
| .maybe_typeck_results | ||
| .unwrap_or_else(|| span_bug!(hir_ty.span, "`hir::Ty` outside of a body")) | ||
| .node_type_opt(hir_ty.hir_id) | ||
| { | ||
| return; | ||
| if self.visit(ty).is_break() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I am not fully sure this change is right, but this is how I am thinking about it. This used node_type before, so visit_ty was assuming every hir_ty would already have a type entry in TypeckResults. But the comment in writeback::visit_ty makes it sound like that is not always true on some error paths, since writeback may just not write a final type entry for that hir_ty. So based on that, I changed this visit_ty to handle missing type entries and just keep walking the type tree.
I hope the thought process is sane. And I am not breaking any invariant (like I am not seeing any test fail :P)
There was a problem hiding this comment.
Should we instead fix writeback to ensure we always have a type recorded, even on error paths?
There was a problem hiding this comment.
Not sure, seems like its an intended behavior as mentioned here in writeback visit_ty:
rust/compiler/rustc_hir_typeck/src/writeback.rs
Lines 350 to 351 in 124feda
Shourya742
changed the title
| if self.visit(ty).is_break() { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe ?
| } | |
| if let Some(ty) = self | |
| .maybe_typeck_results | |
| .unwrap_or_else(|| span_bug!(hir_ty.span, "`hir::Ty` outside of a body")) | |
| .node_type_opt(hir_ty.hir_id) | |
| && self.visit(ty).is_break() { | |
| return; | |
| } |
There was a problem hiding this comment.
Yup, but I am following the pattern how its written for other visit_* methods with node_type_opt. I initially wrote the same but changed to the current one following other sites. :P
5 participants
closes: #157772
r? @BoxyUwU