Allow entity wrappers in entity set and map types #18408
Conversation
Victoronz
added
A-ECS
I don't understand the attack here. |
You're right, I didn't use the correct language here. For the current implementation, because To be more accurate with my intent: "To ensure this implementation remains sound under future extensions/refactors, we introduce a new trait...". It is feasible to want to extend the implementation to more hashers, whether that is to be able to use Even aside from the soundness, we want to have a trait that limits |
Do you have concrete plans for those extensions? Otherwise it feels like premature generalization. If we don't need it yet, then there's no reason to build it yet! Either way, I still don't see why
Why would we want to limit the combinations? As I said above, |
That might be the case, I did conflate the purposes of "must be a sound
The problem is that both the
The intent is not for It seems we might want to expand on the |
Ah, this must be the miscommunication! I've been assuming it works the same was as I'd be in favor of changing that, if we can. I think it would just be a doc change, since all the existing impls do follow that. And those of us who don't read docs very carefully are likely to assume that
Yup, I agree we need an |
I made a rename/doc PR concerning this (and other doc/name issues) in #18470! |
If we split the trait, we'd end up with this structure:
The The only way to prevent this is to have a layer that accounts for the full picture, which means an impl on one side that carries a generic of the other. (Instead of requiring that a In the case of this PR, you are right that we can punt on is We still need to limit inputs to Meaning that while we can punt on the safety parts, the combination checking and consequent bounds on our set/map types will need to remain, just under a different name. :P |
I said here While we might not need the traits themselves, we should still describe this situation in the safety comment of |
Why do we need to limit the combinations? The only safety requirement we have is that the hashing be deterministic. It's fine for
Sure, it does. A deterministic
|
|
This is a really subtle discussion! It has design considerations on multiple levels, which I find/can make it difficult to hone in on where the different aspects come in/from, I hope the back and forth isn't getting frustrating!
Safety-wise, panicking is completely fine! First, let me paint a situation from a standpoint of compatibility: Let's have a look from a performance standpoint: The problems that are created by incompatible
Yeah! Though I think even panicking non-deterministically is fine, "being deterministic" is a property only required when function execution does not diverge.
Calling And because Note, we can simply say "matches in All of this is extremely subtle, to the point where I believe, that by this technicality, the official Rust docs for Does this mean that those official docs have to change? Probably not, because most people don't actually have to worry about this. It is unsafe code (and thus Ultimately, I believe restricting combinations can avoid pushing the entire headache toward the user side: The first two benefit from performance and |
Victoronz
added
D-Complex
Nope! I'm worried we'll scare off other reviewers, though :).
This may be the fundamental disagreement. I think we should follow a "simple things should be simple, complex things should be possible" approach. So I agree that we want good defaults here:
I mean, But, why would we lift the This may be the other fundamental disagreement: I think we might agree that we don't need
(Yeah, it's fine for
This is arguing semantics, but I don't think it's useful to consider a Either way, I think "makes the same sequence of calls to the provided |
Yeah, this has become a large wall of text. :)
Ah this was a mistake on my part, I moreso meant a
A bit of miscommunication here: by "restriction" I mean the combination restriction,
This is more of a consequence of "diverging programs are safe" than anything we're describing. We can omit talking about this detail entirely!
I agree that we can require "delegate the As for the usefulness of these semantics, I agree it is not useful for an implementor to worry about, but moreso because they have no control over it. What is important to communicate is that "Correct The reason we are able to use I think the simplest solution for now to punt on more |
Victoronz
added
S-Waiting-on-Author
…alent (#18470) # 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`](https://docs.rs/hashbrown/latest/hashbrown/trait.Equivalent.html) 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](#18408 (comment)). 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`.
…alent (#18470) # 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`](https://docs.rs/hashbrown/latest/hashbrown/trait.Equivalent.html) 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](#18408 (comment)). 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`.
# Objective Newest installment of the #16547 series. In #18319 we introduced `Entity` defaults to accomodate the most common use case for these types, however that resulted in the switch of the `T` and `N` generics of `UniqueEntityArray`. Swapping generics might be somewhat acceptable for `UniqueEntityArray`, it is not at all acceptable for map and set types, which we would make generic over `T: EntityEquivalent` in #18408. Leaving these defaults in place would result in a glaring inconsistency between these set collections and the others. Additionally, the current standard in the engine is for "entity" to mean `Entity`. APIs could be changed to accept `EntityEquivalent`, however that is a separate and contentious discussion. ## Solution Name these set collections `UniqueEntityEquivalent*`, and retain the `UniqueEntity*` name for an alias of the `Entity` case. While more verbose, this allows for all generics to be in proper order, full consistency between all set types*, and the "entity" name to be restricted to `Entity`. On top of that, `UniqueEntity*` now always have 1 generic less, when previously this was not enforced for the default case. *`UniqueEntityIter<I: Iterator<T: EntityEquivalent>>` is the sole exception to this. Aliases are unable to enforce bounds (`lazy_type_alias` is needed for this), so for this type, doing this split would be a mere suggestion, and in no way enforced. Iterator types are rarely ever named, and this specific one is intended to be aliased when it sees more use, like we do for the corresponding set collection iterators. Furthermore, the `EntityEquivalent` precursor `Borrow<Entity>` was used exactly because of such iterator bounds! Because of that, we leave it as is. While no migration guide for 0.15 users, for those that upgrade from main: `UniqueEntityVec<T>` -> `UniqueEntityEquivalentVec<T>` `UniqueEntitySlice<T>` -> `UniqueEntityEquivalentSlice<T>` `UniqueEntityArray<N, T>` -> `UniqueEntityEquivalentArray<T, N>`
# Objective Newest installment of the #16547 series. In #18319 we introduced `Entity` defaults to accomodate the most common use case for these types, however that resulted in the switch of the `T` and `N` generics of `UniqueEntityArray`. Swapping generics might be somewhat acceptable for `UniqueEntityArray`, it is not at all acceptable for map and set types, which we would make generic over `T: EntityEquivalent` in #18408. Leaving these defaults in place would result in a glaring inconsistency between these set collections and the others. Additionally, the current standard in the engine is for "entity" to mean `Entity`. APIs could be changed to accept `EntityEquivalent`, however that is a separate and contentious discussion. ## Solution Name these set collections `UniqueEntityEquivalent*`, and retain the `UniqueEntity*` name for an alias of the `Entity` case. While more verbose, this allows for all generics to be in proper order, full consistency between all set types*, and the "entity" name to be restricted to `Entity`. On top of that, `UniqueEntity*` now always have 1 generic less, when previously this was not enforced for the default case. *`UniqueEntityIter<I: Iterator<T: EntityEquivalent>>` is the sole exception to this. Aliases are unable to enforce bounds (`lazy_type_alias` is needed for this), so for this type, doing this split would be a mere suggestion, and in no way enforced. Iterator types are rarely ever named, and this specific one is intended to be aliased when it sees more use, like we do for the corresponding set collection iterators. Furthermore, the `EntityEquivalent` precursor `Borrow<Entity>` was used exactly because of such iterator bounds! Because of that, we leave it as is. While no migration guide for 0.15 users, for those that upgrade from main: `UniqueEntityVec<T>` -> `UniqueEntityEquivalentVec<T>` `UniqueEntitySlice<T>` -> `UniqueEntityEquivalentSlice<T>` `UniqueEntityArray<N, T>` -> `UniqueEntityEquivalentArray<T, N>`
2 participants
Objective
Related to #16547.
Currently we wrap
HashSet/HashMap/IndexSet/IndexMapover theEntity+EntityHashcombination. This allows them to be used withEntitySetfunctionality.However, when
Entityis newtyped, these wrappers can suddenly no longer be used!This causes types like
MainEntityHashSet/MainEntityHashMapto fall back to thehashbrown/indexmapversions again.Solution
We allow keying our map and set wrappers with
K: TrustedEntityBorrowtypes, not justEntity.This requires a new trait to be sound:
TrustedBuildHasher<H: Hash>: BuildHasher.Because both a
BuildHasherand the hashedHashtype could introduce nondeterminism into the final hash, we have to guarantee that such combinations produce valid results. Collisions are fine, set and map types can deal with them, only nondeterministic hashes present an issue.On account of our wrappers only using
EntityHash, we need only bound them onEntityHash: TrustedBuilderHasher<T>. Not allTrustedEntityBorrowtypes make sense to be hashed withEntityHashhowever, so the onlyEntityHash: TrustedBuildHasherimplementations for now are withEntity, entity newtypes, and theEntityRef-family.Adding a generic in place of
Entitypresents an issue forHashMapandIndexMap:If we want to retain the main case of
HashMap<Entity, V>, one would think to makeEntitythe default for K.But if we were to do that, we'd need to swap the positions for the
KandVgenerics, as defaulted type parameters need to trail.Because map types with swapped
KandVwould be very misleading, we abstain from generics, and use a type alias for theEntityHashMap/EntityIndexMapcase instead.A type alias and the aliased type need to have separate names, we name these new, broader versions
EntityEquivalent*instead of justEntity*.To keep the names consistent, we do the same for the set versions.
The name
EntityEquivalentis more accurate to the actual meaning of theTrustedEntityBorrowtrait, so it may be worth renaming it in another PR alongside the currently not so usedEntityBorrow. (EntityBorrowcould have a separate name, so we do not need aTrustedprefix forEntityEquivalent)While the additions/deletions seem quite ominous, what this PR does is conceptually not that complex:
In
hash.rsTrustedBuildHashertrait and its impls forEntity/EntityRef-familyIn
hash_set.rs/hash_map.rs/index_set.rs/index_map.rs:Entity*toEntityEquivalent*Entity*namesEntitykey withKgenericKonTrustedEntityBorrow + Hashplus whatever specific impls requirewhere EntityHash: TrustedBuildHasher<K>bounds to type definitions and impl blocksIn
sync_world.rs:TrustedBuildHasherimpls withMainEntity/RenderEntityMainEntityHashSet/Mapaliases ofEntityEquivalentHashSet/Maprespectively.The rest:
HashMap::defaulttoMainEntityHashMap::defaulthash_set::IterIt is painful to have wrapper types that take up this much space and should really not be necessary, but on the other hand, it clarifies/increases our justification to request the required changes upstream (
hashbrown, thenindexmap)!With future upstream changes, we'd ultimately only need to retain the
TrustedBuildHashertrait, some aliases and the added bounds in some impls.If we'd then like to keep the wrappers around, we still could as well, but they could be made a lot lighter.
Migration Guide
MainEntityHashSet/MainEntityHashMapare no longer aliases of thehashbrowntypes, so some associated functions have to use the proper names or aliases to be used in place ofHashMap. Example:HashMap::default->MainEntityHashMap::default`Types associated with any of
EntityHashSet,EntityHashMap,EntityIndexSet,EntityIndexMapnow have an additionalKgeneric. To maintain the previous meaning, useEntityforK.