-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[red-knot] Allow explicit specialization of generic classes #17023
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
Conversation
|
d0a457c
to
cd5b3fb
Compare
* main: [red-knot] Add property tests for callable types (#17006) [red-knot] Disjointness for callable types (#17094) [red-knot] Flatten `Type::Callable` into four `Type` variants (#17126) mdtest.py: do a full mdtest run immediately when the script is executed (#17128) [red-knot] Fix callable subtyping for standard parameters (#17125) [red-knot] Fix more `redundant-cast` false positives (#17119) Sync vendored typeshed stubs (#17106) [red-knot] support Any as a class in typeshed (#17107) Visit `Identifier` node as part of the `SourceOrderVisitor` (#17110) [red-knot] Don't infer Todo for quite so many tuple type expressions (#17116) CI: Run pre-commit on depot machine (#17120) Error instead of `panic!` when running Ruff from a deleted directory (#16903) (#17054) Control flow graph: setup (#17064) [red-knot] Playground improvements (#17109) [red-knot] IDE crate (#17045) Update dependency vite to v6.2.4 (#17104) [red-knot] Add redundant-cast error (#17100) [red-knot] Narrowing on `in tuple[...]` and `in str` (#17059)
Co-authored-by: Alex Waygood <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the tracked structs Salsa bug, this is ready for review!
reveal_type(Bounded[int]()) # revealed: Bounded[int] | ||
reveal_type(Bounded[IntSubclass]()) # revealed: Bounded[IntSubclass] | ||
|
||
# error: [invalid-argument-type] "Object of type `str` cannot be assigned to parameter 1 (`T`) of class `Bounded`; expected type `int`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagnostics look like this because I'm reusing the call binding mechanism to match parameters and check types. It would be nice for this diagnostic to talk about type parameters, but would prefer to do that as a follow-on.
> TODO: This test is currently disabled pending | ||
> [an upstream Salsa fix](https://github.com/salsa-rs/salsa/pull/741). Once that has been merged, | ||
> re-enable this test by changing the language codes below back to `py`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on salsa-rs/salsa#741
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to unblock this PR on the salsa fix, the other thing we can do is temporarily revert the change that makes mdtests reuse the same Salsa db, and live with slow mdtests until the Salsa fix lands.
/// The member resolves to a member on the class itself or any of its proper superclasses. | ||
/// | ||
/// TODO: Should this be made private...? | ||
pub(super) fn class_member(self, db: &'db dyn Db, name: &str) -> SymbolAndQualifiers<'db> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these methods exist on both ClassType
and ClassLiteralType
. The "real" implementations are on ClassLiteralType
, since that's what represents a class body in the AST. The ClassType
methods wrap that, applying the specialization in the case of a generic alias.
crates/red_knot_python_semantic/resources/mdtest/generics/classes.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/generics/scoping.md
Outdated
Show resolved
Hide resolved
Minor comment: is this outdated or am I misunderstanding something? Are you talking about If this interpretation is correct, that would mean that non-generic class types also fall under |
No, it (and ruff/crates/red_knot_python_semantic/src/types/class.rs Lines 170 to 173 in 43554e2
So a non-generic class (i.e. the class itself, not an instance of it) would be a Type::ClassLiteral(ClassLiteralType::NonGeneric(NonGenericClass { .. })) A generic class that hasn't been specialized would be a Type::ClassLiteral(ClassLiteralType::Generic(GenericClass { .. })) and a generic class that has been specialized would be a Type::GenericAlias(GenericAlias { .. }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic!
Haven't reviewed nearly all of it yet, but I'm going to switch focus to the Salsa bug, so submitting the comments I have.
crates/red_knot_python_semantic/resources/mdtest/generics/scoping.md
Outdated
Show resolved
Hide resolved
> TODO: This test is currently disabled pending | ||
> [an upstream Salsa fix](https://github.com/salsa-rs/salsa/pull/741). Once that has been merged, | ||
> re-enable this test by changing the language codes below back to `py`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to unblock this PR on the salsa fix, the other thing we can do is temporarily revert the change that makes mdtests reuse the same Salsa db, and live with slow mdtests until the Salsa fix lands.
crates/red_knot_python_semantic/resources/mdtest/stubs/class.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!!
} | ||
} | ||
|
||
pub(crate) fn signature(self, db: &'db dyn Db) -> Signature<'db> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your longer-term thinking about the reuse of Signature
and call-binding for specialization? It seems to me that it maybe doesn't buy us very much, since type params are always positional-only, meaning we don't need most of the interesting code from call binding, and it will be hard to integrate e.g. correct handling of constrained typevars, and better diagnostics. (I guess what I'm saying here is I can see why this was a quick way to get something working, but I'm not convinced this is a path we want to go down, vs just writing separate custom code for specialization.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, though my original thinking was to make it lean on the existing call binding mechanism more:
ruff/crates/red_knot_python_semantic/src/types/infer.rs
Lines 5692 to 5695 in 43554e2
// TODO: Move this logic into a custom callable, and update `find_name_in_mro` to return | |
// this callable as the `__class_getitem__` method on `type`. That probably requires | |
// updating all of the subscript logic below to use custom callables for all of the _other_ | |
// special cases, too. |
i.e. explicit specialization would happen in a custom callable, which __getitem__
would resolve to on generic classes. Though that would require adding hooks to Signature
to allow customized diagnostics.
crates/red_knot_python_semantic/src/types/property_tests/type_generation.rs
Show resolved
Hide resolved
* origin/main: [red-knot] Default `python-platform` to current platform (#17183) [red-knot] Add new 'unreachable code' test case (#17306) [red-knot] mypy_primer: Run on `async-utils` (#17303) [red-knot] Add custom `__setattr__` support (#16748) [red-knot] Add `__init__` arguments check when doing `try_call` on a class literal (#16512) [`flake8-pie`] Avoid false positive for multiple assignment with `auto()` (`PIE796`) (#17274) [syntax-errors] Async comprehension in sync comprehension (#17177) [`airflow`] Expand module path check to individual symbols (`AIR302`) (#17278) [syntax-errors] Check annotations in annotated assignments (#17283) [syntax-errors] Extend annotation checks to `await` (#17282) [red-knot] Add support for `assert_never` (#17287) [`flake8-pytest-style`] Avoid false positive for legacy form of `pytest.raises` (`PT011`) (#17231) [red-knot] Do not show types for literal expressions on hover (#17290) [red-knot] Fix dead-code clippy warning (#17291) [red-knot] Reachability analysis (#17199) [red-knot] Don't use latency-sensitive for handlers (#17227)
* main: [red-knot] Allow explicit specialization of generic classes (#17023) [`airflow`] Refactor `AIR301` logic and fix typos (`AIR301`) (#17293) [`airflow`] Extract `AIR312` from `AIR302` rules (`AIR302`, `AIR312`) (#17152) [red-knot] Improve handling of visibility constraints in external modules when resolving `*` imports (#17286) [red-knot] Add more tests for `*` imports (#17315)
…h#17023) This PR lets you explicitly specialize a generic class using a subscript expression. It introduces three new Rust types for representing classes: - `NonGenericClass` - `GenericClass` (not specialized) - `GenericAlias` (specialized) and two enum wrappers: - `ClassType` (a non-generic class or generic alias, represents a class _type_ at runtime) - `ClassLiteralType` (a non-generic class or generic class, represents a class body in the AST) We also add internal support for specializing callables, in particular function literals. (That is, the internal `Type` representation now attaches an optional specialization to a function literal.) This is used in this PR for the methods of a generic class, but should also give us most of what we need for specializing generic _functions_ (which this PR does not yet tackle). --------- Co-authored-by: Alex Waygood <[email protected]> Co-authored-by: Carl Meyer <[email protected]>
This PR lets you explicitly specialize a generic class using a subscript expression. It introduces three new Rust types for representing classes:
NonGenericClass
GenericClass
(not specialized)GenericAlias
(specialized)and two enum wrappers:
ClassType
(a non-generic class or generic alias, represents a class type at runtime)ClassLiteralType
(a non-generic class or generic class, represents a class body in the AST)We also add internal support for specializing callables, in particular function literals. (That is, the internal
Type
representation now attaches an optional specialization to a function literal.) This is used in this PR for the methods of a generic class, but should also give us most of what we need for specializing generic functions (which this PR does not yet tackle).