PyCQA
Handle code guarded by 'if TYPE_CHECKING' correctly
This PR finds a problem that has been observed in real code.
Consider the following code:
from typing import TYPE_CHECKING
...
if TYPE_CHECKING
from a import A, B
from b import C
...
def f() -> "B":
...
def f()
# Oops! C is actually used here.
C()
This PR ignores definitions that are guarded by TYPE_CHECKING in order to find mistakes like this. This constant is always False when mypy is not running anyway.
there's also if False: and MYPY = False\nif MYPY: and various other conventions for accomplishing the same thing
I don't think skipping the body there is quite right either as it would hide any errors in that block -- the block is not always just imports
your current patch breaks this for instance:
from typing import TYPE_CHECKING
if TYPE_CHECKING:
T = ...
def f() -> 'T':
pass
There is also this case:
from typing import TYPE_CHECKING
from a import a
if TYPE_CHECKING:
a()
Which probably should not raise an error? But on second thought maybe it should, since a could be moved inside the check?
pyflakes currently doesn't do any branch analysis which in some cases requires solving the halting problem
I'd argue that that code should raise an error, as it does during type checking time
your current patch breaks this for instance:
This means the solution to this problem will have to be more complicated. Basically imports within the TYPE_CHECKING should be kept around in case they are used in string annotations, but ignored for all other purposes?
Yes in some capacity, I also don't think this is limited to imports as any code can appear in such a block (functions, classes, etc.)
do you plan to handle if False / if MYPY? they're also common ways to approach this problem but I don't think if False is universally used and I don't think if MYPY is reliably detectable as being this particular case -- and the omission of handling those seems odd but I don't quite know the best thing to do there
do you plan to handle if False / if MYPY? they're also common ways to approach this problem but I don't think if False is universally used and I don't think if MYPY is reliably detectable as being this particular case -- and the omission of handling those seems odd but I don't quite know the best thing to do there
I wasn't planning on handling those. if False would be feasible I guess but as you say, MYPY could be hard to get right in all cases.
do you plan to handle if False / if MYPY? they're also common ways to approach this problem but I don't think if False is universally used and I don't think if MYPY is reliably detectable as being this particular case -- and the omission of handling those seems odd but I don't quite know the best thing to do there
I wasn't planning on handling those.
if Falsewould be feasible I guess but as you say,MYPYcould be hard to get right in all cases.
cool, just checking :)
~~Just double-checking: this is a feasible approach then?~~
~~Asking because adding during_type_checking to all Binding subclass constructors and handling all other types of bindings will expand the patch a bit.~~
Never mind, it was easy. Will rebase the PR once we agree on an implementation.
- Having a separate stack would mean a lot of work keeping track of both stacks when creating scopes and iterating over the stacks. I don't see how this could be done in a good way. I am not at this point comfortable with any large refactors to this class.
- Adding a parameter to the constructors of all
Bindingsubclasses would mean a lot of places suddenly needing to know aboutduring_type_checking. Not a good solution either.
I think it is really good to keep this new functionality in a single place. But at this point I almost feel that the current during_type_checking attribute on Binding is the cleanest solution. It follows used in the sense that it is a mutable property, albeit not one meant to be changed after the binding is put into the scope.
Here is another solution that could be acceptable:
- Add the parameter to all
Bindingconstructors. All differentBindings are then created in a new method:
so instead of e.g.def createBinding(self, klass, *args, **kwargs): return klass(*args, during_type_checking=self._in_type_checking, **kwargs)
we would writeimportation = SubmoduleImportation(alias.name, node)importation = self.createBinding(SubmoduleImportation, alias.name, node)
@asottile
added to each of the types as part of their init signature
The current version of this PR now does this.
this regresses the following code sample:
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from typing import TypedDict
else:
TypedDict = dict
class C(TypedDict):
x: int
Will continue working on this PR when we have CI up and running. We need the following PRs merged:
- https://github.com/PyCQA/pyflakes/pull/532
- https://github.com/PyCQA/pyflakes/pull/531
@PetterS sounds good -- we have a dual sign off and if you feel like you have enough context feel free to approve PRs. I've gone ahead and merged #532 since it's going to block other things anyway. You might also be interested in the the different approach taken by @srittau in #535 -- I hadn't considered an approach like that and it might just work perfectly for your thing as well!
@asottile Should I merge this to master again? Is this likely to be merged?
Thanks for the comments! Now I have to remember what I wrote before! 😄 Will try to update the PR soon.
@asottile ping on this.
I already approved, pyflakes (unfortunately) has multi-sign-off
@asottile is there anyone specific that should be signing off?
I've been going with the loosest interpretation of that possible and haven't had any pushback -- that is "anyone" + "a maintainer" being 2
LGTM, although I would personally default the
for_annotationstoFalse. Especially the test code would be a bit cleaner.
I +1 this suggestion and what I had done in my copy of this same effort #622. I didn't bring it up because it's kinda a nit, but since the comment has been made I'll let @PetterS know there is at least one other person who was biting their tongue before :sweat_smile:
@asottile is there anyone specific that should be signing off?
I've been going with the loosest interpretation of that possible and haven't had any pushback -- that is "anyone" + "a maintainer" being 2
Great, let's merge it then!
@asottile is there anyone specific that should be signing off?
I've been going with the loosest interpretation of that possible and haven't had any pushback -- that is "anyone" + "a maintainer" being 2
Great, let's merge it then!
wellll now there's two open discussions that should probably be decided / resolved first
Any updates on merging this PR?
just checking:
are you able to see the merge conflict message?
and the comment about unresolved discussions?
