Skip to content

Fix ComponentsRegistrator unsoundness #20187

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SkiFire13
Copy link
Contributor

Objective

Solution

  • Avoid implementing DerefMut for ComponentsRegistrator
  • To avoid potential breakage, expose the any_queued_mut and num_queued_mut on ComponentsRegistrator. These are the only methods taking &mut self on Components and for which the DerefMut impl could have been useful for.

@SkiFire13 SkiFire13 force-pushed the fix-components-registrator-unsoundness branch from 7ab3413 to c7d5ec6 Compare July 18, 2025 11:46
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Makes sense to me. &mut Components should definitely never be exposed to users. I do wonder if it's even worth exposing any_queued_mut and num_queued_mut as they seem pretty niche and are just faster versions of any_queued and num_queued. Feels like it's not worth complicating the api of ComponentsRegistrator for that. But I don't see any problems arising from exposing them. So not a big deal.

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jul 19, 2025
@JaySpruce JaySpruce added A-ECS Entities, components, systems, and events S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Bug An unexpected or incorrect behavior labels Jul 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior 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.

ComponentsRegistrator exposes access to &mut Components
4 participants