FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add more return types to the `numbers` module by AlexWaygood · Pull Request #11353 · python/typeshed · GitHub

  1. python
  2. typeshed

Repository navigation

Add more return types to the numbers module - #11353

Merged
AlexWaygood merged 5 commits into
python:mainfrom
AlexWaygood:numbers-compromise
Feb 1, 2024
ConversationCommits (5)ChecksFiles changed
Merged

Add more return types to the numbers module#11353
AlexWaygood merged 5 commits into
python:mainfrom
AlexWaygood:numbers-compromise

Conversation

AlexWaygood commented Feb 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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):

typeshed/stdlib/fractions.pyi

Lines 105 to 106 in 547cbc7

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

Sign in to view

This comment has been minimized.

Sign in to view

AlexWaygood commented Feb 1, 2024

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

AlexWaygood mentioned this pull request Feb 1, 2024
Merged
JelleZijlstra approved these changes Feb 1, 2024
View reviewed changes

JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread stdlib/numbers.pyi

AlexWaygood commented Feb 1, 2024

Copy link
Copy Markdown
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.

Sign in to view

AlexWaygood commented Feb 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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?

AlexWaygood commented Feb 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 :)

github-actions Bot commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

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

AlexWaygood merged commit 4d8ee11 into python:main Feb 1, 2024
AlexWaygood deleted the numbers-compromise branch February 1, 2024 18:12
cdce8p mentioned this pull request Feb 3, 2024
Merged
This was referenced Feb 6, 2024
Closed
Merged
hauntsaninja mentioned this pull request Feb 7, 2024
Closed
AlexWaygood mentioned this pull request Mar 15, 2024
Closed
MegaIng mentioned this pull request Jun 17, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JelleZijlstra JelleZijlstra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL