Sort bounds by DefPathHash during astconv #83005
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
r=me after blessing the ui tests i've already had a similar bug in the past iirc, not sure if there's a way to prevent bugs like this from happening 🤔 |
|
I had opened #83006 to track removing the |
cc rust-lang#82920 The value of a `DefId` is not stable across compilation sessions. If we sort by `DefIds`, and then include the sorted list as part of a query result, then the query will no longer be a pure function of its input (e.g. the `DefId` can change while the `DefPathHash` remains the same). For reasons that are not yet clear, this issue lead to segfaults and garbage slice index values when the project in the linked issue was built with a particular incremental cache.
Aaron1011
force-pushed
the
sort-def-path-hash
branch
from
c5343eb
to
9339982
Compare
|
@bors r=lcnr |
|
📌 Commit 9339982 has been approved by |
bors
added
S-waiting-on-bors
|
This most general fix for this kind of issue is something like #83007. However, benchmarks currently show that the cost is extremely high. |
This comment has been minimized.
This comment has been minimized.
|
It looks like I'm getting different output locally. Did this somehow make the order less stable? |
|
@michaelwoerister You worked on |
|
@bors r- |
bors
added
S-waiting-on-author
|
Let me take a look ... |
| @@ -1318,7 +1319,8 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { | |||
|
|
|||
| // De-duplicate auto traits so that, e.g., `dyn Trait + Send + Send` is the same as | |||
| // `dyn Trait + Send`. | |||
| auto_traits.sort_by_key(|i| i.trait_ref().def_id()); | |||
| // Sort by `DefPathHash` so that the order is stable across compilation sessions | |||
| auto_traits.sort_by_key(|i| self.tcx().def_path_hash(i.trait_ref().def_id())); | |||
| auto_traits.dedup_by_key(|i| i.trait_ref().def_id()); | |||
There was a problem hiding this comment.
This is probably the reason for the test failures: dedup_by_key assumes that the Vec is sorted by the given key. So this should be auto_traits.dedup_by_key(|i| self.tcx().def_path_hash(i.trait_ref().def_id()));
Maybe it would make sense to use a rustc_data_structures::sorted_map::SortedMap here.
There was a problem hiding this comment.
Since def path hash equality implies that the DefIds are equal, I thought that deduplicating by DefId would still work, since equal DefIds should still be adjacent to each other.
There was a problem hiding this comment.
Also, the failure appears to be caused be re-ordered errors, not duplicated errors.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
After some further consideration, I don't think sorting by rust/compiler/rustc_interface/src/util.rs Lines 500 to 503 in b3e19a2 This is generated by Cargo from a large amount of information: https://github.com/rust-lang/cargo/blob/c4adfd553b7210d1aeb462d6d4b1dabfd2d0910e/src/cargo/core/compiler/context/compilation_files.rs#L489-L565 In particular, the name of the host platform is included: https://github.com/rust-lang/cargo/blob/c4adfd553b7210d1aeb462d6d4b1dabfd2d0910e/src/cargo/core/compiler/context/compilation_files.rs#L582-L585 This means that we'll get different This means that sorting by Instead, I think we should avoid sorting by both |
|
It turns out that the difference in ordering between my machine and the PR builder was caused by my local |
|
The fact that we sort the vectors by |
The problem is that the value of a
The relevant query result is |
|
Closing in favor of #83074, which avoids the sorting alogether. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc #82920
The value of a
DefIdis not stable across compilation sessions. If wesort by
DefIds, and then include the sorted list as part of a queryresult, then the query will no longer be a pure function of its input
(e.g. the
DefIdcan change while theDefPathHashremains the same).For reasons that are not yet clear, this issue lead to segfaults and
garbage slice index values when the project in the linked issue was
built with a particular incremental cache.