Make re-export collection deterministic by Aaron1011 · Pull Request #65043 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation53 Commits2 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
Fixes #65036
Previously, we were using an FxHashMap to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.
See #65042 for a long-term strategy to detect this kind of issue
(rust_highfive has picked a reviewer for you, use r? to override)
Looks good but r? @petrochenkov should really review this since resolve is their wheelhouse. :)
I think we usually deal with this via collection into a Vec and sorting just before metadata serialization; that might be a better approach here?
I personally feel that IndexMap is a more reliable by-construction approach that takes care of every iteration site so you don't have to care about all of them. Maybe when we get the wrapper for FxHashMap we should use that instead.
We do have a StableMap in the data structures crate as of a recent PR, so maybe using that here would be better. It does not expose iteration order at all (lacking those methods entirely).
But again, I'm fine landing this in the mean time. An indexmap seems like a reasonable answer too.
Stable map looks unsuitable since Idents don't implement Ord.
I think we usually deal with this via collection into a Vec and sorting just before metadata serialization
That was my first reaction too.
Changing Resolutions, which are used for a thousand other things beside serializing reexports, is such an unproportionally heavy hammer for fixing this.
We actually have a method for visiting resolutions in a stable fashion (fn for_each_child_stable) which is already used in diagnostic reporting with the same purpose.
⌛ Trying commit 2c9bf5f9320945e4fc29927e8de60dd2fad8172f with merge 9f13bf7d334cc30a5fbe83d0309ccf107c0d512e...
This comment has been minimized.
Awaiting bors try build completion
⌛ Trying commit 2c9bf5f9320945e4fc29927e8de60dd2fad8172f with merge 0e298d5daf4f422edf242c37161329ee79515942...
I personally feel that
IndexMapis a more reliable by-construction approach
Actually, I'm not sure that the insertion order itself is stable for the resolutions, if it's not, then IndexMap maintaining it won't matter much.
Interesting. I'm pretty happy I reassigned this PR. :)
☀️ Try build successful - checks-azure
Build commit: 0e298d5daf4f422edf242c37161329ee79515942 (0e298d5daf4f422edf242c37161329ee79515942)
I personally feel that IndexMap is a more reliable by-construction approach
Actually, I'm not sure that the insertion order itself is stable for the resolutions, if it's not, then IndexMap maintaining it won't matter much.
We have StableMap since #64131. It works the same as FxHashMap, but you can't iterate it, you have to use .into_sorted_vec() instead.
@petrochenkov: This looks like a wash performance wise - a few minor speedups and a few minor slowdowns. I suspect that much of it is just noise.
Yeah, I just wanted to check what performance impact IndexMap can have, sorry for all this noise.
I still think using for_each_child_stable in finalize_resolutions_in is a preferable solution.
@petrochenkov: All other things being equal, I think it's better to reduce the amount of non-determinism in the compiler. This issue was a pain to track down, and I'm worried that it might crop up again if we're relying on every caller using for_each_child_stable whenever they need to iterate over to it.
Let's check whether IndexMap provides what we need or not - #65117.
Looks like resolutions are added in stable order, so we can use an IndexMap.
@Aaron1011
Could you remove for_each_child_stable in this PR then, and replace its uses with for_each_child.
(Replacing other iterations through resolutions would also be nice, the use of for_each_child looks a bit cleaner in all cases.)
Previously, we were using an FxHashMap to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR rust-lang#64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.
See rust-lang#65042 for a long-term strategy to detect this kind of issue
Now that Resolutions has a deterministic iteration order, it's no
longer necessary to sort its entries before iterating over them
📌 Commit add0a42 has been approved by petrochenkov
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
bors added a commit that referenced this pull request
…nkov
Make re-export collection deterministic
Fixes #65036
Previously, we were using an FxHashMap to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.
See #65042 for a long-term strategy to detect this kind of issue
Previously, we were using an FxHashMap to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic.
Clarification to avoid spreading this myth further: FxHashMap is deterministic. However, inserting a new element can surprisingly affect the entire iteration order. Also see #63713 (comment).
@RalfJung: I meant 'non-deterministic with respect to platforms' - that is, the crate metadata order wouldn't look the same across different platforms. Sorry for the confusion.
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.