Skip to content

Commit

Permalink
[flake8-type-checking] Apply TC008 more eagerly in `TYPE_CHECKING…
Browse files Browse the repository at this point in the history
…` blocks and disapply it in stubs (#15180)

Co-authored-by: Alex Waygood <[email protected]>
  • Loading branch information
Daverball and AlexWaygood authored Jan 8, 2025
1 parent 235fdfc commit 339167d
Show file tree
Hide file tree
Showing 8 changed files with 387 additions and 20 deletions.
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: ...
3 changes: 2 additions & 1 deletion crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2352,7 +2352,8 @@ impl<'a> Checker<'a> {
let parsed_expr = parsed_annotation.expression();
self.visit_expr(parsed_expr);
if self.semantic.in_type_alias_value() {
if self.enabled(Rule::QuotedTypeAlias) {
// stub files are covered by PYI020
if !self.source_type.is_stub() && self.enabled(Rule::QuotedTypeAlias) {
flake8_type_checking::rules::quoted_type_alias(
self,
parsed_expr,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ 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_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
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, FixAvailab
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast as ast;
use ruff_python_ast::{Expr, Stmt};
use ruff_python_semantic::{Binding, SemanticModel};
use ruff_python_semantic::{Binding, SemanticModel, TypingOnlyBindingsStatus};
use ruff_python_stdlib::typing::{is_pep_593_generic_type, is_standard_library_literal};
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -68,14 +68,20 @@ impl Violation for UnquotedTypeAlias {
///
/// ## Why is this bad?
/// Unnecessary string forward references can lead to additional overhead
/// in runtime libraries making use of type hints, as well as lead to bad
/// in runtime libraries making use of type hints. They can also have bad
/// interactions with other runtime uses like [PEP 604] type unions.
///
/// For explicit type aliases the quotes are only considered redundant
/// if the type expression contains no subscripts or attribute accesses
/// this is because of stubs packages. Some types will only be subscriptable
/// at type checking time, similarly there may be some module-level
/// attributes like type aliases that are only available in the stubs.
/// PEP-613 type aliases are only flagged by the rule if Ruff can have high
/// confidence that the quotes are unnecessary. Specifically, any PEP-613
/// type alias where the type expression on the right-hand side contains
/// subscripts or attribute accesses will not be flagged. This is because
/// type aliases can reference types that are, for example, generic in stub
/// files but not at runtime. That can mean that a type checker expects the
/// referenced type to be subscripted with type arguments despite the fact
/// that doing so would fail at runtime if the type alias value was not
/// quoted. Similarly, a type alias might need to reference a module-level
/// attribute that exists in a stub file but not at runtime, meaning that
/// the type alias value would need to be quoted to avoid a runtime error.
///
/// ## Example
/// Given:
Expand All @@ -101,6 +107,15 @@ impl Violation for UnquotedTypeAlias {
/// ## Fix safety
/// This rule's fix is marked as safe, unless the type annotation contains comments.
///
/// ## See also
/// This rule only applies to type aliases in non-stub files. For removing quotes in other
/// contexts or in stub files, see:
///
/// - [`quoted-annotation-in-stub`](quoted-annotation-in-stub.md): A rule that
/// removes all quoted annotations from stub files
/// - [`quoted-annotation`](quoted-annotation.md): A rule that removes unnecessary quotes
/// from *annotations* in runtime files.
///
/// ## References
/// - [PEP 613 – Explicit Type Aliases](https://peps.python.org/pep-0613/)
/// - [PEP 695: Generic Type Alias](https://peps.python.org/pep-0695/#generic-type-alias)
Expand Down Expand Up @@ -219,7 +234,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, TypingOnlyBindingsStatus::Disallowed)
.is_some()
{
return;
}

Expand Down Expand Up @@ -291,11 +310,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_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_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 +335,10 @@ 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_type_checking_block().into())
.is_none()
}
_ => false,
}
Expand Down
Loading

0 comments on commit 339167d

Please sign in to comment.