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

Update rustc_hash and come to terms with the consequences #1877

Open
sezna opened this issue Aug 22, 2024 · 1 comment
Open

Update rustc_hash and come to terms with the consequences #1877

sezna opened this issue Aug 22, 2024 · 1 comment
Labels
bug Something isn't working low-priority backlog items with low priority

Comments

@sezna
Copy link
Contributor

sezna commented Aug 22, 2024

We appear to iterate over some hash maps in some places. Updating rustc_hash to a 2.x.x version causes all sorts of snapshot tests to fail with ordering issues.

I think we have two options:

  1. Investigate these iterations and make them deterministic where important.
  2. Update rustc_hash when we do a breaking change, so that our ordering can as well change.

Or...stay on 1.x.x forever?
@sezna sezna added bug Something isn't working low-priority backlog items with low priority labels Aug 22, 2024
@sezna sezna mentioned this issue Aug 26, 2024
Merged
@sezna
Copy link
Contributor Author

sezna commented Aug 26, 2024

@idavis has said to be sure that the performance doesn't degrade with this update -- they observed that in the past.

github-merge-queue bot pushed a commit that referenced this issue Aug 26, 2024
@sezna
Bump rust dependency versions (except pyo3, rustc_hash, and `miet…
b2f7ddd 
…te`) (#1876)

This PR updates all of our Rust dependencies to their latest versions,
with the exception of:
- `pyo3`, which will require some more work as its API has changed-
- `miette`, which doesn't seem to support wasm on certain platforms
post-v5? Needs more investigation
- `rustc_hash`, which had a breaking change in the order of iteration
over `FxHashMap`s.

Note: we apparently iterate over hashmaps in various places, and then do
snapshot tests on the results. `rustc_hash` changed some iteration
ordering, so updating this dependency caused some expect tests to need
updating. In a perfect world, we'd track down these iterations and make
them deterministic. But, the effort/outcome ratio is so low there, since
iteration order is deterministic within a single version of
`rustc_hash`, I say we just update the expect tests and move on.

I also removed patch-version pins from our `Cargo.toml` where they
existed, since we usually just specify major.minor.


#1886: issue to track updating `pyo3`
#1877:  issue to track updating `rustc_hash`

I'm not sure we want to update `miette` any time soon -- it breaks us in
a big way and we are happy with the current version I think. But we
should have a discussion about that. So I filed #1887 to track that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working low-priority backlog items with low priority
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@sezna