Skip to content

Update docs to explain default Hasher issue#18350

Merged
alice-i-cecile merged 5 commits intobevyengine:mainfrom
krunchington:hashmap-hasher-issue-docs
Mar 17, 2025
Merged

Update docs to explain default Hasher issue#18350
alice-i-cecile merged 5 commits intobevyengine:mainfrom
krunchington:hashmap-hasher-issue-docs

Conversation

@krunchington
Copy link
Copy Markdown
Contributor

@krunchington krunchington commented Mar 17, 2025

Objective

I experienced an issue where HashMap::new was not returning a value typed appropriately for a HashMap<K,V> declaration that omitted the Hasher- e.g. the Default Hasher for the type is different than what the new method produces.

After discussion on discord, this appears to be an issue in hashbrown, and working around it would be very nontrivial, requiring a newtype on top of the hashbrown implementation. Rather than doing that, it was suggested that we add docs to make the issue more visible and provide a clear workaround.

Solution

Updated the docs for bevy_platform_support::collections. I couldn't update Struct docs because they're re-exports, so I had to settle for the module.

Note that the [HashMap::new] link wasn't generating properly- I'm not sure why. I see the method in the docs.rs site, https://docs.rs/hashbrown/0.15.1/hashbrown/struct.HashMap.html#method.new, but not on the generated internal documentation. I wonder if hashbrown isn't actually implementing the new or something?

Testing

n/a although I did generate and open the docs on my Ubuntu machine.


Showcase

before:
image

after:
image

Copy link
Copy Markdown
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some slight changes to wording and formatting. Unless Bevy wants to new-type HashMap/HashSet I agree this is the best we can do to document the issue.

@krunchington
Copy link
Copy Markdown
Contributor Author

I committed the suggested changes but I suspect the docs CI will fail on the HashMap::new link again. I can unlink that if it does.

@krunchington
Copy link
Copy Markdown
Contributor Author

NVM it was the example with imports. Should be fixed now though I had to put a dev dependency on bevy_ecs

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Utils Utility functions and types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 17, 2025
Copy link
Copy Markdown
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 17, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

CI is upset at you:

error[wildcard]: found 1 wildcard dependency for crate 'bevy_platform_support'

This needs to be fixed before we can merge.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Mar 17, 2025
@krunchington
Copy link
Copy Markdown
Contributor Author

CI is upset at you:

error[wildcard]: found 1 wildcard dependency for crate 'bevy_platform_support'

This needs to be fixed before we can merge.

Damn I knew I should have checked how other crates pulled internal dependencies and not yolo'd it. Should be fixed now!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 17, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2025
Merged via the queue into bevyengine:main with commit e5158ed Mar 17, 2025
36 checks passed
mockersf added a commit to mockersf/bevy that referenced this pull request Mar 18, 2025
@krunchington krunchington deleted the hashmap-hasher-issue-docs branch March 22, 2025 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Utils Utility functions and types C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants