[red-knot] Add support for unpacking for target
#15058
Conversation
dhruvmanila
changed the title
for target
dhruvmanila
force-pushed
the
dhruv/unpack-union
branch
3 times, most recently
from
15d1613 to
9292856
Compare
dhruvmanila
force-pushed
the
dhruv/unpack-for-target
branch
from
8b38f74 to
c9799b6
Compare
|
dhruvmanila
force-pushed
the
dhruv/unpack-for-target
branch
from
c9799b6 to
94b8bdc
Compare
dhruvmanila
force-pushed
the
dhruv/unpack-for-target
branch
3 times, most recently
from
7cd3be6 to
fe681d0
Compare
| reveal_type(b) # revealed: str | bytes | ||
| ``` | ||
|
|
||
| ## For statement |
There was a problem hiding this comment.
I've intentionally left out certain test cases, refer to the "Question" in PR description.
There was a problem hiding this comment.
I think the tests you have here look good!
|
|
||
| debug_assert_eq!(&self.current_assignments, &[]); | ||
| self.push_assignment(for_stmt.into()); | ||
| let current_assignment = match &**target { |
There was a problem hiding this comment.
This is basically the same logic as in the assignment statement, I'm going to wait until I implement unpacking support in other places to get an overview of what can be simplified to avoid repetition.
| let value = unpack.value(db); | ||
| let scope = unpack.scope(db); | ||
|
|
||
| let result = infer_expression_types(db, value); | ||
| let value_ty = result.expression_ty(value.node_ref(db).scoped_expression_id(db, scope)); | ||
|
|
There was a problem hiding this comment.
Inferring the value type is moved in the Unpacker
| // TODO | ||
| //self.visit_expr(&assign.value); | ||
| return; | ||
| } | ||
| Stmt::For(for_stmt) => { | ||
| self.visit_target(&for_stmt.target); | ||
| // TODO | ||
| //self.visit_expr(&for_stmt.iter); |
There was a problem hiding this comment.
This is actually a bug, calling visit_expr on the expression panics in the following case:
[] = *data
() = *dataThis is because in infer_target, we completely skip the inference when there are no elements in the list / tuple target.
I'm going to fix this in a follow-up.
dhruvmanila
requested review from
AlexWaygood,
MichaReiser,
carljm and
sharkdp
as code owners
| reveal_type(b) # revealed: str | bytes | ||
| ``` | ||
|
|
||
| ## For statement |
There was a problem hiding this comment.
I think the tests you have here look good!
dhruvmanila
force-pushed
the
dhruv/unpack-for-target
branch
from
fe681d0 to
6a116dc
Compare
## Summary This PR resolves #15058 (comment) by inferring the value expression even if there are no targets in the list / tuple expression. ## Test Plan Remove TODO from corpus tests, making sure it doesn't panic.
## Summary Resolves #16365 Add support for unpacking `with` statement targets. ## Test Plan Added some test cases, alike the ones added by #15058. --------- Co-authored-by: Carl Meyer <[email protected]>
Summary
Related to #13773
This PR adds support for unpacking
forstatement targets.This involves updating the
valuefield in theUnpacktarget to use an enum which specifies the "where did the value expression came from?". This is because for an iterable expression, we need to unpack the iterator type while for assignment statement we need to unpack the value type itself. And, this needs to be done in the unpack query.Question
One of the ways unpacking works in
forstatement is by looking at the union of the types because if the iterable expression is a tuple then the iterator type will be union of all the types in the tuple. This means that the test cases that will test the unpacking inforstatement will also implicitly test the unpacking union logic. I was wondering if it makes sense to merge these cases and only add the ones that are specific to the union unpacking or for statement unpacking logic.Test Plan
Add test cases involving iterating over a tuple type. I've intentionally left out certain cases for now and I'm curious to know any thoughts on the above query.