Handle code guarded by 'if TYPE_CHECKING' correctly #530

PetterS · 2020-04-15T17:24:33Z

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.

asottile · 2020-04-15T17:45:42Z

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

asottile · 2020-04-15T17:49:27Z

I'm adding tests demonstrating that: ^ #531

PetterS · 2020-04-15T17:50:01Z

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?

asottile · 2020-04-15T17:53:29Z

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

PetterS · 2020-04-15T17:54:55Z

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?

asottile · 2020-04-15T17:57:42Z

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.)

PetterS · 2020-04-15T18:31:57Z

The updated code passes all tests including those added in #531.

asottile · 2020-04-15T18:36:07Z

pyflakes/checker.py

-                        scope[n.fullName].used = (self.scope, node)
-                    except KeyError:
-                        pass
+                if isinstance(n, Importation):


it's not just imports -- it's any code in there

for instance:

from typing import TYPE_CHECKING if TYPE_CHECKING: class C: ...

Right, I wasn't too worried about that case, but let's of course fix that!

asottile · 2020-04-15T18:36:26Z

pyflakes/checker.py

@@ -2120,6 +2131,7 @@ def IMPORT(self, node):
            else:
                name = alias.asname or alias.name
                importation = Importation(name, node, alias.name)
+            importation.during_type_checking = self._in_type_checking


this should be part of the construction

Will do! There is quite a hierarchy of classes that will touch but it makes sense I suppose.

This commit finds a problem that has been observed in read 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 commit ignores all code that is guarded by TYPE_CHECKING in order to find mistakes like this. This constant is always False when mypy is not running anyway.

asottile · 2020-04-15T18:39:58Z

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

PetterS · 2020-04-15T18:41:54Z

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.

asottile · 2020-04-15T18:47:00Z

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.

cool, just checking :)

PetterS · 2020-04-15T18:48:37Z

~~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.

asottile · 2020-04-15T19:03:18Z

pyflakes/checker.py

@@ -1072,6 +1074,8 @@ def addBinding(self, node, value):
                break
        existing = scope.get(value.name)

+        value.during_type_checking = self._in_type_checking


my comment before was because attribute stuffing is a smell (and a type checker such as mypy would error accordingly on this line), this should either be:

added to each of the types as part of their __init__ signature

stored separately alongside the type (a tuple?)

and not tacked onto (~essentially immutable by interface) objects after construction

(another alternative)
3. stored in a separate structure

Yeah, I agree. But I think we should store it separately somehow since otherwise we will have to change so many places where we create various bindings. Makes sense to only have addBinding know that this has to be done.

I think this is sound from a type-checking perspective, though? Since value is documented to be an instance of Binding we know that this property exists.

@asottile Binding has another attribute used that is used in a similar way as during_type_checking currently is. So it's not exactly immutable but I see what you mean since it's more obvious that used will change perhaps.

But I think we can store this property somewhere else in the scope. Shouldn't be too complicated.

Uploaded an example. Not really sure about it, but at least it is another solution.

pyflakes/checker.py

This reverts commit a6f0dd2.

PetterS · 2020-04-16T19:06:49Z

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 Binding subclasses would mean a lot of places suddenly needing to know about during_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 Binding constructors. All different Bindings are then created in a new method:

def createBinding(self, klass, *args, **kwargs):
  return klass(*args, during_type_checking=self._in_type_checking, **kwargs)

so instead of e.g.

importation = SubmoduleImportation(alias.name, node)

we would write

importation = self.createBinding(SubmoduleImportation, alias.name, node)

PetterS · 2020-04-19T08:16:23Z

@asottile

added to each of the types as part of their init signature

The current version of this PR now does this.

asottile

cool!

asottile · 2020-04-19T16:37:35Z

pyflakes/test/test_imports.py

+        ''', m.UndefinedName)
+
+    @skipIf(version_info < (3,), 'has type annotations')
+    def test_usesTypingImportsForAnnotations(self):


fwiw, the new tests are using snake_case

pyflakes/test/test_imports.py

asottile · 2020-04-19T16:39:34Z

pyflakes/checker.py

        self.name = name
        self.source = source
        self.used = False
+        assert isinstance(during_type_checking, bool)
+        self.during_type_checking = during_type_checking


maybe for_annotations is a more descriptive name here? (idk, I'm bikeshedding on this, feel free to ignore this suggestion, it just feels a little like the meaning of the name gets lost a bit)

It's fine; takes a couple of seconds to change.

PetterS · 2021-03-24T08:40:34Z

@asottile Should I merge this to master again? Is this likely to be merged?

asottile

sorry I lapsed on reviewing this -- looks good, just some very minor comments

asottile · 2021-03-26T00:38:53Z

pyflakes/checker.py

+        if not isinstance(body_nodes, list):
+            body_nodes = [body_nodes]


does this code ever run? I think the body is always a list here (?)

in IFEXP body is not a list.

Yes, there is a test that will fail if this if statement is removed.

asottile · 2021-03-26T00:43:24Z

pyflakes/test/test_imports.py

@@ -1031,6 +1039,48 @@ def test_futureImportStar(self):
        from __future__ import *
        ''', m.FutureFeatureNotDefined)

