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

Add more return types to the numbers module #11353

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Feb 1, 2024

This PR represents a compromise between what the numbers module was supposed to be, and the reality of where we are today with Python's typing system.

This module is supposed to provide a framework of ABCs that would allow for third-party implementations of Complex, Rational, Real and Integral numbers. As such, the annotations we should be giving to the Real class should be something like this:

class Real:
    @abstractmethod
    def __mod__(self, other: Real) -> Real: ...
    @abstractmethod
    def __round__(self, ndigits: int) -> Real: ...
    @abstractmethod
    def __divmod__(self, other: Real) -> tuple[Real, Real]: ...

Unfortunately, that doesn't work! The reason is that fractions.Fraction inherits from numbers.Real, and it overrides these abstract methods to provide concrete implementations. The concrete implementations return concrete types such as int, which type checkers don't recognise as subtypes of Real (because the numbers module has never been supported by static type checkers, and never will be):

@overload
def __divmod__(a, b: int | Fraction) -> tuple[int, Fraction]: ...

As such, for every method I tried adding types to according to the above strategy, mypy emitted an error warning me that fractions.Fraction incompatibly overrode numbers.Real. (There were also similar errors for a numbers.Real subclass in our Pillow stubs.)

Instead of returning Complex from methods that really should be returning Complex, therefore, this PR annotates those methods as returning SupportsComplex. SupportsComplex works pretty well here:

  • It's a supertype of numbers.Complex, so it's not incorrect
  • It's a good deal more precise than Any
  • It's vague enough that it will permit third-party subclassers to return their own implementations of numbers.Complex from these methods, which is what the module was designed for anyway.

In a similar way, I've annotated methods that should be returning numbers.Integral as returning SupportsIndex, and I've annotated methods that should be returning numbers.Real as returning SupportsFloat.

I haven't attempted to add any new argument annotations as part of this PR. The return annotations were hard enough.

This comment has been minimized.

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

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

That looks like the kind of diff I'd expect, given that type checkers don't support these ABCs at all. I think it's a net win:

  • We introduce 3 new errors in third-party code
  • But we get rid of 6
  • 1 existing error gets a different, more verbose error message

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good step forward.

However, we can probably do a bit better by using protocols that support a few more common unary dunder methods that exist on common number classes: __abs__ and others.

Also, I don't think we can realistically annotate the parameters to binary dunder methods on these classes. You'd think that say Integral.__add__ should accept any other Integral, but that's impossible to implement in a concrete class (how would it know to add itself to any other Integral that might exist somewhere?) and inherently an LSP violation, because Integral is a subclass of Complex, and Complex.__add__ should allow any complex numbers. So I endorse keeping parameter annotations out of this PR.

stdlib/numbers.pyi Show resolved Hide resolved
@AlexWaygood
Copy link
Member Author

However, we can probably do a bit better by using protocols that support a few more common unary dunder methods that exist on common number classes: __abs__ and others.

Okay, I've had a go! Let's see what primer says...

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Feb 1, 2024

That last commit made the typing a lot more complex, didn't change the primer output at all, and broke our test cases for unittest.TestCase.assertAlmostEqual. I'm inclined to just revert it, and merge the PR as it was before that commit?

This reverts commit 687709c.
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Feb 1, 2024

That last commit made the typing a lot more complex, didn't change the primer output at all, and broke our test cases for unittest.TestCase.assertAlmostEqual. I'm inclined to just revert it, and merge the PR as it was before that commit?

Everything I tried locally seemed to break either our test cases for pow or our test cases for unittest.TestCase.assertAlmostEqual. I've gone ahead and reverted the last commit for now, but I'd be happy if somebody managed to find a way to improve the types even more after this PR :)

Copy link
Contributor

github-actions bot commented Feb 1, 2024

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

steam.py (https://github.com/Gobot1234/steam.py)
- steam/ext/tf2/currency.py:100: note:      Superclass:
- steam/ext/tf2/currency.py:100: note:          @overload
- steam/ext/tf2/currency.py:100: note:          def __sub__(a, int | Fraction, /) -> Fraction
- steam/ext/tf2/currency.py:100: note:          @overload
- steam/ext/tf2/currency.py:100: note:          def __sub__(a, float, /) -> float
- steam/ext/tf2/currency.py:100: note:          @overload
- steam/ext/tf2/currency.py:100: note:          def __sub__(a, complex, /) -> complex
- steam/ext/tf2/currency.py:100: note:      Subclass:
- steam/ext/tf2/currency.py:100: note:          def __sub__(self, int | str | float | Fraction | Decimal, /) -> Metal

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/python_api.py:400: error: Unused "type: ignore" comment  [unused-ignore]
+ src/_pytest/python_api.py:444: error: Unused "type: ignore" comment  [unused-ignore]
+ src/_pytest/python_api.py:445: error: Unused "type: ignore" comment  [unused-ignore]
+ src/_pytest/python_api.py:453: error: Unused "type: ignore" comment  [unused-ignore]
+ src/_pytest/python_api.py:457: error: Argument 1 to "abs" has incompatible type "SupportsComplex"; expected "SupportsAbs[Never]"  [arg-type]

ibis (https://github.com/ibis-project/ibis)
+ ibis/util.py:168: error: Unsupported left operand type for < (Never)  [operator]
+ ibis/util.py:168: error: Argument 1 to "abs" has incompatible type "SupportsComplex"; expected "SupportsAbs[Never]"  [arg-type]
- ibis/common/temporal.py:184: error: Incompatible return value type (got "int", expected "timedelta")  [return-value]
- ibis/common/temporal.py:184: error: Argument 1 to "int" has incompatible type "timedelta | Real"; expected "str | Buffer | SupportsInt | SupportsIndex | SupportsTrunc"  [arg-type]
+ ibis/common/temporal.py:184: error: No overload variant of "int" matches argument type "timedelta | Real"  [call-overload]
+ ibis/common/temporal.py:184: note: Possible overload variants:
+ ibis/common/temporal.py:184: note:     def __new__(cls, str | Buffer | SupportsInt | SupportsIndex | SupportsTrunc = ..., /) -> int
+ ibis/common/temporal.py:184: note:     def __new__(cls, str | bytes | bytearray, /, base: SupportsIndex) -> int

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.

2 participants