-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Add basic subtyping between class literal and callable #17469
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
[red-knot] Add basic subtyping between class literal and callable #17469
Conversation
|
|
To note: currently we don't emit an error for this class A:
def __new__(cls) -> A:
return object.__new__(cls)
def __init__(self, x: int) -> None:
self.x = x |
|
I'm not sure this will have a great impact without our understanding of |
carljm
left a comment
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.
Thank you!
| # TODO: This should not error | ||
| # This errors because the metaclass `__call__` is not seen as overriding `type.__call__` | ||
| # and we don't yet check `__new__` or `__init__`. |
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.
I agree that this should not error, but for a different reason -- I think (as discussed in Discord) that the text in the spec about ignoring certain metaclass __call__ methods for purposes of converting a constructor to a callable is making unwarranted assumptions that can lead to false positive errors. I think it is more correct to always respect the signature of the metaclass __call__ method.
If this causes problems in practice (including with the spec conformance suite, and we can't effectuate an update to the spec), we can consider a future change. But for now I think we should unconditionally use metaclass __call__ if present (which I think will make this test pass as written and remove the need for the TODO comment?)
| class MetaWithSelfReturn(type): | ||
| def __call__(cls) -> Self: | ||
| return super().__call__() | ||
|
|
||
| class C(metaclass=MetaWithSelfReturn): ... | ||
|
|
||
| # TODO: This should not error | ||
| # error: [static-assert-error] "Static assertion error: argument evaluates to `False`" | ||
| static_assert(is_subtype_of(TypeOf[C], Callable[[], C])) | ||
| static_assert(not is_subtype_of(TypeOf[C], Callable[[object], C])) |
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.
I'm not sure it makes sense to include this test at all, and if we do, it should not behave as asserted here.
The Self annotation above is in the context of class MetaWithSelfReturn, so it is requiring that the __call__ method return an instance of MetaWithSelfReturn, not an instance of C. So C should not be a subtype of any callable type returning C.
I think we should just omit this test for now.
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.
Ah yes my bad, thanks
| let overrides_type_call = match new_function.function(db).signature(db) { | ||
| FunctionSignature::Single(signature) | ||
| | FunctionSignature::Overloaded(_, Some(signature)) => { | ||
| match signature.return_ty { | ||
| Some(ty) => !self.is_subtype_of(db, ty.to_meta_type(db)), | ||
| _ => false, | ||
| } | ||
| } | ||
| FunctionSignature::Overloaded(..) => false, | ||
| }; | ||
| if overrides_type_call { |
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.
As discussed above, I think for now let's just always use the metaclass __call__ signature, if present, and add a comment // TODO: this intentionally diverges from step 1 in https://typing.python.org/en/latest/spec/constructors.html#converting-a-constructor-to-callable, which makes unwarranted assumptions
carljm
left a comment
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.
Looks great, thank you!
* main: (37 commits) [red-knot] Add list of failing/slow ecosystem projects (#17474) [red-knot] mypy_primer: extend ecosystem checks (#17544) [red-knot] Move `InstanceType` to its own submodule (#17525) [red-knot] mypy_primer: capture backtraces (#17543) [red-knot] mypy_primer: Use upstream repo (#17500) [red-knot] `typing.dataclass_transform` (#17445) Update dependency react-resizable-panels to v2.1.8 (#17513) Update dependency smol-toml to v1.3.3 (#17505) Update dependency uuid to v11.1.0 (#17517) Update actions/setup-node action to v4.4.0 (#17514) [red-knot] Fix variable name (#17532) [red-knot] Add basic subtyping between class literal and callable (#17469) [`pyupgrade`] Add fix safety section to docs (`UP030`) (#17443) [`perflint`] Allow list function calls to be replaced with a comprehension (`PERF401`) (#17519) Update pre-commit dependencies (#17506) [red-knot] Simplify visibility constraint handling for `*`-import definitions (#17486) [red-knot] Detect (some) invalid protocols (#17488) [red-knot] Correctly identify protocol classes (#17487) Update dependency ruff to v0.11.6 (#17516) Update Rust crate shellexpand to v3.1.1 (#17512) ...
Summary
This covers step 1 from https://typing.python.org/en/latest/spec/constructors.html#converting-a-constructor-to-callable
Part of astral-sh/ty#129
Test Plan
Update is_subtype_of.md and is_assignable_to.md