Typing problems in new pygame.typing module

[aatle]

I noticed a few fixable problems while reading the new pygame.typing module.

Environment:

Commit 9208ee5 (not released yet)

Problems (4):

_HasRectAttribute protocol is implemented wrong

pygame-ce/src_py/typing.py

Lines 50 to 53 in 141a75d

	class _HasRectAttribute(Protocol):
	# An object that has a rect attribute that is either a rect, or a function
	# that returns a rect conforms to the rect protocol
	rect: Union["RectLike", Callable[[], "RectLike"]]

The Union here means that a given class's rect attribute must support holding both a RectLike object and a function returning a RectLike object. Most classes, including pygame.Sprite, would only support one of those, and so would be incompatible.
In addition to this, the attribute must also be writeable, which is impossible if implemented as a method.

Possible fix:

class _HasRectAttribute(Protocol):
    @property
    def rect(self) -> "RectLike": ...

class _HasRectFunction(Protocol):
    def rect(self) -> "RectLike": ...

_HasRect = Union[_HasRectAttribute, _HasRectFunction]

Test case classes for mypy (all passed):

class Attr:
    rect: RectLike

class Method:
    def rect(self) -> RectLike:
        return [1,1,1,1]

class ReadProperty:
    @property
    def rect(self) -> RectLike:
        return [1,1,1,1]

class ReadWriteProperty:
    @property
    def rect(self) -> RectLike:
        return [1,1,1,1]
    @rect.setter
    def rect(self, value):
        ...

class FunctionAttr:
    rect: Callable[[], RectLike]

class ClassAttr:
    rect: ClassVar[RectLike]

_CanBeRect covers an invalid case

pygame-ce/src_py/typing.py

Line 47 in 141a75d

_CanBeRect = SequenceLike[Union[float, Coordinate]]

Slightly imprecise; [(1, 2), 3, 4] would be compatible, but is not actually supported.
Fixed:

 _CanBeRect = Union[SequenceLike[float], SequenceLike[Coordinate]]

Custom defined _PathProtocol uses a bound TypeVar instead of constrained TypeVar

pygame-ce/src_py/typing.py

Lines 9 to 15 in 141a75d

	if sys.version_info >= (3, 9):
	from os import PathLike as _PathProtocol
	else:
	_T = TypeVar("_T", bound=Union[str, bytes])
	class _PathProtocol(Protocol[_T]):
	def __fspath__(self) -> _T: ...

As is common when supporting both str and bytes, a constrained TypeVar should be used (which disallows mixing (str | bytes) for the type var). (There is an AnyStr type var included in the standard typing module, but we cannot use it since it is invariant and protocols can't have invariant type vars.)
Modeling after PathLike definition in the python typeshed, type var fix:

    _AnyStr_co = TypeVar("_AnyStr_co", str, bytes, covariant=True)

    class _PathProtocol(Protocol[ _AnyStr_co]):
        def __fspath__(self) -> _AnyStr_co: ...

SequenceLike protocol may be a bit strict

Protocol methods:

pygame-ce/src_py/typing.py

Lines 31 to 32 in 141a75d

	def __getitem__(self, __i: SupportsIndex) -> _T_co: ...
	def __len__(self) -> int: ...

The __getitem__ signature mandates that an adhering class's __getitem__ index parameter must be typed with SupportsIndex. However, some classes do not use SupportsIndex for the index parameter (using int or slice instead), including collections.abc.Sequence.
This means that collections.abc.Sequence itself and a method signature like (self, index: int) are not valid SequenceLikes.
So, replacing SupportsIndex with int will support more sequence classes, but at the cost of supporting less objects as indices.

[ankith26]

1. I think the existing implementation is correct. A Union of two types does not make the typechecker check for both. The test case you provided passes with the current implementation as well. But maybe I'm not understanding your point correctly here.
2. Yes this is definitely an issue, your suggested fix is good.
3. Yes this is definitely an issue, your suggested fix is good.
4. I didn't realise that int is incompatible with SupportsIndex (this is surprising). I think the solution to this problem is to use Union[SupportsIndex, int] then. It is important to keep including SupportsIndex because we want our SequenceLike to be compatible with the actual python Sequence itself

[ankith26]

Feel free to open a PR for these issues, so that the fixes are correctly attributed to you :)

[aatle]

Sure, I will work on a PR.

After looking further into the problems (because my past testing did fail with the current implementation), I realize that you are mostly correct about 1, but maybe misunderstanding 4.

1. (Thanks for the correction.) The current implementation is nearly correct except for another thing: implicitly, the rect attribute is declared as writeable, when in practice it is used as read-only. Mutability makes the attribute invariant, similar to list being invariant. (Reason why my tests failed on mypy.)
Using @property to type an attribute as not writeable is the common solution in the standard typeshed.
Alternative fix (note: is basically equivalent to original proposed fix):

class _HasRect(Protocol):
    @property
    def rect(self) -> "RectLike" | Callable[[], "RectLike"]: ...

4. Sorry for the misunderstanding; int is compatible with SupportsIndex. Also, this proposed change is a decision rather than a correction.
To define a method compatible with the current protocol, one must type the parameter with a broader type than the protocol's as function parameters are contravariant.
However, the protocol's parameter is already broad (SupportsIndex), so less method implementations are compatible, including collections.abc.Sequence, which types it as int (+override for slice).
Proposed change:

def __getitem__(self, index: int, /) -> _T_co: ...

Applying this would make collections.abc.Sequence a valid subtype of SequenceLike, but also make it so that SupportsIndex objects cannot be used as an index for SequenceLike even though it is probably supported. So I am not sure whether this should be changed.

Extra problem: Best practice is to put @abstractmethod on methods and properties with no implementation/body.

[damusss]

@aatle I don't want to sound stupid but why can't the rect attribute be writable?

[aatle]

@damusss The rect attribute does not need to be written to when handling a RectLike, so it is declared as a property with no setter. (This is required for the Union type in the protocol to work, because of variance.)
Note that this does not disallow the rect attribute being implemented as writable; rather, we make rect mutability not required in implementations.

[damusss]

@damusss The rect attribute does not need to be written to when handling a RectLike, so it is declared as a property with no setter. (This is required for the Union type in the protocol to work, because of variance.)
Note that this does not disallow the rect attribute being implemented as writable; rather, we make rect mutability not required in implementations.

I thought making it a property would disallow it being implemented writable, that makes sense then. Python typehinting is deeper than I thought lol