[red-knot] Infer target types for unpacked tuple assignment#13316
[red-knot] Infer target types for unpacked tuple assignment#13316
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
9ea561f to
c66a85d
Compare
|
c66a85d to
04818bc
Compare
This comment was marked as resolved.
This comment was marked as resolved.
925889e to
c8dbb21
Compare
carljm
left a comment
There was a problem hiding this comment.
This is great! A few issues, but seems quite close. Make sure for any bugs you fix in updating the PR, you also add a new test that would fail without that fix.
| TODO: Remove duplicate diagnostics. This is happening because for a sequence-like | ||
| assignment target, multiple definitions are created and the inference engine runs | ||
| on each of them which results in duplicate diagnostics. |
There was a problem hiding this comment.
Fixing this will require creating a new Salsa tracked struct for unpacking assignments to ensure we just do the unpacking once, correct?
I don't think I realized in our previous conversations that this would not just be a performance issue, but also a matter of diagnostics correctness. I think this raises the priority on a follow-up PR to create an Unpack tracked struct so we can Salsa-cache the unpacking and do it just once. (There are other unpacking cases we'll need to handle -- for loops and comprehensions -- which is why this shouldn't have an assignment-specific name.)
There was a problem hiding this comment.
Yeah, that's correct, we'd need a way to do unpacking once and cache it for future lookup. I can take this up as a follow-up at a high priority, will need to think about the required changes.
d1e492a to
b158bc5
Compare
|
@carljm Thanks for the great review, I've addressed all of the feedback except for the duplicate diagnostic issue (#13316 (comment)) which I'll fix as a follow-up. |
| ### Starred expression (5) | ||
|
|
||
| ```py | ||
| # TODO: Remove 'not-iterable' diagnostic | ||
| [a, b, *c] = (1, 2, 3, 4) # error: "Object of type `None` is not iterable" | ||
| reveal_type(a) # revealed: Literal[1] | ||
| reveal_type(b) # revealed: Literal[2] | ||
| # TODO: Should be List[int] once support for assigning to starred expression is added | ||
| reveal_type(c) # revealed: @Todo | ||
| ``` |
There was a problem hiding this comment.
The new test case where the starred expression isn't at the same position as other test cases.
| reveal_type(b) # revealed: LiteralString | ||
| ``` | ||
|
|
||
| ### Starred expression (1) |
There was a problem hiding this comment.
These test cases are similar to that of the Tuple type above and it only differs such that we use strings and the correct revealed type.
carljm
left a comment
There was a problem hiding this comment.
Changes look great! I noticed a few more things in reviewing just now, sorry I didn't catch these yesterday!
crates/red_knot_python_semantic/src/semantic_index/definition.rs
Outdated
Show resolved
Hide resolved
AlexWaygood
left a comment
There was a problem hiding this comment.
Nice -- this is complex stuff to get right!! A couple of small points on top of Micha's and Carl's comments:
|
Given it's quite late now for @dhruvmanila , I'm going to rebase this, apply my one suggested change, and go ahead and merge, to reduce our number of outstanding PRs and thus potential conflicts! |
b905919 to
efac63d
Compare
|
Thanks @carljm for the reviews and merging! |
Summary
This PR adds support for unpacking tuple expression in an assignment statement where the target expression can be a tuple or a list (the allowed sequence targets).
The implementation introduces a new
infer_assignment_targetwhich can then be used for other targets like the ones in for loops as well. This delegates it to theinfer_definition. The final implementation uses a recursive function that visits the target expression in source order and compares the variable node that corresponds to the definition. At the same time, it keeps track of where it is on the assignment value type.The logic also accounts for the number of elements on both sides such that it matches even if there's a gap in between. For example, if there's a starred expression like
(a, *b, c) = (1, 2, 3), then the type ofawill beLiteral[1]and the type ofbwill beLiteral[2].There are a couple of follow-ups that can be done:
forloopTest Plan
Add various test cases using the new markdown test framework.
Validate that existing test cases pass.