-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Remove hir::Map::find
#106401
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
Remove hir::Map::find
#106401
Conversation
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
@@ -172,10 +172,10 @@ pub fn expr_or_init<'a, 'b, 'tcx: 'b>(cx: &LateContext<'tcx>, mut expr: &'a Expr | |||
pub fn find_binding_init<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<&'tcx Expr<'tcx>> { | |||
let hir = cx.tcx.hir(); | |||
if_chain! { | |||
if let Some(Node::Pat(pat)) = hir.find(hir_id); | |||
if let Node::Pat(pat) = hir.get(hir_id); |
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.
Looking through the Clippy changes, it seems that get
asserts that this node exists. I'm not 100% sure that this will just work in all cases in Clippy.
So if your assertion that all nodes exist by construction is true this should be fine. Otherwise I can see many new ICE issues in the future. I'm not informed enough to help with a decision regarding this, though.
Currently, not all HirId have an associated node. So this PR introduces a strong risk of ICEs. Before this PR, we should remove those holes:
Waiting for the removal of save-analysis will create more opportunities for (1). In the future, we may use the creation of |
☔ The latest upstream changes (presumably #106432) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot author |
I'm not exactly sure what value this function gives us. I though that by construction all HIR ids are present in the HIR map, and I can't seem to find a case where
Map::find
would ever returnNone
.Before attempting this PR, I actually tried replacing the body of
hir::Map::find
with one that unwraps, and nothing seemed to change. Perhaps I'm overlooking something really critical, though?r? @cjgillot but feel free to reassign.