From fc6faea9bed5e5a2da3fcdc38ca1b01febe2f94f Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Thu, 13 Apr 2023 17:28:42 +0200 Subject: [PATCH 01/13] Disallow forward refs for classes Though technically allowed, this enforces that subclasses are defined after the classes they inherit from which improves readability. --- pyi.py | 31 +------------------------------ tests/forward_refs.pyi | 3 ++- tests/forward_refs_annassign.pyi | 15 ++++++++++++++- 3 files changed, 17 insertions(+), 32 deletions(-) diff --git a/pyi.py b/pyi.py index 735e6358..8e084768 100644 --- a/pyi.py +++ b/pyi.py @@ -22,7 +22,7 @@ from flake8 import checker from flake8.options.manager import OptionManager # type: ignore[import] from flake8.plugins.pyflakes import FlakesChecker # type: ignore[import] -from pyflakes.checker import ClassDefinition, ClassScope, FunctionScope, ModuleScope +from pyflakes.checker import FunctionScope, ModuleScope if sys.version_info >= (3, 9): from ast import unparse @@ -218,35 +218,6 @@ def LAMBDA(self, node: ast.Lambda) -> None: super().LAMBDA(node) self.handleNode, self.deferHandleNode = self.deferHandleNode, self.handleNode # type: ignore[method-assign] - def CLASSDEF(self, node: ast.ClassDef) -> None: - if not isinstance(self.scope, ModuleScope): - # This shouldn't be necessary because .pyi files don't nest - # scopes much, but better safe than sorry. - super().CLASSDEF(node) - return - - # What follows is copied from pyflakes 1.3.0. The only changes are the - # deferHandleNode calls. - for decorator in node.decorator_list: - self.handleNode(decorator, node) - for baseNode in node.bases: - self.deferHandleNode(baseNode, node) - for keywordNode in node.keywords: - self.deferHandleNode(keywordNode, node) - self.pushScope(ClassScope) - # doctest does not process doctest within a doctest - # classes within classes are processed. - if ( - self.withDoctest - and not self._in_doctest() - and not isinstance(self.scope, FunctionScope) - ): - self.deferFunction(lambda: self.handleDoctests(node)) - for stmt in node.body: - self.handleNode(stmt, node) - self.popScope() - self.addBinding(node, ClassDefinition(node.name, node)) - def handleNodeDelete(self, node: ast.AST) -> None: """Null implementation. diff --git a/tests/forward_refs.pyi b/tests/forward_refs.pyi index 5f74f795..45384be3 100644 --- a/tests/forward_refs.pyi +++ b/tests/forward_refs.pyi @@ -8,7 +8,8 @@ __author__: str = ... def make_default_c() -> C: ... -class D(C): +# Disallow forward refs for base classes +class D(C): # F821 undefined name 'C' parent: C def __init__(self) -> None: ... diff --git a/tests/forward_refs_annassign.pyi b/tests/forward_refs_annassign.pyi index c8874735..77774316 100644 --- a/tests/forward_refs_annassign.pyi +++ b/tests/forward_refs_annassign.pyi @@ -8,7 +8,8 @@ __author__: str def make_default_c() -> C: ... -class D(C): +# Disallow forward refs for base classes +class D(C): # F821 undefined name 'C' parent: C def __init__(self) -> None: ... @@ -16,3 +17,15 @@ class C: other: C = ... def __init__(self) -> None: ... def from_str(self, s: str) -> C: ... + +class Baz(metaclass=Meta): # F821 undefined name 'Meta' + ... + +class Foo(Bar, Baz, metaclass=Meta): # F821 undefined name 'Bar' # F821 undefined name 'Meta' + ... + +class Meta(type): + ... + +class Bar(metaclass=Meta): + ... From 9eeb64ae86b9cea0f579f9a835e476e27a3833cc Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Thu, 13 Apr 2023 17:29:38 +0200 Subject: [PATCH 02/13] Correctly handle forward references in annotations By default pyflakes requires a future import for annotations in order to process forward references in annotations. This makes it so that the explicit import is not needed, which gets rid of a lot of F821 errors. --- pyi.py | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/pyi.py b/pyi.py index 8e084768..841ba37f 100644 --- a/pyi.py +++ b/pyi.py @@ -22,7 +22,7 @@ from flake8 import checker from flake8.options.manager import OptionManager # type: ignore[import] from flake8.plugins.pyflakes import FlakesChecker # type: ignore[import] -from pyflakes.checker import FunctionScope, ModuleScope +from pyflakes.checker import ModuleScope if sys.version_info >= (3, 9): from ast import unparse @@ -162,6 +162,20 @@ class TypeVarInfo(NamedTuple): class PyiAwareFlakesChecker(FlakesChecker): + @property + def annotationsFutureEnabled(self): + """pyflakes can already handle forward refs for annotations, but only via + `from __future__ import annotations`. + + We don't want to bother including this in every file, so we just set this to `True`. + """ + return True + + @annotationsFutureEnabled.setter + def annotationsFutureEnabled(self, value: bool): + """noop, we never want change the value to anything but `True` anyway.""" + pass + def deferHandleNode(self, node: ast.AST | None, parent) -> None: self.deferFunction(lambda: self.handleNode(node, parent)) @@ -184,29 +198,6 @@ def ASSIGN( self.deferHandleNode(tree.value, tree) - def ANNASSIGN(self, node: ast.AnnAssign) -> None: - """ - Annotated assignments don't have annotations evaluated on function - scope, hence the custom implementation. Compared to the pyflakes - version, we defer evaluation of the annotations (and values on - module level). - """ - if node.value: - # Only bind the *target* if the assignment has value. - # Otherwise it's not really ast.Store and shouldn't silence - # UndefinedLocal warnings. - self.handleNode(node.target, node) - if not isinstance(self.scope, FunctionScope): - self.deferHandleNode(node.annotation, node) - if node.value: - # If the assignment has value, handle the *value*... - if isinstance(self.scope, ModuleScope): - # ...later (if module scope). - self.deferHandleNode(node.value, node) - else: - # ...now. - self.handleNode(node.value, node) - def LAMBDA(self, node: ast.Lambda) -> None: """This is likely very brittle, currently works for pyflakes 1.3.0. From d7b3761f258289019860406a55dcda82a4ca3316 Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Fri, 14 Apr 2023 10:07:52 +0200 Subject: [PATCH 03/13] Add more tests --- tests/forward_refs_annassign.pyi | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/forward_refs_annassign.pyi b/tests/forward_refs_annassign.pyi index 77774316..3433c632 100644 --- a/tests/forward_refs_annassign.pyi +++ b/tests/forward_refs_annassign.pyi @@ -29,3 +29,12 @@ class Meta(type): class Bar(metaclass=Meta): ... + +# Allow circular references in annotations +class A: + foo: B + bar: dict[str, B] + +class B: + foo: A + bar: dict[str, A] From bf4f452c801cad1d18e7a352ee714013a83a3590 Mon Sep 17 00:00:00 2001 From: Tomas R Date: Wed, 26 Apr 2023 15:37:19 +0200 Subject: [PATCH 04/13] Allow recursive definitions Co-authored-by: Alex Waygood --- pyi.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pyi.py b/pyi.py index 85e2bf40..0d22736a 100644 --- a/pyi.py +++ b/pyi.py @@ -159,7 +159,25 @@ class TypeVarInfo(NamedTuple): ) +class PyflakesPreProcessor(ast.NodeTransformer): + """Transform AST prior to passing it to pyflakes. + + This reduces false positives on recursive class definitions. + """ + + def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef: + self.generic_visit(node) + node.bases = [ + base.value if isinstance(base, ast.Subscript) else base + for base in node.bases + ] + return node + + class PyiAwareFlakesChecker(FlakesChecker): + def __init__(self, tree: ast.AST, filename: str) -> None: + super().__init__(PyflakesPreProcessor().visit(tree), filename) + @property def annotationsFutureEnabled(self): """pyflakes can already handle forward refs for annotations, but only via From 5391d8448487d36f71cccdd315fa0cedc5a737b2 Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Wed, 26 Apr 2023 15:47:36 +0200 Subject: [PATCH 05/13] Update tests --- pyi.py | 3 ++- tests/forward_refs_annassign.pyi | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/pyi.py b/pyi.py index 0d22736a..38882327 100644 --- a/pyi.py +++ b/pyi.py @@ -168,6 +168,7 @@ class PyflakesPreProcessor(ast.NodeTransformer): def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef: self.generic_visit(node) node.bases = [ + # Remove the subscript to prevent F821 errors from being raised for (valid) recursive definitions. base.value if isinstance(base, ast.Subscript) else base for base in node.bases ] @@ -189,7 +190,7 @@ def annotationsFutureEnabled(self): @annotationsFutureEnabled.setter def annotationsFutureEnabled(self, value: bool): - """noop, we never want change the value to anything but `True` anyway.""" + """noop, we never want to change the value to anything but `True` anyway.""" pass def deferHandleNode(self, node: ast.AST | None, parent) -> None: diff --git a/tests/forward_refs_annassign.pyi b/tests/forward_refs_annassign.pyi index 00f5bf8f..565d0cfb 100644 --- a/tests/forward_refs_annassign.pyi +++ b/tests/forward_refs_annassign.pyi @@ -30,7 +30,6 @@ class Meta(type): class Bar(metaclass=Meta): ... -# Allow circular references in annotations class A: foo: B bar: dict[str, B] @@ -38,3 +37,12 @@ class A: class B: foo: A bar: dict[str, A] + +# This recursive definitions is allowed. +# We allow it by disabling checks for anything in the subscript +class Leaf: ... +class Tree(list[Tree | Leaf]): ... + +# This recursive definition is not allowed +class Parent(Child): ... # F821 undefined name 'Child' +class Child(Parent): ... From 04e80bfc49f2cf0419544e5bd7578c2d9e5ba562 Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Wed, 26 Apr 2023 15:48:40 +0200 Subject: [PATCH 06/13] Accidentally deleted a comment --- tests/forward_refs_annassign.pyi | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/forward_refs_annassign.pyi b/tests/forward_refs_annassign.pyi index 565d0cfb..bb40c3a1 100644 --- a/tests/forward_refs_annassign.pyi +++ b/tests/forward_refs_annassign.pyi @@ -30,6 +30,7 @@ class Meta(type): class Bar(metaclass=Meta): ... +# Allow circular references in annotations class A: foo: B bar: dict[str, B] From 713c67fc8be10dcd6a163c06dc086e8625b0e3c9 Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Wed, 26 Apr 2023 15:49:49 +0200 Subject: [PATCH 07/13] Fix typos --- tests/forward_refs_annassign.pyi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/forward_refs_annassign.pyi b/tests/forward_refs_annassign.pyi index bb40c3a1..59a8deb5 100644 --- a/tests/forward_refs_annassign.pyi +++ b/tests/forward_refs_annassign.pyi @@ -39,11 +39,11 @@ class B: foo: A bar: dict[str, A] -# This recursive definitions is allowed. +# This recursive definition is allowed. # We allow it by disabling checks for anything in the subscript class Leaf: ... class Tree(list[Tree | Leaf]): ... -# This recursive definition is not allowed +# This recursive definition is not allowed. class Parent(Child): ... # F821 undefined name 'Child' class Child(Parent): ... From 98ff42388ca4dc066b68828cc1721f1a48e06953 Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Wed, 26 Apr 2023 16:01:43 +0200 Subject: [PATCH 08/13] Fix for 3.7 --- pyi.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyi.py b/pyi.py index 38882327..c8c6ea82 100644 --- a/pyi.py +++ b/pyi.py @@ -176,8 +176,8 @@ def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef: class PyiAwareFlakesChecker(FlakesChecker): - def __init__(self, tree: ast.AST, filename: str) -> None: - super().__init__(PyflakesPreProcessor().visit(tree), filename) + def __init__(self, tree: ast.AST, **kwargs: Any) -> None: + super().__init__(PyflakesPreProcessor().visit(tree), **kwargs) @property def annotationsFutureEnabled(self): From de0f018741abeeaa5c6c135cd788e99f0a17ba59 Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Thu, 27 Apr 2023 10:29:01 +0200 Subject: [PATCH 09/13] Apply review suggestions Co-authored-by: Alex Waygood --- pyi.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pyi.py b/pyi.py index c8c6ea82..884fee19 100644 --- a/pyi.py +++ b/pyi.py @@ -168,7 +168,8 @@ class PyflakesPreProcessor(ast.NodeTransformer): def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef: self.generic_visit(node) node.bases = [ - # Remove the subscript to prevent F821 errors from being raised for (valid) recursive definitions. + # Remove the subscript to prevent F821 errors from being raised + # for (valid) recursive definitions. base.value if isinstance(base, ast.Subscript) else base for base in node.bases ] @@ -176,8 +177,8 @@ def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef: class PyiAwareFlakesChecker(FlakesChecker): - def __init__(self, tree: ast.AST, **kwargs: Any) -> None: - super().__init__(PyflakesPreProcessor().visit(tree), **kwargs) + def __init__(self, tree: ast.AST, *args: Any, **kwargs: Any) -> None: + super().__init__(PyflakesPreProcessor().visit(tree), *args, **kwargs) @property def annotationsFutureEnabled(self): @@ -190,7 +191,7 @@ def annotationsFutureEnabled(self): @annotationsFutureEnabled.setter def annotationsFutureEnabled(self, value: bool): - """noop, we never want to change the value to anything but `True` anyway.""" + """Does nothing, as we always want this property to be `True`.""" pass def deferHandleNode(self, node: ast.AST | None, parent) -> None: From 7ec2476ce41fac294dade0458943747212375df3 Mon Sep 17 00:00:00 2001 From: Tomas Roun Date: Thu, 27 Apr 2023 14:15:36 +0200 Subject: [PATCH 10/13] Add changelog entry --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cb7e514..43d2ece1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,13 @@ ## Unreleased * flake8-pyi no longer supports being run with flake8 <5.0.4. +* Improve handling of forward references for annotations and classes. + For annotations, forward references no longer raise an error. + For classes, forward references are now only allowed for recursive or + otherwise circular type definitions where forward references cannot + be avoided. In all other cases, classes must be defined before they + can be referenced as is standard in normal Python code. + The purpose of this change is to improve code readability. ## 23.4.1 From 7718b0935f96b155511bb75efdcd688514bb1e2e Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sun, 30 Apr 2023 21:29:14 +0100 Subject: [PATCH 11/13] More expansive changelog entry --- CHANGELOG.md | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43d2ece1..53fd9af5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,13 +3,27 @@ ## Unreleased * flake8-pyi no longer supports being run with flake8 <5.0.4. -* Improve handling of forward references for annotations and classes. - For annotations, forward references no longer raise an error. - For classes, forward references are now only allowed for recursive or - otherwise circular type definitions where forward references cannot - be avoided. In all other cases, classes must be defined before they - can be referenced as is standard in normal Python code. - The purpose of this change is to improve code readability. +* The way in which flake8-pyi modifies pyflakes runs has been improved: + * When flake8-pyi is installed, pyflakes now correctly recognises an annotation as + being equivalent to a binding assignment in a stub file, reducing false + positives from flake8's F821 error code. + * When flake8-pyi is installed, there are now fewer pyflakes positives from class + definitions that have forward references in the bases tuple for the purpose of + creating recursive or circular type definitions. These are invalid in `.py` files, + but are supported in stub files. + * When flake8-pyi is installed, pyflakes will also *complain* about code which (in + combination with flake8-pyi) it previously had no issue with. For example, it will + now complain about this code: + + ```py + class Foo(Bar): ... + class Bar: ... + ``` + + Although the above code is legal in a stub file, it is considered poor style, and + the forward reference serves no purpose (there is no recursive or circular + definition). As such, it is now disallowed by pyflakes when flake8-pyi is + installed. ## 23.4.1 From f686ef75f3fef30f311d1cba3f2f9d0173df2120 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sun, 30 Apr 2023 21:34:30 +0100 Subject: [PATCH 12/13] More tests --- tests/forward_refs.pyi | 4 ++++ tests/forward_refs_annassign.pyi | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/tests/forward_refs.pyi b/tests/forward_refs.pyi index 3429939b..378a1285 100644 --- a/tests/forward_refs.pyi +++ b/tests/forward_refs.pyi @@ -17,3 +17,7 @@ class C: other: C def __init__(self) -> None: ... def from_str(self, s: str) -> C: ... + +class Outer: + class Inner1: ... + class Inner2(list[Outer.Inner1]): ... diff --git a/tests/forward_refs_annassign.pyi b/tests/forward_refs_annassign.pyi index 59a8deb5..51f94791 100644 --- a/tests/forward_refs_annassign.pyi +++ b/tests/forward_refs_annassign.pyi @@ -47,3 +47,10 @@ class Tree(list[Tree | Leaf]): ... # This recursive definition is not allowed. class Parent(Child): ... # F821 undefined name 'Child' class Child(Parent): ... + +class MyClass: + foo: int + bar = foo + +baz: MyClass +eggs = baz From 463f2602f4765a9a72b70e3c70bec67bd1bdc370 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 1 May 2023 16:17:40 +0100 Subject: [PATCH 13/13] Clarify comment Co-authored-by: Akuli --- pyi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyi.py b/pyi.py index 884fee19..4c9f9ecd 100644 --- a/pyi.py +++ b/pyi.py @@ -169,7 +169,7 @@ def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef: self.generic_visit(node) node.bases = [ # Remove the subscript to prevent F821 errors from being raised - # for (valid) recursive definitions. + # for (valid) recursive definitions: Foo[Bar] --> Foo base.value if isinstance(base, ast.Subscript) else base for base in node.bases ]