Support typing.Self in methods#17689
Merged
carljm merged 11 commits intoastral-sh:mainfrom May 7, 2025
Merged
Conversation
Contributor
|
Glyphack
force-pushed
the
typing-self
branch
4 times, most recently
from
bd01e2e to
2e1d689
Compare
Member
|
@Glyphack I went ahead and rebased your PR ontop the Red Knot renaming. Make sure to pull (force) before making new changes. |
Glyphack
force-pushed
the
typing-self
branch
3 times, most recently
from
ae0a3b1 to
c8f6ec1
Compare
Glyphack
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Glyphack
force-pushed
the
typing-self
branch
4 times, most recently
from
924215b to
e6fd4e2
Compare
carljm
reviewed
May 7, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This is looking really good! A few minor fixups and I think it should be good to go. Would be awesome if we can get the second part (assuming Self for un-annotated self parameters) in soon as well!
Comment on lines
1105
to
1106
| // TODO: infer and if there is self add generic context? | ||
| for param in func_def.parameters.iter_non_variadic_params() {} |
Contributor
There was a problem hiding this comment.
If you're intending to leave this TODO for a follow-up PR, you'll need to remove a bit more of this code to make clippy happy; it doesn't like the unused param variable.
It seems to me that the way this should be handled is to assume the type type[Self] for the cls argument to __new__ (if un-annotated), and similarly assume the type Self for the self argument, if un-annotated. If we do that, it seems like the existing constructor call specialization inference should handle this?
Contributor
Author
There was a problem hiding this comment.
Thanks, I left this code just to show what I tried for adding the generic context. I will remove it before merging. There is a TODO already in the tests for this. I'll add it in my next PR.
So we should not need to add the generic context like I wanted to do in this place and just apply the logic for cls and self when inferring the parameters of a function?
Comment on lines
4741
to
4790
| let class = class_ty.expect_class_type(db); | ||
| let Some(class_def) = class_ty.definition(db) else { | ||
| debug_assert!(false, "enclosing_class_symbol must return a class type"); | ||
| return Ok(Type::unknown()); | ||
| }; | ||
| let TypeDefinition::Class(d) = class_def else { | ||
| debug_assert!(false, "class type must have a definition"); | ||
| return Ok(Type::unknown()); | ||
| }; |
Contributor
There was a problem hiding this comment.
I'm not sure we should make these assumptions -- and I don't think we need to? If we use "Self" as the name of the typevar, then we don't need class.name(db), and I think we can just use class_ty.to_instance(db) to get the upper bound.
Contributor
Author
There was a problem hiding this comment.
I think I also need the definition of the class. Or maybe I'm doing something bad here? My reasoning for using the class definition as the definition of this type var was that the definition is related to that class.
Co-authored-by: Micha Reiser <[email protected]>
Add todo comments for failing cases Add todo comments for failing cases Implement more test cases Cargo clippy
Use debug assert for failure cases
Co-authored-by: Carl Meyer <[email protected]>
Co-authored-by: Carl Meyer <[email protected]>
Glyphack
commented
May 7, 2025
| ast::name::Name::new("Self"), | ||
| class_def, | ||
| Some(TypeVarBoundOrConstraints::UpperBound(instance)), | ||
| TypeVarVariance::Invariant, |
Contributor
Author
There was a problem hiding this comment.
This one was added after I rebased it should be invariant right?
Contributor
There was a problem hiding this comment.
I don't think it matters since Self will never be part of the generic context of a generic class; variance only matters when determining subtyping between generic classes. So yeah, invariant is a fine default.
Contributor
Author
|
Thank you for the help. I start working on the second PR to recognize |
carljm
approved these changes
May 7, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks good to me! I'm going to go ahead and land it; feel free to consider my comments for possible inclusion in a follow-up PR. None of them are critical.
| ast::name::Name::new("Self"), | ||
| class_def, | ||
| Some(TypeVarBoundOrConstraints::UpperBound(instance)), | ||
| TypeVarVariance::Invariant, |
Contributor
There was a problem hiding this comment.
I don't think it matters since Self will never be part of the generic context of a generic class; variance only matters when determining subtyping between generic classes. So yeah, invariant is a fine default.
Comment on lines
+4778
to
+4788
| debug_assert!( | ||
| false, | ||
| "enclosing_class_symbol must return a type with class definition" | ||
| ); | ||
| return Ok(Type::unknown()); | ||
| }; | ||
| let Some(instance) = class_ty.to_instance(db) else { | ||
| debug_assert!( | ||
| false, | ||
| "enclosing_class_symbol must return type that can be instantiated" | ||
| ); |
Contributor
There was a problem hiding this comment.
I am not 100% sure that these debug-asserts will always remain true, in the face of e.g. class decorators. But we also haven't fully decided yet how to handle class decorators. I think it's fine to have these here for now; they'll at least force us to consider the impact here if we ever do violate these invariants.
Contributor
There was a problem hiding this comment.
I think another option here would be to split the enclosing_class_symbol function in half, and turn the first half of it into enclosing_class_definition instead, returning a Definition, and use a separate function to get the inferred type for that definition. Then you'd have the definition in hand already, and wouldn't have to pull it back out of the type.
| /// and finds the closest class definition. | ||
| /// | ||
| /// Returns `None` if no enclosing class is found.a | ||
| pub(crate) fn enclosing_class_symbol<'db>( |
Contributor
There was a problem hiding this comment.
Orthogonal to this PR, but this should really be renamed to enclosing_class_type -- it returns a Type, not a Symbol.
|
In Python <= 3.10, could using typing_extensions.Self be considered as well? (same as e.g. pyright does it) |
Contributor
This was already supported in this PR. I add a test for it in #17995 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Summary
Fixes: astral-sh/ty#159
This PR adds the support for using
Selfin methods.When the type of an annotation is
TypingSelfit is converted to a type var based on:https://typing.python.org/en/latest/spec/generics.html#self
I am naming the type var type that is created on the fly same as the class name so the type revealed shows that it's the class. Pyright for example names it
Self@ClassNameI'm not sure if that's better or not.I just skipped Protocols because it had more problems and the tests was not useful.
Also I need to create a follow up PR that implicitly assumes
selfargument has typeSelf.In order to infer the type in the
in_type_expressionmethod I needed to have scope id and semantic index available. I used the idea from this PR to pass additional context to this method.Also I think in all places that
in_type_expressionis called we need to have this context becauseSelfcan be there so I didn't split the method into one version with context and one without.Test Plan
Added new tests from spec.
Notes:
This test previously did not emit diagnostic but now there is a diagnostic.
I think it’s appropriate to make diagnostic?
mypy does it: https://mypy-play.net/?mypy=latest&python=3.12&gist=0be580f15a4fe36399738284c5c9688d
pyright does not: https://pyright-play.net/?code=GYJw9gtgBALgngBwJYDsDmUkQWEMoDKApgDbABQ5AxiQIYDO9UAQg0QFzlTdQAmRwKAH0hKIgHcRAChr0ANFAAe7TChgBKKAFoAfIVLAVAOhPkgA