Skip to content

Commit

Permalink
[red-knot] Fix merged type after if-else without explicit else branch (
Browse files Browse the repository at this point in the history
…#14621)

## Summary

Closes: #14593

The final type of a variable after if-statement without explicit else
branch should be similar to having an explicit else branch.

## Test Plan

Originally failed test cases from the bug are added.

---------

Co-authored-by: Carl Meyer <carl@astral.sh>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
  • Loading branch information
3 people authored Nov 28, 2024
1 parent 976c37a commit 3f6c65e
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 12 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# Consolidating narrowed types after if statement

## After if-else statements, narrowing has no effect if the variable is not mutated in any branch

```py
def optional_int() -> int | None: ...

x = optional_int()

if x is None:
pass
else:
pass

reveal_type(x) # revealed: int | None
```

## Narrowing can have a persistent effect if the variable is mutated in one branch

```py
def optional_int() -> int | None: ...

x = optional_int()

if x is None:
x = 10
else:
pass

reveal_type(x) # revealed: int
```

## An if statement without an explicit `else` branch is equivalent to one with a no-op `else` branch

```py
def optional_int() -> int | None: ...

x = optional_int()
y = optional_int()

if x is None:
x = 0

if y is None:
pass

reveal_type(x) # revealed: int
reveal_type(y) # revealed: int | None
```

## An if-elif without an explicit else branch is equivalent to one with an empty else branch

```py
def optional_int() -> int | None: ...

x = optional_int()

if x is None:
x = 0
elif x > 50:
x = 50

reveal_type(x) # revealed: int
```
30 changes: 18 additions & 12 deletions crates/red_knot_python_semantic/src/semantic_index/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,22 @@ where
let mut constraints = vec![constraint];
self.visit_body(&node.body);
let mut post_clauses: Vec<FlowSnapshot> = vec![];
for clause in &node.elif_else_clauses {
let elif_else_clauses = node
.elif_else_clauses
.iter()
.map(|clause| (clause.test.as_ref(), clause.body.as_slice()));
let has_else = node
.elif_else_clauses
.last()
.is_some_and(|clause| clause.test.is_none());
let elif_else_clauses = elif_else_clauses.chain(if has_else {
// if there's an `else` clause already, we don't need to add another
None
} else {
// if there's no `else` branch, we should add a no-op `else` branch
Some((None, Default::default()))
});
for (clause_test, clause_body) in elif_else_clauses {
// snapshot after every block except the last; the last one will just become
// the state that we merge the other snapshots into
post_clauses.push(self.flow_snapshot());
Expand All @@ -779,24 +794,15 @@ where
for constraint in &constraints {
self.record_negated_constraint(*constraint);
}
if let Some(elif_test) = &clause.test {
if let Some(elif_test) = clause_test {
self.visit_expr(elif_test);
constraints.push(self.record_expression_constraint(elif_test));
}
self.visit_body(&clause.body);
self.visit_body(clause_body);
}
for post_clause_state in post_clauses {
self.flow_merge(post_clause_state);
}
let has_else = node
.elif_else_clauses
.last()
.is_some_and(|clause| clause.test.is_none());
if !has_else {
// if there's no else clause, then it's possible we took none of the branches,
// and the pre_if state can reach here
self.flow_merge(pre_if);
}
}
ast::Stmt::While(ast::StmtWhile {
test,
Expand Down

0 comments on commit 3f6c65e

Please sign in to comment.