From a62c735f9ed496ce47a7f5bda4f8771b28e2c0c5 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 17 Oct 2023 21:50:26 -0400 Subject: [PATCH] Lazily evaluate all PEP 695 type alias values (#8033) ## Summary In https://github.com/astral-sh/ruff/pull/7968, I introduced a regression whereby we started to treat imports used _only_ in type annotation bounds (with `__future__` annotations) as unused. The root of the issue is that I started using `visit_annotation` for these bounds. So we'd queue up the bound in the list of deferred type parameters, then when visiting, we'd further queue it up in the list of deferred type annotations... Which we'd then never visit, since deferred type annotations are visited _before_ deferred type parameters. Anyway, the better solution here is to use a dedicated flag for these, since they have slightly different behavior than type annotations. I've also fixed what I _think_ is a bug whereby we previously failed to resolve `Callable` in: ```python type RecordCallback[R: Record] = Callable[[R], None] from collections.abc import Callable ``` IIUC, the values in type aliases should be evaluated lazily, like type parameters. Closes https://github.com/astral-sh/ruff/issues/8017. ## Test Plan `cargo test` --- .../test/fixtures/pyflakes/F401_19.py | 17 ++++++++++++ .../test/fixtures/pyflakes/F821_20.py | 5 ++++ .../ruff_linter/src/checkers/ast/deferred.rs | 4 +-- crates/ruff_linter/src/checkers/ast/mod.rs | 26 ++++++++++--------- crates/ruff_linter/src/rules/pyflakes/mod.rs | 2 ++ ...les__pyflakes__tests__F401_F401_19.py.snap | 4 +++ ...les__pyflakes__tests__F821_F821_20.py.snap | 14 ++++++++++ crates/ruff_python_semantic/src/model.rs | 15 +++++++++-- 8 files changed, 71 insertions(+), 16 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pyflakes/F401_19.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pyflakes/F821_20.py create mode 100644 crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_19.py.snap create mode 100644 crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F821_F821_20.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_19.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_19.py new file mode 100644 index 0000000000..ea2f5fe8a8 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_19.py @@ -0,0 +1,17 @@ +"""Test that type parameters are considered used.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Callable + + from .foo import Record as Record1 + from .bar import Record as Record2 + +type RecordCallback[R: Record1] = Callable[[R], None] + + +def process_record[R: Record2](record: R) -> None: + ... diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F821_20.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F821_20.py new file mode 100644 index 0000000000..1954bec533 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F821_20.py @@ -0,0 +1,5 @@ +"""Test lazy evaluation of type alias values.""" + +type RecordCallback[R: Record] = Callable[[R], None] + +from collections.abc import Callable diff --git a/crates/ruff_linter/src/checkers/ast/deferred.rs b/crates/ruff_linter/src/checkers/ast/deferred.rs index 85c900d1b0..c29f61354e 100644 --- a/crates/ruff_linter/src/checkers/ast/deferred.rs +++ b/crates/ruff_linter/src/checkers/ast/deferred.rs @@ -1,4 +1,4 @@ -use ruff_python_ast::{Expr, TypeParam}; +use ruff_python_ast::Expr; use ruff_python_semantic::{ScopeId, Snapshot}; use ruff_text_size::TextRange; @@ -10,7 +10,7 @@ pub(crate) struct Deferred<'a> { pub(crate) scopes: Vec, pub(crate) string_type_definitions: Vec<(TextRange, &'a str, Snapshot)>, pub(crate) future_type_definitions: Vec<(&'a Expr, Snapshot)>, - pub(crate) type_param_definitions: Vec<(&'a TypeParam, Snapshot)>, + pub(crate) type_param_definitions: Vec<(&'a Expr, Snapshot)>, pub(crate) functions: Vec, pub(crate) lambdas: Vec<(&'a Expr, Snapshot)>, pub(crate) for_loops: Vec, diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index bc3a3c3f9c..0cd527f761 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -582,9 +582,9 @@ where if let Some(type_params) = type_params { self.visit_type_params(type_params); } - // The value in a `type` alias has annotation semantics, in that it's never - // evaluated at runtime. - self.visit_annotation(value); + self.deferred + .type_param_definitions + .push((value, self.semantic.snapshot())); self.semantic.pop_scope(); self.visit_expr(name); } @@ -1389,9 +1389,14 @@ where } } // Step 2: Traversal - self.deferred - .type_param_definitions - .push((type_param, self.semantic.snapshot())); + if let ast::TypeParam::TypeVar(ast::TypeParamTypeVar { + bound: Some(bound), .. + }) = type_param + { + self.deferred + .type_param_definitions + .push((bound, self.semantic.snapshot())); + } } } @@ -1766,12 +1771,9 @@ impl<'a> Checker<'a> { for (type_param, snapshot) in type_params { self.semantic.restore(snapshot); - if let ast::TypeParam::TypeVar(ast::TypeParamTypeVar { - bound: Some(bound), .. - }) = type_param - { - self.visit_annotation(bound); - } + self.semantic.flags |= + SemanticModelFlags::TYPE_PARAM_DEFINITION | SemanticModelFlags::TYPE_DEFINITION; + self.visit_expr(type_param); } } self.semantic.restore(snapshot); diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index 172de5a243..4cb3a9f895 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -50,6 +50,7 @@ mod tests { #[test_case(Rule::UnusedImport, Path::new("F401_16.py"))] #[test_case(Rule::UnusedImport, Path::new("F401_17.py"))] #[test_case(Rule::UnusedImport, Path::new("F401_18.py"))] + #[test_case(Rule::UnusedImport, Path::new("F401_19.py"))] #[test_case(Rule::ImportShadowedByLoopVar, Path::new("F402.py"))] #[test_case(Rule::UndefinedLocalWithImportStar, Path::new("F403.py"))] #[test_case(Rule::LateFutureImport, Path::new("F404.py"))] @@ -135,6 +136,7 @@ mod tests { #[test_case(Rule::UndefinedName, Path::new("F821_17.py"))] #[test_case(Rule::UndefinedName, Path::new("F821_18.py"))] #[test_case(Rule::UndefinedName, Path::new("F821_19.py"))] + #[test_case(Rule::UndefinedName, Path::new("F821_20.py"))] #[test_case(Rule::UndefinedExport, Path::new("F822_0.py"))] #[test_case(Rule::UndefinedExport, Path::new("F822_1.py"))] #[test_case(Rule::UndefinedExport, Path::new("F822_2.py"))] diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_19.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_19.py.snap new file mode 100644 index 0000000000..d0b409f39e --- /dev/null +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_19.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/pyflakes/mod.rs +--- + diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F821_F821_20.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F821_F821_20.py.snap new file mode 100644 index 0000000000..143f82be67 --- /dev/null +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F821_F821_20.py.snap @@ -0,0 +1,14 @@ +--- +source: crates/ruff_linter/src/rules/pyflakes/mod.rs +--- +F821_20.py:3:24: F821 Undefined name `Record` + | +1 | """Test lazy evaluation of type alias values.""" +2 | +3 | type RecordCallback[R: Record] = Callable[[R], None] + | ^^^^^^ F821 +4 | +5 | from collections.abc import Callable + | + + diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 8f742ae452..7ab3fed0d3 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1600,6 +1600,16 @@ bitflags! { /// ``` const FUTURE_ANNOTATIONS = 1 << 14; + /// The model is in a type parameter definition. + /// + /// For example, the model could be visiting `Record` in: + /// ```python + /// from typing import TypeVar + /// + /// Record = TypeVar("Record") + /// + const TYPE_PARAM_DEFINITION = 1 << 15; + /// The context is in any type annotation. const ANNOTATION = Self::TYPING_ONLY_ANNOTATION.bits() | Self::RUNTIME_ANNOTATION.bits(); @@ -1610,11 +1620,12 @@ bitflags! { /// The context is in any deferred type definition. const DEFERRED_TYPE_DEFINITION = Self::SIMPLE_STRING_TYPE_DEFINITION.bits() | Self::COMPLEX_STRING_TYPE_DEFINITION.bits() - | Self::FUTURE_TYPE_DEFINITION.bits(); + | Self::FUTURE_TYPE_DEFINITION.bits() + | Self::TYPE_PARAM_DEFINITION.bits(); /// The context is in a typing-only context. const TYPING_CONTEXT = Self::TYPE_CHECKING_BLOCK.bits() | Self::TYPING_ONLY_ANNOTATION.bits() | - Self::STRING_TYPE_DEFINITION.bits(); + Self::STRING_TYPE_DEFINITION.bits() | Self::TYPE_PARAM_DEFINITION.bits(); } }