Skip to content

Conversation

@toofishes
Copy link
Contributor

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

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

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

@DanielNoord
Copy link
Contributor

@elprans Sorry to ping again, but would it be possible to get a review on this and the other open typing related PRs?

@elprans
Copy link
Member

elprans commented Dec 18, 2024

@elprans Sorry to ping again, but would it be possible to get a review on this and the other open typing related PRs?

Right on. Sorry for the review delays.

@elprans elprans merged commit d0797f1 into MagicStack:master Dec 18, 2024
@DanielNoord
Copy link
Contributor

Awesome! Appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants