From 5896c0326fd205141dcfe3248b8de652e46313ab Mon Sep 17 00:00:00 2001 From: Daverball Date: Sat, 7 Oct 2023 16:39:33 +0200 Subject: [PATCH 01/10] fix: Avoid TC201 false positives for top-level type checking decls This also adds TC2xx checks for the RHS of an explicit `TypeAlias` --- flake8_type_checking/checker.py | 127 +++++++++++++++++++++++------- flake8_type_checking/constants.py | 1 + tests/test_tc101.py | 31 ++++++++ tests/test_tc200.py | 33 ++++++++ tests/test_tc201.py | 72 +++++++++++++++++ 5 files changed, 237 insertions(+), 27 deletions(-) diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index 5322cd5..089a12d 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -3,6 +3,7 @@ import ast import fnmatch import os +import sys from ast import Index, literal_eval from contextlib import suppress from dataclasses import dataclass @@ -26,6 +27,7 @@ TC101, TC200, TC201, + TOP_LEVEL_PROPERTY, py38, ) @@ -399,13 +401,21 @@ def __init__( #: Tuple of (node, import name) for all import defined within a type-checking block # This lets us identify imports that *are* needed at runtime, for TC004 errors. self.type_checking_block_imports: set[tuple[Import, str]] = set() + + #: Set of variable names for all declarations defined within a type-checking block + # This lets us avoid false positives for annotations referring to e.g. a TypeAlias + # defined within a type checking block + self.type_checking_block_declarations: set[str] = set() + + #: Set of all the class names defined within the file + # This lets us avoid false positives for classes referring to themselves self.class_names: set[str] = set() #: All type annotations in the file, without quotes around them - self.unwrapped_annotations: list[tuple[int, int, str]] = [] + self.unwrapped_annotations: list[tuple[int, int, str, bool]] = [] #: All type annotations in the file, with quotes around them - self.wrapped_annotations: list[tuple[int, int, str]] = [] + self.wrapped_annotations: list[tuple[int, int, str, bool]] = [] #: Whether there is a `from __futures__ import annotations` is present in the file self.futures_annotation: Optional[bool] = None @@ -566,6 +576,10 @@ def visit_If(self, node: ast.If) -> Any: # first element in the else block - 1 start_of_else_block = node.orelse[0].lineno - 1 + # mark all the top-level statements + for stmt in node.body: + setattr(stmt, TOP_LEVEL_PROPERTY, True) + # Check for TC005 errors. if ((node.end_lineno or node.lineno) - node.lineno == 1) and ( len(node.body) == 1 and isinstance(node.body[0], ast.Pass) @@ -730,34 +744,34 @@ def visit_Constant(self, node: ast.Constant) -> ast.Constant: super().visit_Constant(node) return node - def add_annotation(self, node: ast.AST) -> None: + def add_annotation(self, node: ast.AST, is_alias: bool = False) -> None: """Map all annotations on an AST node.""" if isinstance(node, ast.Ellipsis) or node is None: return if isinstance(node, ast.BinOp): if not isinstance(node.op, ast.BitOr): return - self.add_annotation(node.left) - self.add_annotation(node.right) + self.add_annotation(node.left, is_alias) + self.add_annotation(node.right, is_alias) elif (py38 and isinstance(node, Index)) or isinstance(node, ast.Attribute): - self.add_annotation(node.value) + self.add_annotation(node.value, is_alias) elif isinstance(node, ast.Subscript): if getattr(node.value, 'id', '') != 'Literal': - self.add_annotation(node.value) - self.add_annotation(node.slice) + self.add_annotation(node.value, is_alias) + self.add_annotation(node.slice, is_alias) else: - self.add_annotation(node.value) + self.add_annotation(node.value, is_alias) elif isinstance(node, (ast.Tuple, ast.List)): for n in node.elts: - self.add_annotation(n) + self.add_annotation(n, is_alias) elif isinstance(node, ast.Constant) and isinstance(node.value, str): # Register annotation value setattr(node, ANNOTATION_PROPERTY, True) - self.wrapped_annotations.append((node.lineno, node.col_offset, node.value)) + self.wrapped_annotations.append((node.lineno, node.col_offset, node.value, is_alias)) elif isinstance(node, ast.Name): # Register annotation value setattr(node, ANNOTATION_PROPERTY, True) - self.unwrapped_annotations.append((node.lineno, node.col_offset, node.id)) + self.unwrapped_annotations.append((node.lineno, node.col_offset, node.id, is_alias)) @staticmethod def set_child_node_attribute(node: Any, attr: str, val: Any) -> Any: @@ -779,11 +793,63 @@ def visit_Attribute(self, node: ast.Attribute) -> Any: return node def visit_AnnAssign(self, node: ast.AnnAssign) -> None: - """Remove all annotation assignments.""" + """ + Remove all annotation assignments. + + But also keep track of any explicit `TypeAlias` assignments, we should treat the RHS like + an annotation as well, but only for the 2xx codes, since the RHS will not automatically + become a ForwardRef, like a true annotation would. + """ self.add_annotation(node.annotation) if node_value := getattr(node, 'value', None): self.visit(node_value) + # node is an explicit TypeAlias + if ( + node.value is not None + and isinstance(node.target, ast.Name) + and ( + (isinstance(node.annotation, ast.Name) and node.annotation.id == 'TypeAlias') + or (isinstance(node.annotation, ast.Constant) and node.annotation.value == 'TypeAlias') + ) + ): + self.add_annotation(node.value, is_alias=True) + + if getattr(node, TOP_LEVEL_PROPERTY, False): + self.type_checking_block_declarations.add(node.target.id) + + def visit_Assign(self, node: ast.Assign) -> ast.Assign: + """ + Keep track of any top-level variable declarations inside type-checking blocks. + + For simplicity we only accept single value assignments, i.e. there should only be one + target and it should be an `ast.Name`. + """ + if ( + getattr(node, TOP_LEVEL_PROPERTY, False) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + ): + self.type_checking_block_declarations.add(node.targets[0].id) + + super().visit_Assign(node) + return node + + if sys.version_info >= (3, 12): + + def visit_TypeAlias(self, node: ast.TypeAlias) -> None: + """ + Remove all type aliases, but treat the value on the RHS like an annotation. + + Keep track of any type aliases declared inside a type checking block using the new + `type Alias = value` syntax. + """ + # TODO: Check if the new syntax is affected by futures import and results in a ForwardRef + self.add_annotation(node.value, is_alias=True) + + if getattr(node, TOP_LEVEL_PROPERTY, False): + self.type_checking_block_declarations.add(node.name.id) + def register_function_ranges(self, node: Union[FunctionDef, AsyncFunctionDef]) -> None: """ Note down the start and end line number of a function. @@ -1071,7 +1137,10 @@ def futures_excess_quotes(self) -> Flake8Generator: """TC101.""" # If futures imports are present, any ast.Constant captured in add_annotation should yield an error if self.visitor.futures_annotation: - for lineno, col_offset, annotation in self.visitor.wrapped_annotations: + for lineno, col_offset, annotation, is_alias in self.visitor.wrapped_annotations: + if is_alias: # TypeAlias value will not be affected by a futures import + continue + yield lineno, col_offset, TC101.format(annotation=annotation), None else: """ @@ -1096,21 +1165,24 @@ def futures_excess_quotes(self) -> Flake8Generator: For anyone with more insight into how this might be tackled, contributions are very welcome. """ - for lineno, col_offset, annotation in self.visitor.wrapped_annotations: + for lineno, col_offset, annotation, is_alias in self.visitor.wrapped_annotations: + if is_alias: # TypeAlias value will not be affected by a futures import + continue + for _, import_name in self.visitor.type_checking_block_imports: if import_name in annotation: break else: for class_name in self.visitor.class_names: - if class_name == annotation: + if class_name in annotation: break else: yield lineno, col_offset, TC101.format(annotation=annotation), None def missing_quotes(self) -> Flake8Generator: """TC200.""" - for lineno, col_offset, annotation in self.visitor.unwrapped_annotations: + for lineno, col_offset, annotation, _ in self.visitor.unwrapped_annotations: # Annotations inside `if TYPE_CHECKING:` blocks do not need to be wrapped # unless they're used before definition, which is already covered by other # flake8 rules (and also static type checkers) @@ -1123,7 +1195,7 @@ def missing_quotes(self) -> Flake8Generator: def excess_quotes(self) -> Flake8Generator: """TC201.""" - for lineno, col_offset, annotation in self.visitor.wrapped_annotations: + for lineno, col_offset, annotation, _ in self.visitor.wrapped_annotations: # False positives for TC201 can be harmful, since fixing them, rather than # ignoring them will incur a runtime TypeError, so we should be even more # careful than with TC101 and favor false negatives even more, as such we @@ -1134,15 +1206,16 @@ def excess_quotes(self) -> Flake8Generator: continue # See comment in futures_excess_quotes - for _, import_name in self.visitor.type_checking_block_imports: - if import_name in annotation: - break - else: - for class_name in self.visitor.class_names: - if class_name in annotation: - break - else: - yield lineno, col_offset, TC201.format(annotation=annotation), None + if any(import_name in annotation for _, import_name in self.visitor.type_checking_block_imports): + continue + + if any(class_name in annotation for class_name in self.visitor.class_names): + continue + + if any(variable_name in annotation for variable_name in self.visitor.type_checking_block_declarations): + continue + + yield lineno, col_offset, TC201.format(annotation=annotation), None @property def errors(self) -> Flake8Generator: diff --git a/flake8_type_checking/constants.py b/flake8_type_checking/constants.py index 0edfd2c..4fe3795 100644 --- a/flake8_type_checking/constants.py +++ b/flake8_type_checking/constants.py @@ -4,6 +4,7 @@ ATTRIBUTE_PROPERTY = '_flake8-type-checking__parent' ANNOTATION_PROPERTY = '_flake8-type-checking__is_annotation' +TOP_LEVEL_PROPERTY = '_flake8-type-checking__is_top_level' ATTRS_DECORATORS = [ 'attrs.define', diff --git a/tests/test_tc101.py b/tests/test_tc101.py index 45247e7..93e07c3 100644 --- a/tests/test_tc101.py +++ b/tests/test_tc101.py @@ -2,6 +2,7 @@ File tests TC101: Annotation is wrapped in unnecessary quotes """ +import sys import textwrap import pytest @@ -113,8 +114,38 @@ def foo(self) -> 'X': '''), set(), ), + # Make sure we didn't introduce any regressions while solving #167 + # since we started to treat the RHS sort of like an annotation for + # some of the use-cases + ( + textwrap.dedent(''' + from __future__ import annotations + if TYPE_CHECKING: + from foo import Foo + + x: TypeAlias = 'Foo' + '''), + set(), + ), ] +if sys.version_info >= (3, 12): + examples.append( + ( + # Make sure we didn't introduce any regressions while solving #167 + # using new type alias syntax + # TODO: Make sure we actually need to wrap Foo if we use future + textwrap.dedent(''' + from __future__ import annotations + if TYPE_CHECKING: + from foo import Foo + + type x = 'Foo' + '''), + set(), + ) + ) + @pytest.mark.parametrize(('example', 'expected'), examples) def test_TC101_errors(example, expected): diff --git a/tests/test_tc200.py b/tests/test_tc200.py index 084fdb0..116759d 100644 --- a/tests/test_tc200.py +++ b/tests/test_tc200.py @@ -2,6 +2,7 @@ File tests TC200: Annotation should be wrapped in quotes """ +import sys import textwrap import pytest @@ -102,13 +103,45 @@ class FooDict(TypedDict): '''), { '9:5 ' + TC200.format(annotation='TypeAlias'), + '9:17 ' + TC200.format(annotation='Sequence'), '12:9 ' + TC200.format(annotation='Sequence'), '15:9 ' + TC200.format(annotation='Sequence'), '18:9 ' + TC200.format(annotation='Sequence'), }, ), + # RHS on an explicit TypeAlias should also emit a TC200 + ( + textwrap.dedent(''' + from typing import TypeAlias + + if TYPE_CHECKING: + from collections.abc import Sequence + + Foo: TypeAlias = Sequence[int] + '''), + { + '7:17 ' + TC200.format(annotation='Sequence'), + }, + ), ] +if sys.version_info >= (3, 12): + # RHS on an explicit TypeAlias should also emit a TC200 + # new type alias syntax + examples.append( + ( + textwrap.dedent(''' + if TYPE_CHECKING: + from collections.abc import Sequence + + type Foo = Sequence[int] + '''), + { + '5:17 ' + TC200.format(annotation='Sequence'), + }, + ) + ) + @pytest.mark.parametrize(('example', 'expected'), examples) def test_TC200_errors(example, expected): diff --git a/tests/test_tc201.py b/tests/test_tc201.py index d0f8efd..638b384 100644 --- a/tests/test_tc201.py +++ b/tests/test_tc201.py @@ -2,6 +2,7 @@ File tests TC201: Annotation is wrapped in unnecessary quotes """ +import sys import textwrap import pytest @@ -87,8 +88,79 @@ class Foo(Protocol): '''), set(), ), + ( + # Regression test for Issue #168 + textwrap.dedent(''' + if TYPE_CHECKING: + Foo = str | int + Bar: TypeAlias = Foo | None + T = TypeVar('T') + Ts = TypeVarTuple('Ts') + P = ParamSpec('P') + + x: 'Foo | None' + y: 'Bar | None' + Z: TypeAlias = 'Foo' + + def foo(a: 'T', *args: Unpack['Ts']) -> None: + pass + + def bar(*args: 'P.args', **kwargs: 'P.kwargs') -> None: + pass + '''), + set(), + ), + ( + # Inverse regression test for Issue #168 + # The declarations are inside a Protocol so they should not + # count towards declarations inside a type checking block + textwrap.dedent(''' + if TYPE_CHECKING: + class X(Protocol): + Foo = str | int + Bar: TypeAlias = Foo | None + T = TypeVar('T') + Ts = TypeVarTuple('Ts') + P = ParamSpec('P') + + x: 'Foo | None' + y: 'Bar | None' + Z: TypeAlias = 'Foo' + + def foo(a: 'T', *args: Unpack['Ts']) -> None: + pass + + def bar(*args: 'P.args', **kwargs: 'P.kwargs') -> None: + pass + '''), + { + '10:3 ' + TC201.format(annotation='Foo | None'), + '11:3 ' + TC201.format(annotation='Bar | None'), + '12:15 ' + TC201.format(annotation='Foo'), + '14:11 ' + TC201.format(annotation='T'), + '14:30 ' + TC201.format(annotation='Ts'), + '17:15 ' + TC201.format(annotation='P.args'), + '17:35 ' + TC201.format(annotation='P.kwargs'), + }, + ), ] +if sys.version_info >= (3, 12): + examples.append( + ( + # Regression test for Issue #168 + # using new type alias syntax + textwrap.dedent(''' + if TYPE_CHECKING: + type Foo = str | int + + x: 'Foo | None' + type Z = 'Foo' + '''), + set(), + ) + ) + @pytest.mark.parametrize(('example', 'expected'), examples) def test_TC201_errors(example, expected): From 55e96da2dbf24f5a33658ce8e52b3de9e17636f2 Mon Sep 17 00:00:00 2001 From: Daverball Date: Sat, 7 Oct 2023 16:47:48 +0200 Subject: [PATCH 02/10] Fixes offset in py312 specific test --- tests/test_tc200.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_tc200.py b/tests/test_tc200.py index 116759d..9276dc1 100644 --- a/tests/test_tc200.py +++ b/tests/test_tc200.py @@ -137,7 +137,7 @@ class FooDict(TypedDict): type Foo = Sequence[int] '''), { - '5:17 ' + TC200.format(annotation='Sequence'), + '5:11 ' + TC200.format(annotation='Sequence'), }, ) ) From b7d478987a0f134ea5f60637d95a73edd9c7d4f3 Mon Sep 17 00:00:00 2001 From: Daverball Date: Sat, 7 Oct 2023 22:17:22 +0200 Subject: [PATCH 03/10] Don't visit the value expression for an explicit `TypeAlias` It should behave like an annotation for the purposes of determining typing only imports. --- flake8_type_checking/checker.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index 089a12d..a7afb89 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -801,23 +801,24 @@ def visit_AnnAssign(self, node: ast.AnnAssign) -> None: become a ForwardRef, like a true annotation would. """ self.add_annotation(node.annotation) - if node_value := getattr(node, 'value', None): - self.visit(node_value) - # node is an explicit TypeAlias - if ( - node.value is not None - and isinstance(node.target, ast.Name) - and ( - (isinstance(node.annotation, ast.Name) and node.annotation.id == 'TypeAlias') - or (isinstance(node.annotation, ast.Constant) and node.annotation.value == 'TypeAlias') - ) + if node.value is None: + return + + # node is an explicit TypeAlias assignment + if isinstance(node.target, ast.Name) and ( + (isinstance(node.annotation, ast.Name) and node.annotation.id == 'TypeAlias') + or (isinstance(node.annotation, ast.Constant) and node.annotation.value == 'TypeAlias') ): self.add_annotation(node.value, is_alias=True) if getattr(node, TOP_LEVEL_PROPERTY, False): self.type_checking_block_declarations.add(node.target.id) + # if it wasn't a TypeAlias we need to visit the value expression + else: + self.visit(node.value) + def visit_Assign(self, node: ast.Assign) -> ast.Assign: """ Keep track of any top-level variable declarations inside type-checking blocks. From ed10ad6cf9948e538242b0b16c683b7322fe3943 Mon Sep 17 00:00:00 2001 From: Daverball Date: Sat, 7 Oct 2023 22:53:17 +0200 Subject: [PATCH 04/10] fix: Extracts names from wrapped annotations for matching. Previously we performed a simple substring match, but this is prone to error for cases like `Foo` vs `FooBar` which are separate names, but would produce false negatives. --- flake8_type_checking/checker.py | 91 ++++++++++++++++++++++----------- 1 file changed, 61 insertions(+), 30 deletions(-) diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index a7afb89..3b6ba9d 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -3,12 +3,13 @@ import ast import fnmatch import os +import re import sys from ast import Index, literal_eval from contextlib import suppress from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Literal, cast +from typing import TYPE_CHECKING, Literal, NamedTuple, cast from classify_imports import Classified, classify_base @@ -58,6 +59,9 @@ def ast_unparse(node: ast.AST) -> str: ) +NAME_RE = re.compile(r'[A-Za-z_]\w*') + + class AttrsMixin: """ Contains necessary logic for cattrs + attrs support. @@ -356,6 +360,25 @@ def import_type(self) -> ImportTypeValue: return cast('ImportTypeValue', classify_base(self.full_name.partition('.')[0])) +class UnwrappedAnnotation(NamedTuple): + """Represents a single `ast.Name` in an unwrapped annotation.""" + + lineno: int + col_offset: int + annotation: str + is_alias: bool + + +class WrappedAnnotation(NamedTuple): + """Represents a wrapped annotation i.e. a string constant.""" + + lineno: int + col_offset: int + annotation: str + names: set[str] + is_alias: bool + + class ImportVisitor(DunderAllMixin, AttrsMixin, FastAPIMixin, PydanticMixin, ast.NodeVisitor): """Map all imports outside of type-checking blocks.""" @@ -412,10 +435,10 @@ def __init__( self.class_names: set[str] = set() #: All type annotations in the file, without quotes around them - self.unwrapped_annotations: list[tuple[int, int, str, bool]] = [] + self.unwrapped_annotations: list[UnwrappedAnnotation] = [] #: All type annotations in the file, with quotes around them - self.wrapped_annotations: list[tuple[int, int, str, bool]] = [] + self.wrapped_annotations: list[WrappedAnnotation] = [] #: Whether there is a `from __futures__ import annotations` is present in the file self.futures_annotation: Optional[bool] = None @@ -767,11 +790,13 @@ def add_annotation(self, node: ast.AST, is_alias: bool = False) -> None: elif isinstance(node, ast.Constant) and isinstance(node.value, str): # Register annotation value setattr(node, ANNOTATION_PROPERTY, True) - self.wrapped_annotations.append((node.lineno, node.col_offset, node.value, is_alias)) + self.wrapped_annotations.append( + WrappedAnnotation(node.lineno, node.col_offset, node.value, set(NAME_RE.findall(node.value)), is_alias) + ) elif isinstance(node, ast.Name): # Register annotation value setattr(node, ANNOTATION_PROPERTY, True) - self.unwrapped_annotations.append((node.lineno, node.col_offset, node.id, is_alias)) + self.unwrapped_annotations.append(UnwrappedAnnotation(node.lineno, node.col_offset, node.id, is_alias)) @staticmethod def set_child_node_attribute(node: Any, attr: str, val: Any) -> Any: @@ -1068,8 +1093,8 @@ def unused_imports(self) -> Flake8Generator: unused_imports = set(self.visitor.imports) - self.visitor.names used_imports = set(self.visitor.imports) - unused_imports already_imported_modules = [self.visitor.imports[name].module for name in used_imports] - annotation_names = [i[2] for i in self.visitor.wrapped_annotations] + [ - i[2] for i in self.visitor.unwrapped_annotations + annotation_names = [n for i in self.visitor.wrapped_annotations for n in i.names] + [ + i.annotation for i in self.visitor.unwrapped_annotations ] for name in unused_imports: @@ -1138,11 +1163,11 @@ def futures_excess_quotes(self) -> Flake8Generator: """TC101.""" # If futures imports are present, any ast.Constant captured in add_annotation should yield an error if self.visitor.futures_annotation: - for lineno, col_offset, annotation, is_alias in self.visitor.wrapped_annotations: - if is_alias: # TypeAlias value will not be affected by a futures import + for item in self.visitor.wrapped_annotations: + if item.is_alias: # TypeAlias value will not be affected by a futures import continue - yield lineno, col_offset, TC101.format(annotation=annotation), None + yield item.lineno, item.col_offset, TC101.format(annotation=item.annotation), None else: """ If we have no futures import and we have no imports inside a type-checking block, things get more tricky: @@ -1161,25 +1186,31 @@ def futures_excess_quotes(self) -> Flake8Generator: The annotation is *not* broken down into its components, but rather returns an ast.Constant with a string value representation of the annotation. In other words, you get one element, with the value `'Dict[int]'`. - Because we can't match exactly, I've erred on the side of caution below, opting for some false negatives - instead of some false positives. + We use a RegEx to extract all the variable names in the annotation into a set we can match against, but + unlike with unwrapped annotations we don't put them all into separate entries, because there would be false + positives for annotations like the one above, since int does not need to be wrapped, but Dict might, but + the inverse would be true for something like: - For anyone with more insight into how this might be tackled, contributions are very welcome. + `x: 'set[Pattern[str]]'` + + Which could be turned into the following and still be fine: + + `x: set['Pattern[str]'] + + So we don't try to unwrap the annotations as far as possible, we just check if the entire + annotation can be unwrapped or not. """ - for lineno, col_offset, annotation, is_alias in self.visitor.wrapped_annotations: - if is_alias: # TypeAlias value will not be affected by a futures import + for item in self.visitor.wrapped_annotations: + if item.is_alias: # TypeAlias value will not be affected by a futures import + continue + + if any(import_name in item.names for _, import_name in self.visitor.type_checking_block_imports): continue - for _, import_name in self.visitor.type_checking_block_imports: - if import_name in annotation: - break + if any(class_name in item.names for class_name in self.visitor.class_names): + continue - else: - for class_name in self.visitor.class_names: - if class_name in annotation: - break - else: - yield lineno, col_offset, TC101.format(annotation=annotation), None + yield item.lineno, item.col_offset, TC101.format(annotation=item.annotation), None def missing_quotes(self) -> Flake8Generator: """TC200.""" @@ -1196,27 +1227,27 @@ def missing_quotes(self) -> Flake8Generator: def excess_quotes(self) -> Flake8Generator: """TC201.""" - for lineno, col_offset, annotation, _ in self.visitor.wrapped_annotations: + for item in self.visitor.wrapped_annotations: # False positives for TC201 can be harmful, since fixing them, rather than # ignoring them will incur a runtime TypeError, so we should be even more # careful than with TC101 and favor false negatives even more, as such we # give up immediately if the annotation contains square brackets, because # we can't know if subscripting the type at runtime is safe without inspecting # the type's source code. - if '[' in annotation: + if '[' in item.annotation: continue # See comment in futures_excess_quotes - if any(import_name in annotation for _, import_name in self.visitor.type_checking_block_imports): + if any(import_name in item.names for _, import_name in self.visitor.type_checking_block_imports): continue - if any(class_name in annotation for class_name in self.visitor.class_names): + if any(class_name in item.names for class_name in self.visitor.class_names): continue - if any(variable_name in annotation for variable_name in self.visitor.type_checking_block_declarations): + if any(variable_name in item.names for variable_name in self.visitor.type_checking_block_declarations): continue - yield lineno, col_offset, TC201.format(annotation=annotation), None + yield item.lineno, item.col_offset, TC201.format(annotation=item.annotation), None @property def errors(self) -> Flake8Generator: From 9d1f7d7268370f5fe2f06689aa6e4d0ba5f9b9e6 Mon Sep 17 00:00:00 2001 From: Daverball Date: Sun, 8 Oct 2023 14:39:59 +0200 Subject: [PATCH 05/10] Improves name regex for `Literal` and adds tests. --- flake8_type_checking/checker.py | 5 +---- flake8_type_checking/constants.py | 3 +++ tests/test_name_extraction.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 tests/test_name_extraction.py diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index 3b6ba9d..bc1e4ce 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -3,7 +3,6 @@ import ast import fnmatch import os -import re import sys from ast import Index, literal_eval from contextlib import suppress @@ -18,6 +17,7 @@ ATTRIBUTE_PROPERTY, ATTRS_DECORATORS, ATTRS_IMPORTS, + NAME_RE, TC001, TC002, TC003, @@ -59,9 +59,6 @@ def ast_unparse(node: ast.AST) -> str: ) -NAME_RE = re.compile(r'[A-Za-z_]\w*') - - class AttrsMixin: """ Contains necessary logic for cattrs + attrs support. diff --git a/flake8_type_checking/constants.py b/flake8_type_checking/constants.py index 4fe3795..f12a34f 100644 --- a/flake8_type_checking/constants.py +++ b/flake8_type_checking/constants.py @@ -1,3 +1,4 @@ +import re import sys import flake8 @@ -6,6 +7,8 @@ ANNOTATION_PROPERTY = '_flake8-type-checking__is_annotation' TOP_LEVEL_PROPERTY = '_flake8-type-checking__is_top_level' +NAME_RE = re.compile(r'(? Date: Sun, 8 Oct 2023 16:41:09 +0200 Subject: [PATCH 06/10] Properly handles 3.12 style type aliases. Adds new error codes for type alias specific errors. --- README.md | 2 + flake8_type_checking/checker.py | 75 ++++++++++++-------- flake8_type_checking/constants.py | 4 +- tests/test_tc007.py | 69 ++++++++++++++++++ tests/test_tc008.py | 113 ++++++++++++++++++++++++++++++ tests/test_tc200.py | 35 +-------- tests/test_tc201.py | 20 +----- 7 files changed, 235 insertions(+), 83 deletions(-) create mode 100644 tests/test_tc007.py create mode 100644 tests/test_tc008.py diff --git a/README.md b/README.md index a881650..7268ee7 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,8 @@ And depending on which error code range you've opted into, it will tell you | TC004 | Move import out of type-checking block. Import is used for more than type hinting. | | TC005 | Found empty type-checking block | | TC006 | Annotation in typing.cast() should be a string literal | +| TC007 | Type alias needs to be made into a string literal | +| TC008 | Type alias does not need to be a string literal | ## Choosing how to handle forward references diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index bc1e4ce..ef8013a 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -24,6 +24,8 @@ TC004, TC005, TC006, + TC007, + TC008, TC100, TC101, TC200, @@ -363,7 +365,7 @@ class UnwrappedAnnotation(NamedTuple): lineno: int col_offset: int annotation: str - is_alias: bool + type: Literal['annotation', 'alias', 'new-alias'] class WrappedAnnotation(NamedTuple): @@ -373,7 +375,7 @@ class WrappedAnnotation(NamedTuple): col_offset: int annotation: str names: set[str] - is_alias: bool + type: Literal['annotation', 'alias', 'new-alias'] class ImportVisitor(DunderAllMixin, AttrsMixin, FastAPIMixin, PydanticMixin, ast.NodeVisitor): @@ -764,36 +766,34 @@ def visit_Constant(self, node: ast.Constant) -> ast.Constant: super().visit_Constant(node) return node - def add_annotation(self, node: ast.AST, is_alias: bool = False) -> None: + def add_annotation(self, node: ast.AST, type: Literal['annotation', 'alias', 'new-alias'] = 'annotation') -> None: """Map all annotations on an AST node.""" if isinstance(node, ast.Ellipsis) or node is None: return if isinstance(node, ast.BinOp): if not isinstance(node.op, ast.BitOr): return - self.add_annotation(node.left, is_alias) - self.add_annotation(node.right, is_alias) + self.add_annotation(node.left, type) + self.add_annotation(node.right, type) elif (py38 and isinstance(node, Index)) or isinstance(node, ast.Attribute): - self.add_annotation(node.value, is_alias) + self.add_annotation(node.value, type) elif isinstance(node, ast.Subscript): + self.add_annotation(node.value, type) if getattr(node.value, 'id', '') != 'Literal': - self.add_annotation(node.value, is_alias) - self.add_annotation(node.slice, is_alias) - else: - self.add_annotation(node.value, is_alias) + self.add_annotation(node.slice, type) elif isinstance(node, (ast.Tuple, ast.List)): for n in node.elts: - self.add_annotation(n, is_alias) + self.add_annotation(n, type) elif isinstance(node, ast.Constant) and isinstance(node.value, str): # Register annotation value setattr(node, ANNOTATION_PROPERTY, True) self.wrapped_annotations.append( - WrappedAnnotation(node.lineno, node.col_offset, node.value, set(NAME_RE.findall(node.value)), is_alias) + WrappedAnnotation(node.lineno, node.col_offset, node.value, set(NAME_RE.findall(node.value)), type) ) elif isinstance(node, ast.Name): # Register annotation value setattr(node, ANNOTATION_PROPERTY, True) - self.unwrapped_annotations.append(UnwrappedAnnotation(node.lineno, node.col_offset, node.id, is_alias)) + self.unwrapped_annotations.append(UnwrappedAnnotation(node.lineno, node.col_offset, node.id, type)) @staticmethod def set_child_node_attribute(node: Any, attr: str, val: Any) -> Any: @@ -832,7 +832,7 @@ def visit_AnnAssign(self, node: ast.AnnAssign) -> None: (isinstance(node.annotation, ast.Name) and node.annotation.id == 'TypeAlias') or (isinstance(node.annotation, ast.Constant) and node.annotation.value == 'TypeAlias') ): - self.add_annotation(node.value, is_alias=True) + self.add_annotation(node.value, 'alias') if getattr(node, TOP_LEVEL_PROPERTY, False): self.type_checking_block_declarations.add(node.target.id) @@ -867,8 +867,7 @@ def visit_TypeAlias(self, node: ast.TypeAlias) -> None: Keep track of any type aliases declared inside a type checking block using the new `type Alias = value` syntax. """ - # TODO: Check if the new syntax is affected by futures import and results in a ForwardRef - self.add_annotation(node.value, is_alias=True) + self.add_annotation(node.value, 'new-alias') if getattr(node, TOP_LEVEL_PROPERTY, False): self.type_checking_block_declarations.add(node.name.id) @@ -1073,9 +1072,9 @@ def __init__(self, node: ast.Module, options: Optional[Namespace]) -> None: self.missing_futures_import, # TC101 self.futures_excess_quotes, - # TC200 + # TC200, TC006 self.missing_quotes, - # TC201 + # TC201, TC007 self.excess_quotes, ] @@ -1090,8 +1089,8 @@ def unused_imports(self) -> Flake8Generator: unused_imports = set(self.visitor.imports) - self.visitor.names used_imports = set(self.visitor.imports) - unused_imports already_imported_modules = [self.visitor.imports[name].module for name in used_imports] - annotation_names = [n for i in self.visitor.wrapped_annotations for n in i.names] + [ - i.annotation for i in self.visitor.unwrapped_annotations + annotation_names = [n for i in self.visitor.wrapped_annotations if i.type != 'new-alias' for n in i.names] + [ + i.annotation for i in self.visitor.unwrapped_annotations if i.type != 'new-alias' ] for name in unused_imports: @@ -1161,7 +1160,7 @@ def futures_excess_quotes(self) -> Flake8Generator: # If futures imports are present, any ast.Constant captured in add_annotation should yield an error if self.visitor.futures_annotation: for item in self.visitor.wrapped_annotations: - if item.is_alias: # TypeAlias value will not be affected by a futures import + if item.type != 'annotation': # TypeAlias value will not be affected by a futures import continue yield item.lineno, item.col_offset, TC101.format(annotation=item.annotation), None @@ -1198,7 +1197,7 @@ def futures_excess_quotes(self) -> Flake8Generator: annotation can be unwrapped or not. """ for item in self.visitor.wrapped_annotations: - if item.is_alias: # TypeAlias value will not be affected by a futures import + if item.type != 'annotation': # TypeAlias value will not be affected by a futures import continue if any(import_name in item.names for _, import_name in self.visitor.type_checking_block_imports): @@ -1210,21 +1209,34 @@ def futures_excess_quotes(self) -> Flake8Generator: yield item.lineno, item.col_offset, TC101.format(annotation=item.annotation), None def missing_quotes(self) -> Flake8Generator: - """TC200.""" - for lineno, col_offset, annotation, _ in self.visitor.unwrapped_annotations: + """TC200 and TC007.""" + for item in self.visitor.unwrapped_annotations: + # A new style alias does never need to be wrapped + if item.type == 'new-alias': + continue + # Annotations inside `if TYPE_CHECKING:` blocks do not need to be wrapped # unless they're used before definition, which is already covered by other # flake8 rules (and also static type checkers) - if self.visitor.in_type_checking_block(lineno, col_offset): + if self.visitor.in_type_checking_block(item.lineno, item.col_offset): continue for _, name in self.visitor.type_checking_block_imports: - if annotation == name: - yield lineno, col_offset, TC200.format(annotation=annotation), None + if item.annotation == name: + if item.type == 'alias': + error = TC007.format(alias=item.annotation) + else: + error = TC200.format(annotation=item.annotation) + yield item.lineno, item.col_offset, error, None def excess_quotes(self) -> Flake8Generator: - """TC201.""" + """TC201 and TC008.""" for item in self.visitor.wrapped_annotations: + # A new style type alias should never be wrapped + if item.type == 'new-alias': + yield item.lineno, item.col_offset, TC008.format(alias=item.annotation), None + continue + # False positives for TC201 can be harmful, since fixing them, rather than # ignoring them will incur a runtime TypeError, so we should be even more # careful than with TC101 and favor false negatives even more, as such we @@ -1244,7 +1256,12 @@ def excess_quotes(self) -> Flake8Generator: if any(variable_name in item.names for variable_name in self.visitor.type_checking_block_declarations): continue - yield item.lineno, item.col_offset, TC201.format(annotation=item.annotation), None + if item.type == 'alias': + error = TC008.format(alias=item.annotation) + else: + error = TC201.format(annotation=item.annotation) + + yield item.lineno, item.col_offset, error, None @property def errors(self) -> Flake8Generator: diff --git a/flake8_type_checking/constants.py b/flake8_type_checking/constants.py index f12a34f..8195d63 100644 --- a/flake8_type_checking/constants.py +++ b/flake8_type_checking/constants.py @@ -31,7 +31,9 @@ TC004 = "TC004 Move import '{module}' out of type-checking block. Import is used for more than type hinting." TC005 = 'TC005 Found empty type-checking block' TC006 = "TC006 Annotation '{annotation}' in typing.cast() should be a string literal" +TC007 = "TC007 Type alias '{alias}' needs to be made into a string literal" +TC008 = "TC008 Type alias '{alias}' does not need to be a string literal" TC100 = "TC100 Add 'from __future__ import annotations' import" TC101 = "TC101 Annotation '{annotation}' does not need to be a string literal" TC200 = "TC200 Annotation '{annotation}' needs to be made into a string literal" -TC201 = "TC201 Annotation '{annotation}' does not need to be a string" +TC201 = "TC201 Annotation '{annotation}' does not need to be a string literal" diff --git a/tests/test_tc007.py b/tests/test_tc007.py new file mode 100644 index 0000000..2fb78f6 --- /dev/null +++ b/tests/test_tc007.py @@ -0,0 +1,69 @@ +""" +File tests TC007: + Type alias should be wrapped in quotes +""" +import sys +import textwrap + +import pytest + +from flake8_type_checking.constants import TC007 +from tests.conftest import _get_error + +examples = [ + # No error + ('', set()), + ('x: TypeAlias = "int"', set()), + ('from typing import Dict\nx: TypeAlias = Dict[int]', set()), + ('if TYPE_CHECKING:\n\tfrom typing import Dict\nx: TypeAlias = Dict', {'3:15 ' + TC007.format(alias='Dict')}), + ("if TYPE_CHECKING:\n\tfrom typing import Dict\nx: TypeAlias = 'Dict'", set()), + ("if TYPE_CHECKING:\n\tfrom typing import Dict as d\nx: TypeAlias = 'd[int]'", set()), + ('if TYPE_CHECKING:\n\tfrom typing import Dict\nx: TypeAlias = Dict[int]', {'3:15 ' + TC007.format(alias='Dict')}), + # Regression test for issue #163 + ( + textwrap.dedent(''' + from typing import TYPE_CHECKING + + if TYPE_CHECKING: + from collections.abc import Sequence + from typing_extensions import TypeAlias + + Foo: TypeAlias = Sequence[int] + '''), + set(), + ), + # Inverse regression test for issue #163 + ( + textwrap.dedent(''' + from typing import TYPE_CHECKING + from typing_extensions import TypeAlias + + if TYPE_CHECKING: + from collections.abc import Sequence + + Foo: TypeAlias = Sequence[int] + '''), + { + '8:17 ' + TC007.format(alias='Sequence'), + }, + ), +] + +if sys.version_info >= (3, 12): + # RHS on an explicit TypeAlias with 3.12 syntax should not emit a TC107 + examples.append( + ( + textwrap.dedent(''' + if TYPE_CHECKING: + from collections.abc import Sequence + + type Foo = Sequence[int] + '''), + set(), + ) + ) + + +@pytest.mark.parametrize(('example', 'expected'), examples) +def test_TC007_errors(example, expected): + assert _get_error(example, error_code_filter='TC007') == expected diff --git a/tests/test_tc008.py b/tests/test_tc008.py new file mode 100644 index 0000000..0868324 --- /dev/null +++ b/tests/test_tc008.py @@ -0,0 +1,113 @@ +""" +File tests TC008: + Type alias is wrapped in unnecessary quotes +""" +from __future__ import annotations + +import sys +import textwrap + +import pytest + +from flake8_type_checking.constants import TC008 +from tests.conftest import _get_error + +examples = [ + ('', set()), + ("x: TypeAlias = 'int'", {'1:15 ' + TC008.format(alias='int')}), + # this used to emit an error before fixing #164 if we wanted to handle + # this case once again we could add a whitelist of subscriptable types + ("x: TypeAlias = 'Dict[int]'", set()), + ("from __future__ import annotations\nx: TypeAlias = 'int'", {'2:15 ' + TC008.format(alias='int')}), + ("if TYPE_CHECKING:\n\tfrom typing import Dict\nx: TypeAlias = 'Dict'", set()), + ("if TYPE_CHECKING:\n\tfrom typing import Dict\nx: TypeAlias = 'Dict[int]'", set()), + ( + "from __future__ import annotations\nfrom typing import Dict\nx: TypeAlias = Dict['int']", + {'3:20 ' + TC008.format(alias='int')}, + ), + ( + "from __future__ import annotations\nif TYPE_CHECKING:\n\tfrom typing import Dict\nx: TypeAlias = Dict['int']", + {'4:20 ' + TC008.format(alias='int')}, + ), + ( + textwrap.dedent(''' + from __future__ import annotations + + if TYPE_CHECKING: + import something + + x: TypeAlias = "something" + '''), + set(), + ), + ( + # Regression test for Issue #164 + textwrap.dedent(''' + from wtforms import Field + from wtforms.fields.core import UnboundField + + foo: TypeAlias = 'UnboundField[Field]' + '''), + set(), + ), + ( + # avoid false positive for annotations that make + # use of a newly defined class + textwrap.dedent(''' + class Foo(Protocol): + pass + + x: TypeAlias = 'Foo | None' + '''), + set(), + ), + ( + # Regression test for Issue #168 + textwrap.dedent(''' + if TYPE_CHECKING: + Foo: TypeAlias = str | int + + Bar: TypeAlias = 'Foo' + '''), + set(), + ), + ( + # Inverse regression test for Issue #168 + # The declarations are inside a Protocol so they should not + # count towards declarations inside a type checking block + textwrap.dedent(''' + if TYPE_CHECKING: + class X(Protocol): + Foo: str | int + + Bar: TypeAlias = 'Foo' + '''), + { + '6:17 ' + TC008.format(alias='Foo'), + }, + ), +] + +if sys.version_info >= (3, 12): + examples.extend( + [ + ( + # new style type alias should never be wrapped + textwrap.dedent(''' + if TYPE_CHECKING: + type Foo = 'str' + + type Bar = 'Foo' + '''), + { + '3:15 ' + TC008.format(alias='str'), + '5:11 ' + TC008.format(alias='Foo'), + }, + ) + ] + ) + + +@pytest.mark.parametrize(('example', 'expected'), examples) +def test_TC008_errors(example, expected): + assert _get_error(example, error_code_filter='TC008') == expected diff --git a/tests/test_tc200.py b/tests/test_tc200.py index 9276dc1..0a5ab77 100644 --- a/tests/test_tc200.py +++ b/tests/test_tc200.py @@ -2,7 +2,6 @@ File tests TC200: Annotation should be wrapped in quotes """ -import sys import textwrap import pytest @@ -90,7 +89,7 @@ class FooDict(TypedDict): from typing import NamedTuple, Protocol from typing_extensions import TypeAlias, TypedDict - Foo: TypeAlias = Sequence[int] + Foo: TypeAlias = 'Sequence[int]' class FooTuple(NamedTuple): seq: Sequence[int] @@ -103,45 +102,13 @@ class FooDict(TypedDict): '''), { '9:5 ' + TC200.format(annotation='TypeAlias'), - '9:17 ' + TC200.format(annotation='Sequence'), '12:9 ' + TC200.format(annotation='Sequence'), '15:9 ' + TC200.format(annotation='Sequence'), '18:9 ' + TC200.format(annotation='Sequence'), }, ), - # RHS on an explicit TypeAlias should also emit a TC200 - ( - textwrap.dedent(''' - from typing import TypeAlias - - if TYPE_CHECKING: - from collections.abc import Sequence - - Foo: TypeAlias = Sequence[int] - '''), - { - '7:17 ' + TC200.format(annotation='Sequence'), - }, - ), ] -if sys.version_info >= (3, 12): - # RHS on an explicit TypeAlias should also emit a TC200 - # new type alias syntax - examples.append( - ( - textwrap.dedent(''' - if TYPE_CHECKING: - from collections.abc import Sequence - - type Foo = Sequence[int] - '''), - { - '5:11 ' + TC200.format(annotation='Sequence'), - }, - ) - ) - @pytest.mark.parametrize(('example', 'expected'), examples) def test_TC200_errors(example, expected): diff --git a/tests/test_tc201.py b/tests/test_tc201.py index 638b384..07b0bc4 100644 --- a/tests/test_tc201.py +++ b/tests/test_tc201.py @@ -2,7 +2,6 @@ File tests TC201: Annotation is wrapped in unnecessary quotes """ -import sys import textwrap import pytest @@ -125,7 +124,7 @@ class X(Protocol): x: 'Foo | None' y: 'Bar | None' - Z: TypeAlias = 'Foo' + Z: TypeAlias = Foo def foo(a: 'T', *args: Unpack['Ts']) -> None: pass @@ -136,7 +135,6 @@ def bar(*args: 'P.args', **kwargs: 'P.kwargs') -> None: { '10:3 ' + TC201.format(annotation='Foo | None'), '11:3 ' + TC201.format(annotation='Bar | None'), - '12:15 ' + TC201.format(annotation='Foo'), '14:11 ' + TC201.format(annotation='T'), '14:30 ' + TC201.format(annotation='Ts'), '17:15 ' + TC201.format(annotation='P.args'), @@ -145,22 +143,6 @@ def bar(*args: 'P.args', **kwargs: 'P.kwargs') -> None: ), ] -if sys.version_info >= (3, 12): - examples.append( - ( - # Regression test for Issue #168 - # using new type alias syntax - textwrap.dedent(''' - if TYPE_CHECKING: - type Foo = str | int - - x: 'Foo | None' - type Z = 'Foo' - '''), - set(), - ) - ) - @pytest.mark.parametrize(('example', 'expected'), examples) def test_TC201_errors(example, expected): From 92ac05d04a5e14462e03a10631e04f3aadba4d05 Mon Sep 17 00:00:00 2001 From: Daverball Date: Sun, 8 Oct 2023 16:56:12 +0200 Subject: [PATCH 07/10] Adds a couple of test cases for type aliases to TC001-TC003 tests Fixes unnecessary filtering out of new-style aliases --- flake8_type_checking/checker.py | 4 ++-- tests/test_tc001_to_tc003.py | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index ef8013a..21e614e 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -1089,8 +1089,8 @@ def unused_imports(self) -> Flake8Generator: unused_imports = set(self.visitor.imports) - self.visitor.names used_imports = set(self.visitor.imports) - unused_imports already_imported_modules = [self.visitor.imports[name].module for name in used_imports] - annotation_names = [n for i in self.visitor.wrapped_annotations if i.type != 'new-alias' for n in i.names] + [ - i.annotation for i in self.visitor.unwrapped_annotations if i.type != 'new-alias' + annotation_names = [n for i in self.visitor.wrapped_annotations for n in i.names] + [ + i.annotation for i in self.visitor.unwrapped_annotations ] for name in unused_imports: diff --git a/tests/test_tc001_to_tc003.py b/tests/test_tc001_to_tc003.py index 2b04368..4b1f6b0 100644 --- a/tests/test_tc001_to_tc003.py +++ b/tests/test_tc001_to_tc003.py @@ -9,6 +9,7 @@ from __future__ import annotations +import sys import textwrap from typing import List, Set, Tuple @@ -101,6 +102,16 @@ def get_tc_001_to_003_tests(import_: str, ERROR: str) -> L: (f'import {import_} as x\ndef example() -> x:\n\tpass', {'1:0 ' + ERROR.format(module='x')}), ] + used_for_type_alias_only: L = [ + (f'import {import_}\nx: TypeAlias = {import_}', {f"1:0 {ERROR.format(module=f'{import_}')}"}), + ] + + if sys.version_info >= (3, 12): + # new style type alias + used_for_type_alias_only.append( + (f'import {import_}\ntype x = {import_}', {f"1:0 {ERROR.format(module=f'{import_}')}"}) + ) + other_useful_test_cases: L = [ ( textwrap.dedent(f''' @@ -193,6 +204,7 @@ class Migration: *used_for_annotations_only, *used_for_arg_annotations_only, *used_for_return_annotations_only, + *used_for_type_alias_only, *other_useful_test_cases, ] From 802482ca877bc280918c8d81f97be8af98915004 Mon Sep 17 00:00:00 2001 From: Daverball Date: Sun, 8 Oct 2023 16:58:35 +0200 Subject: [PATCH 08/10] Fixes comment [skip ci] --- tests/test_tc007.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_tc007.py b/tests/test_tc007.py index 2fb78f6..b93bdf5 100644 --- a/tests/test_tc007.py +++ b/tests/test_tc007.py @@ -50,7 +50,7 @@ ] if sys.version_info >= (3, 12): - # RHS on an explicit TypeAlias with 3.12 syntax should not emit a TC107 + # RHS on an explicit TypeAlias with 3.12 syntax should not emit a TC007 examples.append( ( textwrap.dedent(''' From 568801ab2067a1780b56703252d59d7c4fc049de Mon Sep 17 00:00:00 2001 From: Daverball Date: Mon, 9 Oct 2023 09:44:34 +0200 Subject: [PATCH 09/10] Updates a couple of docstrings to better reflect current state --- flake8_type_checking/checker.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index 21e614e..37d2b6d 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -819,8 +819,8 @@ def visit_AnnAssign(self, node: ast.AnnAssign) -> None: Remove all annotation assignments. But also keep track of any explicit `TypeAlias` assignments, we should treat the RHS like - an annotation as well, but only for the 2xx codes, since the RHS will not automatically - become a ForwardRef, like a true annotation would. + an annotation as well, but we have to keep in mind that the RHS will not automatically become + a ForwardRef with a future import, like a true annotation would. """ self.add_annotation(node.annotation) @@ -862,10 +862,13 @@ def visit_Assign(self, node: ast.Assign) -> ast.Assign: def visit_TypeAlias(self, node: ast.TypeAlias) -> None: """ - Remove all type aliases, but treat the value on the RHS like an annotation. + Remove all type aliases. Keep track of any type aliases declared inside a type checking block using the new - `type Alias = value` syntax. + `type Alias = value` syntax. We need to keep in mind that the RHS using this syntax + will always become a ForwardRef, so none of the names are needed at runtime, so we + don't visit the RHS and also have to treat the annotation differently from a regular + annotation when emitting errors. """ self.add_annotation(node.value, 'new-alias') From bcc4654ddc269894f0b5b51c4a34785f6d078d8a Mon Sep 17 00:00:00 2001 From: Daverball Date: Mon, 9 Oct 2023 10:47:40 +0200 Subject: [PATCH 10/10] Improves robustness of detecting type checking only declarations --- flake8_type_checking/checker.py | 52 +++++++++++++++++++++++++------ flake8_type_checking/constants.py | 2 +- tests/test_tc200.py | 6 ++++ 3 files changed, 50 insertions(+), 10 deletions(-) diff --git a/flake8_type_checking/checker.py b/flake8_type_checking/checker.py index 37d2b6d..b65e4fe 100644 --- a/flake8_type_checking/checker.py +++ b/flake8_type_checking/checker.py @@ -7,6 +7,7 @@ from ast import Index, literal_eval from contextlib import suppress from dataclasses import dataclass +from itertools import chain from pathlib import Path from typing import TYPE_CHECKING, Literal, NamedTuple, cast @@ -17,6 +18,7 @@ ATTRIBUTE_PROPERTY, ATTRS_DECORATORS, ATTRS_IMPORTS, + GLOBAL_PROPERTY, NAME_RE, TC001, TC002, @@ -30,7 +32,6 @@ TC101, TC200, TC201, - TOP_LEVEL_PROPERTY, py38, ) @@ -584,8 +585,44 @@ def is_true_when_type_checking(self, node: ast.AST) -> bool | Literal['TYPE_CHEC return 'TYPE_CHECKING' return False + def visit_Module(self, node: ast.Module) -> ast.Module: + """ + Mark global statments. + + We propagate this marking when visiting control flow nodes, that don't affect + scope, such as if/else, try/except. Although for simplicity we don't handle + quite all the possible cases, since we're only interested in type checking blocks + and it's not realistic to encounter these for example inside a TryStar/With/Match. + + If we're serious about handling all the cases it would probably make more sense + to override generic_visit to propagate this property for a sequence of node types + and attributes that contain the statements that should propagate global scope. + """ + for stmt in node.body: + setattr(stmt, GLOBAL_PROPERTY, True) + + self.generic_visit(node) + return node + + def visit_Try(self, node: ast.Try) -> ast.Try: + """Propagate global statements.""" + if getattr(node, GLOBAL_PROPERTY, False): + for stmt in chain(node.body, (s for h in node.handlers for s in h.body), node.orelse, node.finalbody): + setattr(stmt, GLOBAL_PROPERTY, True) + + self.generic_visit(node) + return node + def visit_If(self, node: ast.If) -> Any: - """Look for a TYPE_CHECKING block.""" + """ + Look for a TYPE_CHECKING block. + + Also recursively propagate global, since if/else does not affect scope. + """ + if getattr(node, GLOBAL_PROPERTY, False): + for stmt in chain(node.body, getattr(node, 'orelse', ()) or ()): + setattr(stmt, GLOBAL_PROPERTY, True) + type_checking_condition = self.is_true_when_type_checking(node.test) == 'TYPE_CHECKING' # If it is, note down the line-number-range where the type-checking block exists @@ -598,10 +635,6 @@ def visit_If(self, node: ast.If) -> Any: # first element in the else block - 1 start_of_else_block = node.orelse[0].lineno - 1 - # mark all the top-level statements - for stmt in node.body: - setattr(stmt, TOP_LEVEL_PROPERTY, True) - # Check for TC005 errors. if ((node.end_lineno or node.lineno) - node.lineno == 1) and ( len(node.body) == 1 and isinstance(node.body[0], ast.Pass) @@ -834,7 +867,7 @@ def visit_AnnAssign(self, node: ast.AnnAssign) -> None: ): self.add_annotation(node.value, 'alias') - if getattr(node, TOP_LEVEL_PROPERTY, False): + if getattr(node, GLOBAL_PROPERTY, False) and self.in_type_checking_block(node.lineno, node.col_offset): self.type_checking_block_declarations.add(node.target.id) # if it wasn't a TypeAlias we need to visit the value expression @@ -849,9 +882,10 @@ def visit_Assign(self, node: ast.Assign) -> ast.Assign: target and it should be an `ast.Name`. """ if ( - getattr(node, TOP_LEVEL_PROPERTY, False) + getattr(node, GLOBAL_PROPERTY, False) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name) + and self.in_type_checking_block(node.lineno, node.col_offset) ): self.type_checking_block_declarations.add(node.targets[0].id) @@ -872,7 +906,7 @@ def visit_TypeAlias(self, node: ast.TypeAlias) -> None: """ self.add_annotation(node.value, 'new-alias') - if getattr(node, TOP_LEVEL_PROPERTY, False): + if getattr(node, GLOBAL_PROPERTY, False) and self.in_type_checking_block(node.lineno, node.col_offset): self.type_checking_block_declarations.add(node.name.id) def register_function_ranges(self, node: Union[FunctionDef, AsyncFunctionDef]) -> None: diff --git a/flake8_type_checking/constants.py b/flake8_type_checking/constants.py index 8195d63..657fa61 100644 --- a/flake8_type_checking/constants.py +++ b/flake8_type_checking/constants.py @@ -5,7 +5,7 @@ ATTRIBUTE_PROPERTY = '_flake8-type-checking__parent' ANNOTATION_PROPERTY = '_flake8-type-checking__is_annotation' -TOP_LEVEL_PROPERTY = '_flake8-type-checking__is_top_level' +GLOBAL_PROPERTY = '_flake8-type-checking__is_global' NAME_RE = re.compile(r'(?