Skip to content

Remove hir::Map::find#106401

Closed
compiler-errors wants to merge 2 commits intorust-lang:masterfrom
compiler-errors:eliminate-hir-find
Closed

Remove hir::Map::find#106401
compiler-errors wants to merge 2 commits intorust-lang:masterfrom
compiler-errors:eliminate-hir-find

Conversation

@compiler-errors
Copy link
Contributor

@compiler-errors compiler-errors commented Jan 3, 2023 •
edited
Loading

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 return None.

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.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

flip1995
flip1995 reviewed Jan 3, 2023
View reviewed changes
src/tools/clippy/clippy_utils/src/lib.rs
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);
Copy link
Member

@flip1995 flip1995 Jan 3, 2023

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.

@cjgillot
Copy link
Contributor

cjgillot commented Jan 3, 2023

Currently, not all HirId have an associated node. So this PR introduces a strong risk of ICEs.
For instance, the HirId in LangItemTrait, WhereBoundPredicate, Let (and probably a few others) do not have an associated Node.

Before this PR, we should remove those holes:

  1. by removing the useless HirIds;
  2. by adding new variants to Node;
  3. by checking in HIR validation that the ItemLocalId -> Node map is full.

Waiting for the removal of save-analysis will create more opportunities for (1).

In the future, we may use the creation of LocalDefId to create nodes that mimick HIR, like @spastorino's refactor to lower RPITIT as associated types. We will need new OwnerNode::RPITIT and Node::RPITIT variants to be returned in that case.

@bors
Copy link
Collaborator

bors commented Jan 4, 2023

☔ The latest upstream changes (presumably #106432) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Contributor Author

compiler-errors commented Jan 9, 2023

@rustbot 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 Jan 9, 2023
@compiler-errors compiler-errors deleted the eliminate-hir-find branch August 11, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flip1995 flip1995 flip1995 left review comments

Assignees

@cjgillot cjgillot

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@compiler-errors @rustbot @cjgillot @bors @flip1995