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] Infer target types for unpacked tuple assignment #13316

Merged
merged 14 commits into from
Oct 15, 2024
Merged
273 changes: 273 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/unpacking.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,273 @@
# Unpacking

## Tuple

### Simple tuple

```py
(a, b, c) = (1, 2, 3)
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: Literal[2]
reveal_type(c) # revealed: Literal[3]
```

### Simple list

```py
[a, b, c] = (1, 2, 3)
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: Literal[2]
reveal_type(c) # revealed: Literal[3]
```

### Simple mixed

```py
[a, (b, c), d] = (1, (2, 3), 4)
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: Literal[2]
reveal_type(c) # revealed: Literal[3]
reveal_type(d) # revealed: Literal[4]
```

### Multiple assignment

```py
a, b = c = 1, 2
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: Literal[2]
reveal_type(c) # revealed: tuple[Literal[1], Literal[2]]
```

### Nested tuple with unpacking

```py
(a, (b, c), d) = (1, (2, 3), 4)
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: Literal[2]
reveal_type(c) # revealed: Literal[3]
reveal_type(d) # revealed: Literal[4]
```

### Nested tuple without unpacking

```py
(a, b, c) = (1, (2, 3), 4)
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: tuple[Literal[2], Literal[3]]
reveal_type(c) # revealed: Literal[4]
```

### Uneven unpacking (1)

```py
# TODO: Add diagnostic (there aren't enough values to unpack)
(a, b, c) = (1, 2)
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: Literal[2]
reveal_type(c) # revealed: Unknown
```

### Uneven unpacking (2)

```py
# TODO: Add diagnostic (too many values to unpack)
(a, b) = (1, 2, 3)
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: Literal[2]
```

### Starred expression (1)

```py
# TODO: Add diagnostic (need more values to unpack)
# TODO: Remove 'not-iterable' diagnostic
[a, *b, c, d] = (1, 2) # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: Literal[1]
# TODO: Should be list[Any] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: Literal[2]
reveal_type(d) # revealed: Unknown
```

### Starred expression (2)

```py
[a, *b, c] = (1, 2) # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: Literal[1]
# TODO: Should be list[Any] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: Literal[2]
```

### Starred expression (3)

```py
# TODO: Remove 'not-iterable' diagnostic
[a, *b, c] = (1, 2, 3) # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: Literal[1]
# TODO: Should be list[int] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: Literal[3]
```

### Starred expression (4)

```py
# TODO: Remove 'not-iterable' diagnostic
[a, *b, c, d] = (1, 2, 3, 4, 5, 6) # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: Literal[1]
# TODO: Should be list[int] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: Literal[5]
reveal_type(d) # revealed: Literal[6]
```

### 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
```

### Non-iterable unpacking

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.
Comment on lines +139 to +141
Copy link
Contributor

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.)

Copy link
Member Author

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.


```py
# error: "Object of type `Literal[1]` is not iterable"
# error: "Object of type `Literal[1]` is not iterable"
a, b = 1
reveal_type(a) # revealed: Unknown
reveal_type(b) # revealed: Unknown
```

### Custom iterator unpacking

```py
class Iterator:
def __next__(self) -> int:
return 42


class Iterable:
def __iter__(self) -> Iterator:
return Iterator()


(a, b) = Iterable()
reveal_type(a) # revealed: int
reveal_type(b) # revealed: int
```

### Custom iterator unpacking nested

```py
class Iterator:
def __next__(self) -> int:
return 42


class Iterable:
def __iter__(self) -> Iterator:
return Iterator()


