[ty] Add subtyping between Callable types and class literals with __init__#17638
Merged
carljm merged 19 commits intoastral-sh:mainfrom May 28, 2025
Merged
Conversation
MatthewMckee4
changed the title
Contributor
|
Contributor
Author
|
I think i still have some thinking to do regarding the logic here, and perhaps without |
4 tasks
MatthewMckee4
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
MatthewMckee4
changed the title
__init__
AlexWaygood
changed the title
__init____init__
MatthewMckee4
commented
May 4, 2025
Comment on lines
1272
to
1278
| class C: | ||
| def __new__(cls, *args, **kwargs) -> "C": | ||
| return super().__new__(cls) | ||
|
|
||
| def __init__(self, x: int) -> None: ... | ||
|
|
||
| static_assert(not is_subtype_of(TypeOf[C], Callable[[int], C])) |
Contributor
Author
There was a problem hiding this comment.
As per the spec this "shouldn't" pass, but there's maybe an argument that it should pass
Contributor
Author
There was a problem hiding this comment.
(As is the test without "not" should pass)
Contributor
There was a problem hiding this comment.
Why does this not pass according to the spec? It seems like the spec would say in this case that the callable type of C would be the union of the __new__ signature, and the __init__ bound-method signature with replaced return type. That union seems like it should be a subtype of Callable[[int], C]?
Contributor
Author
There was a problem hiding this comment.
Ah, so yeah you're right, but what is happening here (after i added ":Any" to args and kwargs)
is we end up calling is_subtype with
self: (*args: Any, **kwargs: Any) -> C
target: (int, /) -> C
which returns false because self isnt fully static
MatthewMckee4
commented
May 4, 2025
Contributor
Author
|
Addressing some of the mypy primer issues
|
carljm
reviewed
May 9, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks pretty solid! Sorry for the slow review. A few comments/questions.
Comment on lines
1272
to
1278
| class C: | ||
| def __new__(cls, *args, **kwargs) -> "C": | ||
| return super().__new__(cls) | ||
|
|
||
| def __init__(self, x: int) -> None: ... | ||
|
|
||
| static_assert(not is_subtype_of(TypeOf[C], Callable[[int], C])) |
Contributor
There was a problem hiding this comment.
Why does this not pass according to the spec? It seems like the spec would say in this case that the callable type of C would be the union of the __new__ signature, and the __init__ bound-method signature with replaced return type. That union seems like it should be a subtype of Callable[[int], C]?
| ) | ||
| .symbol; | ||
|
|
||
| // TODO: should be the concrete value of `Self` |
Contributor
There was a problem hiding this comment.
I'm not actually sure what the spec means by "the concrete value of Self" here; I don't know what else that could be, other than what you have here (for non-generic classes, anyway).
| .symbol; | ||
|
|
||
| // TODO: should be the concrete value of `Self` | ||
| let correct_return_type = Type::instance(db, ClassType::NonGeneric(self)); |
Contributor
There was a problem hiding this comment.
What about generic classes? Can we add a test for that case? Do we need a TODO?
Should this just be self_ty.to_instance() instead, so we don't have to worry about those distinctions here?
Contributor
Author
There was a problem hiding this comment.
I think generic cases need some more work, do you think into_callable belongs in ClassLiteral or should it maybe be in ClassType?
| (Some(dunder_new_function), Some(synthesized_dunder_init_callable)) => { | ||
| UnionType::from_elements( | ||
| db, | ||
| vec![dunder_new_function, synthesized_dunder_init_callable], |
Contributor
There was a problem hiding this comment.
Shouldn't this be dunder_new_bound_method instead of dunder_new_function? Otherwise we wrongly include the cls argument. I feel like that might be why the one test you commented on above is saying "not subtype", when it seems like it should say "subtype".
Contributor
Author
There was a problem hiding this comment.
I already call into_bound_method_type further up
Comment on lines
979
to
980
| (Some(dunder_constructor_function), None) | ||
| | (None, Some(dunder_constructor_function)) => dunder_constructor_function, |
Contributor
There was a problem hiding this comment.
And similarly here I feel like we need to be returning the bound method version of __new__?
MichaReiser
changed the title
__init____init__
Contributor
Author
|
@carljm is there any urgency for this? If anyone can provide any more insights on another review I'd be happy to implement them asap. I'm aware you did give some comments but I'm still a bit unsure about them |
Contributor
|
@MatthewMckee4 Sorry for the delay, I'm aware this is just waiting for me to look at it again! It's PyCon this week so a number of us are distracted :) |
Contributor
Author
|
All good, there's no urgency on my end, just wanted to make sure this wasn't forgotten. Thanks |
* main: (246 commits) [ty] Simplify signature types, use them in `CallableType` (astral-sh#18344) [ty] Support ephemeral uv virtual environments (astral-sh#18335) Add a `ViolationMetadata::rule` method (astral-sh#18234) Return `DiagnosticGuard` from `Checker::report_diagnostic` (astral-sh#18232) [flake8_use_pathlib]: Replace os.symlink with Path.symlink_to (PTH211) (astral-sh#18337) [ty] Support cancellation and retry in the server (astral-sh#18273) [ty] Synthetic function-like callables (astral-sh#18242) [ty] Support publishing diagnostics in the server (astral-sh#18309) Add Autofix for ISC003 (astral-sh#18256) [`pyupgrade`]: new rule UP050 (`useless-class-metaclass-type`) (astral-sh#18334) [pycodestyle] Make `E712` suggestion not assume a context (astral-sh#18328) put similar dunder-call tests next to each other (astral-sh#18343) [ty] Derive `PartialOrd, Ord` for `KnownInstanceType` (astral-sh#18340) [ty] Simplify `Type::try_bool()` (astral-sh#18342) [ty] Simplify `Type::normalized` slightly (astral-sh#18339) [ty] Move arviz off the list of selected primer projects (astral-sh#18336) [ty] Add --config-file CLI arg (astral-sh#18083) [ty] Tell the user why we inferred a certain Python version when reporting version-specific syntax errors (astral-sh#18295) [ty] Implement implicit inheritance from `Generic[]` for PEP-695 generic classes (astral-sh#18283) [ty] Add hint if async context manager is used in non-async with statement (astral-sh#18299) ...
carljm
force-pushed
the
class-literal-init-callable-subtyping
branch
from
4aa33be to
084a338
Compare
carljm
approved these changes
May 28, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Made a few updates here, this looks good to me now, and the primer diff looks great. Gonna go ahead and merge!
AlexWaygood
reviewed
May 29, 2025
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
4 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
Allow classes with
__init__to be subtypes ofCallableFixes astral-sh/ty#358
Test Plan
Update is_subtype_of.md