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

Further improve pow #6325

Merged
merged 5 commits into from
Nov 18, 2021
Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Nov 17, 2021

With the current stubs, mypy produces this output with the following code snippet:

from fractions import Fraction

reveal_type(pow(2, 4))                            # Revealed type is "Any"
reveal_type(pow(5, -7))                           # Revealed type is "Any"
reveal_type(pow(Fraction(), 4, None))             # error: No overload variant of "pow" matches argument types "Fraction", "int", "None"
reveal_type(pow(Fraction(3, 7), complex(1, 8)))   # error: Cannot infer type argument 1 of "pow"
reveal_type(pow(complex(4, -8), Fraction(2, 3)))  # error: Cannot infer type argument 1 of "pow"
reveal_type(4 .__pow__(7, 4))                     # Revealed type is "Any"
reveal_type(4 .__pow__(6, None))                  # Revealed type is "Any"

With the patch that this PR proposes, the output changes to the following:

from fractions import Fraction

reveal_type(pow(2, 4))                            # Revealed type is "builtins.int"
reveal_type(pow(5, -7))                           # Revealed type is "builtins.float"
reveal_type(pow(Fraction(), 4, None))             # Revealed type is "fractions.Fraction*"
reveal_type(pow(Fraction(3, 7), complex(1, 8)))   # Revealed type is "builtins.complex"
reveal_type(pow(complex(4, -8), Fraction(2, 3)))  # Revealed type is "builtins.complex"
reveal_type(4 .__pow__(7, 4))                     # Revealed type is "builtins.int"
reveal_type(4 .__pow__(6, None))                  # Revealed type is "builtins.int"

All other cases that I tested remain unchanged. A more exhaustive test script is attached, as are results of the test script with the current stubs, and with this PR applied:

The exact command I ran to test this patch was:

mypy --python-version 3.10 --no-site-packages --show-traceback --no-implicit-optional --disallow-any-generics --warn-incomplete-stub --no-error-summary --custom-typeshed-dir . --warn-unused-ignores test_pow.py

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood
Copy link
Member Author

I have no idea how this could be implemented, but it would be great if typeshed could have some kind of regression tests for really thorny cases like pow. Something like

assert_revealed_type_equality(pow(Fraction(), 4, None), Fraction)

stubtest and mypy_primer are amazing tools, but the corner cases of pow aren't common enough for these tools to reveal any change in behaviour here.

@JelleZijlstra
Copy link
Member

Yes, that's #1339. We never got around to actually implementing this, and stubtest and mypy-primer have removed some of the need for it, but it would still be useful.

@JelleZijlstra JelleZijlstra merged commit 48cfe5d into python:master Nov 18, 2021
@AlexWaygood AlexWaygood deleted the further-improve-pow branch November 18, 2021 07:37
@posita
Copy link
Contributor

posita commented Dec 10, 2021

I love this:

_SupportsSomeKindOfPow

🤣

@@ -179,6 +179,9 @@ class super(object):
@overload
def __init__(self) -> None: ...

_PositiveInteger = Literal[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25]
Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexWaygood, could you answer a couple of my questions?

  1. As far I understand the reason to have _PositiveInteger/_NegativeInteger is because we can't create "all positive/negative ints" types, right? So by defining a Literal we have at least some type inference for small exponents.
  2. Why did you choose to stop at 25/-20? My only guess now is that it fits in one line according to the formatting rules in typeshed 😅
  3. Why wasn't 0 exponent included in _PositiveInteger? I know it wouldn't be strictly "positive" integer then but 2**0 produces int in Python and currently it is Any according to typeshed.

Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Yup, you're quite correct! There's no way (not yet, anyway) of expressing "positive integer" in a general way, but if you're calling it with literal types, they're likely to be quite small -- and this hack is better than nothing :)
  2. Yup, once again spot on! I can't remember whether this is exactly the maximum line length black will allow under the settings typeshed has in pyproject.toml -- but it's close to it, and they're nice round numbers ;)
  3. Um... good catch! No reason at all! Feel free to submit a PR correcting my oversight! 🙂

@srittau
Copy link
Collaborator

srittau commented Feb 2, 2022

I love this:

_SupportsSomeKindOfPow

rofl

Too bad emoijs don't work as names, otherwise we could have named this _SupportsSomeKindOf💥.

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.

5 participants