track `if typing.TYPE_CHECKING` to warn about non runtime bindings #622

terencehonles · 2021-03-23T20:45:27Z

When importing or defining values in if typing.TYPE_CHECKING blocks the bound names will not be available at runtime and may cause errors when used in the following way:

import typing

if typing.TYPE_CHECKING:
    from module import Type  # some slow import or circular reference

def method(value) -> Type:  # the import is needed by the type checker
    assert isinstance(value, Type)  # this is a runtime error

This change allows pyflakes to track what names are bound for runtime use, and allows it to warn when a non runtime name is used in a runtime context.

tox.ini

asottile · 2021-03-23T21:49:31Z

#530 predates this and has similar goals / problems -- perhaps collaborate with that PR?

terencehonles · 2021-03-23T22:25:19Z

#530 predates this and has similar goals / problems -- perhaps collaborate with that PR?

@asottile It looks like I actually ended up writing about the same code 😅 however there are a few things I have which the other one doesn't:

The logic handles the non runtime bindings in either the body or the else of the if/else expression
The runtime attribute defaults to True (it generally will be) which needs less changes in other parts of the codebase.
The lines https://github.com/PyCQA/pyflakes/pull/530/files#diff-5df44d79406311387e0056faae7a8092d1ce9cc85c7d15420186e45a0c712283R1178-R1181 continues looking up the stack for a runtime value, but it's more likely that the non runtime value will be declared at a higher scope and therefore the code I wrote warns and aborts immediately.

It also looks like the code was waiting review for almost a year. Is there something you wanted changed from that PR? I'm willing to work with @PetterS, but I didn't realize there was an PR that old for this already opened.

asottile · 2021-03-23T23:54:12Z

ah shoot, that's probably on me -- I might've missed the last round of reviews on there and then it lapsed (and now conflicts)

PetterS · 2021-03-24T08:39:53Z

I can revive that PR if we agree that it it is likely to be merged.

This is a problem that has been observed for real code, so would be good to fix.

terencehonles · 2021-03-24T17:17:58Z

I can revive that PR if we agree that it it is likely to be merged.

This is a problem that has been observed for real code, so would be good to fix.

It should be pretty easy to xref this PR in order to see what might also need to change since I based my PR off the most recent release (I was not aware of your PR when starting). We're 99% inline with each other's implementations.

PetterS · 2021-03-25T08:01:40Z

ah shoot, that's probably on me -- I might've missed the last round of reviews on there and then it lapsed (and now conflicts)

Merged it with master again now.

terencehonles · 2023-09-27T11:52:33Z

#530 predates this and has similar goals / problems -- perhaps collaborate with that PR?

@asottile since the PR in comment above is not going to be updated by its author

#530 (comment)

any chance you'd be willing to review this PR?

As mentioned above #622 (comment) it had used negated logic compared to the other PR in order to minimize the number of changes to the code base, and I've continued to both use and update this PR for changes on the development branch.

I can seek a 2nd approver if we can move forward with this PR.

asottile

I don't think the tests cover all the cases you intend to demonstrate here

asottile · 2023-09-27T13:14:21Z

pyflakes/checker.py

+            ClassDefinition(
+                node.name, node, runtime=not self._in_type_check_guard))


if you're going to reindent things can you use this style? https://github.com/asottile/add-trailing-comma#multi-line-method-invocation-style----why

alternatively I could reformat the codebase and the you'd deal with a round of merge conflicts -- either way

asottile · 2023-09-27T13:14:49Z

pyflakes/checker.py

    nodeDepth = 0
    offset = None
    _in_annotation = AnnotationState.NONE
+    _in_type_check_guard = False


these are actually all incorrect -- should be assigned in __init__ since they are not class vars

asottile · 2023-09-27T13:15:21Z

pyflakes/checker.py

+                if (not n.runtime and not (
+                        self._in_type_check_guard
+                        or self._in_annotation)):
+                    self.report(messages.UndefinedName, node, name)


should we make a separate type for this (so the message can clarify "only something something type checking"?)

asottile · 2023-09-27T13:16:12Z

pyflakes/checker.py

@@ -1073,12 +1078,18 @@ def handleNodeLoad(self, node, parent):
                    self.report(messages.InvalidPrintSyntax, node)

            try:


this try is now way too broad -- it originally only guarded the name lookup but now has a whole bunch of unrelated code in it

asottile · 2023-09-27T13:16:39Z

pyflakes/checker.py

