Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-type-checking] Adds implementation for TC007 and TC008 #12927

Merged
merged 42 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
3de92a5
Adds initial implementation for TCH008
Daverball Aug 16, 2024
e1ca904
Fixes mkdocs and clippy complaints
Daverball Aug 16, 2024
3201588
Fixes runtime forward reference binding lookup
Daverball Aug 16, 2024
bc45146
Simulates proper runtime name lookups for more accurate results
Daverball Aug 17, 2024
0df3d12
Adds missing test case back and fixes range overlap check
Daverball Aug 17, 2024
4cb32e1
Fixes docstring
Daverball Aug 17, 2024
d9cea8f
Refactors class name book keeping code
Daverball Aug 17, 2024
096eb9d
Removes unnecessary borrow
Daverball Aug 17, 2024
e420697
Fixes some logical errors and simplifies class name lookups
Daverball Aug 18, 2024
fa9d4df
Guards against TC010/TCH008 overlap
Daverball Aug 18, 2024
dd78b52
Adds implementation and tests for TCH007
Daverball Aug 20, 2024
08b7ddc
Fixes TCH004/TCH007 overlap
Daverball Aug 20, 2024
bd31311
Merge branch 'main' into feat/tch007-tch008
Daverball Sep 2, 2024
b185e4b
Merge branch 'main' into feat/tch007-tch008
Daverball Oct 24, 2024
b17c959
Clean up unused import that was accidentally left in
Daverball Oct 24, 2024
643ddb6
Simplifies `quote_type_expression`
Daverball Oct 24, 2024
09986c7
Apply suggestions from code review
Daverball Nov 9, 2024
4044dfa
Re-formats code slightly.
Daverball Nov 9, 2024
f51ea65
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 9, 2024
81bb172
Fixes errors and only marks fix as unsafe when there are comments
Daverball Nov 9, 2024
2555f8a
Removes unnecessary `return` statement
Daverball Nov 9, 2024
a36bab6
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 13, 2024
f2f384e
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 14, 2024
6799411
Fixes new semantic flag values
Daverball Nov 14, 2024
231804b
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 16, 2024
bfffb7b
`quote_type_annotation` is once again no longer guaranteed to work
Daverball Nov 16, 2024
fd051a8
Addressed some of the feedback from the code review
Daverball Nov 19, 2024
0fc0d4b
Avoids changing the behavior of `TCH004` for now.
Daverball Nov 19, 2024
f1ec697
Apply suggestions from code review
Daverball Nov 20, 2024
fd8c045
Fixes snapshot and applies a couple of other suggestions
Daverball Nov 20, 2024
410bfe4
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 20, 2024
42c33b2
Remove dummy file that got committed by accident
Daverball Nov 20, 2024
f98400a
Improves documentation and naming of type alias related methods/flags
Daverball Nov 21, 2024
4933178
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 22, 2024
cce3f00
Adjusts type alias naming to be more distinctive
Daverball Nov 22, 2024
823caaa
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 22, 2024
c8e0d99
Fixes error in `ruff.schema.json` introduced by merge
Daverball Nov 22, 2024
536c6a4
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 26, 2024
64c7d30
Extends docstring for `simulate_runtime_load`
Daverball Nov 26, 2024
1bf6315
Fixes cross references in docstring
Daverball Nov 26, 2024
fa9c1a4
Merge branch 'main' into feat/tch007-tch008
Daverball Nov 27, 2024
8cf8f27
Address some of the code review comments
Daverball Nov 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from __future__ import annotations

from typing import Dict, TypeAlias, TYPE_CHECKING

if TYPE_CHECKING:
from typing import Dict

from foo import Foo

OptStr: TypeAlias = str | None
Bar: TypeAlias = Foo[int]

a: TypeAlias = int # OK
b: TypeAlias = Dict # OK
c: TypeAlias = Foo # TC007
d: TypeAlias = Foo | None # TC007
e: TypeAlias = OptStr # TC007
f: TypeAlias = Bar # TC007
g: TypeAlias = Foo | Bar # TC007 x2
h: TypeAlias = Foo[str] # TC007
i: TypeAlias = (Foo | # TC007 x2 (fix removes comment currently)
Bar)

