[red-knot] Handle explicit class specialization in type expressions #17434
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
dcreager
commented
Apr 16, 2025
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md
Outdated
Show resolved
Hide resolved
Member
Author
|
6 new
|
Contributor
Yeah I don't think we support narrowing the type of |
Contributor
This may be involved in us inferring an unknown specialization when we shouldn't, but it seems to me that those false positives are also demonstrating a simpler issue, which is that we should in general consider |
carljm
approved these changes
Apr 16, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Love how simple this was to implement!
Looks like the main follow-up this reveals is not in the code touched in this PR, but more like a knock-on effect of supporting this: we need a more sophisticated understanding of subtyping (variance) and assignability (dynamic types) for specialized generics. Both are important but the latter is probably higher priority and simpler to implement.
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_equivalent_to.md
Outdated
Show resolved
Hide resolved
| pass | ||
|
|
||
| static_assert(is_subtype_of(B, A[int])) | ||
| static_assert(not is_subtype_of(B, A[bool])) |
Contributor
There was a problem hiding this comment.
This is really a test of variance, which is opening another whole can of worms we'll have to handle soon. This subtype relation should be true if A is contravariant in T (because that means that A[bool] is a supertype of A[int]), false if it is invariant or covariant.
Variance for PEP 695 typevars is determined by inference, based on the positions in which the typevar occurs in the definition of the class. If the typevar occurs in covariant positions only (that is, method return types, including the implicit getter "method" of every normal mutable attribute) it is covariant, if it occurs in contravariant positions only (accepted as a method argument type, including the implicit setter "method" of every normal mutable attribute) then it is contravariant, and if it appears in both, it is invariant. (Note that this means a type variable used as the type of a normal mutable attribute must be invariant, since it appears in both covariant and contravariant position via the implicit getter and setter of that mutable attribute.) Also note that callable types can invert variance of a position: that is, a typevar occurring as argument to a Callable type accepted as a method argument is in covariant position. The method argument is a contravariant position, but the argument of the Callable type is also a contravariant position, so this reverses the variance and that typevar is actually occurring in a covariant position.
A class like A which never uses its typevar in any position at all can safely be considered "bivariant" (in plain English, the type of the typevar has no impact whatsoever on the class, so it just doesn't matter to subtyping at all!)
So all that said: technically, given the code we see here, this assertion is wrong: B should be a subtype of A[bool], because A is bivariant in T, which means that any specialization of A is a subtype of any other specialization of A -- that is, all specializations of A are equivalent.
As far as this PR is concerned, the only thing I would suggest is a TODO comment on this assertion :)
Member
Author
There was a problem hiding this comment.
Added a bunch of tests for each of the four variance cases, and for each of subtyping, assignability, equivalence, and gradual equivalence. All TODOs for now where they're not producing the right answer.
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md
Outdated
Show resolved
Hide resolved
| } | ||
| _ => { | ||
| self.infer_type_expression(slice); | ||
| todo_type!("generics") |
Member
Author
There was a problem hiding this comment.
Done. I got rid of the todo type completely, and instead added new diagnostics for when (a) you try to specialize a non-generic class, and (b) when you use any other subscript expression in a type expression. Added quite a bit of churn in the mdtest suite, but I think it's worth it.
* main: [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406)
dcreager
commented
Apr 17, 2025
Member
Author
dcreager
left a comment
dcreager
left a comment
There was a problem hiding this comment.
I anticipate that there's about to a lot of new false positives on the ecosystem check, since we're now printing out diagnostics when you try to specialize a non-generic class — which will be (incorrectly) reported for basically every generic class in the typeshed, since we don't yet recognize them as generic since they use legacy typevars.
| pass | ||
|
|
||
| static_assert(is_subtype_of(B, A[int])) | ||
| static_assert(not is_subtype_of(B, A[bool])) |
Member
Author
There was a problem hiding this comment.
Added a bunch of tests for each of the four variance cases, and for each of subtyping, assignability, equivalence, and gradual equivalence. All TODOs for now where they're not producing the right answer.
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_equivalent_to.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md
Outdated
Show resolved
Hide resolved
| } | ||
| _ => { | ||
| self.infer_type_expression(slice); | ||
| todo_type!("generics") |
Member
Author
There was a problem hiding this comment.
Done. I got rid of the todo type completely, and instead added new diagnostics for when (a) you try to specialize a non-generic class, and (b) when you use any other subscript expression in a type expression. Added quite a bit of churn in the mdtest suite, but I think it's worth it.
Contributor
|
Contributor
4153 new |
Member
Author
Yeahhhhh, I'm going to back that out and wait to add those diagnostics until we understand that |
carljm
approved these changes
Apr 17, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
I think there are a few assertions around non-fully-static generics that we need to fix up here, but otherwise looks great!
crates/red_knot_python_semantic/resources/mdtest/generics/variance.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/generics/variance.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/generics/variance.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/generics/variance.md
Outdated
Show resolved
Hide resolved
Comment on lines
7196
to
7201
| Type::StringLiteral(_) => { | ||
| // Don't emit a diagnostic, since we haven't determined the type of the deferred | ||
| // string annotation yet. | ||
| self.infer_type_expression(slice); | ||
| Type::unknown() | ||
| } |
Contributor
There was a problem hiding this comment.
Trying to understand what case this is handling -- did it come up in ecosystem checks? It seems like it would be permitting an annotation like "list"[str], but AFAIK we don't need to support that, it isn't allowed. Pyright doesn't allow it. Am I misunderstand what this is for?
Member
Author
There was a problem hiding this comment.
We have this in our mdtests, e.g. here
ruff/crates/red_knot_python_semantic/resources/mdtest/generics/classes.md
Lines 166 to 168 in b63de60
Should we change that to "C[T]" per pyright's suggestion?
Member
Author
There was a problem hiding this comment.
Changed to "C[T]" and verified that the noisy new diagnostics didn't fire after removing the StringLiteral match arm.
Contributor
There was a problem hiding this comment.
It looks like mypy does support this. But I don't feel like we should go out of our way to support it, IMO it's an odd thing to do, and the alternative (quote the entire annotation) is easy and the more obvious choice anyway. So I support what you've done here.
* main: [red-knot] class bases are not affected by __future__.annotations (#17456) [red-knot] Add support for overloaded functions (#17366) [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444) [red-knot] more type-narrowing in match statements (#17302) [red-knot] Add some narrowing for assignment expressions (#17448) [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446) Server: Use `min` instead of `max` to limit the number of threads (#17421)
carljm
approved these changes
Apr 18, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Latest additions here look good!
dcreager
added a commit
that referenced
this pull request
Apr 18, 2025
* main: (123 commits) [red-knot] Handle explicit class specialization in type expressions (#17434) [red-knot] allow assignment expression in call compare narrowing (#17461) [red-knot] fix building unions with literals and AlwaysTruthy/AlwaysFalsy (#17451) [red-knot] Type narrowing for assertions (take 2) (#17345) [red-knot] class bases are not affected by __future__.annotations (#17456) [red-knot] Add support for overloaded functions (#17366) [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444) [red-knot] more type-narrowing in match statements (#17302) [red-knot] Add some narrowing for assignment expressions (#17448) [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446) Server: Use `min` instead of `max` to limit the number of threads (#17421) [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406) [red-knot] Super-basic generic inference at call sites (#17301) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
You can now use subscript expressions in a type expression to explicitly specialize generic classes, just like you could already do in value expressions.
This still does not implement bidirectional checking, so a type annotation on an assignment does not influence how we infer a specialization for a (not explicitly specialized) constructor call. You might get an
invalid-assignmenterror if (a) we cannot infer a class specialization from the constructor call (in which case you end up e.g. trying to assignC[Unknown]toC[int]) or if (b) we can infer a specialization, but it doesn't match the annotation.Closes #17432