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.

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

Manually order DefId on 64-bit big-endian #102382

Merged
merged 1 commit into from
Sep 30, 2022
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Sep 27, 2022

DefId uses different field orders on 64-bit big-endian vs. others, in
order to optimize its Hash implementation. However, that also made it
derive different lexical ordering for PartialOrd and Ord. That
caused spurious differences wherever DefIds are sorted, like the
candidate sources list in report_method_error.

Now we manually implement PartialOrd and Ord on 64-bit big-endian to
match the same lexical ordering as other targets, fixing at least one
test, src/test/ui/methods/method-ambig-two-traits-cross-crate.rs.

`DefId` uses different field orders on 64-bit big-endian vs. others, in
order to optimize its `Hash` implementation. However, that also made it
derive different lexical ordering for `PartialOrd` and `Ord`. That
caused spurious differences wherever `DefId`s are sorted, like the
candidate sources list in `report_method_error`.

Now we manually implement `PartialOrd` and `Ord` on 64-bit big-endian to
match the same lexical ordering as other targets, fixing at least one
test, `src/test/ui/methods/method-ambig-two-traits-cross-crate.rs`.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 27, 2022
@rust-highfive
Copy link
Collaborator

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2022
@cjgillot
Copy link
Contributor

Relying on the DefId ordering is a source of incr-comp bug. It would be more interesting to remove this dependency and make the test rely on some other order, like the one of source code, or of spans.

@cuviper
Copy link
Member Author

cuviper commented Sep 28, 2022

Are there any plans to remove DefID ordering altogether? Because this PR is just making the status quo more consistent.

@cjgillot
Copy link
Contributor

Yes, there is #90317, and #92233, which is a prerequisite.

@cuviper
Copy link
Member Author

cuviper commented Sep 28, 2022

OK, well that looks like a huge effort, but it can easily drop the changes I'm making here when that goes through.

It would be more interesting to remove this dependency and make the test rely on some other order, like the one of source code, or of spans.

As for this, I think the test doesn't really care about order at all, just that it needs to be reproducible enough for UI to match. I can look at other more-stable options.

@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2022

📌 Commit fb5002d has been approved by fee1-dead

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2022
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#102276 (Added more const_closure functionality)
 - rust-lang#102382 (Manually order `DefId` on 64-bit big-endian)
 - rust-lang#102421 (remove the unused :: between trait and type to give user correct diag…)
 - rust-lang#102495 (Reinstate `hir-stats.rs` test for stage 1.)
 - rust-lang#102505 (rustdoc: remove no-op CSS `h3.variant, .sub-variant h4 { border-bottom: none }`)
 - rust-lang#102506 (Specify `DynKind::Dyn`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e2bc6b8 into rust-lang:master Sep 30, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 30, 2022
@cuviper cuviper deleted the defid-order branch October 15, 2022 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants