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.

Sign up for GitHub

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] Apply TC008 more eagerly in TYPE_CHECKING blocks and disapply it in stubs #15180

Merged
merged 10 commits into from
Jan 8, 2025
Merged

[flake8-type-checking] Apply TC008 more eagerly in TYPE_CHECKING blocks and disapply it in stubs #15180

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a775f51
[`flake8-type-checking`] Apply `TC008` more eagerly in typing context
Daverball Dec 29, 2024
348fd86
Execution context doesn't work, since it's already set to typing inside
Daverball Dec 29, 2024
f119f26
Merge branch 'main' into feat/tc008-typing-execution-context
Daverball Dec 29, 2024
a9f2d80
Avoids false positives for true forward references
Daverball Dec 29, 2024
39f4e10
Adds some use-before-define test cases
Daverball Dec 29, 2024
44dccdc
Disable `TC008` in stubs. Clean up stuff that's no longer needed.
Daverball Jan 3, 2025
5947f64
Update crates/ruff_linter/src/checkers/ast/mod.rs
Daverball Jan 3, 2025
5db1af5
Applies suggested change
Daverball Jan 3, 2025
ea541a6
Adds note and link to `PYI020`
Daverball Jan 6, 2025
9e8642f
more docs
AlexWaygood Jan 8, 2025
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
25 changes: 25 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC008.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from __future__ import annotations

from typing import TypeAlias, TYPE_CHECKING

from foo import Foo

a: TypeAlias = 'int' # TC008
b: TypeAlias = 'Foo' # TC008
c: TypeAlias = 'Foo[str]' # TC008
d: TypeAlias = 'Foo.bar' # TC008
e: TypeAlias = 'Baz' # OK

type B = 'Foo' # TC008
type C = 'Foo[str]' # TC008
type D = 'Foo.bar' # TC008
type E = 'Baz' # TC008


class Baz:
a: TypeAlias = 'Baz' # False negative in stubs
type A = 'Baz' # TC008

class Nested:
a: TypeAlias = 'Baz' # False negative in stubs
type A = 'Baz' # TC008
31 changes: 31 additions & 0 deletions ...uff_linter/resources/test/fixtures/flake8_type_checking/TC008_typing_execution_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
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]

a: TypeAlias = 'int' # TC008
b: TypeAlias = 'Dict' # TC008
c: TypeAlias = 'Foo' # TC008
d: TypeAlias = 'Foo[str]' # TC008
e: TypeAlias = 'Foo.bar' # TC008
f: TypeAlias = 'Foo | None' # TC008
g: TypeAlias = 'OptStr' # TC008
h: TypeAlias = 'Bar' # TC008
i: TypeAlias = Foo['str'] # TC008
j: TypeAlias = 'Baz' # OK (this would be treated as use before define)
k: TypeAlias = 'k | None' # False negative in type checking block
l: TypeAlias = 'int' | None # TC008 (because TC010 is not enabled)
m: TypeAlias = ('int' # TC008
| None)
n: TypeAlias = ('int' # TC008 (fix removes comment currently)
' | None')


