Introduce SpriteSequence, a covariant supertype of SpriteList. #2647
Conversation
As we are just out of a while loop whose stopping condition is that `sprite_lists` is empty, calling `clear()` on it is redundant.
There was already a method `register_sprite_list` to handle all additions to `sprite_lists`. We add a corresponding method `_unregister_sprite_list` to handle removals. We make the typings of these methods stricter, so that we can enforce the correct typing invariant on `sprite_lists`. We also make that invariant clearer in a comment. `sprite_lists` is unfortunately unsafely visible to everyone. So a user of the class could still violate the invariants. At least now the *intended* usage is safe.
This is similar to the fix done to `check_for_collision_with_list` done in c387717.
Adding type parameters to some `SpriteList`s. One allows to get rid of a cast.
This is done by analogy to `collections.abc.Sequence`, which is a covariant supertype of `list`. Before this commit, many parts of the codebase used `SpriteList`s without type arguments (defaulting to `Unknown`). That was the only way to allow reasonable usages of the given methods and attributes. However, doing so results in weaker typing. Using `SpriteSequence`, we can add correct type arguments to almost all of the references that were using `SpriteList`s before. The only missing pieces are `Scene` and `TileMap`. Unfortunately, their APIs are fundamentally unsound wrt. the type arguments of their `SpriteList`s. We cannot make it sound without breaking their APIs, so we do not change them. As a bonus, we can now create lists of `SpriteList`s with varying type arguments, and generically call `draw` or `update` on them. Previously, the only common supertype of `SpriteList[A]` and `SpriteList[B]` was `object`, which meant it was not possible to call those methods on them. In a sense, that ability mostly subsumes the convenience provided by `Scene`. A `list[SpriteSequence[BasicSprite]]` is almost as convenient, while being type-safe.
@DragonMoffon and I want to change |
There was a problem hiding this comment.
Thank you for starting taking a crack at this. I appreciate it since it's sorely needed (see #2580 and similar 😅).
For the moment, I'm a little confused on some abc-related details as in this comment. If nobody beats me to giving this a closer look, I may need a few days before I can give it the full attention it deserves.
@eruvanos btw, do you think we should add a typing-specific tests folder or similar as an exception to the tests folder being ignored?
| See SpatialHash for more details. | ||
| """ | ||
|
|
||
| @abstractmethod |
There was a problem hiding this comment.
As a heads up, Python's generics are kind of a mess compared to those in the Java ecosystem and anything related to it (including your Scala projects 👀).
I'm confused as to whether/how this actually works. My reading is that typing.Protocol may be a better fit since:
abc.abstractmethodrequires classes to haveABCMetaas their metaclassGenerichas no metaclass at all since it's defined as a C-backed type in_typingmodule.c- We may want to support different spatial structures (quad tree, etc) in the future
There was a problem hiding this comment.
Update: At least on Python 3.13, typing.Generic doesn't work with abc.abstractmethod at all :(
Here's a cut-down version of the output + the gist I wrote:
Trying to instantiate GenericBase...
FAIL: Did NOT raise a TypeError!
Trying to instantiate GenericSub...
FAIL: Did NOT raise a TypeError!
import abc
from typing import Collection, TypeVar, Generic
T_co = TypeVar('T_co', covariant=True)
def does_it_type_error(t: Generic[T_co]) -> Generic[T_co]:
"""A decorator which tries to instantiate the passed class immediately."""
name = t.__name__
try:
print(F"Trying to instantiate {name}...")
t()
print(f" FAIL: Did NOT raise a TypeError!\n")
except TypeError as _:
print(f" PASS: Raised a TypeError.\n")
finally:
return t
@does_it_type_error
class GenericBase(Generic[T_co]):
@abc.abstractmethod
def local_method(self) -> int:
...
@does_it_type_error
class GenericSub(GenericBase[T_co]):
...There was a problem hiding this comment.
Huh, gosh, indeed it needed Protocol. It wasn't checking nearly as much as I thought it was just with Generic. Adding Protocol flagged the use of set[SpriteType_co] in covariant position, which I replaced by collections.abc.Set.
Coming from Scala, as you noticed, I'm generally pleasantly surprised by what the Python type system can express these days. But I sure get confused with Python's idea of what an abstract class/interface/ABC is.
arcade/sprite_list/spatial_hash.py
Outdated
| """ | ||
| Read-only view of a SpatialHash. | ||
|
|
||
| This is useful when the SpriteType is in covariant position. | ||
|
|
||
| See SpatialHash for more details. | ||
| """ |
There was a problem hiding this comment.
Maybe something like this? The wording's less a concern to me than getting the cross-refs solid.
| """ | |
| Read-only view of a SpatialHash. | |
| This is useful when the SpriteType is in covariant position. | |
| See SpatialHash for more details. | |
| """ | |
| """A read-only view of a :py:class:`.SpatialHash` which helps preserve safety. | |
| This works like the read-only views of Python's built-in :py:class:`dict` and other types. As an every-day user, it means | |
| you can put subtypes of the annotated type into the `SpatialHash` or a `SpriteList`, but not superclasses. | |
| This ensures predicable behavior via type safety in cases where: | |
| #. A spatial hash is annotated with a specific type | |
| #. The object could be modified outside the original context to add a broader type | |
| Advanced users who want more information on the specifics should see the comments of :py:class:`~arcade.sprite_list.SpriteList`. | |
| """ |
There was a problem hiding this comment.
I took your suggestion but I amended a few things. LMK if you disagree.
arcade/sprite_list/sprite_list.py
Outdated
| """ | ||
| A read-only view of a SpriteList. | ||
|
|
||
| Like collections.abc.Sequence, a SpriteSequence is covariant in its sprite | ||
| type. This is useful when you want to manipulate a collection of | ||
| SpriteLists of different sprite types. | ||
|
|
||
| See SpriteList for more details. | ||
| """ |
There was a problem hiding this comment.
| """ | |
| A read-only view of a SpriteList. | |
| Like collections.abc.Sequence, a SpriteSequence is covariant in its sprite | |
| type. This is useful when you want to manipulate a collection of | |
| SpriteLists of different sprite types. | |
| See SpriteList for more details. | |
| """ | |
| """A read-only view of a :py:class:`.SpriteList`. | |
| Like other generics such as :py:class:`collections.abc.Sequence`, a `SpriteSequence` requires | |
| sprites be of a covariant type relative to their annotated type. | |
| See :py:class:`.SpriteList` for more details. | |
| """ |
arcade/sprite_list/spatial_hash.py
Outdated
| Args: | ||
| rect: The rectangle to check (left, right, bottom, top) |
There was a problem hiding this comment.
If we're adding this, we may as well correct it.
| Args: | |
| rect: The rectangle to check (left, right, bottom, top) | |
| .. tip:: Use :py:mod:`arcade.types.rect`'s helper functions to create rectangle objects! | |
| Args: | |
| rect: The rectangle to check as a :py:class:`~arcade.types.rect.Rect` object. |
| # It really is a SpriteList with a good type; this would not typecheck otherwise | ||
| custom_sprite: _CustomSpriteSolidColor = sprite_list2[0] # assert_type |
There was a problem hiding this comment.
Are you running this type check manually? My understanding is that we don't run type checks against the test folder at all. See:
Lines 140 to 150 in 94c4280
There was a problem hiding this comment.
I don't think I ran pyright, but I did run mypy manually.
Notably, make ReadOnlySpriteHash inherit Protocol.
| See SpatialHash for more details. | ||
| """ | ||
|
|
||
| @abstractmethod |
There was a problem hiding this comment.
Huh, gosh, indeed it needed Protocol. It wasn't checking nearly as much as I thought it was just with Generic. Adding Protocol flagged the use of set[SpriteType_co] in covariant position, which I replaced by collections.abc.Set.
Coming from Scala, as you noticed, I'm generally pleasantly surprised by what the Python type system can express these days. But I sure get confused with Python's idea of what an abstract class/interface/ABC is.
arcade/sprite_list/spatial_hash.py
Outdated
| """ | ||
| Read-only view of a SpatialHash. | ||
|
|
||
| This is useful when the SpriteType is in covariant position. | ||
|
|
||
| See SpatialHash for more details. | ||
| """ |
There was a problem hiding this comment.
I took your suggestion but I amended a few things. LMK if you disagree.
| # It really is a SpriteList with a good type; this would not typecheck otherwise | ||
| custom_sprite: _CustomSpriteSolidColor = sprite_list2[0] # assert_type |
There was a problem hiding this comment.
I don't think I ran pyright, but I did run mypy manually.
This is done by analogy to
collections.abc.Sequence, which is a covariant supertype oflist.Before this commit, many parts of the codebase used
SpriteLists without type arguments (defaulting toUnknown). That was the only way to allow reasonable usages of the given methods and attributes. However, doing so results in weaker typing.Using
SpriteSequence, we can add correct type arguments to almost all of the references that were usingSpriteLists before.The only missing pieces are
SceneandTileMap. Unfortunately, their APIs are fundamentally unsound wrt. the type arguments of theirSpriteLists. We cannot make it sound without breaking their APIs, so we do not change them.As a bonus, we can now create lists of
SpriteLists with varying type arguments, and generically calldraworupdateon them. Previously, the only common supertype ofSpriteList[A]andSpriteList[B]wasobject, which meant it was not possible to call those methods on them.In a sense, that ability mostly subsumes the convenience provided by
Scene. Alist[SpriteSequence[BasicSprite]]is almost as convenient, while being type-safe.This was prompted by seeing students struggle with the absence of a common supertype for
SpriteList[A]andSpriteList[B]. Students resorted tocasts orAnys (or smuggled their way out of that usingScene) to work around the issue. WithSpriteSequence, this shouldn't be an issue anymore.It turns out that the introduction of
SpriteSequenceallows to more safely type many APIs of Arcade, so hopefully this addition carries its weight.