From bf4b96c5ded81c90ecfba77b94f8eac81f28e010 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 7 Jul 2023 00:21:44 -0400 Subject: [PATCH] Differentiate between runtime and typing-time annotations (#5575) ## Summary In Python, the annotations on `x` and `y` here have very different treatment: ```python def foo(x: int): y: int ``` The `int` in `x: int` is a runtime-required annotation, because `x` gets added to the function's `__annotations__`. You'll notice, for example, that this fails: ```python from typing import TYPE_CHECKING if TYPE_CHECKING: from foo import Bar def f(x: Bar): ... ``` Because `Bar` is required to be available at runtime, not just at typing time. Meanwhile, this succeeds: ```python from typing import TYPE_CHECKING if TYPE_CHECKING: from foo import Bar def f(): x: Bar = 1 f() ``` (Both cases are fine if you use `from __future__ import annotations`.) Historically, we've tracked those annotations that are _not_ runtime-required via the semantic model's `ANNOTATION` flag. But annotations that _are_ runtime-required have been treated as "type definitions" that aren't annotations. This causes problems for the flake8-future-annotations rules, which try to detect whether adding `from __future__ import annotations` would _allow_ you to rewrite a type annotation. We need to know whether we're in _any_ type annotation, runtime-required or not, since adding `from __future__ import annotations` will convert any runtime-required annotation to a typing-only annotation. This PR adds separate state to track these runtime-required annotations. The changes in the test fixtures are correct -- these were false negatives before. Closes https://github.com/astral-sh/ruff/issues/5574. --- crates/ruff/src/checkers/ast/mod.rs | 20 +++-- ...ture_annotations__tests__edge_case.py.snap | 8 ++ ...02_no_future_import_uses_lowercase.py.snap | 7 ++ ..._fa102_no_future_import_uses_union.py.snap | 14 ++++ ..._no_future_import_uses_union_inner.py.snap | 16 ++++ crates/ruff_python_semantic/src/model.rs | 75 +++++++++++++------ 6 files changed, 113 insertions(+), 27 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 7767129c5f..606d8a2ef8 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1811,7 +1811,7 @@ where { if let Some(expr) = &arg_with_default.def.annotation { if runtime_annotation { - self.visit_type_definition(expr); + self.visit_runtime_annotation(expr); } else { self.visit_annotation(expr); }; @@ -1823,7 +1823,7 @@ where if let Some(arg) = &args.vararg { if let Some(expr) = &arg.annotation { if runtime_annotation { - self.visit_type_definition(expr); + self.visit_runtime_annotation(expr); } else { self.visit_annotation(expr); }; @@ -1832,7 +1832,7 @@ where if let Some(arg) = &args.kwarg { if let Some(expr) = &arg.annotation { if runtime_annotation { - self.visit_type_definition(expr); + self.visit_runtime_annotation(expr); } else { self.visit_annotation(expr); }; @@ -1840,7 +1840,7 @@ where } for expr in returns { if runtime_annotation { - self.visit_type_definition(expr); + self.visit_runtime_annotation(expr); } else { self.visit_annotation(expr); }; @@ -1992,7 +1992,7 @@ where }; if runtime_annotation { - self.visit_type_definition(annotation); + self.visit_runtime_annotation(annotation); } else { self.visit_annotation(annotation); } @@ -2089,7 +2089,7 @@ where fn visit_annotation(&mut self, expr: &'b Expr) { let flags_snapshot = self.semantic.flags; - self.semantic.flags |= SemanticModelFlags::ANNOTATION; + self.semantic.flags |= SemanticModelFlags::TYPING_ONLY_ANNOTATION; self.visit_type_definition(expr); self.semantic.flags = flags_snapshot; } @@ -4125,6 +4125,14 @@ impl<'a> Checker<'a> { self.semantic.flags = snapshot; } + /// Visit an [`Expr`], and treat it as a runtime-required type annotation. + fn visit_runtime_annotation(&mut self, expr: &'a Expr) { + let snapshot = self.semantic.flags; + self.semantic.flags |= SemanticModelFlags::RUNTIME_ANNOTATION; + self.visit_type_definition(expr); + self.semantic.flags = snapshot; + } + /// Visit an [`Expr`], and treat it as a type definition. fn visit_type_definition(&mut self, expr: &'a Expr) { let snapshot = self.semantic.flags; diff --git a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__edge_case.py.snap b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__edge_case.py.snap index fc0cdbf8ad..ed1dc0f3df 100644 --- a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__edge_case.py.snap +++ b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__edge_case.py.snap @@ -1,6 +1,14 @@ --- source: crates/ruff/src/rules/flake8_future_annotations/mod.rs --- +edge_case.py:5:13: FA100 Missing `from __future__ import annotations`, but uses `typing.List` + | +5 | def main(_: List[int]) -> None: + | ^^^^ FA100 +6 | a_list: t.List[str] = [] +7 | a_list.append("hello") + | + edge_case.py:6:13: FA100 Missing `from __future__ import annotations`, but uses `typing.List` | 5 | def main(_: List[int]) -> None: diff --git a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_lowercase.py.snap b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_lowercase.py.snap index 5fb0fe7444..14acba04f7 100644 --- a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_lowercase.py.snap +++ b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_lowercase.py.snap @@ -9,4 +9,11 @@ no_future_import_uses_lowercase.py:2:13: FA102 Missing `from __future__ import a 3 | a_list.append("hello") | +no_future_import_uses_lowercase.py:6:14: FA102 Missing `from __future__ import annotations`, but uses PEP 585 collection + | +6 | def hello(y: dict[str, int]) -> None: + | ^^^^^^^^^^^^^^ FA102 +7 | del y + | + diff --git a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union.py.snap b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union.py.snap index cee83994c0..6bf73cdcf1 100644 --- a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union.py.snap +++ b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union.py.snap @@ -17,4 +17,18 @@ no_future_import_uses_union.py:2:13: FA102 Missing `from __future__ import annot 3 | a_list.append("hello") | +no_future_import_uses_union.py:6:14: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union + | +6 | def hello(y: dict[str, int] | None) -> None: + | ^^^^^^^^^^^^^^^^^^^^^ FA102 +7 | del y + | + +no_future_import_uses_union.py:6:14: FA102 Missing `from __future__ import annotations`, but uses PEP 585 collection + | +6 | def hello(y: dict[str, int] | None) -> None: + | ^^^^^^^^^^^^^^ FA102 +7 | del y + | + diff --git a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union_inner.py.snap b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union_inner.py.snap index d153861855..7f5156463a 100644 --- a/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union_inner.py.snap +++ b/crates/ruff/src/rules/flake8_future_annotations/snapshots/ruff__rules__flake8_future_annotations__tests__fa102_no_future_import_uses_union_inner.py.snap @@ -17,6 +17,22 @@ no_future_import_uses_union_inner.py:2:18: FA102 Missing `from __future__ import 3 | a_list.append("hello") | +no_future_import_uses_union_inner.py:6:14: FA102 Missing `from __future__ import annotations`, but uses PEP 585 collection + | +6 | def hello(y: dict[str | None, int]) -> None: + | ^^^^^^^^^^^^^^^^^^^^^ FA102 +7 | z: tuple[str, str | None, str] = tuple(y) +8 | del z + | + +no_future_import_uses_union_inner.py:6:19: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union + | +6 | def hello(y: dict[str | None, int]) -> None: + | ^^^^^^^^^^ FA102 +7 | z: tuple[str, str | None, str] = tuple(y) +8 | del z + | + no_future_import_uses_union_inner.py:7:8: FA102 Missing `from __future__ import annotations`, but uses PEP 585 collection | 6 | def hello(y: dict[str | None, int]) -> None: diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 6773c973f2..38bd3c4783 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -929,7 +929,7 @@ impl<'a> SemanticModel<'a> { /// Return the [`ExecutionContext`] of the current scope. pub const fn execution_context(&self) -> ExecutionContext { if self.in_type_checking_block() - || self.in_annotation() + || self.in_typing_only_annotation() || self.in_complex_string_type_definition() || self.in_simple_string_type_definition() { @@ -974,7 +974,18 @@ impl<'a> SemanticModel<'a> { /// Return `true` if the context is in a type annotation. pub const fn in_annotation(&self) -> bool { - self.flags.contains(SemanticModelFlags::ANNOTATION) + self.in_typing_only_annotation() || self.in_runtime_annotation() + } + + /// Return `true` if the context is in a typing-only type annotation. + pub const fn in_typing_only_annotation(&self) -> bool { + self.flags + .contains(SemanticModelFlags::TYPING_ONLY_ANNOTATION) + } + + /// Return `true` if the context is in a runtime-required type annotation. + pub const fn in_runtime_annotation(&self) -> bool { + self.flags.contains(SemanticModelFlags::RUNTIME_ANNOTATION) } /// Return `true` if the context is in a type definition. @@ -1025,7 +1036,7 @@ impl<'a> SemanticModel<'a> { pub const fn in_forward_reference(&self) -> bool { self.in_simple_string_type_definition() || self.in_complex_string_type_definition() - || (self.in_future_type_definition() && self.in_annotation()) + || (self.in_future_type_definition() && self.in_typing_only_annotation()) } /// Return `true` if the context is in an exception handler. @@ -1147,13 +1158,36 @@ bitflags! { /// Flags indicating the current context of the analysis. #[derive(Debug, Default, Copy, Clone, Eq, PartialEq)] pub struct SemanticModelFlags: u16 { - /// The context is in a type annotation. + /// The context is in a typing-time-only type annotation. /// /// For example, the context could be visiting `int` in: /// ```python - /// x: int = 1 + /// def foo() -> int: + /// x: int = 1 /// ``` - const ANNOTATION = 1 << 0; + /// + /// In this case, Python doesn't require that the type annotation be evaluated at runtime. + /// + /// If `from __future__ import annotations` is used, all annotations are evaluated at + /// typing time. Otherwise, all function argument annotations are evaluated at runtime, as + /// are any annotated assignments in module or class scopes. + const TYPING_ONLY_ANNOTATION = 1 << 0; + + /// The context is in a runtime type annotation. + /// + /// For example, the context could be visiting `int` in: + /// ```python + /// def foo(x: int) -> int: + /// ... + /// ``` + /// + /// In this case, Python requires that the type annotation be evaluated at runtime, + /// as it needs to be available on the function's `__annotations__` attribute. + /// + /// If `from __future__ import annotations` is used, all annotations are evaluated at + /// typing time. Otherwise, all function argument annotations are evaluated at runtime, as + /// are any annotated assignments in module or class scopes. + const RUNTIME_ANNOTATION = 1 << 1; /// The context is in a type definition. /// @@ -1167,7 +1201,7 @@ bitflags! { /// All type annotations are also type definitions, but the converse is not true. /// In our example, `int` is a type definition but not a type annotation, as it /// doesn't appear in a type annotation context, but rather in a type definition. - const TYPE_DEFINITION = 1 << 1; + const TYPE_DEFINITION = 1 << 2; /// The context is in a (deferred) "simple" string type definition. /// @@ -1178,7 +1212,7 @@ bitflags! { /// /// "Simple" string type definitions are those that consist of a single string literal, /// as opposed to an implicitly concatenated string literal. - const SIMPLE_STRING_TYPE_DEFINITION = 1 << 2; + const SIMPLE_STRING_TYPE_DEFINITION = 1 << 3; /// The context is in a (deferred) "complex" string type definition. /// @@ -1189,7 +1223,7 @@ bitflags! { /// /// "Complex" string type definitions are those that consist of a implicitly concatenated /// string literals. These are uncommon but valid. - const COMPLEX_STRING_TYPE_DEFINITION = 1 << 3; + const COMPLEX_STRING_TYPE_DEFINITION = 1 << 4; /// The context is in a (deferred) `__future__` type definition. /// @@ -1202,7 +1236,7 @@ bitflags! { /// /// `__future__`-style type annotations are only enabled if the `annotations` feature /// is enabled via `from __future__ import annotations`. - const FUTURE_TYPE_DEFINITION = 1 << 4; + const FUTURE_TYPE_DEFINITION = 1 << 5; /// The context is in an exception handler. /// @@ -1213,7 +1247,7 @@ bitflags! { /// except Exception: /// x: int = 1 /// ``` - const EXCEPTION_HANDLER = 1 << 5; + const EXCEPTION_HANDLER = 1 << 6; /// The context is in an f-string. /// @@ -1221,7 +1255,7 @@ bitflags! { /// ```python /// f'{x}' /// ``` - const F_STRING = 1 << 6; + const F_STRING = 1 << 7; /// The context is in a nested f-string. /// @@ -1229,7 +1263,7 @@ bitflags! { /// ```python /// f'{f"{x}"}' /// ``` - const NESTED_F_STRING = 1 << 7; + const NESTED_F_STRING = 1 << 8; /// The context is in a boolean test. /// @@ -1241,7 +1275,7 @@ bitflags! { /// /// The implication is that the actual value returned by the current expression is /// not used, only its truthiness. - const BOOLEAN_TEST = 1 << 8; + const BOOLEAN_TEST = 1 << 9; /// The context is in a `typing::Literal` annotation. /// @@ -1250,7 +1284,7 @@ bitflags! { /// def f(x: Literal["A", "B", "C"]): /// ... /// ``` - const LITERAL = 1 << 9; + const LITERAL = 1 << 10; /// The context is in a subscript expression. /// @@ -1258,7 +1292,7 @@ bitflags! { /// ```python /// x["a"]["b"] /// ``` - const SUBSCRIPT = 1 << 10; + const SUBSCRIPT = 1 << 11; /// The context is in a type-checking block. /// @@ -1270,8 +1304,7 @@ bitflags! { /// if TYPE_CHECKING: /// x: int = 1 /// ``` - const TYPE_CHECKING_BLOCK = 1 << 11; - + const TYPE_CHECKING_BLOCK = 1 << 12; /// The context has traversed past the "top-of-file" import boundary. /// @@ -1284,7 +1317,7 @@ bitflags! { /// /// x: int = 1 /// ``` - const IMPORT_BOUNDARY = 1 << 12; + const IMPORT_BOUNDARY = 1 << 13; /// The context has traversed past the `__future__` import boundary. /// @@ -1299,7 +1332,7 @@ bitflags! { /// /// Python considers it a syntax error to import from `__future__` after /// any other non-`__future__`-importing statements. - const FUTURES_BOUNDARY = 1 << 13; + const FUTURES_BOUNDARY = 1 << 14; /// `__future__`-style type annotations are enabled in this context. /// @@ -1311,7 +1344,7 @@ bitflags! { /// def f(x: int) -> int: /// ... /// ``` - const FUTURE_ANNOTATIONS = 1 << 14; + const FUTURE_ANNOTATIONS = 1 << 15; } }