-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Don't flag intentionally empty generators unreachable #15722
Don't flag intentionally empty generators unreachable #15722
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
mypy/checker.py
Outdated
@@ -1063,6 +1064,20 @@ def enter_attribute_inference_context(self) -> Iterator[None]: | |||
yield None | |||
self.inferred_attribute_types = old_types | |||
|
|||
def _is_empty_generator(self, func: FuncItem) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _is_empty_generator(self, func: FuncItem) -> bool: | |
@staticmethod | |
def _is_empty_generator(func: FuncItem) -> bool: |
Or even better: move the function into the global scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy/checker.py
Outdated
@@ -1240,7 +1255,7 @@ def check_func_def( | |||
# have no good way of doing this. | |||
# | |||
# TODO: Find a way of working around this limitation | |||
if len(expanded) >= 2: | |||
if len(expanded) >= 2 or self._is_empty_generator(item): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe update the multiline comment immediately above this line? We're not just suppressing reachability warnings for TypeVars with value restrictions anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't want to muddy the blame for that comment 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e31efd3
to
5e33f47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
teeny tiny nits
mypy/checker.py
Outdated
return ( | ||
len(body := func.body.body) == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think the first walrus here is slightly gratuitous
return ( | |
len(body := func.body.body) == 2 | |
body = func.body.body | |
return ( | |
len(body) == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy/checker.py
Outdated
# We suppress reachability warnings for empty generators (return; yield), since there's | ||
# no way to promote a function into a generator except by adding an "unreachable" yield. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# We suppress reachability warnings for empty generators (return; yield), since there's | |
# no way to promote a function into a generator except by adding an "unreachable" yield. | |
# We also suppress reachability warnings for empty generator functions | |
# (return; yield), since the most idiomatic way to promote a function into a | |
# generator function is often to add an "unreachable" yield. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had "also" at first, but I wanted to be able to remove the first comment w/o touching the second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, I think I'd personally prioritise a readable comment over a tiny bit more churn in git blame
:p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, easy. I've just made the "TODO remove me" part second. :P
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Alex Waygood <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This comment has been minimized.
This comment has been minimized.
@AlexWaygood I removed the comment in a090d56 since now this field appears to have a long-term legitimate use. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Alex Waygood <[email protected]>
Diff from mypy_primer, showing the effect of this PR on open source code: graphql-core (https://github.com/graphql-python/graphql-core): typechecking got 1.05x slower (366.9s -> 386.6s)
(Performance measurements are based on a single noisy sample)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankss!
Fixes #15345.