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] Fix merged type after if-else without explicit else branch #14621

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,16 @@ else:
# TODO should be Never
reveal_type(x) # revealed: Literal[1, 2]
```

## After if-statement without explicit else branch, assume else branch with negative clause

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, need also test for x is not changed in the if branch like:

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

x = optional_int()

if x is None: pass

reveal_type(x)  # revealed: int | None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I didn't see it being tested else where. I added another variable in the same mdtest here. I wonder if a new mdtest file would be better to test post if block narrowing scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a new mdtest file would be better to test post if block narrowing scenarios.

I think that's a good idea. We should also add tests for scenarios with (multiple) elif branches, but no else branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a new test file and added couple of more tests.


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

x = optional_int()

if x is None:
x = 0

reveal_type(x) # revealed: int
```
16 changes: 10 additions & 6 deletions crates/red_knot_python_semantic/src/semantic_index/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,17 +785,21 @@ where
}
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);
// if there's no else clause, we should act as if we had an empty else clause,
// by again following the same logic as above for elif/else clauses.
post_clauses.push(self.flow_snapshot());
Copy link
Contributor Author

@sransara sransara Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated the logic in above for loop of node.elif_else_clauses in here. Another approach would be to have a dummy else clause. I wasn't sure if a dummy node with a made up range would cause issues down the line. But I can switch if that pattern is preferred than duplicating.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing an actual dummy node will cause issues, and I don't think it's necessary. But I do think we could remove the logic duplication here. Instead of iterating over clauses above, we could iterate over (test, body) pairs, where both elements of the pair are Option. For a normal else clause, test would be None but body would be Some; if there's no else clause, we'd insert a (None, None) at the end.

That said, without trying to write it, I'm not sure if the code necessary to do this might end up being larger / harder to understand than the duplication you currently have here. Open to whatever you think is best, having worked on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion. This is better. Slight change I did was with body. It is a vector/slice. So I opted to using empty slice instead if Option. I have updated the code now.

self.flow_restore(pre_if);
for constraint in &constraints {
self.record_negated_constraint(*constraint);
}
}
for post_clause_state in post_clauses {
self.flow_merge(post_clause_state);
}
}
ast::Stmt::While(ast::StmtWhile {
Expand Down