Improve docs for single_line_if_else_max_width & add tests

[truls-p]

Closes #5496.

I am afraid I do not have much experience with ASTs and rustfmt in general, but I have tried to understand the implications of the single_line_if_else_max_width config option by stepping through the code and coming up with some examples + tests.

Please let me know if you have any comments or I have missed something significant that should be added to Configurations.md to further clarify this config option.

[ytmimi]

When using version = "Two" the if-else expression at the end of a block will be formatted on a single line

Are you saying that this would only apply to the last if-else expression? I don't think that's correct, but I could be wrong.

[truls-p]

Thanks for having a look! I agree that this behavior sounds strange and maybe my research and testing has come up short here, or maybe the proposed documentation is not clear enough. Let me try to explain what I see happening. When we try to use single_line_if_else_max_width in expr.rs self.allow_single_line needs to be true (see here), which only happens when we have an if and expression type is == ExprType::SubExpression (see here). Now, the version = "Two" is relevant here where we set expr_type to ExprType::SubExpression. The two tests I added, 70.rs and 70_version_two.rs tries to test this specific behavior.

Here is also another example to play around with. If you run rustfmt --check on this you will see we get a diff for both functions. However, if we change version to "Two" we only get a diff for the last if in foo.
Config options:

single_line_if_else_max_width = 80
version = "One"

main.rs:

fn bar() {}
fn foo() -> usize {
    let some_long_name = true;
    if some_long_name && some_long_name && some_long_name { bar() } else { bar() }
    return 1
}

fn baz() {
    let some_long_name = true;
    if some_long_name && some_long_name && some_long_name { bar() } else { bar() }
}

fn main() {
    let i = foo();
    baz();
    println!("{}", i);
}

[ytmimi]

Here are some other examples to consider:

What about something like this where the if-else is on the right hand side of an assignment expression?

fn foo() -> usize {
    let x = if some_long_name && some_long_name && some_long_name { bar() } else { bar() };
    let some_long_name = true;
    x
}

Also what about conditionally passing an argument to a function call or returning a value from a closure?

fn main() {
    let x = 0;
    f(if x == 0 { 0 } else { 10 })
    g(|x| if x == 0 { 0 } else { 10 })
}

Again, really appreciate your work on this, and helping us to nail down the description for this option. I think a little rewording is still needed, and maybe it's worth investigating how we set the expr_type argument. For example, when we call rewrite on a top level ast::Expr node we always pass expr_type as ExprType::SubExpression. See impl Rewrite for ast::Expr. The only place in the code base I could find where we don't hard code the expr_type argument to format_expr is when it gets called from format_stmt.

We happen to define our own borrowed version of a ast::Stmt:

rustfmt/src/stmt.rs

Lines 13 to 16 in 38659ec

	pub(crate) struct Stmt<'a> {
	inner: &'a ast::Stmt,
	is_last: bool,
	}

And the rewrite method for that does follow the behavior that you outlined above where the last expression is set to a ExprType::SubExpression if Version=Two is also set

rustfmt/src/stmt.rs

Lines 76 to 85 in 38659ec

	impl<'a> Rewrite for Stmt<'a> {
	fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
	let expr_type = if context.config.version() == Version::Two && self.is_last_expr() {
	ExprType::SubExpression
	} else {
	ExprType::Statement
	};
	format_stmt(context, shape, self.as_ast_node(), expr_type)
	}
	}

So maybe that's what explains the last expression behavior that you documented.

I also came across this interesting comment in the code base that would be nice to have a test for.

If one of the if-else blocks contain a statement, a comment, an attribute, or is empty, the if-else expression will be broken into multiple lines.

It would also be great to have tests for each of these cases that you outlined!

[truls-p]

Thanks for the detailed feedback so far!
These are good examples and I have added them to the documentation in the example section and mentioned them explicitly (the let, function call, and closure) in the description of the config option. I have also reworded the part about if-else expressions at the end of blocks. More on this below.

I have also added some more test cases for the nested if-else case (note how the inner if-else is on a single line when using Version = "Two" because it is the last expression in the block) and the comment about the if-else blocks needing to be simple.

Regarding the Version = "Two" and how the expr_type is set I believe I now have a slightly better understanding about how this all works. My understanding is that as we walk the nodes everything starts out as a Stmt. This is why, for Version = "One", when we look at an if-else at the end of a block we hit the following block of code and expr_type is set to ExprType::Statement.

rustfmt/src/stmt.rs

Lines 76 to 85 in 38659ec

	impl<'a> Rewrite for Stmt<'a> {
	fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
	let expr_type = if context.config.version() == Version::Two && self.is_last_expr() {
	ExprType::SubExpression
	} else {
	ExprType::Statement
	};
	format_stmt(context, shape, self.as_ast_node(), expr_type)
	}
	}

However, in the let, function call, and closure examples we set the expr_type for the if-else much further down the call stack. As an example, for the if-else inside the function call we call we hit rewrite_cond where the expr_type is set to ExprType::SubExpression by default.

rustfmt/src/expr.rs

Line 610 in 38659ec

_ => to_control_flow(expr, ExprType::SubExpression).and_then(|control_flow| {

Then, crucially, expr_type == ExprType::SubExpression evaluates to true in fn to_control_flow.

rustfmt/src/expr.rs

Lines 645 to 658 in 38659ec

	fn to_control_flow(expr: &ast::Expr, expr_type: ExprType) -> Option<ControlFlow<'_>> {
	match expr.kind {
	ast::ExprKind::If(ref cond, ref if_block, ref else_block) => {
	let (pat, cond) = extract_pats_and_cond(cond);
	Some(ControlFlow::new_if(
	cond,
	pat,
	if_block,
	else_block.as_ref().map(|e| &**e),
	expr_type == ExprType::SubExpression,
	false,
	expr.span,
	))
	}

On the other hand, for the if-else at the end of a block we go straight from impl<'a> Rewrite for Stmt<'a> to fn format_stmt to fn format_expr, and finally to fn control_flow, where the expr_type is either ExprType::Statement or ExprType::SubExpression (set in impl<'a> Rewrite for Stmt<'a>) depending on whether we use Version = "One" or Version = "Two".

I also dug up the original PR for the impl<'a> Rewrite for Stmt<'a>:
#3631
Note the tests added for #3614: https://github.com/rust-lang/rustfmt/pull/3631/files#diff-c331b0359e60165a708105a49c54a82dc01be48cd219a4c83f91ba0ba961dcb2

Please let me know your thoughts!

[ytmimi]

Again, thanks for your work on this. Take a look at my suggestions and let me know if you have any questions.

[truls-p]

Thanks again for all the help, @ytmimi ! I think I have now incorporated all the requested changes.

[ytmimi]

Great! I'll look to do a follow up after work

[ytmimi]

Thanks for all the help with updating the docs and your first contribution to rustfmt 🎉. I think we're good to go, but will hold off on merging to let @calebcartwright take a look!

[calebcartwright]

Thanks for the contribution! I imagine the tests are probably useful in their own right, but I think this is the wrong direction to take with the documentation updates.

I've shared more details below, and will probably have some related thoughts to share on the associated issue

[calebcartwright]

I have some reservations about any attempts to reference, or perhaps to some readers, codify formatting behavior here. The Style Guide is what articulates the rules for the formatting behavior, and the exact prescriptions for when if-else can/can't be single lined can be found here.

This option exists as the mechanism to control the value used as the "small" threshold as described in the Guide, and I don't think we should have much more than that here.

Basically, the Config docs, and any particular option within them, isn't really the right place to try to explain the broader formatting rules for a given syntax construct