Skip to content
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.

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

Single line if's for 1 stmt block #6031

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
79 changes: 79 additions & 0 deletions Configurations.md
Original file line number Diff line number Diff line change
Expand Up @@ -2481,6 +2481,85 @@ fn main() {

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

## `single_line_if`

Allows if blocks to format on one line only if they contain a single expression of `return`, `continue` or `break`. Useful in the case of keeping `let-else` guards format consistent with `if` guards.

Note that line will still break if:
1. The total length (condition and block) is over the `max_width`/`single_line_simple_if_max_width length`
2. The block ends with a trailing semicolon
3. The block contains a single or multi-lined comment
4. The block contains `return` or `break` and returns a value

- **Default value**: `false`
- **Possible values**: `true`, `false`
- **Stable**: No

#### `false` (default):

```rust
fn main() {
if true {
break;
}

if false {
return;
}

if false {
return 5;
}

if width == is_49_characters____long {
continue;
}

if width == is_50_characters_____long {
continue;
}

if width == is_51_characters______long {
continue;
}
}
```

#### `true`:

```rust
fn main() {
if true { break }

if false { return }

if false {
return 5;
}

if width == is_49_characters____long { continue }

if width == is_50_characters_____long { continue }

if width == is_51_characters______long {
continue;
}
}
```

## `single_line_simple_if_max_width`

Maximum line length for single line if with a simple inner expression. Useful in the case of keeping `let-else` guards format consistent with `if` guards.

A value of `0` (zero) results in if blocks always being broken into multiple lines. Note this occurs when `use_small_heuristics` is set to `Off`.

- **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.

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

## `space_after_colon`

Expand Down
10 changes: 10 additions & 0 deletions src/config/config_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ macro_rules! create_config {
| "fn_call_width"
| "single_line_if_else_max_width"
| "single_line_let_else_max_width"
| "single_line_simple_if_max_width"
| "attr_fn_like_width"
| "struct_lit_width"
| "struct_variant_width"
Expand Down Expand Up @@ -273,6 +274,7 @@ macro_rules! create_config {
| "fn_call_width"
| "single_line_if_else_max_width"
| "single_line_let_else_max_width"
| "single_line_simple_if_max_width"
| "attr_fn_like_width"
| "struct_lit_width"
| "struct_variant_width"
Expand Down Expand Up @@ -421,6 +423,14 @@ macro_rules! create_config {
"single_line_let_else_max_width",
);
self.single_line_let_else_max_width.2 = single_line_let_else_max_width;

let single_line_simple_if_max_width = get_width_value(
self.was_set().single_line_simple_if_max_width(),
self.single_line_simple_if_max_width.2,
heuristics.single_line_simple_if_max_width,
"single_line_simple_if_max_width",
);
self.single_line_simple_if_max_width.2 = single_line_simple_if_max_width;
}

fn set_heuristics(&mut self) {
Expand Down
7 changes: 7 additions & 0 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ create_config! {
single_line_let_else_max_width: usize, 50, true, "Maximum line length for single line \
let-else statements. A value of zero means always format the divergent `else` block \
over multiple lines.";
single_line_simple_if_max_width: usize, 50, false, "Maximum line length for single line \
if statement. A value of zero means always format to multiple lines.";

// Comments. macros, and strings
wrap_comments: bool, false, false, "Break comments to fit on the line";
Expand All @@ -81,6 +83,7 @@ create_config! {
"Format hexadecimal integer literals";

// Single line expressions and items
single_line_if: bool, false, false, "Simple if statements can format to a single line";
empty_item_single_line: bool, true, false,
"Put empty-body functions and impls on a single line";
struct_lit_single_line: bool, true, false,
Expand Down Expand Up @@ -490,6 +493,8 @@ mod test {
single_line_let_else_max_width: usize, 50, false, "Maximum line length for single \
line let-else statements. A value of zero means always format the divergent \
`else` block over multiple lines.";
single_line_simple_if_max_width: usize, 50, false, "Maximum line length for \
single line if statement. A value of zero means always format to multiple lines.";

// Options that are used by the tests
stable_option: bool, false, true, "A stable option";
Expand Down Expand Up @@ -634,6 +639,7 @@ array_width = 60
chain_width = 60
single_line_if_else_max_width = 50
single_line_let_else_max_width = 50
single_line_simple_if_max_width = 50
wrap_comments = false
format_code_in_doc_comments = false
doc_comment_code_block_width = 100
Expand All @@ -645,6 +651,7 @@ format_macro_matchers = false
format_macro_bodies = true
skip_macro_invocations = []
hex_literal_case = "Preserve"
single_line_if = false
empty_item_single_line = true
struct_lit_single_line = true
fn_single_line = false
Expand Down
6 changes: 6 additions & 0 deletions src/config/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ pub struct WidthHeuristics {
// Maximum line length for single line let-else statements. A value of zero means
// always format the divergent `else` block over multiple lines.
pub(crate) single_line_let_else_max_width: usize,
// Maximum line length for single line if statement.
// A value of zero means always format to multiple lines.
pub(crate) single_line_simple_if_max_width: usize,
}

impl fmt::Display for WidthHeuristics {
Expand All @@ -261,6 +264,7 @@ impl WidthHeuristics {
chain_width: usize::max_value(),
single_line_if_else_max_width: 0,
single_line_let_else_max_width: 0,
single_line_simple_if_max_width: 0,
}
}

Expand All @@ -274,6 +278,7 @@ impl WidthHeuristics {
chain_width: max_width,
single_line_if_else_max_width: max_width,
single_line_let_else_max_width: max_width,
single_line_simple_if_max_width: max_width,
}
}

Expand All @@ -296,6 +301,7 @@ impl WidthHeuristics {
chain_width: (60.0 * max_width_ratio).round() as usize,
single_line_if_else_max_width: (50.0 * max_width_ratio).round() as usize,
single_line_let_else_max_width: (50.0 * max_width_ratio).round() as usize,
single_line_simple_if_max_width: (50.0 * max_width_ratio).round() as usize,
}
}
}
Expand Down
80 changes: 78 additions & 2 deletions src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1102,8 +1102,18 @@ impl<'a> Rewrite for ControlFlow<'a> {
};
let block_str = {
let old_val = context.is_if_else_block.replace(self.else_block.is_some());
let result =
rewrite_block_with_visitor(context, "", self.block, None, None, block_shape, true);
let result = if self.keyword == "if" {
format_if(
context,
shape,
self.block,
&cond_str,
used_width,
block_shape,
)
} else {
rewrite_block_with_visitor(context, "", self.block, None, None, block_shape, true)
};
context.is_if_else_block.replace(old_val);
result?
};
Expand Down Expand Up @@ -1158,6 +1168,72 @@ impl<'a> Rewrite for ControlFlow<'a> {
}
}

fn format_if(
context: &RewriteContext<'_>,
shape: Shape,
block: &ast::Block,
cond_str: &str,
used_width: usize,
block_shape: Shape,
) -> Option<String> {
let max_width = if context.config.single_line_if() {
std::cmp::min(
shape.width,
context.config.single_line_simple_if_max_width(),
)
} else {
shape.width
};
let available_space = max_width.saturating_sub(used_width);
let allow_single_line = context.config.single_line_if()
&& available_space > 0
&& allow_single_line_if(&cond_str, block);

let result = if allow_single_line {
let mut single_line_attempt =
rewrite_block_inner(block, None, None, true, context, block_shape)?;
if !single_line_attempt.contains('\n') && single_line_attempt.len() > available_space {
single_line_attempt =
rewrite_block_inner(block, None, None, false, context, block_shape)?;
}
Some(single_line_attempt)
} else {
rewrite_block_with_visitor(context, "", block, None, None, block_shape, true)
};
result
}

fn allow_single_line_if(result: &str, block: &ast::Block) -> bool {
if result.contains('\n') {
return false;
}
Comment on lines +1207 to +1209
Copy link
Contributor

Choose a reason for hiding this comment

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

Does result contain any rewritten attributes? Would the #[allow(unused)] prevent rustfmt from writing this on a single line?

fn main() {
    #[allow(unused)]
    if true {
        println!("case 1");
    }

    #[allow(unused)]
    // What about if there's a comment here?
    if true {
        println!("case 2 with comment");
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We also don't want to allow single lining if the block contains a comment. I think the block rewriting handles that but just a call out that we'd need a test for it.

Copy link
Author

@Sjael Sjael Jan 20, 2024

Choose a reason for hiding this comment

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

result here is the condition string, so it's saying if we haven't gone to a new line due to the condition, proceed with checking if we can go for 1 line with the block. So yes, it contains rewritten attributes if I understand you correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ask because I overlooked this case when adding single_line_let_else_max_width and then we needed #5902 to correct the issue. I want to make sure that we handle this up front. It would be unfortunate if leading attributes or comments prevented the guard clause from being reformatted.

if block.stmts.len() == 0 {
return true;
}
if block.stmts.len() == 1 {
return is_simple_control_flow_stmt(&block.stmts[0]);
}
false
}

fn is_simple_control_flow_stmt(stmt: &ast::Stmt) -> bool {
match stmt.kind {
ast::StmtKind::Expr(ref expr) => match expr.kind {
ast::ExprKind::Continue(..) => true,
ast::ExprKind::Break(_, ref opt_expr) | ast::ExprKind::Ret(ref opt_expr) => {
if let Some(_) = *opt_expr {
// Do not allow single line if block has `return/break` with a returned value
false
} else {
true
}
}
_ => false,
},
_ => false,
}
}

fn rewrite_label(opt_label: Option<ast::Label>) -> Cow<'static, str> {
match opt_label {
Some(label) => Cow::from(format!("{}: ", label.ident)),
Expand Down
59 changes: 59 additions & 0 deletions tests/source/single_line_if/false_0.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// rustfmt-single_line_if: false
// rustfmt-single_line_if_else_max_width: 0

fn main() {
let x = if true { 1 } else { 2 };

funk(if test() { 1 } else { 2 }, arg2);

if true { 1 } else { 2 }

if test() { 1 } else { 2 }

if let Some(a) = b { return }

if let Some(a) = b { do_something(); return }

if let Some(a) = b { return } else { continue }

let a = if 1 > 2 {
unreachable!()
} else {
10
};

let a = if x { 1 } else if y { 2 } else { 3 };

if true { continue }

if true {
continue
}

if width == is_49_characters____long { continue }
if width == is_50_characters_____long { continue }
if width == is_51_characters______long { continue }

if name == super_duper_really_really_mega_ultra_giga_long_name_with_a_cherry_on_top { return }

if true { return } else { break }

let x = if true { return } else { break };

let b = if cond() {
5
} else {
// Brief comment.
10
};

let c = if cond() {
statement();

5
} else {
10
};

if cond() { statement(); } else { other_statement(); }
}
Loading
Loading