Newtype hashbrown

[bushrat011899]

Objective

• Fixes Mismatched types and inaccessible methods for HashMap and HashSet in 0.16-rc #18690
• Closes #2065
• Alternative to Use a RandomState HashBuilder for re-exported HashMap and HashSet #18691

The changes to the Hash made in #15801 to the BuildHasher resulted in serious migration problems and downgraded UX for users of Bevy's re-exported hashmaps. Once merged, we need to go in and remove the migration guide added as part of #15801.

Solution

• Newtype HashMap and HashSet instead of type aliases
• Added Deref/Mut to allow accessing future hashbrown methods without maintenance from Bevy
• Added bidirectional From implementations to provide escape hatch for API incompatibility
• Added inlinable re-exports of all methods directly to Bevy's types. This ensures HashMap::new() works (since the Deref implementation wont cover these kinds of invocations).

Testing

• CI

---

Migration Guide

• If you relied on Bevy's HashMap and/or HashSet types to be identical to hashbrown, consider using From and Into to convert between the hashbrown and Bevy types as required.
• If you relied on hashbrown/serde or hashbrown/rayon features, you may need to enable bevy_platform_support/serialize and/or bevy_platform_support/rayon respectively.

---

Notes

• Did not replicate the Rayon traits, users will need to rely on the Deref/Mut or From implementations for those methods.
• Did not re-expose the unsafe methods from hashbrown. In most cases users will still have access via Deref/Mut anyway.
• I have added inline to all methods as they are trivial wrappings of existing methods.
• I chose to make HashMap::new and HashSet::new const, which is different to hashbrown. We can do this because we default to a fixed-state build-hasher. Mild ergonomic win over using HashMap::with_hasher(FixedHasher).

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[Victoronz]

No need to copy over the inherent methods! We can just implement Deref/DerefMut/into_inner to make the inner methods accessible, like how I did with EntityHashMap.
The offending methods/associated functions we can then shadow of course.

[bushrat011899]

No need to copy over the inherent methods!

The issue is cases where a user wants to use the typename to access a method:

let map = HashMap::new();

Since I have to add forwarding implementations for some methods, I just grabbed the entire public API.

[bushrat011899]

Unrelated CI failure

[bushrat011899]

Unrelated CI failure

[alice-i-cecile]

Good implementation and I agree with your choices but needs better docs :)

It's annoying to have to do this but this is effectively a one time cost.

[bushrat011899]

Good implementation and I agree with your choices but needs better docs :)

Yeah since this is "Bevy's" hashmap/set implementation it does deserve better documentation. I've gone through and added at a minimum links to the underlying hashbrown documentation, and examples to most methods.

It's annoying to have to do this but this is effectively a one time cost.

Agreed. Plus we insulate ourselves from further API changes pretty strongly this way.

[bushrat011899]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

I think because this PR is intended for 0.16 I should omit the migration guide in the repo? Happy to add it there if anyone disagrees tho!

[Victoronz]

To mitigate the breaking changes fully, there should also be this impl for the wrapped HashSet.