type C = Foo # OK
type D = Foo | None # OK
type E = OptStr # OK
type F = Bar # OK
type G = Foo | Bar # OK
type H = Foo[str] # OK
type I = (Foo | # OK
Bar)
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
from __future__ import annotations

from typing import TypeAlias, TYPE_CHECKING

from foo import Foo

if TYPE_CHECKING:
from typing import Dict

OptStr: TypeAlias = str | None
Bar: TypeAlias = Foo[int]
else:
Bar = Foo

a: TypeAlias = 'int' # TC008
b: TypeAlias = 'Dict' # OK
c: TypeAlias = 'Foo' # TC008
d: TypeAlias = 'Foo[str]' # OK
e: TypeAlias = 'Foo.bar' # OK
f: TypeAlias = 'Foo | None' # TC008
g: TypeAlias = 'OptStr' # OK
h: TypeAlias = 'Bar' # TC008
i: TypeAlias = Foo['str'] # TC008
j: TypeAlias = 'Baz' # OK
k: TypeAlias = 'k | None' # OK
l: TypeAlias = 'int' | None # TC008 (because TC010 is not enabled)
m: TypeAlias = ('int' # TC008
| None)
n: TypeAlias = ('int' # TC008 (fix removes comment currently)
' | None')

type B = 'Dict' # TC008
type D = 'Foo[str]' # TC008
type E = 'Foo.bar' # TC008
type G = 'OptStr' # TC008
type I = Foo['str'] # TC008
type J = 'Baz' # TC008
type K = 'K | None' # TC008
type L = 'int' | None # TC008 (because TC010 is not enabled)
type M = ('int' # TC008
| None)
type N = ('int' # TC008 (fix removes comment currently)
' | None')


class Baz:
a: TypeAlias = 'Baz' # OK
type A = 'Baz' # TC008

class Nested:
a: TypeAlias = 'Baz' # OK
type A = 'Baz' # TC008
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,12 @@ def f():

def test_annotated_non_typing_reference(user: Annotated[str, Depends(get_foo)]):
pass


def f():
from typing import TypeAlias, TYPE_CHECKING

if TYPE_CHECKING:
from pandas import DataFrame

x: TypeAlias = DataFrame | None
12 changes: 11 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::rules::{flake8_import_conventions, flake8_pyi, pyflakes, pylint, ruff};
use crate::rules::{
flake8_import_conventions, flake8_pyi, flake8_type_checking, pyflakes, pylint, ruff,
};

