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

[Daverball]

Summary

quoted-type-alias (TC008) can be applied more liberally for annotated type aliases defined inside a if TYPE_CHECKING block.

This also disables TC008 in stub files, since that use-case is already covered by quoted-annotation-in-stub (PYI020).

Test Plan

cargo nextest run

[Daverball]

The stub test case will fail until #15179 is merged, due to the circularity issues with TC007, when TC007 isn't skipped in stub files.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+4 -0 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)

apache/airflow (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ airflow/decorators/condition.py:32:35: TC008 [*] Remove quotes from type alias
+ airflow/decorators/condition.py:33:35: TC008 [*] Remove quotes from type alias

latchbio/latch (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ src/latch/ldata/_transfer/upload.py:30:32: TC008 [*] Remove quotes from type alias
+ src/latch/ldata/_transfer/upload.py:31:35: TC008 [*] Remove quotes from type alias

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
TC008	4	4	0	0	0

[Daverball]

The remaining ecosystem hits look correct to me.

[Daverball]

Actually, we probably need to carve out an additional exception for | if the target version is < 3.10 and we're not inside a stub file.

But that's getting out of scope, since it has nothing to do with execution context. I'll create a follow-up PR for that.

[MichaReiser]

Thank you.

Do you have any references that I could read why it is okay to apply TC008 more eagerly in typing only context? Having a more detailed explanation might also be useful for users reading the Changelog

[Daverball]

The exceptions TC008 currently carves out for attribute and subscript access only are there because some types will only be subscriptable at type checking time (E.g. a class is made generic in a pyi file, but the class in the corresponding py file is not generic, so it can't be subscripted at runtime) and some attributes will be internal to the stubs (E.g. convenience type aliases like webob.request._FieldStorageWithFile, if you import webob.request and access the attribute as webob.request._FieldStorageWithFile at runtime, it will be a NameError).

Neither of these cases are relevant in typing only contexts like type checking blocks or stub files. Since they will never be evaluated by CPython, only by static analysis tools, which includes most if not all runtime type checkers. So we only really need to adhere to what type checkers consider correct.

[Daverball]

As for references. I don't think this is something that's explicitly specced out anywhere, except maybe in some specific type checker's documentation. These are just some nuances/heuristics I have learned over the course of many hundreds of hours of working with mypy and to some degree pyright and pytype via typeshed contributions.

Once ruff performs deeper analysis it might be possible to detect some of these errors without having to carve out large sweeping exceptions like these two. Although it would involve analyzing both the stubs and the actual source for modules with external stubs.

[MichaReiser]

Hmm okay, thanks for the explanation. This does make sense but I think it might be better to wait for @AlexWaygood or @carljm to review this change.

I was interested whether this would impact runtime type checkers but it seems they replace all types declared in TYPE_CHECKING blocks with Any (source)

[AlexWaygood]

@Daverball before I review this PR, could you clarify this a little bit for me:

Actually, we probably need to carve out an additional exception for | if the target version is < 3.10 and we're not inside a stub file.

But that's getting out of scope, since it has nothing to do with execution context. I'll create a follow-up PR for that.

Does this PR introduce a false positive that you then plan to fix in #15201? Or does this false positive that you identify here already exist on main?

If the former, then I think the false positive should be fixed in this PR, because otherwise there's a chance that we review and merge this PR, then look at your other PR, decide it's not the correct fix, and we're stuck with a false positive on main for a prolonged period of time. But if the latter, then I'm happy for things to be split into two PRs, as you've done :-)

[Daverball]

@AlexWaygood Yes, the false positive already exists on main, otherwise I would've fixed it in this PR. I just happened to notice the bug while I was implementing this.

[AlexWaygood]

Great, thanks!

[AlexWaygood]

This preview rule seems to already overlap with our stable rule PYI020. If you run uvx ruff check --select=PYI,TC --preview on this snippet saved as a .pyi file, you already get two diagnostics (one from PYI20, one from TC008) using the latest version of Ruff:

type Y = "list[str]"

This PR makes the problem worse; with this PR branch, running cargo run -- check foo.pyi --select=TC,PYI --preview --no-cache means that the problem is extended to old-style type aliases. Now we get duplicate diagnostics on this as well in a stub-file context:

from typing import TypeAlias

X: TypeAlias = "list[str]"

We'll need to decide how to resolve the overlap between the two rules before we can consider stabilising TC008. I think PYI020` is likely to be a rule that stub authors will already have enabled (since it's already stable, and since the whole category is focussed on writing better stubs), so my instinct is to say that it would be best if TC008 did not emit any lints at all regarding stub files. That feels like the way of resolving the overlap between the rules that will be least disruptive to users.

This is obviously not a perfect solution. In an ideal world, I think we'd have a single rule (probably TC008) that just adjusted its behaviour depending on whether it was analyzing a stub file or a runtime file (what you're doing now). Perhaps that's something we could do as part of rule recategorisation. As it stands, however, this would require deprecating PYI020. That feels unnecessarily disruptive to me, considering that it's a stable rule with no known problems in its implementation.

[Daverball]

@AlexWaygood I'm fine with disabling this rule in stub files, if the use-case is already served by flake8-pyi, that simplifies some things too.

[Daverball]

@AlexWaygood Maybe this means we want to disable TC010 in stubs as well. Although the overlap there should be much smaller, if there is one. It only seems to apply to type expressions that are contained within value expressions, like a cast or a generic subscript in the rhs of an assignment or a function default. And it's also possible that PYI020 doesn't apply in that specific context, so there isn't actually any overlap.

[AlexWaygood]

Thanks for updating this! It overall looks reasonable to me now

[AlexWaygood]

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)

[Daverball]

@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?

[AlexWaygood]

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

[AlexWaygood]

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

[MichaReiser]

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:

[Daverball]

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.

[Daverball]

@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.

[AlexWaygood]

@AlexWaygood Maybe this means we want to disable TC010 in stubs as well. Although the overlap there should be much smaller, if there is one. It only seems to apply to type expressions that are contained within value expressions, like a cast or a generic subscript in the rhs of an assignment or a function default. And it's also possible that PYI020 doesn't apply in that specific context, so there isn't actually any overlap.

PYI020 bans all quotes in all type expressions in stubs (with very narrow exceptions for Literal and Annotated), so yes, it looks like there's overlap there as well, which we should fix.

[Daverball]

@AlexWaygood Bumping this in case it slipped through the cracks.

[AlexWaygood]

Thanks!