Allow hoisting else block stmts for while_let_loop

[borngraced]

View all comments

fixes: #16393

changelog: [while_let_loop]: Allow hoisting else block stmts

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[borngraced]

Is it okay to eliminate too many argument warnings by allowing clippy too_many_arguments or perhaps I'll need to refactor the fn particularly.. 🤔

[ada4a]

Is it okay to eliminate too many argument warnings by allowing clippy too_many_arguments or perhaps I'll need to refactor the fn particularly.. 🤔

Yeah it's usually fine... but if you are able to come up with a refactoring, then by all means!

[ada4a]

This should be fine to remove now, as the suggestion is contained in the .stderr file

[ada4a]

One problem with this is that, since one of the statements is moved out, it's a bit unclear what exactly should the ellipses be replaced with. Not sure how to fix this, short of reintroducing body reconstruction in the suggestion

[borngraced]

what do you think about adding a help note to explain what the user should do with the { .. }? like so:

 help: try
     |
  LL ~     while let Some(x) = y { .. }
  LL +     println!("fail");
     |
     = note: the statements above are hoisted from the `else` block; move the original loop body into the `while let`

[ada4a]

it would be nice to add tests for:

• multiple statements
• a semicolon-less statement, like if true { whatever; }

[borngraced]

80bc351

[ada4a]

Suggested change

	format!("\n{indent} let {pat_str}{ty_str} = {init_str};\n{indent} ..\n{indent}",)
	format!("\n{indent} let {pat_str}{ty_str} = {init_str};\n{indent} ..\n{indent}")

I wish rustfmt would handle this...

[borngraced]

filed an issue rust-lang/rustfmt#6768

[samueltardieu]

It would really be great to have autofixes, that was the goal of hoisting the statements out of the loop. We should attempt this, even though this might produce larger output. If needed, there are ways to present one view to the human, and one to tools so that they can do large transforms (tool-only suggestions).

View changes since this review

[samueltardieu]

This could be written more concisely:

Suggested change

	}
	let (let_pat, let_expr, inner_expr, hoistable_stmts) = if let Some(if_let) = higher::IfLet::hir(cx, init)
	&& let Some(else_expr) = if_let.if_else
	&& is_simple_break_expr(else_expr)
	{
	(if_let.let_pat, if_let.let_expr, Some(if_let.if_then), None)
	} else if els.is_some_and(is_simple_break_block)
	&& let Some((pat, _)) = let_info
	{
	(pat, init, None, None)
	} else if let Some(els_block) = els
	&& let Some((pat, _)) = let_info
	&& let Some(hoistable) = extract_hoistable_stmts(els_block)
	{
	(pat, init, None, Some(hoistable))
	} else if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal) = init.kind
	&& arm1.guard.is_none()
	&& arm2.guard.is_none()
	&& is_simple_break_expr(arm2.body)
	{
	(arm1.pat, scrutinee, Some(arm1.body), None)
	} else {
	return;
	};
	let while_let_info = WhileLetInfo {
	let_pat,
	let_expr,
	has_trailing_exprs,
	let_info,
	inner_expr,
	hoistable_stmts,
	};
	could_be_while_let(cx, expr, while_let_info);

I'm not even sure the WhileLetInfo structure is really useful.

[samueltardieu]

Why would that be a problem? A break to an inner loop would be hoisted fine along with the inner loop, while a break to an outer loop would be hoisted fine and, as expected, break from the outer loop. The important point here is that you have a break statement for the loop you're trying to transform at the end of the else. You don't care if the statements before the break are diverging. The only important things to check IMO is that the hoisted statement should not contain a break or a continue to the loop it getting hoisted out of, except the last statement or expression of course.

Note that you can check the target of a break by HirId (which will be the loop_expr.hir_id if loop_expr is the loop expression). You could modify is_simple_break_expr as:

/// Checks if `expr` contains a single unlabeled `break` expression or statement, possibly embedded
/// inside other blocks.
fn is_simple_break_expr(expr: &Expr<'_>, target_id: HirId) -> bool {
    matches!(peel_blocks_with_stmt(expr).kind, ExprKind::Break(dest, None) if dest.target_id == Ok(target_id))
}

(and modify is_simple_break_loop accordingly)

Or am I missing something?

[borngraced]

The only breaks/continues that would be problematic are ones targeting the loop being transformed itself, because that loop's structure changes to a while let...so you're right.

[samueltardieu]

I'd also like to see some tests where labels are applied on the loop being transformed.

[borngraced]

all done

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[borngraced]

@rustbot ready

[rustbot]

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.

[rustbot]

☔ The latest upstream changes (possibly #17644) made this pull request unmergeable. Please resolve the merge conflicts.