Leave the many methods in place for now, since there isn't an obvious name for the panicking version of get. This has the odd result that you can now call them with a single entity, but it's at least backwards-compatible. (If someone thinks of a great name for these, I'm happy to add new methods with the better name!)

IMO we should just deprecate the panicking versions here. We're steadily pushing towards removing this panic-by-default philosophy now that we have system error handling.

IMO we should just deprecate the panicking versions here. We're steadily pushing towards removing this panic-by-default philosophy now that we have system error handling.

Sounds good! I updated the PR to deprecate those, as well.

If I've understood the structure correctly, this uses the same fetch_query_data impl for both get and get_mut, this means that the O(n^2) equality check that the [Entity; N] and &[Entity] impls do will happen for immutable fetching as well, which is unnecessary!

Edit: Ah, that is what the read-only bool is for, nevermind!

I would like to note that reducing API surface isn't necessarily a good thing, but rather a trade-off.
While on one hand it is nice that get can be made more powerful, on the other, removing this API surface too means removing documentation/example surface, makes searching for the variations more difficult, and diverges more from the way std structures its API. I wouldn't refer to it as deduplication, but rather as reorganizing complexity.
While not necessarily bad, these things should be considered carefully.

I am somewhat iffy about the impls for [Entity; N], &[Entity] and EntityHashSet being combined in one trait.
Mutable &[Entity] fetching is a performance footgun; get_many style fetching was always intended for low N, because of the O(n^2) equality check.

While this O(n^2) could be replaced with HashSet-style equality checking, I think it'd be more appropriate to make this explicit to the caller. The decision of how to handle duplicates is better left to the caller before fetching, which we can do by exposing some try_into/to_-style conversion logic for entity lists.
At the very least, it should be clearly documented that these equality checks are expensive.

I am also not too keen on the slice -> Vec mapping in a get method call, which looks very implicit.
iter_many(slice) should be encouraged instead, because it is then up to the user whether they want to consume the entire thing, and what exactly they want to collect it into. Its use will also be clearer to the reader.

The EntityHashSet impl seems too opinionated/restrictive to me, since the API implies that creating a Map from a Set is an encouraged way for creating an EntityHashMap.
To create an EntityHashMap, there is no need to restrict ourselves to passing an EntityHashSet, any unique list of entities/EntitySet can work for this. Even non-unique entity lists could be used, at the cost of k contains() calls on the map we are constructing.
If we start out with anything other than a EntityHashSet, converting to one to be capable of using this API is an expensive and unnecessary operation!

I'd also suggest here that we encourage iter_many.collect() for map creation. This does require Entity in the Query, but that can be mitigated by dedicated collection method if need be.

On the other hand, many users of EntityHashSet just want to access the data, and not create a map type, for whom this impl is also inefficient for.

When designing EntitySet, it was immensely helpful to be able to search for get_many and other method variations on both Github and various places of discussions! With this merge, such searches would be very difficult, if downright not doable.

Overall, how to fetch from a Query is one of the first things new users might learn about; I think it important to keep consistency here, and not to have too many "avoid doing this" inside get alone.
Keeping them separate also makes it easier to draw parallels/intuition between how get_*, std and related methods (like the iter_* counterparts) work!

That being said, these are my opinions!

Funny to see the constant-in-trait approach again.

Yeah, I stole the idea from QueryFilter::IS_ARCHETYPAL :). It's a neat way to work around the lack of specialization!

Can you run a quick set of benchmarks before I merge this to ensure we haven't somehow regressed Query::get performance?

Benchmarks always seem a little flaky on my machine, but I think I can do one better and check that the assembly hasn't changed.
... Here, I wrote

use crate::prelude::Component;
#[derive(Component)]
struct C(usize);

#[unsafe(no_mangle)]
#[inline(never)]
pub fn call_get(query: Query<&C>, entity: Entity) -> Option<usize> {
    Some(query.get(entity).ok()?.0)
}

and ran

cargo asm -p bevy_ecs --features bevy_reflect --lib --simplify call_get

and the resulting assembly was completely identical before or after the change:

