Skip to content

Improve docs for single_line_if_else_max_width & add tests #5509

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Improve docs for single_line_if_else_max_width & add tests #5509

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
41af7e7
Add details to documentation + examples
truls-p Aug 18, 2022
f60a569
Add tests for `single_line_if_else_max_width`
truls-p Aug 18, 2022
37d9b88
Refine documentation + add additional tests for `single_line_if_else_…
truls-p Aug 24, 2022
d6f3a90
Update Configurations.md
truls-p Aug 25, 2022
a372baa
Reformat description in Configurations.md
truls-p Aug 25, 2022
c021610
Add some more whitespace in examples, but remove unnecessary spaces
truls-p Aug 25, 2022
63ccc1c
Add missing newlines in tests/source files
truls-p Aug 25, 2022
a536146
Some small tweaks to Configuration.md
truls-p Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion Configurations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2379,14 +2379,48 @@ Don't reformat out of line modules

## `single_line_if_else_max_width`

Maximum line length for single line if-else expressions. A value of `0` (zero) results in if-else expressions always being broken into multiple lines. Note this occurs when `use_small_heuristics` is set to `Off`.
Maximum line length for single line if-else expressions. A value of `0` (zero) results in if-else expressions always being broken into multiple lines. Note this occurs when `use_small_heuristics` is set to `Off`. 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. When using `version = "Two"` the if-else expression at the end of a block will be formatted on a single line. The last if-else expression in the last example snippet of this section would be formatted on a single line with `version = "Two"`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


- **Default value**: `50`
- **Possible values**: any positive integer that is less than or equal to the value specified for [`max_width`](#max_width)
- **Stable**: Yes

By default this option is set as a percentage of [`max_width`](#max_width) provided by [`use_small_heuristics`](#use_small_heuristics), but a value set directly for `single_line_if_else_max_width` will take precedence.

#### `50` (default):

```rust
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name {
baz()
} else {
buzz()
};
if some_long_name && some_other_long_name {
1
} else {
2
}
}
```

#### `70`:

```rust
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name { baz() } else { buzz() };
if some_long_name && some_other_long_name {
1
} else {
2
}
}
```

See also [`max_width`](#max_width) and [`use_small_heuristics`](#use_small_heuristics)

## `space_after_colon`
Expand Down
8 changes: 8 additions & 0 deletions tests/source/issue-5496/70.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// rustfmt-version: One
// rustfmt-single_line_if_else_max_width: 70
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name { baz() } else { buzz() };
if some_long_name && some_other_long_name { 1 } else { 2 }
}
12 changes: 12 additions & 0 deletions tests/source/issue-5496/70_version_two.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// rustfmt-version: Two
// rustfmt-single_line_if_else_max_width: 70
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name { baz() } else { buzz() };
if some_long_name && some_other_long_name {
1
} else {
2
}
}
6 changes: 6 additions & 0 deletions tests/source/issue-5496/default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name { baz() } else { buzz() };
if some_long_name && some_other_long_name { 1 } else { 2 }
}
12 changes: 12 additions & 0 deletions tests/target/issue-5496/70.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// rustfmt-version: One
// rustfmt-single_line_if_else_max_width: 70
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name { baz() } else { buzz() };
if some_long_name && some_other_long_name {
1
} else {
2
}
}
8 changes: 8 additions & 0 deletions tests/target/issue-5496/70_version_two.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// rustfmt-version: Two
// rustfmt-single_line_if_else_max_width: 70
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name { baz() } else { buzz() };
if some_long_name && some_other_long_name { 1 } else { 2 }
}
14 changes: 14 additions & 0 deletions tests/target/issue-5496/default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
fn foo() -> usize {
let some_long_name = true;
let some_other_long_name = false;
let bar = if some_long_name && some_other_long_name {
baz()
} else {
buzz()
};
if some_long_name && some_other_long_name {
1
} else {
2
}
}