Fix if_then_some_else_none FP when encountering await codes
#16178
Conversation
This comment has been minimized.
This comment has been minimized.
| && !contains_return(then_block.stmts) | ||
| && then_block.expr.is_none_or(|expr| !contains_return(expr)) | ||
| && then_block.expr.is_none_or(|expr| { | ||
| !contains_return(expr) | ||
| // `bool::then` doesn't work with async code | ||
| && for_each_expr_without_closures(expr, |e| { | ||
| if desugar_await(e).is_some() { | ||
| ControlFlow::Break(()) | ||
| } else { | ||
| ControlFlow::Continue(()) | ||
| } | ||
| }) | ||
| .is_none() | ||
| }) |
There was a problem hiding this comment.
Could we just use can_move_expr_to_closure on the then expression here? I'm not sure if it would introduce FNs but it seems like it answers exactly the question of "can we move that code into a closure", taking into account control flow constructs and the like (yield, return, continue, break, ...).
There was a problem hiding this comment.
There's also a bug in the current logic here because it only runs the visitor on the tail expression and not the statements, so it would miss await in statements of the block.
If can_move_expr_to_closure doesn't work out it might still be better to remove the contains_return and instead just have a single for_each_expr_without_closures on the block that checks for everything we want to ignore. That saves us from visiting the whole block twice.
Rather than using desugar_await(e).is_some() it might also be better to just check for ExprKind::Yield since that's ultimately the root cause. It also can't really be used in bool::then and await uses yield.
There was a problem hiding this comment.
Thank you. I think we need to use can_move_expr_to_closure consistently.
And I also modified can_partially_move_ty to make Ref return false because Ref can't be moved partially.
|
No changes for 6159c19 |
This comment has been minimized.
This comment has been minimized.
|
The new FP in
Opened a separate issue for the latter: #16193 |
Yes. The new issue #16193 seems to be caused by Even though the |
… when encountering `await` codes
|
The change to |
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Agree. I've made minimal changes this time. |
changelog: Fix FP of [
if_then_some_else_none] when thethenblock containsawaitcodes.That is because
bool:thendoesn't work with await code.Closes: #16176