Skip to content

Remove incorrect equality comparisons for asset load error types #15890

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

Merged
merged 25 commits into from
Oct 14, 2024

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Oct 13, 2024

Objective

The type AssetLoadError has PartialEq and Eq impls, which is problematic due to the fact that the AssetLoaderError and AddAsyncError variants lie in their impls: they will return true for any Box<dyn Error> with the same TypeId, even if the actual value is different. This can lead to subtle bugs if a user relies on the equality comparison to ensure that two values are equal.

The same is true for DependencyLoadState, RecursiveDependencyLoadState.

More generally, it is an anti-pattern for large error types involving dynamic dispatch, such as AssetLoadError, to have equality comparisons. Directly comparing two errors for equality is usually not desired -- if some logic needs to branch based on the value of an error, it is usually more correct to check for specific variants and inspect their fields.

As far as I can tell, the only reason these errors have equality comparisons is because the LoadState enum wraps AssetLoadError for its Failed variant. This equality comparison is only used to check for == LoadState::Loaded, which we can easily replace with an is_loaded method.

Solution

Remove the {Partial}Eq impls from LoadState, which also allows us to remove it from the error types.

Adopted from #14428 by @JoJoJet.

Migration Guide

The types bevy_asset::AssetLoadError and bevy_asset::LoadState no longer support equality comparisons. If you need to check for an asset's load state, consider checking for a specific variant using LoadState::is_loaded or the matches! macro. Similarly, consider using the matches! macro to check for specific variants of the AssetLoadError type if you need to inspect the value of an asset load error in your code.

DependencyLoadState and RecursiveDependencyLoadState are not released yet, so no migration needed,

@BenjaminBrienen BenjaminBrienen self-assigned this Oct 13, 2024
@BenjaminBrienen BenjaminBrienen added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 13, 2024
@BenjaminBrienen BenjaminBrienen added this to the 0.15 milestone Oct 13, 2024
@mockersf
Copy link
Member

mockersf commented Oct 13, 2024

Could you either open your PR against @JoJoJet branch so that they can merge it, or copy the original title/description in this PR?
If merged, this would add a commit "continuation of #14428" in the git log of Bevy which wouldn't be helpful.

Same for sections of the description, it's better for a PR to be self contained rather than have to refer to another. Some are also used by automated tools which don't know how to follow redirections

@BenjaminBrienen BenjaminBrienen changed the title Continuation of #14428 Remove equality comparisons for asset load error types Oct 13, 2024
@BenjaminBrienen BenjaminBrienen changed the title Remove equality comparisons for asset load error types Remove incorrect equality comparisons for asset load error types by Oct 13, 2024
@BenjaminBrienen BenjaminBrienen changed the title Remove incorrect equality comparisons for asset load error types by Remove incorrect equality comparisons for asset load error types Oct 13, 2024
@BenjaminBrienen
Copy link
Contributor Author

Could you either open your PR against @JoJoJet branch so that they can merge it, or copy the original title/description in this PR?
If merged, this would add a commit "continuation of #14428" in the git log of Bevy which wouldn't be helpful.

Same for sections of the description, it's better for a PR to be self contained rather than have to refer to another. Some are also used by automated tools which don't know how to follow redirections

Done!

@BenjaminBrienen
Copy link
Contributor Author

Please feel free to leave a reply on my review comments and then I'll feel comfortable refining the PR with that feedback.

@BenjaminBrienen
Copy link
Contributor Author

@alice-i-cecile feedback is addressed

I also realized that the merge conflict involved 2 new error types, which should have the same process done to them. It is much better now, I think. I'll update the migration guide.

@BenjaminBrienen
Copy link
Contributor Author

@andriyDev I've gone through and double checked all the assertions. Sorry for the copy paste errors!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 14, 2024
@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 Oct 14, 2024
Merged via the queue into bevyengine:main with commit 93fc2d1 Oct 14, 2024
28 checks passed
@BenjaminBrienen BenjaminBrienen deleted the no-error-eq branch October 14, 2024 09:06
@BenjaminBrienen BenjaminBrienen removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 10, 2024
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants