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

Improve ast types; revert several "redundant numeric union" changes from #7906 #9130

Merged
merged 7 commits into from
Nov 14, 2022

Conversation

kasium
Copy link
Contributor

@kasium kasium commented Nov 8, 2022

Since mypy 0.990 type promotions was limited.
This means that complex is not longer promoted to int/float, therefore we should adapt the types to list all possible types

Closes #9129

Since mypy  0.990 type promotions was limited.
This means that complex is not longer promoted to int/float, therefore
we should adapt the types to list all possible types

Closes python#9129
@kasium
Copy link
Contributor Author

kasium commented Nov 8, 2022

Mypy flake8 doesn't like it

./stdlib/_ast.pyi:332:12: Y041 Use "complex" instead of "float | complex" (see "The numeric tower" in PEP 484)

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member

Well I guess we can exclude Y041 (it is not very useful IMO), or if people still like it, you can put a # noqa: Y041 on those lines.

@kasium
Copy link
Contributor Author

kasium commented Nov 8, 2022

@ilevkivskyi I added a noqa for now and opened an issue at flake8-pyi

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I'd like to discuss this a little more before we go ahead with this. We may want to do a complete revert of #7906 if we need to go down this route.

@github-actions

This comment has been minimized.

.flake8 Outdated Show resolved Hide resolved
@ilevkivskyi
Copy link
Member

I don't think we need to do a full revert. It needs to be decided on case by case basis. For example, in function argument types using promotions is OK (unless function behaves differently for int and float).

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Changes to stubs look good!

@kasium
Copy link
Contributor Author

kasium commented Nov 14, 2022

Hi,
can we please get this merged? Do you know when mypy will pick this up?

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 14, 2022

Hi @kasium! The discussion in the mypy issue means that it looks like the behaviour change in mypy is about to be (partially) reverted: python/mypy#14077. So I don't think there's a massive rush to get this merged :)

Having said that, I'm happy for the changes to the ast and typed-ast stubs to go in. The typing specs don't precisely specify how type checkers should deal with implicit promotions with respect to attribute annotations. As such, we should err on the side of fully specified behaviour, and not rely on implicit promotions for these cases over at typeshed. So the changes to the ast and typed-ast stubs look good to me.

I'd rather fix flake8-pyi first, however, so that it only emits Y041 for parameter annotations. Then we won't need to switch off Y041 in typeshed's .flake8 config file, and this PR can be focused solely on the changes to the ast and typed-ast stub. I should be able to create a flake8-pyi fix and release at some point in the next few days.

@AlexWaygood
Copy link
Member

I've asked if there's any chance of getting a mypy patch release with python/mypy#14077 included: python/mypy#13871 (comment).

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 14, 2022

Update: mypy doesn't want to include python/mypy#14077 in a patch release. So let's merge this as a quick fix.

@AlexWaygood AlexWaygood changed the title Adapt number types in ast Revert several "redundant numeric union" changes from #7906 Nov 14, 2022
@AlexWaygood AlexWaygood changed the title Revert several "redundant numeric union" changes from #7906 Improve ast types; revert several "redundant numeric union" changes from #7906 Nov 14, 2022
@github-actions
Copy link
Contributor

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

flake8-pyi (https://github.com/PyCQA/flake8-pyi)
- pyi.py:1126: error: Subclass of "complex" and "int" cannot exist: would have incompatible method signatures  [unreachable]
- pyi.py:1275: error: Subclass of "complex" and "int" cannot exist: would have incompatible method signatures  [unreachable]
- pyi.py:1275: error: Right operand of "and" is never evaluated  [unreachable]
- pyi.py:1276: error: Statement is unreachable  [unreachable]
- pyi.py:1287: error: Subclass of "complex" and "int" cannot exist: would have incompatible method signatures  [unreachable]
- pyi.py:1288: error: Right operand of "and" is never evaluated  [unreachable]
- pyi.py:1290: error: Statement is unreachable  [unreachable]
- pyi.py:1313: error: Subclass of "complex" and "int" cannot exist: would have incompatible method signatures  [unreachable]

@AlexWaygood AlexWaygood merged commit f9cd5ee into python:main Nov 14, 2022
@AlexWaygood
Copy link
Member

Thanks for the PR @kasium!

@kasium
Copy link
Contributor Author

kasium commented Nov 14, 2022

Thanks for the fast work!

svalentin added a commit to svalentin/mypy that referenced this pull request Nov 14, 2022
svalentin added a commit to svalentin/mypy that referenced this pull request Nov 14, 2022
svalentin added a commit to python/mypy that referenced this pull request Nov 14, 2022
improve ast types; revert several "redundant numeric union" changes from #7906
see python/typeshed#9130
@AlexWaygood
Copy link
Member

@kasium — looks like the ast fixes will be included in the next mypy patch release, possibly coming out later today: python/mypy#13871 (comment)

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.

Adapt ast types for mypy 0.990
4 participants