-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 3 commits
ab08c35
566d109
27c8307
31c7d87
4edec74
1e59838
c14d470
05a969f
bbceded
ba7eb39
4d1286e
29014a8
dfe42a6
d38c038
b6cf916
dfff0ad
fa89118
864acb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicated the logic in above There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 { | ||
|
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.
IMHO, need also test for
x
is not changed in theif
branch like: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.
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.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.
I think that's a good idea. We should also add tests for scenarios with (multiple)
elif
branches, but noelse
branch.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.
I added a new test file and added couple of more tests.