Skip to content
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] Follow-up from unpacked tuple assignment work #13773

Open
1 of 7 tasks
dhruvmanila opened this issue Oct 16, 2024 · 2 comments · Fixed by #14125 or #13979
Open
1 of 7 tasks

[red-knot] Follow-up from unpacked tuple assignment work #13773

dhruvmanila opened this issue Oct 16, 2024 · 2 comments · Fixed by #14125 or #13979
Assignees
Labels
red-knot Multi-file analysis & type inference
Milestone

Comments

@dhruvmanila
Copy link
Member

dhruvmanila commented Oct 16, 2024

As #13316 is merged, there are some follow-up work that needs to be done. The following is a list in the order of priority where the highest priority work is on the top:

  • Avoiding duplicate diagnostics. Considering (a, b) = (1, 2), as the inference runs for each symbol, if there are any diagnostics that's raised during that part (like "too many values to unpack"), then there will be multiple diagnostics added to the builder one for each symbol. Refer [red-knot] Infer target types for unpacked tuple assignment #13316 (comment)
  • Use the unpacking logic in other places where it's relevant like
    • for loops e.g., for (x, y) in ((1, 2), (3, 4)): ...
    • with statements e.g., with (item1, item2) as (x, y): ...
    • Comprehensions e.g., [_ for (x, y) in [(1, 2), (3, 4)]]
  • Combine starred element types. For example, in (a, *b, c) = (1, 2, 3, 4), the Literal[2] and Literal[3] type should be combined into list[int] to be assigned to *b. (Requires generic support over list)
    // TODO: Combine the types into a list type. If the
    // starred_element_types is empty, then it should be `List[Any]`.
    // combine_types(starred_element_types);
    element_types.push(Type::Todo);
  • Additional diagnostics when the number of targets is not equal to the number of elements on the RHS
@dhruvmanila dhruvmanila added the red-knot Multi-file analysis & type inference label Oct 16, 2024
@dhruvmanila dhruvmanila self-assigned this Oct 24, 2024
dhruvmanila added a commit that referenced this issue Nov 4, 2024
## Summary

This PR adds a new salsa query and an ingredient to resolve all the
variables involved in an unpacking assignment like `(a, b) = (1, 2)` at
once. Previously, we'd recursively try to match the correct type for
each definition individually which will result in creating duplicate
diagnostics.

This PR still doesn't solve the duplicate diagnostics issue because that
requires a different solution like using salsa accumulator or
de-duplicating the diagnostics manually.

Related: #13773 

## Test Plan

Make sure that all unpack assignment test cases pass, there are no
panics in the corpus tests.

## Todo

- [x] Look at the performance regression
@carljm carljm added this to the Red Knot 2024 milestone Nov 7, 2024
@carljm carljm changed the title Follow-up from unpacked tuple assignment work [red-knot] Follow-up from unpacked tuple assignment work Nov 7, 2024
@dhruvmanila
Copy link
Member Author

Regarding "Use the unpacking logic in other places where it's relevant like" task, I think the challenge here would be to implement the logic to "combine" union types into a single type. For example, in a for loop like:

for (x, y) in ((1, 2), (3, 4), (5, 6)): ...

The inferred type for the iterable would be:

tuple[Literal[1], Literal[2]] | tuple[Literal[3], Literal[4]] | tuple[Literal[5], Literal[6]]

But, we should combine it to:

tuple[int, int]
# or
tuple[Literal[1, 3, 5], Literal[2, 4, 6]]

Or, another example where the types are "mixed":

for (x, y) in ((1, 2), ("a", "b")): ...

We should combine it to:

tuple[int | str, int | str]
# or
tuple[Literal[1, "a"], Literal[2, "b"]]

Another example:

for (x, y) in ((1, 2), ("a", 3)): ...

where,

tuple[int | str, int]
# or
tuple[Literal[1, "a"], Literal[2, 3]]

@dhruvmanila
Copy link
Member Author

Actually, my previous comment isn't exactly correct. We actually need to add support for union types when unpacking rather than combining the types.

For example, if the type is tuple[int, str] | list[int] and the target has two elements (a, b), then the type of a will be a union of the first tuple type (int) and the iterable type of the list (int) which should be int. And, similarly the type of b would be a union of the second tuple type (str) and the iterable type of the list (int) which would be int | str.

cc @carljm as we talked about this in our 1:1 yesterday.

dhruvmanila added a commit that referenced this issue Dec 20, 2024
## Summary

Refer:
#13773 (comment)

This PR adds support for unpacking union types. 

Unpacking a union type requires us to first distribute the types for all
the targets that are involved in an unpacking. For example, if there are
two targets and a union type that needs to be unpacked, each target will
get a type from each element in the union type.

For example, if the type is `tuple[int, int] | tuple[int, str]` and the
target has two elements `(a, b)`, then
* The type of `a` will be a union of `int` and `int` which are at index
0 in the first and second tuple respectively which resolves to an `int`.
* Similarly, the type of `b` will be a union of `int` and `str` which
are at index 1 in the first and second tuple respectively which will be
`int | str`.

### Refactors

There are couple of refactors that are added in this PR:
* Add a `debug_assertion` to validate that the unpack target is a list
or a tuple
* Add a separate method to handle starred expression

## Test Plan

Update `unpacking.md` with additional test cases that uses union types.
This is done using parameter type hints style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
2 participants