-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Understand typing.Callable
#16493
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
e71d87b to
dbae500
Compare
dbae500 to
05a7a5b
Compare
|
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.
Very cool! Great work.
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Outdated
Show resolved
Hide resolved
Post review changes
|
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 good!
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/invalid_syntax.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Outdated
Show resolved
Hide resolved
* main: [red-knot] Understand `typing.Callable` (#16493) [red-knot] Support unpacking `with` target (#16469) [red-knot] Attribute access and the descriptor protocol (#16416) [`pep8-naming`] Add links to `ignore-names` options in various rules' documentation (#16557) [red-knot] avoid inferring types if unpacking fails (#16530)
AlexWaygood
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.
two tiny nits I spotted that probably don't warrant their own PR, but could maybe be addressed if you're doing further followup work on Callable types
| pub(crate) fn is_keyword_only(&self) -> bool { | ||
| matches!(self.kind, ParameterKind::KeywordOnly { .. }) | ||
| } | ||
|
|
||
| pub(crate) fn is_positional_only(&self) -> bool { |
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.
nit
| pub(crate) fn is_keyword_only(&self) -> bool { | |
| matches!(self.kind, ParameterKind::KeywordOnly { .. }) | |
| } | |
| pub(crate) fn is_positional_only(&self) -> bool { | |
| pub(crate) const fn is_keyword_only(&self) -> bool { | |
| matches!(self.kind, ParameterKind::KeywordOnly { .. }) | |
| } | |
| pub(crate) const fn is_positional_only(&self) -> bool { |
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 might be wrong but I'm not sure if this is going to make any difference as we don't use any of these functions in the const context and I don't know we'll ever be able to (?) because parameters are always going to be dynamic i.e., we cannot statically determine during compile time whether a parameter is positional or not.
## Summary This PR is a follow-up to #16493 that implements member lookup for the general callable type. Based on the discussion around [member lookup here](#16493 (comment)) and [`.to_meta_type()` here](#16493 (comment)). ## Test Plan Add a new test cases.
## Summary
Currently this assertion fails on `main`, because we do not synthesize a
`__call__` attribute for Callable types:
```py
from typing import Protocol, Callable
from knot_extensions import static_assert, is_assignable_to
class Foo(Protocol):
def __call__(self, x: int, /) -> str: ...
static_assert(is_assignable_to(Callable[[int], str], Foo))
```
This PR fixes that.
See previous discussion about this in
#16493 (comment) and
#17682 (comment)
## Test Plan
Existing mdtests updated; a couple of new ones added.
Summary
Part of #15382
This PR implements a general callable type that wraps around a
Signatureand it uses that new type to representtyping.Callable.It also implements
Displaysupport forCallable. The format is as:The
/and*separators are added at the correct boundary for positional-only and keyword-only parameters. Now, astyping.Callableonly has positional-only parameters, the rendered signature would be:The
/separator represents that all the arguments are positional-only.The relationship methods that check assignability, subtype relationship, etc. are not yet implemented and will be done so as a follow-up.
Test Plan
Add test cases for display support for
Signatureand various mdtest fortyping.Callable.