Skip to content

Fix AssetChanged<A> UB#23138

Merged
alice-i-cecile merged 5 commits intobevyengine:mainfrom
Trashtalk217:fix-asset-changed
Mar 1, 2026
Merged

Fix AssetChanged<A> UB#23138
alice-i-cecile merged 5 commits intobevyengine:mainfrom
Trashtalk217:fix-asset-changed

Conversation

@Trashtalk217
Copy link
Copy Markdown
Contributor

Objective

Fixes #23057.

Solution

Uses Nested Queries from #21557 to also register the resource entity to the access set.

Testing

Added an extra test.

@alice-i-cecile alice-i-cecile added this to the 0.19 milestone Feb 25, 2026
@alice-i-cecile
Copy link
Copy Markdown
Member

Regression test please :)

@Trashtalk217 Trashtalk217 added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds labels Feb 25, 2026
@github-project-automation github-project-automation bot moved this to Needs SME Triage in Assets Feb 25, 2026
@Trashtalk217
Copy link
Copy Markdown
Contributor Author

I did. That's the extra test I added.

@Trashtalk217 Trashtalk217 added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Feb 25, 2026
@alice-i-cecile alice-i-cecile added the P-Unsound A bug that results in undefined compiler behavior label Feb 25, 2026
@alice-i-cecile
Copy link
Copy Markdown
Member

Ah sorry, missed that!

Copy link
Copy Markdown
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

Yay!

One nit on comments: GitHub won't let me comment there, but there is a safety comment in init_fetch that says

        // - `update_component_access` declares a read on `state.resource_id`, so it is safe to
        //   read that resource here (see trait-level safety comments on `WorldQuery`, regarding
        //   readonly resource access in `init_fetch`)

and should probably be changed to mention init_nested_access instead.

Trashtalk217 and others added 2 commits February 26, 2026 16:13
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

let mut filter = FilteredAccess::default();
filter.add_component_read(state.resource_id);
filter.add_resource_read(state.resource_id);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need both a component read and resource read?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This ensures that this correctly conflicts with FilteredResources, which only uses resource accesses, and Query<&mut MyResource>, which only uses component accesses. This is fixed in #22910, which comes right after this PR.

@Trashtalk217 Trashtalk217 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 1, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 1, 2026
Merged via the queue into bevyengine:main with commit c8df993 Mar 1, 2026
40 checks passed
@github-project-automation github-project-automation bot moved this from Needs SME Triage to Done in Assets Mar 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior P-Unsound A bug that results in undefined compiler 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

Status: Done

Development

Successfully merging this pull request may close these issues.

Potential for UB in systems using AssetChanged

4 participants