@@ -226,10 +226,11 @@ class Binding:
                the node that this binding was last used.
    """

-    def __init__(self, name, source):
+    def __init__(self, name, source, runtime=True):


this boolean-trap should be keyword only -- same for all the other places it is introduced

see https://adamj.eu/tech/2021/07/10/python-type-hints-how-to-avoid-the-boolean-trap/

This should at least be , *, runtime=True

However as noted earlier in reviews, runtime isnt a good name. I would prefer to see a more explicit in_type_checking or similar, and have one name consistently used everywhere.

Also I suspect that this feature will be a lot cleaner if TYPE_CHECKING is treated as a special scope, which automatically means items in it are not placed into the normal scope logic. Then items in that special scope can only be considered by explicitly including them, only in the contexts when they are useful.

asottile · 2023-09-27T13:17:04Z

pyflakes/checker.py

+        # normalize body and orelse to a list
+        body, orelse = (
+            i if isinstance(i, list) else [i]
+            for i in (node.body, node.orelse))


i is a bad name here -- i should be for incrementing integers only

also this is a very strange way to do this -- don't go through a generator for two assignments just do the two assignments

asottile · 2023-09-27T13:17:29Z

pyflakes/checker.py

+
+        for n in orelse:
+            self.handleNode(n, node)
+        self._in_type_check_guard = orig


this should be a context manager or a try: finally: -- as it is right now it is not exception safe

asottile · 2023-09-27T13:18:03Z

pyflakes/checker.py

+
+        # set the guard and handle the orelse
+        if type_checking:
+            self._in_type_check_guard = True if reverse else orig


True if reverse else orig is better written as reverse or orig -- any time True if or False if should set off alarm bells

asottile · 2023-09-27T13:19:19Z

pyflakes/checker.py

+        # check if the body/orelse should be handled specially because it is
+        # a if TYPE_CHECKING guard.
+        test = node.test
+        reverse = False
+        if isinstance(test, ast.UnaryOp) and isinstance(test.op, ast.Not):
+            test = test.operand
+            reverse = True


I'd rather see a big if statement than a bunch of locals introduced here

asottile · 2023-09-27T13:20:02Z

pyflakes/checker.py

@@ -2038,12 +2089,15 @@ def TUPLE(self, node):
    LIST = TUPLE

    def IMPORT(self, node):
+        runtime = not self._in_type_check_guard


if we're going to invert this maybe it should just be _runtime ? but then that's not a great name so maybe runtime is not a great name?

terencehonles · 2023-10-06T20:17:22Z

Thanks for the review, I'll see when I have time to address it.

When importing or defining values in ``if typing.TYPE_CHECKING`` blocks the bound names will not be available at runtime and may cause errors when used in the following way:: import typing if typing.TYPE_CHECKING: from module import Type # some slow import or circular reference def method(value) -> Type: # the import is needed by the type checker assert isinstance(value, Type) # this is a runtime error This change allows pyflakes to track what names are bound for runtime use, and allows it to warn when a non runtime name is used in a runtime context.

terencehonles mentioned this pull request Mar 23, 2021

allow turning on run tracing from the command line #623

Closed

sigmavirus24 reviewed Mar 23, 2021

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

terencehonles force-pushed the warn-about-using-type-checking-imports-at-runtime branch from ac90967 to a795822 Compare March 23, 2021 22:18

terencehonles force-pushed the warn-about-using-type-checking-imports-at-runtime branch from a795822 to 117d5de Compare April 12, 2021 23:21

terencehonles mentioned this pull request Jun 11, 2021

Handle code guarded by 'if TYPE_CHECKING' correctly #530

Closed

terencehonles force-pushed the warn-about-using-type-checking-imports-at-runtime branch from 117d5de to 3bbd41a Compare October 18, 2021 14:43

terencehonles force-pushed the warn-about-using-type-checking-imports-at-runtime branch from 3bbd41a to 53c3070 Compare August 11, 2022 11:12

terencehonles force-pushed the warn-about-using-type-checking-imports-at-runtime branch from 53c3070 to 58506b3 Compare March 31, 2023 10:58

terencehonles force-pushed the warn-about-using-type-checking-imports-at-runtime branch from 58506b3 to 6dcf6a7 Compare September 27, 2023 11:36

asottile reviewed Sep 27, 2023

View reviewed changes

terencehonles force-pushed the warn-about-using-type-checking-imports-at-runtime branch from 6dcf6a7 to 8c4da4a Compare February 9, 2024 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

track `if typing.TYPE_CHECKING` to warn about non runtime bindings #622

track `if typing.TYPE_CHECKING` to warn about non runtime bindings #622

terencehonles commented Mar 23, 2021

asottile commented Mar 23, 2021

terencehonles commented Mar 23, 2021 •

edited

Loading

asottile commented Mar 23, 2021

PetterS commented Mar 24, 2021 •

edited

Loading

terencehonles commented Mar 24, 2021

PetterS commented Mar 25, 2021

terencehonles commented Sep 27, 2023

asottile left a comment

asottile Sep 27, 2023

asottile Sep 27, 2023

asottile Sep 27, 2023

asottile Sep 27, 2023

asottile Sep 27, 2023

jayvdb Feb 14, 2024

asottile Sep 27, 2023

asottile Sep 27, 2023

asottile Sep 27, 2023

asottile Sep 27, 2023

asottile Sep 27, 2023

asottile Sep 27, 2023

terencehonles commented Oct 6, 2023

		ClassDefinition(
		node.name, node, runtime=not self._in_type_check_guard))

		@@ -1073,12 +1078,18 @@ def handleNodeLoad(self, node, parent):
		self.report(messages.InvalidPrintSyntax, node)

		try:

track if typing.TYPE_CHECKING to warn about non runtime bindings #622

Are you sure you want to change the base?

track if typing.TYPE_CHECKING to warn about non runtime bindings #622

Conversation

terencehonles commented Mar 23, 2021

asottile commented Mar 23, 2021

terencehonles commented Mar 23, 2021 • edited Loading

asottile commented Mar 23, 2021

PetterS commented Mar 24, 2021 • edited Loading

terencehonles commented Mar 24, 2021

PetterS commented Mar 25, 2021

terencehonles commented Sep 27, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles commented Oct 6, 2023

track `if typing.TYPE_CHECKING` to warn about non runtime bindings #622

track `if typing.TYPE_CHECKING` to warn about non runtime bindings #622

terencehonles commented Mar 23, 2021 •

edited

Loading

PetterS commented Mar 24, 2021 •

edited

Loading