Rename EntityBorrow/TrustedEntityBorrow to ContainsEntity/EntityEquivalent

[Victoronz]

Objective

Fixes #9367.

Yet another follow-up to #16547.

These traits were initially based on Borrow<Entity> because that trait was what they were replacing, and it felt close enough in meaning.
However, they ultimately don't quite match: borrow always returns references, whereas EntityBorrow always returns a plain Entity.
Additionally, EntityBorrow can imply that we are borrowing an Entity from the ECS, which is not what it does.

Due to its safety contract, TrustedEntityBorrow is important an important and widely used trait for EntitySet functionality.
In contrast, the safe EntityBorrow does not see much use, because even outside of EntitySet-related functionality, it is a better idea to accept TrustedEntityBorrow over EntityBorrow.

Furthermore, as #9367 points out, abstracting over returning Entity from pointers/structs that contain it can skip some ergonomic friction.

On top of that, there are aspects of #18319 and #18408 that are relevant to naming:
We've run into the issue that relying on a type default can switch generic order. This is livable in some contexts, but unacceptable in others.

To remedy that, we'd need to switch to a type alias approach:
The "defaulted" Entity case becomes a UniqueEntity*/Entity*Map/Entity*Set alias, and the base type receives a more general name. TrustedEntityBorrow does not mesh clearly with sensible base type names.

Solution

Replace any EntityBorrow bounds with TrustedEntityBorrow.
+
Rename them as such:
EntityBorrow -> ContainsEntity
TrustedEntityBorrow -> EntityEquivalent

For EntityBorrow we produce a change in meaning; We designate it for types that aren't necessarily strict wrappers around Entity or some pointer to Entity, but rather any of the myriad of types that contain a single associated Entity.
This pattern can already be seen in the common entity/id methods across the engine.
We do not mean for ContainsEntity to be a trait that abstracts input API (like how AsRef<T> is often used, f.e.), because eliding entity() would be too implicit in the general case.

We prefix "Contains" to match the intuition of a struct with an Entity field, like some contain a length or capacity.
It gives the impression of structure, which avoids the implication of a relationship to the ECS.
HasEntity f.e. could be interpreted as "a currently live entity",

As an input trait for APIs like #9367 envisioned, TrustedEntityBorrow is a better fit, because it does restrict itself to strict wrappers and pointers. Which is why we replace any EntityBorrow/ContainsEntity bounds with TrustedEntityBorrow/EntityEquivalent.

Here, the name EntityEquivalent is a lot closer to its actual meaning, which is "A type that is both equivalent to an Entity, and forms the same total order when compared".
Prior art for this is the Equivalent trait in hashbrown, which utilizes both Borrow and Eq for its one blanket impl!

Given that we lose the Borrow moniker, and Equivalent can carry various meanings, we expand on the safety comment of EntityEquivalent somewhat. That should help prevent the confusion we saw in #18408.

The new name meshes a lot better with the type aliasing approach in #18408, by aligning with the base name EntityEquivalentHashMap.
For a consistent scheme among all set types, we can use this scheme for the UniqueEntity* wrapper types as well!
This allows us to undo the switched generic order that was introduced to UniqueEntityArray by its Entity default.

Even without the type aliases, I think these renames are worth doing!

Migration Guide

Any use of EntityBorrow becomes ContainsEntity.
Any use of TrustedEntityBorrow becomes EntityEquivalent.

[Guvante]

I didn't see this called out in the description (hard to tell from this if the tightening here is important)

[Victoronz]

I touched on it with "We should only accept EntityEquivalent in our APIs".

This PR represent a loosening of EntityBorrow/ContainsEntity, which means it is now too broad/implicit to accept in our API.
By just accepting EntityEquivalent, we have a strong guarantee that only types close to Entity get passed to our code, and that we do not see strange behavior in entity() calls.

[chescock]

Yeah, I wasn't expecting that change, either. It seems reasonable! It's just hard to notice amid the similar non-functional changes. It might help to mention explicitly in the first few lines of the PR description that you're changing the bound on Query::iter_many and friends.

