[ty] Splat variadic arguments into parameter list #18996
Merged
Conversation
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
dcreager
added a commit
that referenced
this pull request
Jun 27, 2025
This PR extracts a lot of the complex logic in the `match_parameters` and `check_types` methods of our call binding machinery into separate helper types. This is setup for #18996, which will update this logic to handle variadic arguments. To do so, it is helpful to have the per-argument logic extracted into a method that we can call repeatedly for each _element_ of a variadic argument. This should be a pure refactoring, with no behavioral changes.
CodSpeed WallTime Performance ReportMerging #18996 will not alter performanceComparing Summary
|
CodSpeed Instrumentation Performance ReportMerging #18996 will not alter performanceComparing Summary
|
Contributor
|
dcreager
commented
Jun 27, 2025
Comment on lines
236
to
237
| For now, we have a workaround that pads out the splatted value with `Unknown` when we encounter this | ||
| case, but a proper fix would retry parameter matching for each expanded union element. |
Member
There was a problem hiding this comment.
Interesting. The spec does say to loop over from step 2 which is the type checking step.
I've opened astral-sh/ty#735 to keep track of it.
dcreager
changed the title
carljm
approved these changes
Jun 28, 2025
dhruvmanila
approved these changes
Jul 1, 2025
Comment on lines
236
to
237
| For now, we have a workaround that pads out the splatted value with `Unknown` when we encounter this | ||
| case, but a proper fix would retry parameter matching for each expanded union element. |
Member
There was a problem hiding this comment.
Interesting. The spec does say to loop over from step 2 which is the type checking step.
I've opened astral-sh/ty#735 to keep track of it.
Comment on lines
1437
to
1435
| let Some(parameter_index) = overload.argument_parameters[argument_index] else { | ||
| // There is no parameter for this argument in this overload. | ||
| continue; | ||
| }; | ||
| if !participating_parameter_indexes.contains(¶meter_index) { | ||
| // This parameter doesn't participate in the filtering process. | ||
| continue; | ||
| for parameter_index in &overload.argument_parameters[argument_index] { | ||
| if !participating_parameter_indexes.contains(parameter_index) { | ||
| // This parameter doesn't participate in the filtering process. | ||
| continue; | ||
| } | ||
| // TODO: For an unannotated `self` / `cls` parameter, the type should be | ||
| // `typing.Self` / `type[typing.Self]` | ||
| let parameter_type = overload.signature.parameters()[*parameter_index] | ||
| .annotated_type() | ||
| .unwrap_or(Type::unknown()); | ||
| current_parameter_types.push(parameter_type); | ||
| } | ||
| // TODO: For an unannotated `self` / `cls` parameter, the type should be | ||
| // `typing.Self` / `type[typing.Self]` | ||
| let parameter_type = overload.signature.parameters()[parameter_index] | ||
| .annotated_type() | ||
| .unwrap_or(Type::unknown()); | ||
| current_parameter_types.push(parameter_type); |
Member
There was a problem hiding this comment.
I think it'd be useful to add a test case for this new scenario where the step 5 needs to be performed for a variadic argument against some number of parameters. These would probably go in call/overloads.md file.
A few examples that I can think of:
from typing import Any, overload
class A: ...
class B: ...
@overload
def f1(x: int) -> A: ...
@overload
def f1(x: Any, y: Any) -> A: ...
@overload
def f2(x: int) -> A: ...
@overload
def f2(x: Any, y: Any) -> B: ...
@overload
def f3(x: int) -> A: ...
@overload
def f3(x: Any, y: Any) -> A: ...
@overload
def f3(x: Any, y: Any, *, z: str) -> B: ...
@overload
def f4(x: int) -> A: ...
@overload
def f4(x: Any, y: Any) -> B: ...
@overload
def f4(x: Any, y: Any, *, z: str) -> B: ...
def _(arg: list[Any]):
# Matches both overload and the return types are equivalent
reveal_type(f1(*arg)) # revealed: A
# Matches both overload but the return types aren't equivalent
reveal_type(f2(*arg)) # revealed: Unknown
# Filters out the final overload and the return types are equivalent
reveal_type(f3(*arg)) # revealed: A
# Filters out the final overload but the return types aren't equivalent
reveal_type(f4(*arg)) # revealed: Unknown| let Some(parameter_index) = overload.argument_parameters[argument_index] else { | ||
| // There is no parameter for this argument in this overload. | ||
| break; | ||
| }; | ||
| // TODO: For an unannotated `self` / `cls` parameter, the type should be | ||
| // `typing.Self` / `type[typing.Self]` | ||
| let current_parameter_type = overload.signature.parameters()[parameter_index] | ||
| .annotated_type() | ||
| .unwrap_or(Type::unknown()); | ||
| if let Some(first_parameter_type) = first_parameter_type { | ||
| if !first_parameter_type.is_equivalent_to(db, current_parameter_type) { | ||
| participating_parameter_index = Some(parameter_index); | ||
| break; | ||
| for parameter_index in &overload.argument_parameters[argument_index] { | ||
| // TODO: For an unannotated `self` / `cls` parameter, the type should be | ||
| // `typing.Self` / `type[typing.Self]` | ||
| let current_parameter_type = overload.signature.parameters()[*parameter_index] | ||
| .annotated_type() | ||
| .unwrap_or(Type::unknown()); | ||
| if let Some(first_parameter_type) = first_parameter_type { | ||
| if !first_parameter_type.is_equivalent_to(db, current_parameter_type) { | ||
| participating_parameter_index = Some(*parameter_index); | ||
| break; | ||
| } | ||
| } else { | ||
| first_parameter_type = Some(current_parameter_type); | ||
| } | ||
| } else { | ||
| first_parameter_type = Some(current_parameter_type); |
Member
There was a problem hiding this comment.
Similar to my other comment, it'd be useful to have a test case for this new scenario as well where one of the parameter is not being considered for the filtering process.
We can use the existing test case as a base to create this new test case.
Member
Author
There was a problem hiding this comment.
I think I added what you're suggesting; let me know if you had something else in mind. (Note that the test currently fails, but I'd like to dig into that as a follow-on PR)
Member
There was a problem hiding this comment.
You can try looking at what the top_materialized_argument_types is which I think for the test case should be the top materialized types for Any and Literal[True] so, vec![object, Literal[True]] which basically gets merged back into tuple[object, Literal[True]].
* main: [`pylint`] Extend invalid string character rules to include t-strings (#19355) Make TC010 docs example more realistic (#19356) Move RDJSON rendering to `ruff_db` (#19293) [`flake8-use-pathlib`] Skip single dots for `invalid-pathlib-with-suffix` (`PTH210`) on versions >= 3.14 (#19331) [`ruff`] Allow `strict` kwarg when checking for `starmap-zip` (`RUF058`) in Python 3.14+ (#19333) [ty] Reduce false positives for `TypedDict` types (#19354) [ty] Remove `ConnectionInitializer` (#19353) [ty] Use `Type::string_literal()` more (#19352) [ty] Add ecosystem-report workflow (#19349) [ty] Make use of salsa `Lookup` when interning values (#19347) [ty] Sync vendored typeshed stubs (#19345) [`pylint`] Make example error out-of-the-box (`PLE2502`) (#19272) [`pydoclint`] Fix `SyntaxError` from fixes with line continuations (`D201`, `D202`) (#19246)
dcreager
added a commit
that referenced
this pull request
Jul 15, 2025
We previously had separate `CallArguments` and `CallArgumentTypes` types in support of our two-phase call binding logic. `CallArguments` would store only the arity/kind of each argument (positional, keyword, variadic, etc). We then performed parameter matching using only this arity/kind information, and then infered the type of each argument, placing the result of this second phase into a new `CallArgumentTypes`. In #18996, we will need to infer the types of splatted arguments _before_ performing parameter matching, since we need to know the argument type to accurately infer its length, which informs how many parameters the splatted argument is matched against. That makes this separation of Rust types no longer useful. This PR merges everything back into a single `CallArguments`. In the case where we are performing two-phase call binding, the types will be initialized to `None`, and updated to the actual argument type during the second `check_types` phase. _[This is a refactoring in support of fixing the merge conflicts on #18996. I've pulled this out into a separate PR to make it easier to review in isolation.]_
* dcreager/merge-arguments: add types iterator add asserting constructor debug assert lengths remove unused From use FromIterator [`pylint`] Extend invalid string character rules to include t-strings (#19355) Make TC010 docs example more realistic (#19356) Move RDJSON rendering to `ruff_db` (#19293) [`flake8-use-pathlib`] Skip single dots for `invalid-pathlib-with-suffix` (`PTH210`) on versions >= 3.14 (#19331) [`ruff`] Allow `strict` kwarg when checking for `starmap-zip` (`RUF058`) in Python 3.14+ (#19333) [ty] Reduce false positives for `TypedDict` types (#19354) [ty] Remove `ConnectionInitializer` (#19353) [ty] Use `Type::string_literal()` more (#19352) [ty] Add ecosystem-report workflow (#19349) [ty] Make use of salsa `Lookup` when interning values (#19347) [ty] Sync vendored typeshed stubs (#19345) [`pylint`] Make example error out-of-the-box (`PLE2502`) (#19272) [`pydoclint`] Fix `SyntaxError` from fixes with line continuations (`D201`, `D202`) (#19246)
Member
Author
|
I think I've covered all of the requested new test cases. @dhruvmanila, can you spot-check what I've added for overloads? |
dhruvmanila
approved these changes
Jul 16, 2025
Member
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
The tests looks great, thank you for adding them.
|
|
||
| ## Filtering overloads with variadic arguments and parameters | ||
|
|
||
| TODO |
Member
There was a problem hiding this comment.
I think this TODO is still remaining to resolve
* main: (76 commits) Move fix suggestion to subdiagnostic (#19464) [ty] Implement non-stdlib stub mapping for classes and functions (#19471) [ty] Disallow illegal uses of `ClassVar` (#19483) [ty] Disallow `Final` in function parameter/return-type annotations (#19480) [ty] Extend `Final` test suite (#19476) [ty] Minor change to diagnostic message for invalid Literal uses (#19482) [ty] Detect illegal non-enum attribute accesses in Literal annotation (#19477) [ty] Reduce size of `TypeInference` (#19435) Run MD tests for Markdown-only changes (#19479) Revert "[ty] Detect illegal non-enum attribute accesses in Literal annotation" [ty] Detect illegal non-enum attribute accesses in Literal annotation [ty] Added semantic token support for more identifiers (#19473) [ty] Make tuple subclass constructors sound (#19469) [ty] Pass down specialization to generic dataclass bases (#19472) [ty] Garbage-collect reachability constraints (#19414) [ty] Implicit instance attributes declared `Final` (#19462) [ty] Expansion of enums into unions of literals (#19382) [ty] Avoid rechecking the entire project when changing the opened files (#19463) [ty] Add warning for unknown `TY_MEMORY_REPORT` value (#19465) [ty] Sync vendored typeshed stubs (#19461) ...
dcreager
added a commit
that referenced
this pull request
Jul 22, 2025
* main: [ty] Use `ThinVec` for sub segments in `PlaceExpr` (#19470) [ty] Splat variadic arguments into parameter list (#18996) [`flake8-pyi`] Skip fix if all `Union` members are `None` (`PYI016`) (#19416) Skip notebook with errors in ecosystem check (#19491) [ty] Consistent use of American english (in rules) (#19488) [ty] Support iterating over enums (#19486) Fix panic for illegal `Literal[…]` annotations with inner subscript expressions (#19489) Move fix suggestion to subdiagnostic (#19464) [ty] Implement non-stdlib stub mapping for classes and functions (#19471) [ty] Disallow illegal uses of `ClassVar` (#19483) [ty] Disallow `Final` in function parameter/return-type annotations (#19480) [ty] Extend `Final` test suite (#19476) [ty] Minor change to diagnostic message for invalid Literal uses (#19482) [ty] Detect illegal non-enum attribute accesses in Literal annotation (#19477) [ty] Reduce size of `TypeInference` (#19435) Run MD tests for Markdown-only changes (#19479) Revert "[ty] Detect illegal non-enum attribute accesses in Literal annotation" [ty] Detect illegal non-enum attribute accesses in Literal annotation [ty] Added semantic token support for more identifiers (#19473) [ty] Make tuple subclass constructors sound (#19469)
UnboundVariable
pushed a commit
to UnboundVariable/ruff
that referenced
this pull request
Jul 23, 2025
* main: (28 commits) [ty] highlight the argument in `static_assert` error messages (astral-sh#19426) [ty] Infer single-valuedness for enums based on `int`/`str` (astral-sh#19510) [ty] Restructure submodule query around `File` dependency [ty] Make `Module` a Salsa ingredient [ty] Reachability analysis for `isinstance(…)` branches (astral-sh#19503) [ty] Normalize single-member enums to their instance type (astral-sh#19502) [ty] Invert `ty_ide` and `ty_project` dependency (astral-sh#19501) [ty] Implement mock language server for testing (astral-sh#19391) [ty] Detect enums if metaclass is a subtype of EnumType/EnumMeta (astral-sh#19481) [ty] perform type narrowing for places marked `global` too (astral-sh#19381) [ty] Use `ThinVec` for sub segments in `PlaceExpr` (astral-sh#19470) [ty] Splat variadic arguments into parameter list (astral-sh#18996) [`flake8-pyi`] Skip fix if all `Union` members are `None` (`PYI016`) (astral-sh#19416) Skip notebook with errors in ecosystem check (astral-sh#19491) [ty] Consistent use of American english (in rules) (astral-sh#19488) [ty] Support iterating over enums (astral-sh#19486) Fix panic for illegal `Literal[…]` annotations with inner subscript expressions (astral-sh#19489) Move fix suggestion to subdiagnostic (astral-sh#19464) [ty] Implement non-stdlib stub mapping for classes and functions (astral-sh#19471) [ty] Disallow illegal uses of `ClassVar` (astral-sh#19483) ... # Conflicts: # crates/ty_ide/src/goto.rs
This was referenced Jul 24, 2025
AlexWaygood
pushed a commit
that referenced
this pull request
Jul 25, 2025
This PR updates our call binding logic to handle splatted arguments. Complicating matters is that we have separated call bind analysis into two phases: parameter matching and type checking. Parameter matching looks at the arity of the function signature and call site, and assigns arguments to parameters. Importantly, we don't yet know the type of each argument! This is needed so that we can decide whether to infer the type of each argument as a type form or value form, depending on the requirements of the parameter that the argument was matched to. This is an issue when splatting an argument, since we need to know how many elements the splatted argument contains to know how many positional parameters to match it against. And to know how many elements the splatted argument has, we need to know its type. To get around this, we now make the assumption that splatted arguments can only be used with value-form parameters. (If you end up splatting an argument into a type-form parameter, we will silently pass in its value-form type instead.) That allows us to preemptively infer the (value-form) type of any splatted argument, so that we have its arity available during parameter matching. We defer inference of non-splatted arguments until after parameter matching has finished, as before. We reuse a lot of the new tuple machinery to make this happen — in particular resizing the tuple spec representing the number of arguments passed in with the tuple length representing the number of parameters the splat was matched with. This work also shows that we might need to change how we are performing argument expansion during overload resolution. At the moment, when we expand parameters, we assume that each argument will still be matched to the same parameters as before, and only retry the type-checking phase. With splatted arguments, this is no longer the case, since the inferred arity of each union element might be different than the arity of the union as a whole, which can affect how many parameters the splatted argument is matched to. See the regression test case in `mdtest/call/function.md` for more details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR updates our call binding logic to handle splatted arguments.
Complicating matters is that we have separated call bind analysis into two phases: parameter matching and type checking. Parameter matching looks at the arity of the function signature and call site, and assigns arguments to parameters. Importantly, we don't yet know the type of each argument! This is needed so that we can decide whether to infer the type of each argument as a type form or value form, depending on the requirements of the parameter that the argument was matched to.
This is an issue when splatting an argument, since we need to know how many elements the splatted argument contains to know how many positional parameters to match it against. And to know how many elements the splatted argument has, we need to know its type.
To get around this, we now make the assumption that splatted arguments can only be used with value-form parameters. (If you end up splatting an argument into a type-form parameter, we will silently pass in its value-form type instead.) That allows us to preemptively infer the (value-form) type of any splatted argument, so that we have its arity available during parameter matching. We defer inference of non-splatted arguments until after parameter matching has finished, as before.
We reuse a lot of the new tuple machinery to make this happen — in particular resizing the tuple spec representing the number of arguments passed in with the tuple length representing the number of parameters the splat was matched with.
This work also shows that we might need to change how we are performing argument expansion during overload resolution. At the moment, when we expand parameters, we assume that each argument will still be matched to the same parameters as before, and only retry the type-checking phase. With splatted arguments, this is no longer the case, since the inferred arity of each union element might be different than the arity of the union as a whole, which can affect how many parameters the splatted argument is matched to. See the regression test case in
mdtest/call/function.mdfor more details.