Skip to content

Add special case for integers raised to a power of 2 #4473

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 4 commits into from
Sep 5, 2020

Conversation

jakebailey
Copy link
Contributor

@jakebailey jakebailey commented Aug 21, 2020

Per: #4352 (comment)

In general, you can't say that the return value of __pow__ is definitely an integer when called on an int, as a negative power means it returns a float. However, it's pretty common to just write x ** 2, moreso than any other literal. Add an overload that recognizes that any int to the second is still an int.

image

(Sorry to have taken so long to submit this.)

@jakebailey
Copy link
Contributor Author

jakebailey commented Aug 21, 2020

Oops, I'm missing the actual pow function, so I'll get that in before this would be merged (though I'm thinking adding it may be annoying, versus __pow__ is rarely directly called).

Two questions I should ask:

  • What is the preferred ordering here? Most specific to least specific? I'm thinking it might be strange for Literal[2] to be presented before more useful options.
  • Should I leave the optional modulo param? I'm thinking there is no modulo that could be provided that would make a power of 2 not be an int.

@JelleZijlstra
Copy link
Member

The more specific overload must be first because the first overload that matches gets picked. If the return type is int regardless of the value of modulo, I don't see why you shouldn't add the modulo param to the Literal[2] overload.

@jakebailey
Copy link
Contributor Author

Done.

I'm thinking this is okay as-is, if you're willing to let pow() and __pow__ not match. Given things are matched top to bottom, I think it might be annoying for users (with editors showing these overloads) to see the Literal[2] overload as the first choice in their tooltips. Whereas directly accessing the dunder method is very rare (and I was targeting x ** 2).

@JelleZijlstra
Copy link
Member

Unfortunately there's a mypy self-test failure because some error message needs to be adjusted. I can deal with it, but if you have time it would be helpful to make a PR to mypy to update the message.

@jakebailey
Copy link
Contributor Author

Hm, isn't this a chicken/egg problem, where mypy needs to be updated to have this new error, but typeshed can't merge until mypy accepts it?

I can take a look later, I've never touched mypy. 🙂

@JelleZijlstra
Copy link
Member

Yes it is, so we'll have to merge the two PRs concurrently, which is why it's annoying to do. :)

@Akuli
Copy link
Collaborator

Akuli commented Aug 22, 2020

x ** 3 might be somewhat common as well, support that too?

@jakebailey
Copy link
Contributor Author

jakebailey commented Aug 22, 2020

It's a slippery slope if more were to be added. When I ran a quick search of a bunch of projects I have checked out for testing, ** 2 was used an order of magnitude more than any other power. #4352 (comment)

@jakebailey
Copy link
Contributor Author

Forgive me for not updating this; I plan to get back into this / what needs to change in mypy on Friday when I have a bit more time.

@jakebailey
Copy link
Contributor Author

I've opened python/mypy#9415 to update that self test.

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.

4 participants