Make sure #[track_caller] in EntityCommands::insert_if_neq works as intended#23986
Merged
alice-i-cecile merged 3 commits intobevyengine:mainfrom Apr 28, 2026
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Updates EntityCommands::insert_if_neq to correctly preserve #[track_caller] information through the deferred command closure, ensuring get_changed_by reports the user callsite as intended (fixing #23962).
Changes:
- Capture
MaybeLocation::caller()outside the queued closure and thread it throughEntityWorldMut::insert_with_caller. - Switch insertion in
insert_if_neqtoinsert_with_caller(..., InsertMode::Replace, caller, RelationshipHookMode::Run). - Add a unit test intended to validate caller tracking for
insert_if_neq.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alice-i-cecile
requested changes
Apr 25, 2026
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Code looks correct, but test needs fixing.
alice-i-cecile
added
C-Bug
alice-i-cecile
added
S-Waiting-on-Author
alice-i-cecile
approved these changes
Apr 26, 2026
alice-i-cecile
added
S-Needs-Review
chescock
approved these changes
Apr 27, 2026
| command_queue.apply(&mut world); | ||
| world.clear_trackers(); | ||
|
|
||
| macro_rules! insert_if_neq_with_expected_caller { |
Contributor
There was a problem hiding this comment.
Oh, does using a macro let you get the same Location for both places? That's a really neat trick!
Comment on lines
+2762
to
+2766
| let entity = Commands::new(&mut command_queue, &world) | ||
| .spawn(P(41u8)) | ||
| .id(); | ||
| command_queue.apply(&mut world); | ||
| world.clear_trackers(); |
Contributor
There was a problem hiding this comment.
This might be simpler if the spawn didn't go through Commands. ... Ah, although I see the previous test does the same thing, so maybe it's better to be consistent.
Suggested change
| let entity = Commands::new(&mut command_queue, &world) | |
| .spawn(P(41u8)) | |
| .id(); | |
| command_queue.apply(&mut world); | |
| world.clear_trackers(); | |
| let entity = world.spawn(P(41u8)).id(); |
alice-i-cecile
added
S-Ready-For-Final-Review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
intended
Objective
Fixes #23962
Solution
As suggested in the issue: "go back to insert_with_caller so that we can call MaybeLocation::caller() outside of the closure and then pass it through explicitly".
Testing
Added a unit test and made sure it fails without the fix.