[red-knot] Add support for overloaded functions#17366
Merged
dhruvmanila merged 17 commits intomainfrom Apr 18, 2025
Merged
Conversation
Contributor
|
dhruvmanila
force-pushed
the
dhruv/overloaded-signature
branch
from
f738bc2 to
cc3386d
Compare
dhruvmanila
force-pushed
the
dhruv/move-callable-type-relation
branch
from
56a3864 to
3fc5654
Compare
dhruvmanila
force-pushed
the
dhruv/overloaded-signature
branch
from
cc3386d to
72a3bc8
Compare
CodSpeed Performance ReportMerging #17366 will degrade performances by 7.47%Comparing Summary
Benchmarks breakdown
|
dhruvmanila
force-pushed
the
dhruv/overloaded-signature
branch
6 times, most recently
from
05e7b1e to
bfce12a
Compare
dhruvmanila
force-pushed
the
dhruv/overloaded-signature
branch
from
df5f44b to
4921acf
Compare
dhruvmanila
changed the title
dhruvmanila
commented
Apr 17, 2025
crates/red_knot_python_semantic/resources/mdtest/dataclasses.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/narrow/truthiness.md
Outdated
Show resolved
Hide resolved
Comment on lines
+116
to
+124
| FunctionSignature::Overloaded(signatures, _) => { | ||
| // TODO: How to display overloads? | ||
| f.write_str("Overload[")?; | ||
| let mut join = f.join(", "); | ||
| for signature in signatures { | ||
| join.entry(&signature.display(self.db)); | ||
| } | ||
| f.write_str("]") | ||
| } |
Member
Author
There was a problem hiding this comment.
This is not the final way to display overloads, I went with it because this is not the main goal for this PR. I'll open a separate issue for this.
Comment on lines
291
to
+295
| pub(crate) fn apply_specialization( | ||
| &mut self, | ||
| &self, | ||
| db: &'db dyn Db, | ||
| specialization: Specialization<'db>, | ||
| ) { | ||
| self.parameters.apply_specialization(db, specialization); | ||
| self.return_ty = self | ||
| .return_ty | ||
| .map(|ty| ty.apply_specialization(db, specialization)); | ||
| ) -> Self { |
Member
Author
There was a problem hiding this comment.
I updated the apply_specialization methods for Signature, Parameters, Parameter to return Self so that it's easier to collect in CallableType::apply_specialization and it also matches what this method do on other types (cc @dcreager)
Contributor
There was a problem hiding this comment.
I noticed this discrepancy on the generics PR and then I vaguely recall thinking I had observed a reason for it, but I don't remember anymore what that reason was :) This change seems reasonable to me. (We were cloning anyway at the call site, so it doesn't seem to be motivated by efficiency.)
Hopefully @dcreager will weigh in if we are missing a reason not to do this.
dhruvmanila
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
4 tasks
sharkdp
reviewed
Apr 17, 2025
|
|
||
| # TODO: Should be `field: str | int = int` once we understand overloads | ||
| reveal_type(C.__init__) # revealed: (field: Unknown = int) -> None | ||
| reveal_type(C.__init__) # revealed: (field: str | int = int) -> None |
sharkdp
reviewed
Apr 17, 2025
|
|
||
| # TODO: should be `Literal["called on instance"] | ||
| reveal_type(C().d) # revealed: LiteralString | ||
| reveal_type(C().d) # revealed: Literal["called on instance"] |
Contributor
There was a problem hiding this comment.
Same here. So cool to see that this is possible now!
AlexWaygood
reviewed
Apr 17, 2025
AlexWaygood
reviewed
Apr 17, 2025
crates/red_knot_python_semantic/resources/mdtest/subscript/tuple.md
Outdated
Show resolved
Hide resolved
Contributor
|
@dhruvmanila I looked at the benchmark regression, and I think this is the explanation for it: Since #17419 we set a size limit on unions of literals. This means that a) this benchmark is now quite fast, therefore sensitive, and b) at a certain point in this benchmark, we fall back from a large union of literals to simply So I think that it's fine to go ahead and land this with the regression. The extra cost is directly related to the new correct functionality. |
Member
Author
Makes sense, thanks for looking into it. |
carljm
approved these changes
Apr 17, 2025
| reveal_type(1 + A()) # revealed: A | ||
|
|
||
| reveal_type(A() + "foo") # revealed: A | ||
| # TODO should be `A` since `str.__add__` doesn't support `A` instances |
Contributor
There was a problem hiding this comment.
So many longstanding TODOs just disappearing! Love it.
crates/red_knot_python_semantic/resources/mdtest/narrow/truthiness.md
Outdated
Show resolved
Hide resolved
| .name | ||
| .scoped_use_id(db, scope); | ||
|
|
||
| if let Symbol::Type(Type::FunctionLiteral(function_literal), Boundness::Bound) = |
Contributor
There was a problem hiding this comment.
We could insert a comment here
Suggested change
| if let Symbol::Type(Type::FunctionLiteral(function_literal), Boundness::Bound) = | |
| // TODO maybe someday: infer union of overloaded callables if we have a maybe-bound overload? | |
| if let Symbol::Type(Type::FunctionLiteral(function_literal), Boundness::Bound) = |
Member
Author
There was a problem hiding this comment.
Hmm, interesting. Would you be able to provide an example of where this might be possible?
Would it be something like this:
def _(flag: bool) -> None:
if flag:
@overload
def foo() -> None: ...
@overload
def foo(x: int) -> int: ...
# Here, the previous `foo` overloads are possibly-unbound
@overload
def foo(x: str) -> str: ...
@overload
def foo(x: bytes) -> bytes: ...
def foo(x: int | str | bytes | None = None) -> int | str | bytes | None:
return x
# Pyright and pyrefly includes all 4 overloads
# red knot and mypy only includes the 2 overloads outside the `if` branch
reveal_type(foo)This actually prompted me to add some test cases to include an overload in a union.
Comment on lines
+5875
to
+5915
| let scope = self.definition(db).scope(db); | ||
| let use_def = semantic_index(db, scope.file(db)).use_def_map(scope.file_scope_id(db)); | ||
| let use_id = self | ||
| .body_scope(db) | ||
| .node(db) | ||
| .expect_function() | ||
| .name | ||
| .scoped_use_id(db, scope); | ||
|
|
||
| if let Symbol::Type(Type::FunctionLiteral(function_literal), Boundness::Bound) = | ||
| symbol_from_bindings(db, use_def.bindings_at_use(use_id)) | ||
| { | ||
| match function_literal.signature(db) { | ||
| FunctionSignature::Single(_) => { | ||
| debug_assert!( | ||
| !function_literal.has_known_decorator(db, FunctionDecorators::OVERLOAD), | ||
| "Expected `FunctionSignature::Overloaded` if the previous function was an overload" | ||
| ); | ||
| } | ||
| FunctionSignature::Overloaded(_, Some(_)) => { | ||
| // If the previous overloaded function already has an implementation, then this | ||
| // new signature completely replaces it. | ||
| } | ||
| FunctionSignature::Overloaded(signatures, None) => { | ||
| return if self.has_known_decorator(db, FunctionDecorators::OVERLOAD) { | ||
| let mut signatures = signatures.clone(); | ||
| signatures.push(internal_signature); | ||
| FunctionSignature::Overloaded(signatures, None) | ||
| } else { | ||
| FunctionSignature::Overloaded(signatures.clone(), Some(internal_signature)) | ||
| }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This works out pretty smoothly!
Comment on lines
291
to
+295
| pub(crate) fn apply_specialization( | ||
| &mut self, | ||
| &self, | ||
| db: &'db dyn Db, | ||
| specialization: Specialization<'db>, | ||
| ) { | ||
| self.parameters.apply_specialization(db, specialization); | ||
| self.return_ty = self | ||
| .return_ty | ||
| .map(|ty| ty.apply_specialization(db, specialization)); | ||
| ) -> Self { |
Contributor
There was a problem hiding this comment.
I noticed this discrepancy on the generics PR and then I vaguely recall thinking I had observed a reason for it, but I don't remember anymore what that reason was :) This change seems reasonable to me. (We were cloning anyway at the call site, so it doesn't seem to be motivated by efficiency.)
Hopefully @dcreager will weigh in if we are missing a reason not to do this.
dhruvmanila
force-pushed
the
dhruv/overloaded-signature
branch
from
3c3633d to
b347ce0
Compare
Member
Author
|
I'm going to go ahead and merge this; most of the new ecosystem changes are in the similar category as documented in the PR description but will put up any follow-up issue if I find anything new and worth prioritizing. |
dcreager
added a commit
that referenced
this pull request
Apr 18, 2025
* 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)
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) ...
19 tasks
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
Part of #15383, this PR adds support for overloaded callables.
Typing spec: https://typing.python.org/en/latest/spec/overload.html
Specifically, it does the following:
FunctionType::signaturemethod to return signatures from a possibly overloaded callable using a newFunctionSignatureenumCallableTypeto accommodate overloaded callable by updating the inner type toBox<[Signature]>CallableTypewith logic specific to overloadsCallableTypeOfspecial form to recognize overloaded callableFor (2), it is required to be done in this PR because otherwise I'd need to add some workaround for
into_callable_typeand I though it would be best to include it in here.For (2), another possible design would be convert
CallableTypein an enum with two variantsCallableType::SingleandCallableType::Overloadbut I decided to go withBox<[Signature]>for now to (a) mirror it to be equivalent tooverloadfield onCallableSignatureand (b) to avoid any refactor in this PR. This could be done in a follow-up to better split the two kind of callables.Design
There were two main candidates on how to represent the overloaded definition:
FunctionType::signaturemethodOverloadtype variantFor context, this is what I had in mind with the new type variant:
The main reasons to choose (1) are the simplicity in the implementation, reusing the existing infrastructure, avoiding any complications that the new type variant has specifically around the different variants between function and methods which would require the overload type to use
Typeinstead.Implementation
The core logic is how to collect all the overloaded functions. The way this is done in this PR is by recording a use on the
Identifiernode that represents the function name in the use-def map. This is then used to fetch the previous symbol using the same name. This way the signatures are going to be propagated from top to bottom (from first overload to the final overload or the implementation) with each function / method. For example:Here, each definition of
fooknows about all the signatures that comes before itself. So, the first overload would only see itself, the second would see the first and itself and so on until the implementation or the final overload.This approach required some updates specifically recognizing
Identifiernode to record the function use because it doesn't useExprName.Test Plan
Update existing test cases which were limited by the overload support and add test cases for the following cases:
Ecosystem changes
WIP
After going through the ecosystem changes (there are a lot!), here's what I've found:
We need assignability check between a callable type and a class literal because a lot of builtins are defined as classes in typeshed whose constructor method is overloaded e.g.,
map,sorted,list.sort,max,minwith thekeyparameter,collections.abc.defaultdict, etc. (astral-sh/ty#129). This makes up most of the ecosystem diff roughly 70 diagnostics. For example:Duplicate diagnostics in unpacking (astral-sh/ty#185) has ~16 diagnostics.
Support for the
callablebuiltin which requiresTypeIssupport. This is 5 diagnostics. For example:Narrowing on
assertwhich has 11 diagnostics. This is being worked on in #17345. For example:Others:
Self: 2Performance
Refer to #17366 (comment).