class Baz: ...
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ mod tests {
// 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"))]
#[test_case(Rule::QuotedTypeAlias, Path::new("TC008.pyi"))]
#[test_case(Rule::QuotedTypeAlias, Path::new("TC008_typing_execution_context.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(
Expand Down
35 changes: 28 additions & 7 deletions crates/ruff_linter/src/rules/flake8_type_checking/rules/type_alias_quotes.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Could we also add a note to the docs for this rule pointing users to PYI020 for stub files? (And we could similarly add a note to the PYI020 docs pointing users to this rule for runtime files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexWaygood Do you happen to know how to cross reference rules from another rule? I couldn't find any examples. Does linking to the corresponding enum variant work?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... no, I'm not sure what the best solution is there. @MichaReiser, do you have any guidance/insight?

Copy link
Member

Choose a reason for hiding this comment

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

I chatted to @MichaReiser this morning and we concluded that neither of us knows of a great way to do this :-) hardcoding a link to https://docs.astral.sh/ruff/rules/quoted-annotation-in-stub/ might be best for now

Copy link
Member

@MichaReiser MichaReiser Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just double checked and yes, there's no custom handling in

ruff/crates/ruff_dev/src/generate_docs.rs

Lines 118 to 187 in bb12fe9

fn process_documentation(documentation: &str, out: &mut String, rule_name: &str) {
let mut in_options = false;
let mut after = String::new();
let mut referenced_options = HashSet::new();
// HACK: This is an ugly regex hack that's necessary because mkdocs uses
// a non-CommonMark-compliant Markdown parser, which doesn't support code
// tags in link definitions
// (see https://github.com/Python-Markdown/markdown/issues/280).
let documentation = Regex::new(r"\[`([^`]*?)`]($|[^\[(])").unwrap().replace_all(
documentation,
|caps: &Captures| {
format!(
"[`{option}`][{option}]{sep}",
option = &caps[1],
sep = &caps[2]
)
},
);
for line in documentation.split_inclusive('\n') {
if line.starts_with("## ") {
in_options = line == "## Options\n";
} else if in_options {
if let Some(rest) = line.strip_prefix("- `") {
let option = rest.trim_end().trim_end_matches('`');
match Options::metadata().find(option) {
Some(OptionEntry::Field(field)) => {
if field.deprecated.is_some() {
eprintln!("Rule {rule_name} references deprecated option {option}.");
}
}
Some(_) => {}
None => {
panic!("Unknown option {option} referenced by rule {rule_name}");
}
}
let anchor = option.replace('.', "_");
out.push_str(&format!("- [`{option}`][{option}]\n"));
after.push_str(&format!("[{option}]: ../settings.md#{anchor}\n"));
referenced_options.insert(option);
continue;
}
}
out.push_str(line);
}
let re = Regex::new(r"\[`([^`]*?)`]\[(.*?)]").unwrap();
for (_, [option, _]) in re.captures_iter(&documentation).map(|c| c.extract()) {
if let Some(OptionEntry::Field(field)) = Options::metadata().find(option) {
if referenced_options.insert(option) {
let anchor = option.replace('.', "_");
after.push_str(&format!("[{option}]: ../settings.md#{anchor}\n"));
}
if field.deprecated.is_some() {
eprintln!("Rule {rule_name} references deprecated option {option}.");
}
}
}
if !after.is_empty() {
out.push('\n');
out.push('\n');
out.push_str(&after);
}
}

It might be possible to use relative links as we do in the linter documentation

ruff/docs/linter.md

Lines 165 to 167 in 68eb0a2

For example, [`unnecessary-iterable-allocation-for-first-element`](rules/unnecessary-iterable-allocation-for-first-element.md)
(`RUF015`) is a rule which checks for potentially unperformant use of `list(...)[0]`. The fix
replaces this pattern with `next(iter(...))` which can result in a drastic speedup:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could probably also link /rules/quoted-annotation-in-stub.md so it's only semi-hardlinked, I had to do something similar in a setting to link to a rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexWaygood I think a reference from TC008 to PYI020 is sufficient. A reference the other way doesn't make that much sense to me, since most PYI rules are so specific to stub files.

Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,11 @@ fn collect_typing_references<'a>(
let Some(binding_id) = checker.semantic().resolve_name(name) else {
return;
};
if checker.semantic().simulate_runtime_load(name).is_some() {
if checker
.semantic()
.simulate_runtime_load(name, false)
.is_some()
{
return;
}

Expand Down Expand Up @@ -291,11 +295,22 @@ fn quotes_are_unremovable(semantic: &SemanticModel, expr: &Expr) -> bool {
ctx: ExprContext::Load,
..
}) => quotes_are_unremovable(semantic, value),
// for subscripts and attributes we don't know whether it's safe
// to do at runtime, since the operation may only be available at
// type checking time. E.g. stubs only generics. Or stubs only
// type aliases.
Expr::Subscript(_) | Expr::Attribute(_) => true,
Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
// for subscripts we don't know whether it's safe to do at runtime
// since the operation may only be available at type checking time.
// E.g. stubs only generics.
if !semantic.in_stub_file() && !semantic.in_type_checking_block() {
return true;
}
quotes_are_unremovable(semantic, value) || quotes_are_unremovable(semantic, slice)
}
Expr::Attribute(ast::ExprAttribute { value, .. }) => {
// for attributes we also don't know whether it's safe
if !semantic.in_stub_file() && !semantic.in_type_checking_block() {
return true;
}
quotes_are_unremovable(semantic, value)
}
Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => {
for elt in elts {
if quotes_are_unremovable(semantic, elt) {
Expand All @@ -305,7 +320,13 @@ fn quotes_are_unremovable(semantic: &SemanticModel, expr: &Expr) -> bool {
false
}
Expr::Name(name) => {
semantic.resolve_name(name).is_some() && semantic.simulate_runtime_load(name).is_none()
semantic.resolve_name(name).is_some()
&& semantic
.simulate_runtime_load(
name,
semantic.in_stub_file() || semantic.in_type_checking_block(),
)
.is_none()
}
_ => false,
}
Expand Down
201 changes: 201 additions & 0 deletions ...apshots/ruff_linter__rules__flake8_type_checking__tests__quoted-type-alias_TC008.pyi.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
---
source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
---
TC008.pyi:7:16: TC008 [*] Remove quotes from type alias
|
5 | from foo import Foo
6 |
7 | a: TypeAlias = 'int' # TC008
| ^^^^^ TC008
8 | b: TypeAlias = 'Foo' # TC008
9 | c: TypeAlias = 'Foo[str]' # TC008
|
= help: Remove quotes

ℹ Safe fix
4 4 |
5 5 | from foo import Foo
6 6 |
7 |-a: TypeAlias = 'int' # TC008
7 |+a: TypeAlias = int # TC008
8 8 | b: TypeAlias = 'Foo' # TC008
9 9 | c: TypeAlias = 'Foo[str]' # TC008
10 10 | d: TypeAlias = 'Foo.bar' # TC008

TC008.pyi:8:16: TC008 [*] Remove quotes from type alias
|
7 | a: TypeAlias = 'int' # TC008
8 | b: TypeAlias = 'Foo' # TC008
| ^^^^^ TC008
9 | c: TypeAlias = 'Foo[str]' # TC008
10 | d: TypeAlias = 'Foo.bar' # TC008
|
= help: Remove quotes

ℹ Safe fix
5 5 | from foo import Foo
6 6 |
7 7 | a: TypeAlias = 'int' # TC008
8 |-b: TypeAlias = 'Foo' # TC008
8 |+b: TypeAlias = Foo # TC008
9 9 | c: TypeAlias = 'Foo[str]' # TC008
10 10 | d: TypeAlias = 'Foo.bar' # TC008
11 11 | e: TypeAlias = 'Baz' # OK

TC008.pyi:9:16: TC008 [*] Remove quotes from type alias
|
7 | a: TypeAlias = 'int' # TC008
8 | b: TypeAlias = 'Foo' # TC008
9 | c: TypeAlias = 'Foo[str]' # TC008
| ^^^^^^^^^^ TC008
10 | d: TypeAlias = 'Foo.bar' # TC008
11 | e: TypeAlias = 'Baz' # OK
|
= help: Remove quotes

ℹ Safe fix
6 6 |
7 7 | a: TypeAlias = 'int' # TC008
8 8 | b: TypeAlias = 'Foo' # TC008
9 |-c: TypeAlias = 'Foo[str]' # TC008
9 |+c: TypeAlias = Foo[str] # TC008
10 10 | d: TypeAlias = 'Foo.bar' # TC008
11 11 | e: TypeAlias = 'Baz' # OK
12 12 |

TC008.pyi:10:16: TC008 [*] Remove quotes from type alias
|
8 | b: TypeAlias = 'Foo' # TC008
9 | c: TypeAlias = 'Foo[str]' # TC008
10 | d: TypeAlias = 'Foo.bar' # TC008
| ^^^^^^^^^ TC008
11 | e: TypeAlias = 'Baz' # OK
|
= help: Remove quotes

ℹ Safe fix
7 7 | a: TypeAlias = 'int' # TC008
8 8 | b: TypeAlias = 'Foo' # TC008
9 9 | c: TypeAlias = 'Foo[str]' # TC008
10 |-d: TypeAlias = 'Foo.bar' # TC008
10 |+d: TypeAlias = Foo.bar # TC008
11 11 | e: TypeAlias = 'Baz' # OK
12 12 |
13 13 | type B = 'Foo' # TC008

TC008.pyi:13:10: TC008 [*] Remove quotes from type alias
|
11 | e: TypeAlias = 'Baz' # OK
12 |
13 | type B = 'Foo' # TC008
| ^^^^^ TC008
14 | type C = 'Foo[str]' # TC008
15 | type D = 'Foo.bar' # TC008
|
= help: Remove quotes

ℹ Safe fix
10 10 | d: TypeAlias = 'Foo.bar' # TC008
11 11 | e: TypeAlias = 'Baz' # OK
12 12 |
13 |-type B = 'Foo' # TC008
13 |+type B = Foo # TC008
14 14 | type C = 'Foo[str]' # TC008
15 15 | type D = 'Foo.bar' # TC008
16 16 | type E = 'Baz' # TC008

TC008.pyi:14:10: TC008 [*] Remove quotes from type alias
|
13 | type B = 'Foo' # TC008
14 | type C = 'Foo[str]' # TC008
| ^^^^^^^^^^ TC008
15 | type D = 'Foo.bar' # TC008
16 | type E = 'Baz' # TC008
|
= help: Remove quotes

ℹ Safe fix
11 11 | e: TypeAlias = 'Baz' # OK
12 12 |
13 13 | type B = 'Foo' # TC008
14 |-type C = 'Foo[str]' # TC008
14 |+type C = Foo[str] # TC008
15 15 | type D = 'Foo.bar' # TC008
16 16 | type E = 'Baz' # TC008
17 17 |

TC008.pyi:15:10: TC008 [*] Remove quotes from type alias
|
13 | type B = 'Foo' # TC008
14 | type C = 'Foo[str]' # TC008
15 | type D = 'Foo.bar' # TC008
| ^^^^^^^^^ TC008
16 | type E = 'Baz' # TC008
|
= help: Remove quotes

ℹ Safe fix
12 12 |
13 13 | type B = 'Foo' # TC008
14 14 | type C = 'Foo[str]' # TC008
15 |-type D = 'Foo.bar' # TC008
15 |+type D = Foo.bar # TC008
16 16 | type E = 'Baz' # TC008
17 17 |
18 18 |

TC008.pyi:16:10: TC008 [*] Remove quotes from type alias
|
14 | type C = 'Foo[str]' # TC008
15 | type D = 'Foo.bar' # TC008
16 | type E = 'Baz' # TC008
| ^^^^^ TC008
|
= help: Remove quotes

ℹ Safe fix
13 13 | type B = 'Foo' # TC008
14 14 | type C = 'Foo[str]' # TC008
15 15 | type D = 'Foo.bar' # TC008
16 |-type E = 'Baz' # TC008
16 |+type E = Baz # TC008
17 17 |
18 18 |
19 19 | class Baz:

TC008.pyi:21:14: TC008 [*] Remove quotes from type alias
|
19 | class Baz:
20 | a: TypeAlias = 'Baz' # False negative in stubs
21 | type A = 'Baz' # TC008
| ^^^^^ TC008
22 |
23 | class Nested:
|
= help: Remove quotes

ℹ Safe fix
18 18 |
19 19 | class Baz:
20 20 | a: TypeAlias = 'Baz' # False negative in stubs
21 |- type A = 'Baz' # TC008
21 |+ type A = Baz # TC008
22 22 |
23 23 | class Nested:
24 24 | a: TypeAlias = 'Baz' # False negative in stubs

TC008.pyi:25:18: TC008 [*] Remove quotes from type alias
|
23 | class Nested:
24 | a: TypeAlias = 'Baz' # False negative in stubs
25 | type A = 'Baz' # TC008
| ^^^^^ TC008
|
= help: Remove quotes

ℹ Safe fix
22 22 |
23 23 | class Nested:
24 24 | a: TypeAlias = 'Baz' # False negative in stubs
25 |- type A = 'Baz' # TC008
25 |+ type A = Baz # TC008
Loading
Loading