+    def test_ignores_typing_imports(self):


let's put these tests into the type_annotations test

asottile · 2021-03-26T00:43:51Z

pyflakes/test/test_type_annotations.py

+    def test_typing_guard_with_elif_branch(self):
+        # This test will actually not raise an error, even though it by analysis can
+        # be shown to have an error (Protocol is not defined outside TYPE_CHECKING).
+        # Pyflakes currently does not to case analysis.


terencehonles

I believe the main thing compared to #622 is this doesn't support if not TYPE_CHECKING, but that's probably not as likely as the other way so it's probably fine to not handle that.

terencehonles · 2021-03-26T01:22:55Z

pyflakes/checker.py

@@ -1216,6 +1226,10 @@ def handleNodeLoad(self, node):
                        scope[n.fullName].used = (self.scope, node)
                    except KeyError:
                        pass
+                if n.for_annotations and not self._in_annotation:


Should this not happen before scope[name].used is marked? It's not really being used and the variable should be eligible to be released.

See: https://github.com/PyCQA/pyflakes/pull/622/files#diff-5df44d79406311387e0056faae7a8092d1ce9cc85c7d15420186e45a0c712283L1204-R1218

If we do this, the following test will change:

from typing import TYPE_CHECKING if TYPE_CHECKING: from a import b b()

It will also return 'a.b' imported but unused after this change. Perhaps that is better.

We can do this.

Done. Thanks.

terencehonles · 2021-03-26T01:24:40Z

pyflakes/checker.py

+        if not isinstance(body_nodes, list):
+            body_nodes = [body_nodes]


in IFEXP body is not a list.

PetterS · 2021-03-26T08:14:34Z

Thanks for the comments! Now I have to remember what I wrote before! 😄 Will try to update the PR soon.

PetterS · 2021-03-26T15:59:29Z

@asottile made changes based on your review now.

PetterS · 2021-06-10T09:34:04Z

@asottile ping on this.

asottile · 2021-06-10T12:52:14Z

@asottile ping on this.

I already approved, pyflakes (unfortunately) has multi-sign-off

terencehonles · 2021-06-10T18:42:32Z

@asottile is there anyone specific that should be signing off?

asottile · 2021-06-10T20:10:36Z

@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

srittau

LGTM, although I would personally default the for_annotations to False. Especially the test code would be a bit cleaner.

sigmavirus24 · 2021-06-10T21:47:35Z

pyflakes/checker.py

        self.name = name
        self.source = source
        self.used = False
+        assert isinstance(for_annotations, bool)


What's the purpose of this assert here?

terencehonles · 2021-06-11T22:07:59Z

LGTM, although I would personally default the for_annotations to False. 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 😅

PetterS · 2021-06-14T18:51:38Z

@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 · 2021-06-14T18:52:46Z

@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

rggjan · 2022-09-21T13:17:31Z

Any updates on merging this PR?

asottile · 2022-09-21T13:27:36Z

Any updates on merging this PR?

just checking:

are you able to see the merge conflict message?

and the comment about unresolved discussions?

rggjan · 2022-09-21T13:48:27Z

are you able to see the merge conflict message?

Actually, I missed this when focusing on the big green checkmarks

and the comment about unresolved discussions?

Yes, and I assumed it was related to the Merged and Closed point here:

Thanks for the hints, though :)

asottile · 2022-09-21T13:51:35Z

Yes, and I assumed it was related to the Merged and Closed point here:

Thanks for the hints, though :)

easy to click through and see one of those is closing an issue as a duplicate and the other is a PR on an unrelated project!

joaoe · 2023-02-08T11:20:46Z

Howdy. Just a friendly reminder that is is sorely needed :) Can anyone rebase and merge ? Thanks.

PetterS · 2023-02-12T20:12:27Z

I will not be updating this PR.

terencehonles · 2023-03-31T11:01:01Z

I haven't been following this PR in awhile, but I've been making updates to my PR #622 since we're using it at my work and I just updated it again now and saw this conversation.

I believe it addresses all comments that were raised in this PR which was developed independently

