Skip to content

max() uses SupportsGreaterThanT #6342

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

Merged
merged 2 commits into from
Nov 21, 2021
Merged

max() uses SupportsGreaterThanT #6342

merged 2 commits into from
Nov 21, 2021

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Nov 19, 2021

Add SupportsGreaterThan and SupportsGreaterThanT

Closes: #6336

Add SupportsGreaterThan and SupportsGreaterThanT

Closes: python#6336
@srittau
Copy link
Collaborator Author

srittau commented Nov 19, 2021

I have no idea what the pre-commit check is trying to tell me.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member

Looks like it is already enabled and is looking for a config file. Which is not yet merged: #6341

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas.git)
+ pandas/core/arrays/sparse/array.py:1550: error: Cannot call function of unknown type  [operator]

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 19, 2021

I don't think the solution can be this simple, unfortunately. Consider the following class definitions:

class OnlyKnowsDunderLT:
    def __init__(self, n):
        self.n = n
    def __lt__(self, other):
        return self.n < other.n
    def __repr__(self):
        return f'OnlyKnowsDunderLT(n={self.n})'

 
class OnlyKnowsDunderGT:
    def __init__(self, n):
        self.n = n
    def __gt__(self, other):
        return self.n > other.n
    def __repr__(self):
        return f'OnlyKnowsDunderGT(n={self.n})'

Here's how they behave with max() in Python 3.10:

>>> max(OnlyKnowsDunderLT(5), OnlyKnowsDunderLT(6))
OnlyKnowsDunderLT(n=6)
>>> max(OnlyKnowsDunderGT(3), OnlyKnowsDunderGT(8))
OnlyKnowsDunderGT(n=8)
>>> max(OnlyKnowsDunderLT(3), OnlyKnowsDunderGT(9))
OnlyKnowsDunderGT(n=9)

I believe that this may be because the following works just fine, due to the way the expression x > y is evaluated by the interpreter:

>>> OnlyKnowsDunderLT(2) > OnlyKnowsDunderLT(8)
False

A solution I was wondering about (which I haven't tested, nor checked against the C code to see whether it would be accurate) was something along these lines:

from typing import Protocol, TypeVar, Union, Any

class _SupportsDunderLT(Protocol):
    def __lt__(self, other: Any) -> bool: ...

class _SupportsDunderGT(Protocol):
    def __gt__(self, other: Any) -> bool: ...

SupportsComparisons = Union[_SupportsDunderGT, _SupportsDunderLT]
SupportsComparisonsT = TypeVar('SupportsComparisonsT', bound=SupportsComparisons)

...and then use SupportsComparisons and SupportsComparisonsT everywhere where the stub for max() currently uses SupportsLessThan and SupportsLessThanT.

@srittau
Copy link
Collaborator Author

srittau commented Nov 21, 2021

@AlexWaygood That's because __lt__ et al. are special cased by Python. Any such special cases should be special cased by type checkers as well (which they usually are).

@srittau srittau merged commit a6e3699 into python:master Nov 21, 2021
@srittau srittau deleted the max branch November 21, 2021 12:28
@AlexWaygood
Copy link
Member

@srittau, the following snippet of code fails mypy, despite running fine at runtime, indicating that __lt__/__gt__ are not currently adequately special-cased by mypy. Do you think that should be reported to mypy as a bug?

from typing import Protocol, Any, TypeVar

class SupportsGreaterThan(Protocol):
    def __gt__(self, other: Any) -> bool: ...
    
SupportsGreaterThanT = TypeVar('SupportsGreaterThanT', bound=SupportsGreaterThan)
    
def mymax(*iterables: SupportsGreaterThanT) -> SupportsGreaterThanT:
    return max(iterables) # type: ignore[type-var]

class HasLessThan:
    def __init__(self, n: int) -> None:
        self.n = n
    def __lt__(self, other: HasLessThan) -> bool:
        return self.n < other.n
    def __repr__(self) -> str:
        return f'HasLessThan(n={self.n})'

mymax(HasLessThan(5), HasLessThan(8))  # error: Value of type variable "SupportsGreaterThanT" of "mymax" cannot be "HasLessThan"  [type-var]

@srittau
Copy link
Collaborator Author

srittau commented Nov 21, 2021

Considering that this is ultimately passed to PyObject_RichCompareBool, I believe this is best handled by mypy.

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.

Annotation for builtin max() function uses wrong comparison operator
4 participants