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.

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

Fix typing issues around Range/RangeValue #1196

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

toofishes
Copy link

I'm glad to see some type hints were added in 0.30.0! I tried them out, and encountered a minor issue.

In order for the _RangeValue protocol to type check properly, it needs to reference the created TypeVar, not itself.

Both pyright and mypy show a ton of errors when type checking this before these changes. (I've omitted non-relevant errors here):

% pyright tests/test_types.py
/Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py
  /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:25 - error: Argument of type "Literal[1]" cannot be assigned to parameter "lower" of type "_RV@Range | None" in function "__init__"
    Type "Literal[1]" is not assignable to type "_RV@Range | None"
      Type "Literal[1]" is not assignable to type "_RangeValue"
        "Literal[1]" is incompatible with protocol "_RangeValue"
          "__lt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool"
          "__gt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool"
      "Literal[1]" is not assignable to "None" (reportArgumentType)
  /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:34 - error: Argument of type "Literal[5]" cannot be assigned to parameter "upper" of type "_RV@Range | None" in function "__init__"
    Type "Literal[5]" is not assignable to type "_RV@Range | None"
      Type "Literal[5]" is not assignable to type "_RangeValue"
        "Literal[5]" is incompatible with protocol "_RangeValue"
          "__lt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool"
          "__gt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool"
      "Literal[5]" is not assignable to "None" (reportArgumentType)
...

% mypy tests/test_types.py | grep arg-type
tests/test_types.py:18: error: Argument "lower" to "Range" has incompatible type "int"; expected "None"  [arg-type]
tests/test_types.py:18: error: Argument "upper" to "Range" has incompatible type "int"; expected "None"  [arg-type]
... (19 more errors)

After this change, the type checking comes back clean:

% pyright tests/test_types.py
0 errors, 0 warnings, 0 informations

% mypy tests/test_types.py | grep arg-type
<no output>

This slight change appears to match what was expected when bound= was implemented: python/typing#59 (comment)

@bryanforbes bryanforbes self-assigned this Oct 21, 2024
asyncpg/types.py Outdated
@@ -63,10 +63,10 @@ class _RangeValue(typing.Protocol):
def __eq__(self, __value: object) -> bool:
...

def __lt__(self, __other: _RangeValue) -> bool:
def __lt__(self: _RV, __other: _RV) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe def __lt__(self, __other: Self) -> bool: should work

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it does not- I tried that first and settled on what was submitted to get it all working. Here's the output with your suggestion (made to both __lt__ and __gt__):

% pyright tests/test_types.py
/Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py
  /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:25 - error: Argument of type "Literal[1]" cannot be assigned to parameter "lower" of type "_RV@Range | None" in function "__init__"
    Type "Literal[1]" is not assignable to type "_RV@Range | None"
      Type "Literal[1]" is not assignable to type "_RangeValue"
        "Literal[1]" is incompatible with protocol "_RangeValue"
          "__lt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool"
          "__gt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool"
      "Literal[1]" is not assignable to "None" (reportArgumentType)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __lt__(self: _RV, __other: _RV) -> bool:
def __lt__(self, __other: Self, /) -> bool:

This will fix it, checked locally with pyright as well.

See typeshed which has the definition of int.__lt__ that pyright (correctly) consider to not be compatible with _RangeValue.__lt__:
https://github.com/python/typeshed/blob/ca65e087f1f9dfc28e89192b60ce7cfc2e92c674/stdlib/builtins.pyi#L321

asyncpg/types.py Outdated
@@ -63,10 +63,10 @@ class _RangeValue(typing.Protocol):
def __eq__(self, __value: object) -> bool:
...

def __lt__(self, __other: _RangeValue) -> bool:
def __lt__(self: _RV, __other: _RV) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __lt__(self: _RV, __other: _RV) -> bool:
def __lt__(self, __other: Self, /) -> bool:

This will fix it, checked locally with pyright as well.

See typeshed which has the definition of int.__lt__ that pyright (correctly) consider to not be compatible with _RangeValue.__lt__:
https://github.com/python/typeshed/blob/ca65e087f1f9dfc28e89192b60ce7cfc2e92c674/stdlib/builtins.pyi#L321

In order for the `_RangeValue` protocol to type check properly, it needs to
reference `Self`, not itself.

Both pyright and mypy show a ton of errors when type checking this before these
changes. (I've omitted non-relevant errors here):

```
% pyright tests/test_types.py
/Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py
  /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:25 - error: Argument of type "Literal[1]" cannot be assigned to parameter "lower" of type "_RV@Range | None" in function "__init__"
    Type "Literal[1]" is not assignable to type "_RV@Range | None"
      Type "Literal[1]" is not assignable to type "_RangeValue"
        "Literal[1]" is incompatible with protocol "_RangeValue"
          "__lt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool"
          "__gt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool"
      "Literal[1]" is not assignable to "None" (reportArgumentType)
  /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:34 - error: Argument of type "Literal[5]" cannot be assigned to parameter "upper" of type "_RV@Range | None" in function "__init__"
    Type "Literal[5]" is not assignable to type "_RV@Range | None"
      Type "Literal[5]" is not assignable to type "_RangeValue"
        "Literal[5]" is incompatible with protocol "_RangeValue"
          "__lt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool"
          "__gt__" is an incompatible type
            Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool"
      "Literal[5]" is not assignable to "None" (reportArgumentType)
...

% mypy tests/test_types.py | grep arg-type
tests/test_types.py:18: error: Argument "lower" to "Range" has incompatible type "int"; expected "None"  [arg-type]
tests/test_types.py:18: error: Argument "upper" to "Range" has incompatible type "int"; expected "None"  [arg-type]
... (19 more errors)
```

After this change, the type checking comes back clean:

```
% pyright tests/test_types.py
0 errors, 0 warnings, 0 informations

% mypy tests/test_types.py | grep arg-type
<no output>
```
Copy link
Contributor

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@toofishes
Copy link
Author

Great suggestion - I made the update. I also verified that this works for datetime (https://github.com/python/typeshed/blob/ca65e087f1f9dfc28e89192b60ce7cfc2e92c674/stdlib/datetime.pyi#L318-L322), as I encountered this using a TstzRange in my codebase.

@toofishes
Copy link
Author

Is there anything else I can do here to get this merged?

@DanielNoord
Copy link
Contributor

Maybe @elprans can help review this? There are a couple other PRs from me related to typing that are also waiting on a review to get merged but I don't want to ping them too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants