Deprecate `Query::get_many` (etc) and make `many` return a Result by alice-i-cecile · Pull Request #18120 · bevyengine/bevy

alice-i-cecile · 2025-03-02T19:56:00Z

Objective

This PR is the less contentious sibling to #18082. See that PR for detailed motivation!

This PR affects APIs that are called much less often: as a result, handling said Result is much less onerous.

Solution

Make the short Query::many methods return a Result.
Deprecate the Query::get_many methods now that they've been renamed.
Fix up all the warnings.

Testing

No more compiler errors left! We'll see what CI finds.

Migration Guide

As part of a broad shift to reduce panics in Bevy, Query::many, Query::many_mut and the same methods on QueryState now return a Result, which should be handled by the caller. The get_many and get_many_mut methods that previously did this operation while returning a Result have been deprecated, as they have effectively been renamed.

alice-i-cecile · 2025-03-02T19:56:50Z

@Carter0 can I have your review here too? Double check that I didn't screw anything up: this was an annoying change set due to all the duplicated APIs.

crates/bevy_ecs/src/query/state.rs

Carter0 · 2025-03-02T22:08:16Z

crates/bevy_ecs/src/system/query.rs

+        entities: [Entity; N],
+    ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> {
+        // Verify that all entities are unique
+        for i in 0..N {


Could you do something like this if you wanted to? It might be easier to understand. Totally a nit though

if len(x) > len(set(x)):

Probably, but I've opted to not change the logic in this PR. Despite the diff, this was just a pair of renames.

Carter0 · 2025-03-02T22:11:06Z

crates/bevy_ecs/src/system/query.rs

-    ///     y: f32,
-    /// }
-    ///
-    /// fn spring_forces(spring_query: Query<&Spring>, mut mass_query: Query<(&Position, &mut Force)>){


Is it possible to move these docs to the result version of many_mut with some changes? Its seems like such a nice example!

I like these too, and I've double-checked: we already have nice doc tests for all of the retained result versions of these methods :)

Carter0

I like it overall! I'm down with returning results!

Though I actually don't really like the 'many' function name, which is a different thing than this PR is trying to deal with. I actually sorta like the name get_many more since it's an action, like we are "getting" components. But whatever haha

Oh also, doc comment stuff

chescock

Yay!

crates/bevy_ecs/src/query/mod.rs

chescock · 2025-03-02T21:51:04Z

crates/bevy_ecs/src/query/state.rs

    /// ```
    #[inline]
-    pub fn get_many<'w, const N: usize>(
+    pub fn many<'w, const N: usize>(


For what it's worth, I'm hoping to deprecate all the querying methods on QueryState in favor of the methods on Query. If we think that's likely to be approved, then we might want to deprecate these two and recommend query(world).many_inner(entities) now so that users don't have to first change get_many() to many() and then change a second time to use query().

I think it's unlikely that that change makes it into 0.16.

Oh, I know! I wasn't even going to attempt it for 0.16 :).

But if we expect to do it in 0.17, then it might be nice to advise users of QueryState::get_many to switch to query directly instead of switching to QueryState::many in 0.16 and then having to update that code again in the next release.

I think that the set of users calling QueryState::get_many is small enough that I'm not concerned about that :)

chescock · 2025-03-02T21:53:06Z

crates/bevy_ecs/src/system/query.rs

+        self,
+        entities: [Entity; N],
+    ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> {
+        // Verify that all entities are unique


Should this delegate to many_inner rather than duplicating the code?

Probably, but I've opted to not change the logic in this PR. Despite the diff, this was just a pair of renames.

alice-i-cecile · 2025-03-02T22:52:07Z

Man, the diff is terrible on Github. Git really does not like the renames. Strongly recommend checking out the code locally if you end up wanting to review further.

Victoronz · 2025-03-03T06:24:06Z

I think it'd be nice to have doc aliases of the old names on the new ones! Even once the old ones disappear, users might still search for the equivalent of the get_many that exists in std.

(I personally prefer get_many over many, but I don't feel particularly strongly about it)

Victoronz · 2025-03-04T00:01:26Z

Hmm, thinking on it a bit more I am actually unsure about this change.

It seems the reasoning for this is the same as for single, because both get_single and get_many had panicking single and many counterparts, but I don't think their situations are that similar.

With single, there is one unambiguous behavior, and it mirrors APIs like first() and next(), so it makes sense to be the default.
It is nicely consistent with the Single system parameter.

However with get_many, it isn't really a default, nor is there a Many system parameter. It is but one way of "getting many values".
The other, more preferable way would be iter_many. Having both many and iter_many together leaves their relationship ambiguous, as it is not clear in what way iter_many "builds" on many.
To me, shorter names are better only as long as they don't introduce ambiguity/increase mental load!

Moreover, get_many's behavior is to eagerly retrieve values for each key, which matches get_-style behavior.

In addition, std has recently decided to rename its get_many to get_disjoint, exactly because they think it isn't designed with "many" in mind, which our get_many isn't either.
Does that mean we want to rename many into disjoint later? The meaning of that without the get would be unclear imo.

Further, unlike usual get methods, the std get_many/get_disjoint method does actually return Result, not Option.

I'd personally say we remove the panicking many and rename get_many into get_disjoint!

But that's my few cents, the decision is still left up to you!

alice-i-cecile · 2025-03-04T00:17:01Z

I'd personally say we remove the panicking many and rename get_many into get_disjoint!

I'd like to avoid more renames as part of this PR: we can always tweak that later. How would you feel about just deprecating the panicking many methods? Trivial migration, and hits the main goals of this PR (deduplication, fewer panics).

Victoronz · 2025-03-04T00:26:43Z

I'd personally say we remove the panicking many and rename get_many into get_disjoint!

I'd like to avoid more renames as part of this PR: we can always tweak that later. How would you feel about just deprecating the panicking many methods? Trivial migration, and hits the main goals of this PR (deduplication, fewer panics).

That'd be completely fine! They always seemed somewhat weird to me. The same criticisms already apply to the current panicking many methods!

bushrat011899

Same reasons I approved the sibling PR. Just noted there appears to be a leftover hunk from a merge conflict.

crates/bevy_ecs/src/system/query.rs

alice-i-cecile · 2025-03-06T21:35:11Z

On reflection, I prefer the simple deprecation approach here. Closing.

# Objective Alternative to and closes #18120. Sibling to #18082, see that PR for broader reasoning. Folks weren't sold on the name `many` (get_many is clearer, and this is rare), and that PR is much more complex. ## Solution - Simply deprecate `Query::many` and `Query::many_mut` - Clean up internal usages Mentions of this in the docs can wait until it's fully removed in the 0.17 cycle IMO: it's much easier to catch the problems when doing that. ## Testing CI! ## Migration Guide `Query::many` and `Query::many_mut` have been deprecated to reduce panics and API duplication. Use `Query::get_many` and `Query::get_many_mut` instead, and handle the `Result`. --------- Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

Deprecate Query::get_many (etc) and make many return a Result

6029554

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 2, 2025

alice-i-cecile added this to the 0.16 milestone Mar 2, 2025

alice-i-cecile requested a review from bushrat011899 March 2, 2025 19:56

Carter0 reviewed Mar 2, 2025

View reviewed changes

crates/bevy_ecs/src/query/state.rs Show resolved Hide resolved

Carter0 reviewed Mar 2, 2025

View reviewed changes

crates/bevy_ecs/src/query/state.rs Show resolved Hide resolved

Carter0 reviewed Mar 2, 2025

View reviewed changes

Carter0 approved these changes Mar 2, 2025

View reviewed changes

chescock approved these changes Mar 2, 2025

View reviewed changes

alice-i-cecile added 3 commits March 2, 2025 14:38

Deduplicate test

cf93767

Hunt down remaining get_many usages

4a8f705

Merge branch 'main' into resultful-many

098698b

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 2, 2025

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 3, 2025

bushrat011899 approved these changes Mar 4, 2025

View reviewed changes

crates/bevy_ecs/src/system/query.rs Show resolved Hide resolved

alice-i-cecile commented Mar 6, 2025

View reviewed changes

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

Merge conflict garbage

85f4a57

alice-i-cecile mentioned this pull request Mar 6, 2025

Deprecated Query::many and many_mut #18183

Merged

alice-i-cecile closed this Mar 6, 2025

BD103 mentioned this pull request Mar 30, 2025

Rename get_many() to many(), making it consistent with single() #18615

Closed

Uh oh!

Conversation

alice-i-cecile commented Mar 2, 2025

Objective

Solution

Testing

Migration Guide

Uh oh!

alice-i-cecile commented Mar 2, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carter0 Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carter0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chescock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Mar 2, 2025

Uh oh!

Victoronz commented Mar 3, 2025

Uh oh!

Victoronz commented Mar 4, 2025

Uh oh!

alice-i-cecile commented Mar 4, 2025

Uh oh!

Victoronz commented Mar 4, 2025

Uh oh!

bushrat011899 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Carter0 Mar 2, 2025 •

edited

Loading

Carter0 left a comment •

edited

Loading