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

Merge subtype visitors #13303

Merged
merged 12 commits into from
Aug 3, 2022
Merged

Conversation

ilevkivskyi
Copy link
Member

Fixes #3297

This removes a significant chunk of code duplication. This is not a pure refactor, there were some cases when one of the visitors (mostly non-proper one) was more correct and/or complete. In few corner cases, where it was hard to decide, I merged behavior with if checks.

cc @JukkaL

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

The pandas errors are correct, isort is kind of expected as subtyping with Type[...] is a mess, but should be easy to fix, xarray is a mystery (and most likely a bug in the PR).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

@hauntsaninja @JelleZijlstra could one of you (or both) please take a look at this?

@JelleZijlstra
Copy link
Member

The starlette error (https://github.com/encode/starlette/blob/0b5b9c45ed281135f5d23d45041a12824d232260/starlette/config.py#L77) seems correct: the last overload is covered by the previous one.

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.

Looks reasonable but I'm not really familiar with this area of the code.

@ilevkivskyi
Copy link
Member Author

Thanks for review! Note I now also make it impossible to call is_subtype() with proper_subtype flag, by moving split point one call lower, at _is_subtype(), which is internal. The motivation is that such a big semantic change as being proper shouldn't be controlled by a flag (otherwise this would invite bugs in future).

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

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

starlette (https://github.com/encode/starlette)
+ starlette/config.py:87: error: Overloaded function signature 5 will never be matched: signature 4's parameter type(s) are the same or broader  [misc]

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/apply.py:510: error: Key expression in dictionary comprehension has incompatible type "Hashable"; expected type "NDFrame"  [misc]
+ pandas/core/generic.py:4411: error: Missing return statement  [return]

@ilevkivskyi ilevkivskyi merged commit d27bff6 into python:master Aug 3, 2022
@ilevkivskyi ilevkivskyi deleted the merge-subtype-visitors branch August 3, 2022 13:46
@hauntsaninja
Copy link
Collaborator

This PR has actually caused new errors on 3.11's typeshed which seem incorrect to me. Again to do with Type[...] subtyping.

You can repro with mypy -c '' --python-version 3.11 --no-silence-site-packages on master

@ilevkivskyi
Copy link
Member Author

Should be fixed by #13461

JukkaL added a commit that referenced this pull request Dec 20, 2022
Avoid the use of a nested function, which are a bit slow when compiled
with mypyc. Also avoid a callable value and instead call a function
directly, which allows using faster native calls.

Based on a quick experiment, this speeds up self check by about 3%.

This addresses some of the slowdown introduced in #13303.
JukkaL added a commit that referenced this pull request Dec 20, 2022
Mypyc isn't good at compiling nested functions, and this one was
in one of the hottest code paths in all of mypy. The nested
function wasn't even used all the time, but mypyc will still
construct a closure object every time.

This adds some code duplication, but it's well worth it. Amazingly,
this speeds up self-check by about 10%!

This addresses some of the slowdown introduced in #13303. #14324
addresses another related slowdown.
JukkaL added a commit that referenced this pull request Dec 20, 2022
Avoid the use of a nested function, which are a bit slow when compiled
with mypyc. Also avoid a callable value and instead call a function
directly, which allows using faster native calls.

Based on a quick experiment, this speeds up self check by about 3%.

This addresses some of the slowdown introduced in #13303.
JukkaL added a commit that referenced this pull request Dec 20, 2022
Mypyc isn't good at compiling nested functions, and this one was in one
of the hottest code paths in all of mypy. The nested function wasn't
even used that often, but mypyc would still construct a closure object
every time.

This adds some code duplication, but it's well worth it. Amazingly, this
speeds up self-check by about 10%, if my measurements are to be trusted!

This addresses some of the slowdown introduced in #13303. #14324
addresses another related slowdown.
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.

is_subtype vs is_proper_subtype
3 participants