[Victoronz]

Fair enough, I'll add this to the PR description!

[chescock]

Looks good! EntityEquivalent does seem more clear than TrustedEntityBorrow, especially as there is no longer any borrowing involved!

[chescock]

I'd consider a deterministic Hash to be one that makes a deterministic sequence of calls to the provided Hasher. The fact that the Hasher can do non-deterministic things doesn't change the fact that the Hash was deterministic.

It might be simpler to say that Hash::hash must be implemented as self.entity().hash(state);, since there isn't really any reason to implement it any other way.

[Victoronz]

This is similar to the argument I made in #18408 but I'll describe my current thoughts here:
I think I have to rewrite the Hash section again, because the "total order equivalence" is something this trait or any specific implementation of the Hash trait just cannot guarantee. The only reason why we are able to use HashSets/HashMaps is that they have recovery logic in case of collisions, they don't need to receive a total order of hashes to produce a "set". All they need is deterministic hashes, deterministic Eq impls, and matching Borrow.

A deterministic Hash impl and a deterministically obtained hash value are not the same, and the latter is also something this trait cannot guarantee.
Because a Hash impl is required by the trait definition to be generic over all H: Hasher, it can only be deterministic for all H if it does not use H at all.
Because determinism cannot be guaranteed for all H, we need to limit the guarantee to those H for which it can!

All we can do is to then explicitly state that for a final hash to be unique, there need to be additional, separate guarantees that H is deterministic for all of its trait methods, and that the Hasher/Hash combination be collisionless.
If the combination is not collisionless, an Eq fallback is required, like what HashSet/HashMap do.

With such a description, both us library authors and end users can determine when a HashSet can be relied on as a "set" by unsafe code, regardless of what the actual hash set type is. Whether that might be hashbrown::HashSet, indexmap::IndexSet, our wrappers, or something else entirely.

[Victoronz]

I changed the requirement to "must delegate Hash". And added an explanation concerning its use!

[chescock]

What's the purpose of the ContainsEntity trait? If we don't want to abstract over it in APIs, then it doesn't seem to offer any advantage over a convention of having an entity() method.

If we aren't going to use it as a bound in iter_many then we might as well get rid of the separate trait and move the entity() method down to EntityEquivalent.

For a consistent scheme among all set types, we can use this scheme for the UniqueEntity* wrapper types as well!
This allows us to undo the switched generic order that was introduced to UniqueEntityArray by its Entity default.

Your plan here is struct UniqueEntityEquivalentArray<T, N> and type UniqueEntityArray<N> = UniqueEntityEquivalentArray<Entity, N>, then?

[Victoronz]

What's the purpose of the ContainsEntity trait? If we don't want to abstract over it in APIs, then it doesn't seem to offer any advantage over a convention of having an entity() method.

If we aren't going to use it as a bound in iter_many then we might as well get rid of the separate trait and move the entity() method down to EntityEquivalent.

Not being a bound doesn't mean that it isn't useful! The intent is that an entity() call should only be implicit if a type is close enough to Entity. If it isn't, the intent is for the user to then call entity() themselves.

It is a common pattern for types that have an Entity associated with them to implement an entity or id method.
But currently, there is no standard or consistent meaning for such methods, and is sometimes even omitted. What this trait does is provide a standard abstraction over that pattern!
When I myself was learning Bevy, I struggled with knowing whether you could get an Entity from a struct, and wished there was a way of navigating to those that had such API.
In that sense ContainsEntity is a searchable, standardized marker, and can be made derivable later on!

For a consistent scheme among all set types, we can use this scheme for the UniqueEntity* wrapper types as well!
This allows us to undo the switched generic order that was introduced to UniqueEntityArray by its Entity default.

Your plan here is struct UniqueEntityEquivalentArray<T, N> and type UniqueEntityArray<N> = UniqueEntityEquivalentArray<Entity, N>, then?

Yeah!

[alice-i-cecile]

Please bother me when this is finished and I'll merge it for you :)

[Victoronz]

@alice-i-cecile Finished!