Fix used-before-assignment false positive for TYPE_CHECKING elif branch imports
#8441
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8441 +/- ##
==========================================
- Coverage 95.81% 95.80% -0.01%
==========================================
Files 174 174
Lines 18334 18320 -14
==========================================
- Hits 17567 17552 -15
- Misses 767 768 +1
|
Pierre-Sassoulas
added
False Positive π¦
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Hey, thanks for this. I'm starting to worry we're putting more effort into a flawed implementation. For instance, with these changes, we now have a false negative for this case:
from typing import TYPE_CHECKING
if TYPE_CHECKING:
pass
elif TYPE_CHECKING:
import dbm
print(dbm)No pressure, but I'm wondering if you explored whether #8431 (comment) was helpful in any way?
|
@jacobtylerwalls thanks, just saw your comment #8431 (comment). That does seem like a more fitting way for these checks, by filtering out the always false nodes. Let me look into that and in the meantime, I will mark this as draft. |
zenlyj
changed the title
used-before-assignment false positive for TYPE_CHECKING elif branch imports used-before-assignment false positive for TYPE_CHECKING elif branch imports
|
Another similar false positive to fix could be multiple-imports. Because an example of that can be seen in the file: "tests/functional/u/used/used_before_assignment_typing.py" on line 29 |
|
@ollie-iterators line 29 is supposed to raise |
|
Oops, I thought multiple imports was because calendar was imported in the if statement and the elif statement. |
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8441 +/- ##
==========================================
- Coverage 95.91% 95.90% -0.01%
==========================================
Files 174 174
Lines 18362 18357 -5
==========================================
- Hits 17611 17605 -6
- Misses 751 752 +1
|
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
|
Seeing music 21 the following test case should be added: import typing as t
if t.TYPE_CHECKING:
from music 21 import Stream
class StreamExample:
def my_func(self):
if t.TYPE_CHECKING:
assert isinstance(self, Stream)The panda one also seem genuine as we return in the try then we raise in the final except, so fixturedef is always going to be defined: def _get_active_fixturedef(
self, argname: str
) -> Union["FixtureDef[object]", PseudoFixtureDef[object]]:
try:
return self._fixture_defs[argname]
except KeyError:
try:
fixturedef = self._getnextfixturedef(argname)
except FixtureLookupError:
if argname == "request":
cached_result = (self, [0], None)
return PseudoFixtureDef(cached_result, Scope.Function)
raise
# Remove indent to prevent the python3 exception
# from leaking into the call.
self._compute_fixture_value(fixturedef) |
|
@Pierre-Sassoulas The music21 case is interesting, because |
|
The music21 assert is just there to give a hint to mypy. (It would be good to not emit a message.) |
|
I agree we should not raise if both are guarded in TYPE_CHECKING, but what about the following: import typing as t
if t.TYPE_CHECKING:
from music21 import Stream
class StreamExample(MusicStream): # Definitely used-before-assignment
def my_func(self):
if t.TYPE_CHECKING:
assert isinstance(self, Stream) # No used-before-assignment
assert isinstance(self, MusicStream) # used-before-assignment imo |
|
The mypy-hint false positive we're discussing is essentially the same as #8167, where I mentioned we'll eventually want to be smart enough to know it was the very same test that was guarding both the declaration and the use of the variable. If this PR is only driving a small amount of traffic into that existing false positive, we may not need to solve it right now on this PR. |
Actually, I think the pandas example should (probably) warn. If That said, we did choose in #5402 to assume that try blocks succeed, and only warn when definitions are made in an except. Someone could make the argument that's that's what's happening here, just with a nested try. Not that I would be all that convinced :-) |
There was a problem hiding this comment.
I think it would be good to add a test (in a separate file) for the failed case from the last review, but pending that, adding a green check!
Pierre-Sassoulas
added
False Negative π¦
Type of Changes
Description
This PR resolves false positives when imports are made in the
TYPE_CHECKINGelse if branch.Update:
As suggested by @jacobtylerwalls, some refactoring is done to change how
TYPE_CHECKINGimports are checked forused-before-assignment. Instead of relying onis_variable_violation(), it now integrates with existing logic inuncertain_nodes_in_false_testsinvolving the filtering of define nodes that are guarded under always false conditions.The issue lies inis_variable_violation(). The logic is supposed to rely on the usage of theTYPE_CHECKINGifstatement node, but it is relying on theelifstatement node for such cases.This solution searches the import node ancestors for theTYPE_CHECKINGbase branch.Additionally, a new static function_is_variable_annotation_in_function()is introduced to refactor the related logic and reduce nesting levels.Closes #8437