/// Run lint rules over the [`Binding`]s.
pub(crate) fn bindings(checker: &mut Checker) {
Expand All @@ -15,6 +17,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
Rule::UnconventionalImportAlias,
Rule::UnsortedDunderSlots,
Rule::UnusedVariable,
Rule::UnquotedTypeAlias,
]) {
return;
}
Expand Down Expand Up @@ -72,6 +75,13 @@ pub(crate) fn bindings(checker: &mut Checker) {
checker.diagnostics.push(diagnostic);
}
}
if checker.enabled(Rule::UnquotedTypeAlias) {
if let Some(diagnostics) =
flake8_type_checking::rules::unquoted_type_alias(checker, binding)
{
checker.diagnostics.extend(diagnostics);
}
}
if checker.enabled(Rule::UnsortedDunderSlots) {
if let Some(diagnostic) = ruff::rules::sort_dunder_slots(checker, binding) {
checker.diagnostics.push(diagnostic);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
// Identify any valid runtime imports. If a module is imported at runtime, and
// used at runtime, then by default, we avoid flagging any other
// imports from that model as typing-only.
// FIXME: This does not seem quite right, if only TC004 is enabled
// then we don't need to collect the runtime imports
let enforce_typing_imports = !checker.source_type.is_stub()
&& checker.any_enabled(&[
Rule::RuntimeImportInTypeCheckingBlock,
Expand Down Expand Up @@ -375,6 +377,8 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
}

if matches!(scope.kind, ScopeKind::Function(_) | ScopeKind::Module) {
// FIXME: This does not seem quite right, if only TC004 is enabled
// then we don't need to collect the runtime imports
Comment on lines +380 to +381
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason we can't fix this today?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, I just didn't want to introduce unrelated changes to this PR, so I opted for a comment as reminder to myself to clean this up in a follow-up pull request.

if enforce_typing_imports {
let runtime_imports: Vec<&Binding> = checker
.semantic
Expand Down
72 changes: 67 additions & 5 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -888,9 +888,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
if let Some(type_params) = type_params {
self.visit_type_params(type_params);
}
self.visit
.type_param_definitions
.push((value, self.semantic.snapshot()));
self.visit_deferred_type_alias_value(value);
self.semantic.pop_scope();
self.visit_expr(name);
}
Expand Down Expand Up @@ -961,7 +959,7 @@ impl<'a> Visitor<'a> for Checker<'a> {

if let Some(expr) = value {
if self.semantic.match_typing_expr(annotation, "TypeAlias") {
self.visit_type_definition(expr);
self.visit_annotated_type_alias_value(expr);
} else {
self.visit_expr(expr);
}
Expand Down Expand Up @@ -1855,6 +1853,45 @@ impl<'a> Checker<'a> {
self.semantic.flags = snapshot;
}

/// Visit an [`Expr`], and treat it as the value expression
/// of a [PEP 613] type alias.
///
/// For example:
/// ```python
/// from typing import TypeAlias
///
/// OptStr: TypeAlias = str | None # We're visiting the RHS
/// ```
///
/// [PEP 613]: https://peps.python.org/pep-0613/
fn visit_annotated_type_alias_value(&mut self, expr: &'a Expr) {
let snapshot = self.semantic.flags;
self.semantic.flags |= SemanticModelFlags::ANNOTATED_TYPE_ALIAS;
self.visit_type_definition(expr);
self.semantic.flags = snapshot;
}

/// Visit an [`Expr`], and treat it as the value expression
/// of a [PEP 695] type alias.
///
/// For example:
/// ```python
/// type OptStr = str | None # We're visiting the RHS
/// ```
///
/// [PEP 695]: https://peps.python.org/pep-0695/#generic-type-alias
fn visit_deferred_type_alias_value(&mut self, expr: &'a Expr) {
let snapshot = self.semantic.flags;
// even though we don't visit these nodes immediately we need to
// modify the semantic flags before we push the expression and its
// corresponding semantic snapshot
self.semantic.flags |= SemanticModelFlags::DEFERRED_TYPE_ALIAS;
self.visit
.type_param_definitions
.push((expr, self.semantic.snapshot()));
self.semantic.flags = snapshot;
}

/// Visit an [`Expr`], and treat it as a type definition.
fn visit_type_definition(&mut self, expr: &'a Expr) {
if self.semantic.in_no_type_check() {
Expand Down Expand Up @@ -2017,6 +2054,21 @@ impl<'a> Checker<'a> {
flags.insert(BindingFlags::UNPACKED_ASSIGNMENT);
}

match parent {
Stmt::TypeAlias(_) => flags.insert(BindingFlags::DEFERRED_TYPE_ALIAS),
Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. }) => {
// TODO: It is a bit unfortunate that we do this check twice
// maybe we should change how we visit this statement
// so the semantic flag for the type alias sticks around
// until after we've handled this store, so we can check
// the flag instead of duplicating this check
if self.semantic.match_typing_expr(annotation, "TypeAlias") {
flags.insert(BindingFlags::ANNOTATED_TYPE_ALIAS);
}
}
_ => {}
}

let scope = self.semantic.current_scope();

if scope.kind.is_module()
Expand Down Expand Up @@ -2272,7 +2324,17 @@ impl<'a> Checker<'a> {

self.semantic.flags |=
SemanticModelFlags::TYPE_DEFINITION | type_definition_flag;
self.visit_expr(parsed_annotation.expression());
let parsed_expr = parsed_annotation.expression();
self.visit_expr(parsed_expr);
if self.semantic.in_type_alias_value() {
if self.enabled(Rule::QuotedTypeAlias) {
flake8_type_checking::rules::quoted_type_alias(
self,
parsed_expr,
string_expr,
);
}
}
self.parsed_type_annotation = None;
} else {
if self.enabled(Rule::ForwardAnnotationSyntaxError) {
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8TypeChecking, "004") => (RuleGroup::Stable, rules::flake8_type_checking::rules::RuntimeImportInTypeCheckingBlock),
(Flake8TypeChecking, "005") => (RuleGroup::Stable, rules::flake8_type_checking::rules::EmptyTypeCheckingBlock),
(Flake8TypeChecking, "006") => (RuleGroup::Preview, rules::flake8_type_checking::rules::RuntimeCastValue),
(Flake8TypeChecking, "007") => (RuleGroup::Preview, rules::flake8_type_checking::rules::UnquotedTypeAlias),
(Flake8TypeChecking, "008") => (RuleGroup::Preview, rules::flake8_type_checking::rules::QuotedTypeAlias),
(Flake8TypeChecking, "010") => (RuleGroup::Stable, rules::flake8_type_checking::rules::RuntimeStringUnion),

// tryceratops
Expand Down
40 changes: 40 additions & 0 deletions crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,23 @@ mod tests {
Ok(())
}

// we test these rules as a pair, since they're opposites of one another
// so we want to make sure their fixes are not going around in circles.
#[test_case(Rule::UnquotedTypeAlias, Path::new("TC007.py"))]
#[test_case(Rule::QuotedTypeAlias, Path::new("TC008.py"))]
fn type_alias_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
let diagnostics = test_path(
Path::new("flake8_type_checking").join(path).as_path(),
&settings::LinterSettings::for_rules(vec![
Rule::UnquotedTypeAlias,
Rule::QuotedTypeAlias,
]),
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote.py"))]
#[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("quote.py"))]
#[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote2.py"))]
Expand Down Expand Up @@ -431,6 +448,29 @@ mod tests {
",
"multiple_modules_different_types"
)]
#[test_case(
r"
from __future__ import annotations

from typing import TYPE_CHECKING, TypeAlias
if TYPE_CHECKING:
from foo import Foo # TC004

a: TypeAlias = Foo | None # OK
",
"tc004_precedence_over_tc007"
)]
#[test_case(
r"
from __future__ import annotations

from typing import TypeAlias

a: TypeAlias = 'int | None' # TC008
b: TypeAlias = 'int' | None # TC010
",
"tc010_precedence_over_tc008"
)]
fn contents(contents: &str, snapshot: &str) {
let diagnostics = test_snippet(
contents,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ pub(crate) use empty_type_checking_block::*;
pub(crate) use runtime_cast_value::*;
pub(crate) use runtime_import_in_type_checking_block::*;
pub(crate) use runtime_string_union::*;
pub(crate) use type_alias_quotes::*;
pub(crate) use typing_only_runtime_import::*;

mod empty_type_checking_block;
mod runtime_cast_value;
mod runtime_import_in_type_checking_block;
mod runtime_string_union;
mod type_alias_quotes;
mod typing_only_runtime_import;
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,14 @@ pub(crate) fn runtime_import_in_type_checking_block(
} else {
// Determine whether the member should be fixed by moving the import out of the
// type-checking block, or by quoting its references.
// TODO: We should check `reference.in_annotated_type_alias()`
// as well to match the behavior of the flake8 plugin
// although maybe the best way forward is to add an
// additional setting to configure whether quoting
// or moving the import is preferred for type aliases
// since some people will consistently use their
// type aliases at runtimes, while others won't, so
// the best solution is unclear.
if checker.settings.flake8_type_checking.quote_annotations
&& binding.references().all(|reference_id| {
let reference = checker.semantic().reference(reference_id);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl Violation for RuntimeStringUnion {
}
}

/// TC006
/// TC010
pub(crate) fn runtime_string_union(checker: &mut Checker, expr: &Expr) {
if !checker.semantic().in_type_definition() {
return;
Expand Down
Loading