And for completeness, it is nice to have into_inner methods for the wrappers, which are an explicit, non-inference dependent convention to get the owned, unwrapped type. (It can also be constified, but here that doesn't matter much)

[djeedai]

Approving on the basis that this looks to do the right thing and fix the migration issue. Thanks for the (tedious) work!

Just to note, I can't help but having that feeling that if the Rust std team didn't make HashMap::new() use the hasher from the generic parameter, and instead force it to RandomState, there's a reason for this choice. Is that a language limitation? Is there an edge case we're not seeing? I'd feel more comfortable with this change if we had an answer to that question first and understood the original design choice.

[djeedai]

Curious here, can't we use a derive? Why do we have to manually impl this?

[bushrat011899]

Purely because this crate doesn't take a dependency on any crates that provide that derive macro. Simple enough to write so I'd rather avoid possible compile time changes.

[bushrat011899]

Yeah, those are the associated functions we have to copy (specifically the functions that do not have a self parameter). However, for the methods (= associated functions that have a self parameter), the deref impl covers them entirely, and they're superfluous! It is important to omit the inherent methods that do not have any significance, because otherwise it is more difficult to find the ones that do. They will appear twice in the generated doc page, once normally, and once under the deref, which is confusing when there is no difference to be found! Lastly, less lines, less maintenance/reviewer load!

I attempted to remove these methods (leaving only the associated functions) but there were several compilation issues I was unable to resolve. Some were due to using Self::len in bevy_reflect, others were caused by drain not being picked up correctly, and I'm sure there are more. Because of that I'm leaving the methods as-is, but would encourage a follow-up to remove these methods where possible if that's still desirable.

To mitigate the breaking changes fully, there should also be this impl for the wrapped HashSet.

And for completeness, it is nice to have into_inner methods for the wrappers, which are an explicit, non-inference dependent convention to get the owned, unwrapped type. (It can also be constified, but here that doesn't matter much)

Agreed! I've added both of these.

[Victoronz]

Yeah, those are the associated functions we have to copy (specifically the functions that do not have a self parameter). However, for the methods (= associated functions that have a self parameter), the deref impl covers them entirely, and they're superfluous! It is important to omit the inherent methods that do not have any significance, because otherwise it is more difficult to find the ones that do. They will appear twice in the generated doc page, once normally, and once under the deref, which is confusing when there is no difference to be found! Lastly, less lines, less maintenance/reviewer load!

I attempted to remove these methods (leaving only the associated functions) but there were several compilation issues I was unable to resolve. Some were due to using Self::len in bevy_reflect, others were caused by drain not being picked up correctly, and I'm sure there are more. Because of that I'm leaving the methods as-is, but would encourage a follow-up to remove these methods where possible if that's still desirable.

I see, but these cases are actually useful to know! Because they should appear in the migration guide for #16912.
From your example, "Uses of Self::method in trait impls should be replaced with a deref and a method call" would be my guess.
I'm guessing that this failure is in impl_reflect_for_hash_set/map where Self::method would see use.
The direct migration would then look like this: self.deref().len() or (*self).len()

It'd be nice to know how exactly other things broke.

[bushrat011899]

I see, but these cases are actually useful to know! Because they should appear in the migration guide for #16912.
From your example, "Uses of Self::method in trait impls should be replaced with a deref and a method call" would be my guess.

Yeah that's definitely the solution, but the goal here is to minimise the work in migrating to 0.16. Remembering that the status quo without this PR is to "just" replace new with either default or with_hasher (and likewise for with_capacity). I'd argue this PR would be a net negative if we made the new syntax nicer but then broke a while heap of other places (with Bevy's own codebase being a canary for the kinds of issues).

I'm guessing that this failure is in impl_reflect_for_hash_set/map where Self::method would see use.
The direct migration would then look like this: self.deref().len() or (*self).len()

Yeah I reckon that'd be the fix.

It'd be nice to know how exactly other things broke.

Yeah I'm still unsure how drain got broken, or if that was some spurious error caused by other issues like Self::len. Regardless, the goal of this PR is to minimise migration issues, so whatever path makes the migration guide smallest would be my preference.

[Victoronz]

I see, but these cases are actually useful to know! Because they should appear in the migration guide for #16912.
From your example, "Uses of Self::method in trait impls should be replaced with a deref and a method call" would be my guess.

Yeah that's definitely the solution, but the goal here is to minimise the work in migrating to 0.16. Remembering that the status quo without this PR is to "just" replace new with either default or with_hasher (and likewise for with_capacity). I'd argue this PR would be a net negative if we made the new syntax nicer but then broke a while heap of other places (with Bevy's own codebase being a canary for the kinds of issues).

I'm guessing that this failure is in impl_reflect_for_hash_set/map where Self::method would see use.
The direct migration would then look like this: self.deref().len() or (*self).len()

Yeah I reckon that'd be the fix.

It'd be nice to know how exactly other things broke.

Yeah I'm still unsure how drain got broken, or if that was some spurious error caused by other issues like Self::len. Regardless, the goal of this PR is to minimise migration issues, so whatever path makes the migration guide smallest would be my preference.

We might disagree here! I see the breaking changes to construction concerning new/From as a magnitude worse than needing to rewrite Self::method.
Inserting a manual deref for a method call is something that rarely needs to be done, here this is the case because we're implementing a trait method with a colliding name, which seldomly happens downstream.
In contrast, every person reading the docs of HashMap will come across the duplication, which I'd deem to be a net-worse experience.

Additionally, unless we replicate every feature-specific impl, f.e. impls gated under the rayon feature, it is still a breaking change in this respect + the missing gated trait impls.
(There are nightly feature impls too, but whether that counts as a breaking change I'd need to read up on).

If that's fine, then instead of removing these methods, we could also make a note of why the duplication exists/where the line was drawn and leave it at that.
If we leave them be, then we should also add in the missing ones:

• HashSet::insert_unique_unchecked
• HashMap::{insert_unique_unchecked, get_many_unchecked_mut, get_many_key_value_unchecked_mut}

[bushrat011899]

We might disagree here! I see the breaking changes to construction concerning new/From as a magnitude worse than needing to rewrite Self::method. Inserting a manual deref for a method call is something that rarely needs to be done, here this is the case because we're implementing a trait method with a colliding name, which seldomly happens downstream.

Yeah I agree! I totally appreciate the position you're taking here, and I suspect our disagreement does just come down to which compromises each of us personally prefers.

In contrast, every person reading the docs of HashMap will come across the duplication, which I'd deem to be a net-worse experience.

This is unfortunate, but I checked the docs page with this PR, and the deref methods are separated into their own section (in the sidebar and the content of the page). Within Visual Studio Code, the duplicate methods aren't suggested by rust-analyser, so the only time you'll see these methods are when you have the proper context on docs.rs.

Additionally, unless we replicate every feature-specific impl, f.e. impls gated under the rayon feature, it is still a breaking change in this respect + the missing gated trait impls.

Yeah when you put it that way I'm convinced. I'll add rayon as a feature (unused by Bevy so no changes really), so the breaking change will just be requiring users to enable bevy_platform_support/rayon if they were using hashbrown/rayon.

There are nightly feature impls too, but whether that counts as a breaking change I'd need to read up on.

Yeah I drew a line here since Bevy otherwise has no support for these APIs (the existing type aliases had no allocator parameter, forcing users to rely on hashbrown directly anyway).

If that's fine, then instead of removing these methods, we could also make a note of why the duplication exists/where the line was drawn and leave it at that.

If we leave them be, then we should also add in the missing ones:

* `HashSet::insert_unique_unchecked`

* `HashMap::{insert_unique_unchecked, get_many_unchecked_mut, get_many_key_value_unchecked_mut}`

Agreed, I've added those methods for completeness.

[alice-i-cecile]

I'm convinced we've found a solution with few enough trade-offs that I'm comfortable moving forward as-is. Merging :)