(a, (b, c), d) = (1, Iterable(), 2)
reveal_type(a) # revealed: Literal[1]
reveal_type(b) # revealed: int
reveal_type(c) # revealed: int
reveal_type(d) # revealed: Literal[2]
```

## String

### Simple unpacking

```py
a, b = 'ab'
reveal_type(a) # revealed: LiteralString
reveal_type(b) # revealed: LiteralString
```
carljm marked this conversation as resolved.
Show resolved Hide resolved

### Uneven unpacking (1)

```py
# TODO: Add diagnostic (there aren't enough values to unpack)
a, b, c = 'ab'
reveal_type(a) # revealed: LiteralString
reveal_type(b) # revealed: LiteralString
reveal_type(c) # revealed: Unknown
```

### Uneven unpacking (2)

```py
# TODO: Add diagnostic (too many values to unpack)
a, b = 'abc'
reveal_type(a) # revealed: LiteralString
reveal_type(b) # revealed: LiteralString
```

### Starred expression (1)
Copy link
Member Author

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.


```py
# TODO: Add diagnostic (need more values to unpack)
# TODO: Remove 'not-iterable' diagnostic
(a, *b, c, d) = "ab" # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: LiteralString
# TODO: Should be list[LiteralString] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: LiteralString
reveal_type(d) # revealed: Unknown
```

### Starred expression (2)

```py
(a, *b, c) = "ab" # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: LiteralString
# TODO: Should be list[Any] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: LiteralString
```

### Starred expression (3)

```py
# TODO: Remove 'not-iterable' diagnostic
(a, *b, c) = "abc" # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: LiteralString
# TODO: Should be list[LiteralString] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: LiteralString
```

### Starred expression (4)

```py
# TODO: Remove 'not-iterable' diagnostic
(a, *b, c, d) = "abcdef" # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: LiteralString
# TODO: Should be list[LiteralString] once support for assigning to starred expression is added
reveal_type(b) # revealed: @Todo
reveal_type(c) # revealed: LiteralString
reveal_type(d) # revealed: LiteralString
```

### Starred expression (5)

```py
# TODO: Remove 'not-iterable' diagnostic
(a, b, *c) = "abcd" # error: "Object of type `None` is not iterable"
reveal_type(a) # revealed: LiteralString
reveal_type(b) # revealed: LiteralString
# TODO: Should be list[int] once support for assigning to starred expression is added
reveal_type(c) # revealed: @Todo
```
2 changes: 1 addition & 1 deletion crates/red_knot_python_semantic/src/semantic_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ class C[T]:
let ast::Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(num),
..
}) = &*assignment.assignment().value
}) = assignment.value()
else {
panic!("should be a number literal")
};
Expand Down
43 changes: 29 additions & 14 deletions crates/red_knot_python_semantic/src/semantic_index/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ use crate::Db;

use super::constraint::{Constraint, PatternConstraint};
use super::definition::{
DefinitionCategory, ExceptHandlerDefinitionNodeRef, MatchPatternDefinitionNodeRef,
WithItemDefinitionNodeRef,
AssignmentKind, DefinitionCategory, ExceptHandlerDefinitionNodeRef,
MatchPatternDefinitionNodeRef, WithItemDefinitionNodeRef,
};

pub(super) struct SemanticIndexBuilder<'db> {
Expand Down Expand Up @@ -566,11 +566,22 @@ where
debug_assert!(self.current_assignment.is_none());
self.visit_expr(&node.value);
self.add_standalone_expression(&node.value);
self.current_assignment = Some(node.into());
for target in &node.targets {
for (target_index, target) in node.targets.iter().enumerate() {
let kind = match target {
ast::Expr::List(_) | ast::Expr::Tuple(_) => Some(AssignmentKind::Sequence),
ast::Expr::Name(_) => Some(AssignmentKind::Name),
_ => None,
};
if let Some(kind) = kind {
self.current_assignment = Some(CurrentAssignment::Assign {
assignment: node,
target_index,
kind,
});
}
self.visit_expr(target);
self.current_assignment = None;
}
self.current_assignment = None;
}
ast::Stmt::AnnAssign(node) => {
debug_assert!(self.current_assignment.is_none());
Expand Down Expand Up @@ -815,12 +826,18 @@ where
let symbol = self.add_symbol(id.clone());
if is_definition {
match self.current_assignment {
Some(CurrentAssignment::Assign(assignment)) => {
Some(CurrentAssignment::Assign {
assignment,
target_index,
kind,
}) => {
self.add_definition(
symbol,
AssignmentDefinitionNodeRef {
assignment,
target: name_node,
target_index,
name: name_node,
kind,
},
);
}
Expand Down Expand Up @@ -1045,7 +1062,11 @@ where

#[derive(Copy, Clone, Debug)]
enum CurrentAssignment<'a> {
Assign(&'a ast::StmtAssign),
Assign {
assignment: &'a ast::StmtAssign,
target_index: usize,
kind: AssignmentKind,
},
AnnAssign(&'a ast::StmtAnnAssign),
AugAssign(&'a ast::StmtAugAssign),
For(&'a ast::StmtFor),
Expand All @@ -1057,12 +1078,6 @@ enum CurrentAssignment<'a> {
WithItem(&'a ast::WithItem),
}

impl<'a> From<&'a ast::StmtAssign> for CurrentAssignment<'a> {
fn from(value: &'a ast::StmtAssign) -> Self {
Self::Assign(value)
}
}

impl<'a> From<&'a ast::StmtAnnAssign> for CurrentAssignment<'a> {
fn from(value: &'a ast::StmtAnnAssign) -> Self {
Self::AnnAssign(value)
Expand Down
Loading
Loading