-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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] 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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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
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.
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_target
which 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 ofa
will beLiteral[1]
and the type ofb
will beLiteral[2]
.There are a couple of follow-ups that can be done:
for
loopTest Plan
Add various test cases using the new markdown test framework.
Validate that existing test cases pass.