[ty] Diagnostic for generic classes that reference typevars in enclosing scope#20822
[ty] Diagnostic for generic classes that reference typevars in enclosing scope#20822
Conversation
dcreager
requested review from
AlexWaygood,
carljm and
sharkdp
as code owners
| /// list. | ||
| pub(crate) fn from_base_classes( | ||
| db: &'db dyn Db, | ||
| definition: impl FnOnce() -> Definition<'db>, |
There was a problem hiding this comment.
See the comment below; this is passed in as a callback so that we don't have to query the class's definition unless it actually references legacy typevars in its base class list. Passing the definition in directly was causing some of the salsa query dependency tracking tests to fail.
There was a problem hiding this comment.
That's probably worth an inline comment as it's non-obvious for future readers.
However, I do think what you did here is cheating the test rather than fixing the underlying issue. The problem is that calling definition adds a dependency on the ast node. The way you cheated the test is by early-returning in the case we're testing, a class with no type vars! But the same problem still exists for classes with type variables.
The more proper fix here is probably to make whatever calls from_base_classes to be behind a salsa query (at least, in positions where we can reach this call across file boundaries).
There was a problem hiding this comment.
The more proper fix here is probably to make whatever calls
from_base_classesto be behind a salsa query (at least, in positions where we can reach this call across file boundaries).
That was an easier real fix than I expected! Done
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-10-13 20:39:45.649900870 +0000
+++ new-output.txt 2025-10-13 20:39:48.939910978 +0000
@@ -432,6 +432,8 @@
generics_scoping.py:15:1: error[type-assertion-failure] Argument does not have asserted type `str`
generics_scoping.py:42:1: error[type-assertion-failure] Argument does not have asserted type `str`
generics_scoping.py:43:1: error[type-assertion-failure] Argument does not have asserted type `bytes`
+generics_scoping.py:65:11: error[invalid-generic-class] Generic class `MyGeneric` must not reference type variables bound in an enclosing scope: `T` referenced in class definition here
+generics_scoping.py:75:11: error[invalid-generic-class] Generic class `Bad` must not reference type variables bound in an enclosing scope: `T` referenced in class definition here
generics_self_advanced.py:11:24: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@prop1`
generics_self_advanced.py:18:1: error[type-assertion-failure] Argument does not have asserted type `ParentA`
generics_self_advanced.py:19:1: error[type-assertion-failure] Argument does not have asserted type `ChildA`
@@ -896,5 +898,5 @@
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Invalid key access on TypedDict `Movie`: Unknown key "title"
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
-Found 898 diagnostics
+Found 900 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details. |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-generic-class |
4 | 0 | 0 |
no-matching-overload |
1 | 0 | 0 |
| Total | 5 | 0 | 0 |
CodSpeed Performance ReportMerging #20822 will not alter performanceComparing Summary
Footnotes
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I'm surprised by the performance regression. This doesn't seem to add that much complexity. It might be worth taking a look at what's happening.
There was a problem hiding this comment.
heh, this seems like a pre-existing bug in ClassLiteral::header_span() that it doesn't cover the type parameters
Co-authored-by: Alex Waygood <[email protected]>
It looks like this may have been fixed by adding the Salsa caching, though there is now a memory-usage regression being reported by mypy_primer |
That is unsurprising, since we'll have a cached result for the newly cached function for every non-generic class. (And technically for every generic class that is generic via inheritance using legacy typevars) It's not a huge regression, so I would lean towards accepting it. |
| #[salsa::tracked( | ||
| cycle_fn=generic_context_cycle_recover, | ||
| cycle_initial=generic_context_cycle_initial, | ||
| heap_size=ruff_memory_usage::heap_size, | ||
| )] |
There was a problem hiding this comment.
Regarding the memory regression. Would it be possible to only cache the cases where we have a type var? Or is this already what this is doing :)
There was a problem hiding this comment.
I don't think we can without walking the base class in the same way
| if let Some(other_typevar) = | ||
| enclosing.binds_named_typevar(self.db(), self_typevar_name) | ||
| { | ||
| report_rebound_typevar( |
There was a problem hiding this comment.
Should we move the check whether the rule is enabled further up (to avoid the entire scope walking?)
It might require extracting the following into a lint_severity(name: &LintName) -> Option<Severity> helper
ruff/crates/ty_python_semantic/src/types/context.rs
Lines 404 to 431 in 2ce3aba
* main: (25 commits) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) Update Rust crate libc to v0.2.177 (#20832) ...
…rable * origin/main: (26 commits) [ty] Add separate type for typevar "identity" (#20813) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) ...
3 participants
Generic classes are not allowed to bind or reference a typevar from an enclosing scope:
It does not matter if the class uses PEP 695 or legacy syntax. It does not matter if the enclosing scope is a generic class or function. The generic class cannot even reference an enclosing typevar in its base class list.
This PR adds diagnostics for these cases.
In addition, the PR adds better fallback behavior for generic classes that violate this rule: any enclosing typevars are not included in the class's generic context. (That ensures that we don't inadvertently try to infer specializations for those typevars in places where we shouldn't.) The
dulwichecosystem project has examples of this that were causing new false positives on #20677.