call_get:
	mov rax, qword ptr [rcx + 8]
	mov r8d, edx
	cmp r8, qword ptr [rax + 16]
	jae .LBB1676_1
	mov r9, qword ptr [rax + 8]
	shr rdx, 32
	lea r8, [r8 + 4*r8]
	cmp dword ptr [r9 + 4*r8], edx
	jne .LBB1676_1
	lea rdx, [r9 + 4*r8]
	mov r8d, dword ptr [rdx + 4]
	mov r9d, 4294967295
	cmp r8, r9
	je .LBB1676_1
	mov rcx, qword ptr [rcx]
	cmp qword ptr [rcx + 56], r8
	jbe .LBB1676_1
	mov r9, qword ptr [rcx + 40]
	mov r10d, r8d
	shr r10d, 6
	mov r9, qword ptr [r9 + 8*r10]
	bt r9, r8
	jae .LBB1676_1
	mov r8d, dword ptr [rdx + 12]
	mov edx, dword ptr [rdx + 16]
	mov rax, qword ptr [rax + 328]
	lea r8, [r8 + 8*r8]
	mov rcx, qword ptr [rcx + 272]
	mov r9, qword ptr [rax + 8*r8 + 24]
	mov rax, qword ptr [rax + 8*r8 + 56]
	mov rax, qword ptr [rax + 8*rcx]
	not rax
	lea rax, [rax + 2*rax]
	shl rax, 4
	mov rax, qword ptr [r9 + rax + 16]
	mov rdx, qword ptr [rax + 8*rdx]
	mov eax, 1
	ret
.LBB1676_1:
	xor eax, eax
	ret

I would like to note that reducing API surface isn't necessarily a good thing, but rather a trade-off.
While on one hand it is nice that get can be made more powerful, on the other, removing this API surface too means removing documentation/example surface, makes searching for the variations more difficult, and diverges more from the way std structures its API. I wouldn't refer to it as deduplication, but rather as reorganizing complexity.
While not necessarily bad, these things should be considered carefully.

Yup, it's definitely a tradeoff!

The main thing I find compelling about this approach is that we have several orthogonal axes here, and it lets us use M + N items instead of M * N. In particular, we need &self, &mut self, and self versions of all of these. (Previously we also needed to double that for panicking versions, but if we're deprecating those then it's not quite as bad.)

I also like that we're able to share a single trait between Query::get and World::entity. (And if we do decide to keep separate methods here, then we may want to revisit whether we really want separate methods there.)

I am also not too keen on the slice -> Vec mapping in a get method call, which looks very implicit.
The EntityHashSet impl seems too opinionated/restrictive to me, since the API implies that creating a Map from a Set is an encouraged way for creating an EntityHashMap.

I really want to keep these consistent between Query::get and World::entity. I'm happy to change those impls in the future, but I think we should change the behavior for World::entity at the same time.

Maybe what we want is a World::iter_entities that takes an EntitySet! Then we can get rid of the slice and EntityHashSet impls and just have the array ones. Users that want a Vec can do iter_entities().collect(), and users that just want to iterate don't have to pay for the collect().

I think that the increased complexity for an essential beginner API (Query::get) isn't worth deduplicating here. Nominating to close :)

I do like the consistency and deduplication in general, I just think it's important to keep the most used APIs as simple as possible.

I do like the consistency and deduplication in general, I just think it's important to keep the most used APIs as simple as possible.

Sounds good!

As a follow-up, if we like consistency... should we consider getting rid of WorldEntityFetch and DynamicComponentFetch, and restoring the _many methods on World and EntityMut? World::entity isn't quite as common as Query::get, but it's still one of the earlier ones you use when working with &mut World. And the combinatoric explosion won't be as bad now that we don't want panicking variants.

The main thing I find compelling about this approach is that we have several orthogonal axes here, and it lets us use M + N items instead of M * N. In particular, we need &self, &mut self, and self versions of all of these. (Previously we also needed to double that for panicking versions, but if we're deprecating those then it's not quite as bad.)

While having less methods is good, combining axes is usually only done when there is one clear conceptual interpretation.
With Index, usize-indexing is the n=1 case of slicing, for example.
Orthogonal axes are not a perfect story in Rust, and hard to solve (see keyword generics), but I don't think it is that bad here yet.

I also like that we're able to share a single trait between Query::get and World::entity. (And if we do decide to keep separate methods here, then we may want to revisit whether we really want separate methods there.)

I am also not too keen on the slice -> Vec mapping in a get method call, which looks very implicit.
The EntityHashSet impl seems too opinionated/restrictive to me, since the API implies that creating a Map from a Set is an encouraged way for creating an EntityHashMap.

I really want to keep these consistent between Query::get and World::entity. I'm happy to change those impls in the future, but I think we should change the behavior for World::entity at the same time.

I too would like consistency, and I'd say it is better to restore the separation in the World methods as well.

Maybe what we want is a World::iter_entities that takes an EntitySet! Then we can get rid of the slice and EntityHashSet impls and just have the array ones. Users that want a Vec can do iter_entities().collect(), and users that just want to iterate don't have to pay for the collect().

Yeah, a World::iter_many_unique is a good idea!