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.

Sign up for GitHub

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

Jump to bottom

Remove PartialOrd/Ord impls for DefId and CrateNum #83006

Closed
Aaron1011 opened this issue Mar 11, 2021 · 4 comments
Closed

Remove PartialOrd/Ord impls for DefId and CrateNum #83006

Aaron1011 opened this issue Mar 11, 2021 · 4 comments
Assignees
@pierwill
Labels
A-incr-comp Area: Incremental compilation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Mar 11, 2021 •
edited by rustbot
Loading

Both DefId and CrateNum are not stable across compilation sessions - the associated DefPathHash is a stable cross-session ID. However, both DefId and CrateNum have Ord impls, which makes it easy to accidentally sort something by the unstable DefID/CrateNum value. If this sorted result makes its way into a query result, it can lead to unstable hash values across compilation sessions. See #82920 for an example of this occuring.

My initial attempt to remove these impls encountered a large number of usages across multiple crates.
@Aaron1011 Aaron1011 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 11, 2021
@Aaron1011 Aaron1011 mentioned this issue Mar 11, 2021
Closed
@camelid camelid added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Mar 12, 2021
@jonas-schievink jonas-schievink added the A-incr-comp Area: Incremental compilation label May 7, 2021
@JohnTitor
Copy link
Member

#85829 refactored CrateNum so DefId is the only item here.

My initial attempt to remove these impls encountered a large number of usages across multiple crates.

Hmm, yes, for example, we use DefId within BTreeMap or FxHashMap. So, is the rough fix to store DefPathHash instead of it?

@pierwill
Copy link
Member

pierwill commented Nov 5, 2021

@rustbot claim

@pierwill
Copy link
Member

pierwill commented Nov 5, 2021

This issue is part of #90317.

@pierwill
Copy link
Member

pierwill commented Nov 5, 2021

@Aaron1011 Looks like we can close this in favor of #90317, yes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pierwill pierwill

Labels
A-incr-comp Area: Incremental compilation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@Aaron1011 @jonas-schievink @pierwill @JohnTitor @camelid