Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Typing problems in new pygame.typing module #3074

Closed
aatle opened this issue Aug 21, 2024 · 6 comments · Fixed by #3080
Closed

Typing problems in new pygame.typing module #3074

aatle opened this issue Aug 21, 2024 · 6 comments · Fixed by #3080
Labels
bug Not working as intended type hints Type hint checking related tasks

Comments

@aatle
Copy link
Contributor

aatle commented Aug 21, 2024

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.
@aatle aatle added the bug Not working as intended label Aug 21, 2024
@Starbuck5 Starbuck5 added the type hints Type hint checking related tasks label Aug 21, 2024
@ankith26
Copy link
Member

  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
Copy link
Member

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

@aatle
Copy link
Contributor Author

aatle commented Aug 22, 2024 •
edited
Loading

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
Copy link
Contributor

damusss commented Aug 22, 2024

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

@aatle
Copy link
Contributor Author

aatle commented Aug 23, 2024

@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.

@aatle aatle mentioned this issue Aug 23, 2024
Merged
@damusss
Copy link
Contributor

damusss commented Aug 23, 2024

@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

@ankith26 ankith26 linked a pull request Aug 23, 2024 that will close this issue
Fix and improve pygame.typing module #3080
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Not working as intended type hints Type hint checking related tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix and improve pygame.typing module aatle/pygame-ce
4 participants
@Starbuck5 @ankith26 @damusss @aatle