PetterS force-pushed the omit_type_checking branch from 824a12d to 11a8266 Compare April 15, 2020 18:31

asottile reviewed Apr 15, 2020

View reviewed changes

PetterS force-pushed the omit_type_checking branch from 11a8266 to 44aab70 Compare April 15, 2020 18:39

PetterS force-pushed the omit_type_checking branch from e2b27b9 to 241262e Compare April 15, 2020 19:02

Make the Binding parameter a constructor and use it for all bindings.

d66aeb8

PetterS force-pushed the omit_type_checking branch from 241262e to d66aeb8 Compare April 15, 2020 19:03

asottile reviewed Apr 15, 2020

View reviewed changes

PetterS added 2 commits April 16, 2020 10:49

Remove constructor arg.

3b60143

Another solution.

a6f0dd2

asottile reviewed Apr 16, 2020

View reviewed changes

pyflakes/checker.py Outdated Show resolved Hide resolved

pyflakes/checker.py Outdated Show resolved Hide resolved

Revert "Another solution."

b938b2b

This reverts commit a6f0dd2.

Add during_type_checking to __init__ and set it during construction.

3474adf

Fix Python 2.7

b8e687a

PetterS mentioned this pull request Apr 19, 2020

Fix test for nightly Python (3.9) #532

Merged

asottile reviewed Apr 19, 2020

View reviewed changes

Snake case.

a30cfcb

PetterS force-pushed the omit_type_checking branch 2 times, most recently from ce08c41 to 7eb8484 Compare March 25, 2021 07:57

Merge branch 'master' into omit_type_checking

a9d4a7f

asottile approved these changes Mar 26, 2021

View reviewed changes

terencehonles reviewed Mar 26, 2021

View reviewed changes

Review

558914b

Move the check for _in_annotation

cbe214a

srittau approved these changes Jun 10, 2021

View reviewed changes

sigmavirus24 reviewed Jun 10, 2021

View reviewed changes

asottile mentioned this pull request Oct 16, 2021

Imports behind TYPE_CHECKING should trigger "undefined name" when used #658

Closed

crusaderky mentioned this pull request Jan 25, 2022

Defer pandas import on worker in P2P shuffle dask/distributed#5695

Merged

2 tasks

tobiasdiez mentioned this pull request Jan 7, 2023

Remove pyflakes? sagemath/sage#30415

Closed

asottile closed this Sep 27, 2023

		if not isinstance(body_nodes, list):
		body_nodes = [body_nodes]

Handle code guarded by 'if TYPE_CHECKING' correctly #530

Handle code guarded by 'if TYPE_CHECKING' correctly #530

Conversation

PetterS commented Apr 15, 2020 • edited Loading

asottile commented Apr 15, 2020

asottile commented Apr 15, 2020

PetterS commented Apr 15, 2020 • edited Loading

asottile commented Apr 15, 2020 • edited Loading

PetterS commented Apr 15, 2020

asottile commented Apr 15, 2020

PetterS commented Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

PetterS Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asottile commented Apr 15, 2020

PetterS commented Apr 15, 2020 • edited Loading

asottile commented Apr 15, 2020

PetterS commented Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetterS Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

PetterS Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetterS commented Apr 16, 2020

PetterS commented Apr 19, 2020

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetterS commented Mar 24, 2021

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

PetterS commented Mar 26, 2021

PetterS commented Mar 26, 2021

PetterS commented Jun 10, 2021

asottile commented Jun 10, 2021

terencehonles commented Jun 10, 2021

asottile commented Jun 10, 2021

srittau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles commented Jun 11, 2021

PetterS commented Jun 14, 2021

asottile commented Jun 14, 2021

rggjan commented Sep 21, 2022

asottile commented Sep 21, 2022

rggjan commented Sep 21, 2022

asottile commented Sep 21, 2022

joaoe commented Feb 8, 2023

PetterS commented Feb 12, 2023

terencehonles commented Mar 31, 2023

PetterS commented Apr 15, 2020 •

edited

Loading

PetterS commented Apr 15, 2020 •

edited

Loading

asottile commented Apr 15, 2020 •

edited

Loading

PetterS commented Apr 15, 2020 •

edited

Loading

PetterS Apr 15, 2020 •

edited

Loading

PetterS commented Apr 15, 2020 •

edited

Loading

PetterS commented Apr 15, 2020 •

edited

Loading

PetterS Apr 15, 2020 •

edited

Loading

PetterS Apr 16, 2020 •

edited

Loading

terencehonles Mar 26, 2021 •

edited

Loading

terencehonles Mar 26, 2021 •

edited

Loading