Fix if_then_some_else_none FP when encountering await codes
#16178
Conversation
rustbot
added
the
S-waiting-on-review
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.
But I also modified can_partially_move_ty to make Ref return false.
|
Lintcheck changes for 5377dba
This comment will be updated if you push new changes |
4 participants
changelog: Fix FP of [
if_then_some_else_none] when thethenblock containsawaitcodes.That is because
bool:thendoesn't work with await code.Closes: #16176