From fc7f6e11fe5292e628d44a5eb49c6c6fd96039a8 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 17 Apr 2024 17:37:46 -0400 Subject: [PATCH 1/6] Implement preserve mode for trailing-semicolon --- Configurations.md | 20 ++++++-- src/config/mod.rs | 4 +- src/config/options.rs | 7 +++ src/utils.rs | 18 +++++-- .../trailing_semicolon/{true.rs => always.rs} | 2 +- .../trailing_semicolon/{false.rs => never.rs} | 2 +- .../configs/trailing_semicolon/preserve.rs | 49 +++++++++++++++++++ .../issue-5797/retain_trailing_semicolon.rs | 2 +- 8 files changed, 91 insertions(+), 13 deletions(-) rename tests/target/configs/trailing_semicolon/{true.rs => always.rs} (89%) rename tests/target/configs/trailing_semicolon/{false.rs => never.rs} (89%) create mode 100644 tests/target/configs/trailing_semicolon/preserve.rs diff --git a/Configurations.md b/Configurations.md index 2d01fb3bb3b..6d858fb5da5 100644 --- a/Configurations.md +++ b/Configurations.md @@ -2780,20 +2780,32 @@ See also: [`match_block_trailing_comma`](#match_block_trailing_comma). Add trailing semicolon after break, continue and return -- **Default value**: `true` -- **Possible values**: `true`, `false` +- **Default value**: `preserve` +- **Possible values**: `always`, `never`, `preserve` - **Stable**: No (tracking issue: [#3378](https://github.com/rust-lang/rustfmt/issues/3378)) -#### `true` (default): +#### `always` (default): ```rust fn foo() -> usize { return 0; } ``` -#### `false`: +#### `never`: +```rust +fn foo() -> usize { + return 0 +} +``` + + +#### `preserve`: ```rust fn foo() -> usize { + return 0; +} + +fn bar() -> usize { return 0 } ``` diff --git a/src/config/mod.rs b/src/config/mod.rs index 9484b2e5829..04f4f212fc3 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -135,7 +135,7 @@ create_config! { brace_style: BraceStyle, BraceStyle::SameLineWhere, false, "Brace style for items"; control_brace_style: ControlBraceStyle, ControlBraceStyle::AlwaysSameLine, false, "Brace style for control flow constructs"; - trailing_semicolon: bool, true, false, + trailing_semicolon: TrailingSemicolon, TrailingSemicolon::Preserve, false, "Add trailing semicolon after break, continue and return"; trailing_comma: SeparatorTactic, SeparatorTactic::Vertical, false, "How to handle trailing commas for lists"; @@ -673,7 +673,7 @@ force_multiline_blocks = false fn_params_layout = "Tall" brace_style = "SameLineWhere" control_brace_style = "AlwaysSameLine" -trailing_semicolon = true +trailing_semicolon = preserve trailing_comma = "Vertical" match_block_trailing_comma = false blank_lines_upper_bound = 1 diff --git a/src/config/options.rs b/src/config/options.rs index 3c5c713a33a..04c78ffc39a 100644 --- a/src/config/options.rs +++ b/src/config/options.rs @@ -49,6 +49,13 @@ pub enum ControlBraceStyle { AlwaysNextLine, } +#[config_type] +pub enum TrailingSemicolon { + Always, + Never, + Preserve, +} + #[config_type] /// How to indent. pub enum IndentStyle { diff --git a/src/utils.rs b/src/utils.rs index 642b6603b1e..26d867a1761 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -10,7 +10,7 @@ use rustc_span::{sym, symbol, BytePos, LocalExpnId, Span, Symbol, SyntaxContext} use unicode_width::UnicodeWidthStr; use crate::comment::{filter_normal_code, CharClasses, FullCodeCharKind, LineClasses}; -use crate::config::{Config, Version}; +use crate::config::{Config, TrailingSemicolon, Version}; use crate::rewrite::RewriteContext; use crate::shape::{Indent, Shape}; @@ -273,7 +273,7 @@ pub(crate) fn contains_skip(attrs: &[Attribute]) -> bool { #[inline] pub(crate) fn semicolon_for_expr(context: &RewriteContext<'_>, expr: &ast::Expr) -> bool { // Never try to insert semicolons on expressions when we're inside - // a macro definition - this can prevent the macro from compiling + // a macro definition - this can prevent the macro from compiling // when used in expression position if context.is_macro_def { return false; @@ -281,7 +281,10 @@ pub(crate) fn semicolon_for_expr(context: &RewriteContext<'_>, expr: &ast::Expr) match expr.kind { ast::ExprKind::Ret(..) | ast::ExprKind::Continue(..) | ast::ExprKind::Break(..) => { - context.config.trailing_semicolon() + match context.config.trailing_semicolon() { + TrailingSemicolon::Always => true, + TrailingSemicolon::Never | TrailingSemicolon::Preserve => false, + } } _ => false, } @@ -301,7 +304,14 @@ pub(crate) fn semicolon_for_stmt( ast::ExprKind::Break(..) | ast::ExprKind::Continue(..) | ast::ExprKind::Ret(..) => { // The only time we can skip the semi-colon is if the config option is set to false // **and** this is the last expr (even though any following exprs are unreachable) - context.config.trailing_semicolon() || !is_last_expr + if !is_last_expr { + true + } else { + match context.config.trailing_semicolon() { + TrailingSemicolon::Always | TrailingSemicolon::Preserve => true, + TrailingSemicolon::Never => false, + } + } } _ => true, }, diff --git a/tests/target/configs/trailing_semicolon/true.rs b/tests/target/configs/trailing_semicolon/always.rs similarity index 89% rename from tests/target/configs/trailing_semicolon/true.rs rename to tests/target/configs/trailing_semicolon/always.rs index 61b6843d677..764ef8792bc 100644 --- a/tests/target/configs/trailing_semicolon/true.rs +++ b/tests/target/configs/trailing_semicolon/always.rs @@ -1,4 +1,4 @@ -// rustfmt-trailing_semicolon: true +// rustfmt-trailing_semicolon: always #![feature(loop_break_value)] diff --git a/tests/target/configs/trailing_semicolon/false.rs b/tests/target/configs/trailing_semicolon/never.rs similarity index 89% rename from tests/target/configs/trailing_semicolon/false.rs rename to tests/target/configs/trailing_semicolon/never.rs index 9fa746e9c0f..f4fce216f1a 100644 --- a/tests/target/configs/trailing_semicolon/false.rs +++ b/tests/target/configs/trailing_semicolon/never.rs @@ -1,4 +1,4 @@ -// rustfmt-trailing_semicolon: false +// rustfmt-trailing_semicolon: never #![feature(loop_break_value)] diff --git a/tests/target/configs/trailing_semicolon/preserve.rs b/tests/target/configs/trailing_semicolon/preserve.rs new file mode 100644 index 00000000000..7804f230a81 --- /dev/null +++ b/tests/target/configs/trailing_semicolon/preserve.rs @@ -0,0 +1,49 @@ +// rustfmt-trailing_semicolon: never + +#![feature(loop_break_value)] + +fn main() { + 'a: loop { + break 'a + } + + let mut done = false; + 'b: while !done { + done = true; + continue 'b + } + + let x = loop { + break 5 + }; + + let x = 'c: loop { + break 'c 5 + }; + + 'a: loop { + break 'a; + } + + let mut done = false; + 'b: while !done { + done = true; + continue 'b; + } + + let x = loop { + break 5; + }; + + let x = 'c: loop { + break 'c 5; + }; +} + +fn foo() -> usize { + return 0 +} + +fn bar() -> usize { + return 0; +} diff --git a/tests/target/issue-5797/retain_trailing_semicolon.rs b/tests/target/issue-5797/retain_trailing_semicolon.rs index 851073971f6..6eaf426cf5f 100644 --- a/tests/target/issue-5797/retain_trailing_semicolon.rs +++ b/tests/target/issue-5797/retain_trailing_semicolon.rs @@ -1,4 +1,4 @@ -// rustfmt-trailing_semicolon: false +// rustfmt-trailing_semicolon: never fn foo() {} fn main() { From 10e6db2f48dcd3bdff73fedebcb796426632d66c Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 17 Apr 2024 17:56:13 -0400 Subject: [PATCH 2/6] Bless tests --- .../single_line_let_else_max_width/100.rs | 14 ++++---- .../single_line_let_else_max_width/50.rs | 16 +++++----- .../single_line_let_else_max_width/zero.rs | 20 ++++++------ .../configs/use_small_heuristics/default.rs | 2 +- .../configs/use_small_heuristics/off.rs | 6 ++-- tests/target/expr.rs | 10 ++---- tests/target/issue-3213/version_two.rs | 2 +- tests/target/let_else.rs | 32 +++++++++---------- 8 files changed, 49 insertions(+), 53 deletions(-) diff --git a/tests/target/configs/single_line_let_else_max_width/100.rs b/tests/target/configs/single_line_let_else_max_width/100.rs index 0409124a5b0..c262a5a0e8d 100644 --- a/tests/target/configs/single_line_let_else_max_width/100.rs +++ b/tests/target/configs/single_line_let_else_max_width/100.rs @@ -9,12 +9,12 @@ fn main() { let Some(c) = opt else { // a comment should always force the block to be multi-lined - return; + return }; let Some(c) = opt else { /* a comment should always force the block to be multi-lined */ - return; + return }; let Some(d) = some_very_very_very_very_long_name else { return }; @@ -26,11 +26,11 @@ fn main() { range: _, }) = slice.as_ref() else { - return; + return }; let Some((base_place, current)) = self.lower_expr_as_place(current, *base, true)? else { - return Ok(None); + return Ok(None) }; let Some(doc_attr) = variant @@ -38,11 +38,11 @@ fn main() { .iter() .find(|attr| attr.path().is_ident("doc")) else { - return Err(Error::new(variant.span(), r#"expected a doc comment"#)); + return Err(Error::new(variant.span(), r#"expected a doc comment"#)) }; let Some((base_place, current)) = self.lower_expr_as_place(current, *base, true) else { - return Ok(None); + return Ok(None) }; let Stmt::Expr( @@ -55,6 +55,6 @@ fn main() { return Err(Error::new( last_stmt.span(), "expected last expression to be `Some(match (..) { .. })`", - )); + )) }; } diff --git a/tests/target/configs/single_line_let_else_max_width/50.rs b/tests/target/configs/single_line_let_else_max_width/50.rs index 6afc2b6f2b0..20c41f7482c 100644 --- a/tests/target/configs/single_line_let_else_max_width/50.rs +++ b/tests/target/configs/single_line_let_else_max_width/50.rs @@ -9,16 +9,16 @@ fn main() { let Some(c) = opt else { // a comment should always force the block to be multi-lined - return; + return }; let Some(c) = opt else { /* a comment should always force the block to be multi-lined */ - return; + return }; let Some(d) = some_very_very_very_very_long_name else { - return; + return }; let Expr::Slice(ast::ExprSlice { @@ -28,11 +28,11 @@ fn main() { range: _, }) = slice.as_ref() else { - return; + return }; let Some((base_place, current)) = self.lower_expr_as_place(current, *base, true)? else { - return Ok(None); + return Ok(None) }; let Some(doc_attr) = variant @@ -40,11 +40,11 @@ fn main() { .iter() .find(|attr| attr.path().is_ident("doc")) else { - return Err(Error::new(variant.span(), r#"expected a doc comment"#)); + return Err(Error::new(variant.span(), r#"expected a doc comment"#)) }; let Some((base_place, current)) = self.lower_expr_as_place(current, *base, true) else { - return Ok(None); + return Ok(None) }; let Stmt::Expr( @@ -57,6 +57,6 @@ fn main() { return Err(Error::new( last_stmt.span(), "expected last expression to be `Some(match (..) { .. })`", - )); + )) }; } diff --git a/tests/target/configs/single_line_let_else_max_width/zero.rs b/tests/target/configs/single_line_let_else_max_width/zero.rs index b5fd0b9edaf..0a46d009f46 100644 --- a/tests/target/configs/single_line_let_else_max_width/zero.rs +++ b/tests/target/configs/single_line_let_else_max_width/zero.rs @@ -4,25 +4,25 @@ fn main() { let Some(a) = opt else {}; let Some(b) = opt else { - return; + return }; let Some(c) = opt else { - return; + return }; let Some(c) = opt else { // a comment should always force the block to be multi-lined - return; + return }; let Some(c) = opt else { /* a comment should always force the block to be multi-lined */ - return; + return }; let Some(d) = some_very_very_very_very_long_name else { - return; + return }; let Expr::Slice(ast::ExprSlice { @@ -32,11 +32,11 @@ fn main() { range: _, }) = slice.as_ref() else { - return; + return }; let Some((base_place, current)) = self.lower_expr_as_place(current, *base, true)? else { - return Ok(None); + return Ok(None) }; let Some(doc_attr) = variant @@ -44,11 +44,11 @@ fn main() { .iter() .find(|attr| attr.path().is_ident("doc")) else { - return Err(Error::new(variant.span(), r#"expected a doc comment"#)); + return Err(Error::new(variant.span(), r#"expected a doc comment"#)) }; let Some((base_place, current)) = self.lower_expr_as_place(current, *base, true) else { - return Ok(None); + return Ok(None) }; let Stmt::Expr( @@ -61,6 +61,6 @@ fn main() { return Err(Error::new( last_stmt.span(), "expected last expression to be `Some(match (..) { .. })`", - )); + )) }; } diff --git a/tests/target/configs/use_small_heuristics/default.rs b/tests/target/configs/use_small_heuristics/default.rs index ad40739233e..829d5f6a24b 100644 --- a/tests/target/configs/use_small_heuristics/default.rs +++ b/tests/target/configs/use_small_heuristics/default.rs @@ -33,6 +33,6 @@ fn format_let_else() { let Some(c) = opt else { return }; let Some(d) = some_very_very_very_very_long_name else { - return; + return }; } diff --git a/tests/target/configs/use_small_heuristics/off.rs b/tests/target/configs/use_small_heuristics/off.rs index b0b4e4ee49f..90e1e1c976d 100644 --- a/tests/target/configs/use_small_heuristics/off.rs +++ b/tests/target/configs/use_small_heuristics/off.rs @@ -28,14 +28,14 @@ fn format_let_else() { let Some(a) = opt else {}; let Some(b) = opt else { - return; + return }; let Some(c) = opt else { - return; + return }; let Some(d) = some_very_very_very_very_long_name else { - return; + return }; } diff --git a/tests/target/expr.rs b/tests/target/expr.rs index 187a1dc976a..8ce3546f72a 100644 --- a/tests/target/expr.rs +++ b/tests/target/expr.rs @@ -125,7 +125,7 @@ fn bar() { loop { if true { - break; + break } } @@ -536,18 +536,14 @@ fn issue3226() { { { { - return Err( - ErrorKind::ManagementInterfaceError("Server exited unexpectedly").into(), - ); + return Err(ErrorKind::ManagementInterfaceError("Server exited unexpectedly").into()) } } } { { { - break Err( - ErrorKind::ManagementInterfaceError("Server exited unexpectedlyy").into(), - ); + break Err(ErrorKind::ManagementInterfaceError("Server exited unexpectedlyy").into()) } } } diff --git a/tests/target/issue-3213/version_two.rs b/tests/target/issue-3213/version_two.rs index de93d04ba95..ee5b2ec56d8 100644 --- a/tests/target/issue-3213/version_two.rs +++ b/tests/target/issue-3213/version_two.rs @@ -3,7 +3,7 @@ fn foo() { match 0 { 0 => { - return AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA; + return AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA } 1 => { AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA diff --git a/tests/target/let_else.rs b/tests/target/let_else.rs index 528d2929734..c222498bb4a 100644 --- a/tests/target/let_else.rs +++ b/tests/target/let_else.rs @@ -21,7 +21,7 @@ fn main() { let Some(x) = opt else { let y = 1; - return y; + return y }; let Some(x) = y.foo( @@ -54,13 +54,13 @@ fn with_comments_around_else_keyword() { let Some(x) = opt /* pre else keyword block-comment */ else { - return; + return }; let Some(x) = opt else /* post else keyword block-comment */ { - return; + return }; let Some(x) = opt @@ -68,19 +68,19 @@ fn with_comments_around_else_keyword() { else /* post else keyword block-comment */ { - return; + return }; let Some(x) = opt // pre else keyword line-comment else { - return; + return }; let Some(x) = opt else // post else keyword line-comment { - return; + return }; let Some(x) = opt @@ -88,7 +88,7 @@ fn with_comments_around_else_keyword() { else // post else keyword line-comment { - return; + return }; } @@ -106,7 +106,7 @@ fn unbreakable_initializer_expr_pre_formatting_let_else_length_near_max_width() // and the else block is formatted over multiple lines because we can't fit the // else block on the same line as the initializer expr. let Some(x) = some_really_really_really_really_really_really_really_long_name___B else { - return; + return }; // Pre Formatting: @@ -126,7 +126,7 @@ fn unbreakable_initializer_expr_pre_formatting_let_else_length_near_max_width() // and the else block is formatted over multiple lines because we can't fit the // else block on the same line as the initializer expr. let Some(x) = some_really_really_really_really_really_really_really_long_name__D else { - return; + return }; } @@ -138,7 +138,7 @@ fn unbreakable_initializer_expr_pre_formatting_length_up_to_opening_brace_near_m // and the else block is formatted over multiple lines because we can't fit the // else block on the same line as the initializer expr. let Some(x) = some_really_really_really_really_really_really_really_really_long_name___E else { - return; + return }; // Pre Formatting: @@ -148,7 +148,7 @@ fn unbreakable_initializer_expr_pre_formatting_length_up_to_opening_brace_near_m // They are formatted on the next line. let Some(x) = some_really_really_really_really_really_really_really_really_long_name_____F else { - return; + return }; } @@ -160,7 +160,7 @@ fn unbreakable_initializer_expr_pre_formatting_length_through_initializer_expr_n // They are formatted on the next line. let Some(x) = some_really_really_really_really_really_really_really_really_really_long_name___G else { - return; + return }; // Pre Formatting: @@ -171,7 +171,7 @@ fn unbreakable_initializer_expr_pre_formatting_length_through_initializer_expr_n let Some(x) = some_really_really_really_really_really_really_really_really_really_long_name____H else { - return; + return }; // Pre Formatting: @@ -186,7 +186,7 @@ fn unbreakable_initializer_expr_pre_formatting_length_through_initializer_expr_n let Some(x) = some_really_really_really_really_really_really_really_really_really_really_long_name______I else { - return; + return }; // Pre Formatting: @@ -240,7 +240,7 @@ fn with_trailing_try_operator() { &ranking_rule_universes[cur_ranking_rule_index], )? else { - return; + return }; // Maybe this is a workaround? @@ -249,7 +249,7 @@ fn with_trailing_try_operator() { logger, &ranking_rule_universes[cur_ranking_rule_index], ) else { - return; + return }; } From 8f944d0bd15af955c0ac6adce8a797de9a6508aa Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 17 Apr 2024 18:41:14 -0400 Subject: [PATCH 3/6] Account for previous double-counting of semicolon size within format_expr --- src/config/mod.rs | 2 +- src/expr.rs | 6 ++++-- src/stmt.rs | 14 ++++++++++++-- src/utils.rs | 25 +++++++++++++++++++++++++ tests/target/expr.rs | 8 ++++++-- 5 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/config/mod.rs b/src/config/mod.rs index 04f4f212fc3..aa973e08646 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -673,7 +673,7 @@ force_multiline_blocks = false fn_params_layout = "Tall" brace_style = "SameLineWhere" control_brace_style = "AlwaysSameLine" -trailing_semicolon = preserve +trailing_semicolon = "Preserve" trailing_comma = "Vertical" match_block_trailing_comma = false blank_lines_upper_bound = 1 diff --git a/src/expr.rs b/src/expr.rs index 6a21d88ac9d..ce93a3c7b5c 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -32,7 +32,7 @@ use crate::types::{rewrite_path, PathContext}; use crate::utils::{ colon_spaces, contains_skip, count_newlines, filtered_str_fits, first_line_ends_with, inner_attributes, last_line_extendable, last_line_width, mk_sp, outer_attributes, - semicolon_for_expr, unicode_str_width, wrap_str, + semicolon_for_expr_extra_hacky_double_counted_spacing, unicode_str_width, wrap_str, }; use crate::vertical::rewrite_with_alignment; use crate::visitor::FmtVisitor; @@ -64,7 +64,9 @@ pub(crate) fn format_expr( if contains_skip(&*expr.attrs) { return Some(context.snippet(expr.span()).to_owned()); } - let shape = if expr_type == ExprType::Statement && semicolon_for_expr(context, expr) { + let shape = if expr_type == ExprType::Statement + && semicolon_for_expr_extra_hacky_double_counted_spacing(context, expr) + { shape.sub_width(1)? } else { shape diff --git a/src/stmt.rs b/src/stmt.rs index e3fe4ebca11..50b3eb86501 100644 --- a/src/stmt.rs +++ b/src/stmt.rs @@ -8,7 +8,7 @@ use crate::rewrite::{Rewrite, RewriteContext}; use crate::shape::Shape; use crate::source_map::LineRangeUtils; use crate::spanned::Spanned; -use crate::utils::semicolon_for_stmt; +use crate::utils::{semicolon_for_expr, semicolon_for_stmt}; pub(crate) struct Stmt<'a> { inner: &'a ast::Stmt, @@ -116,7 +116,7 @@ fn format_stmt( let result = match stmt.kind { ast::StmtKind::Local(ref local) => local.rewrite(context, shape), - ast::StmtKind::Expr(ref ex) | ast::StmtKind::Semi(ref ex) => { + ast::StmtKind::Semi(ref ex) => { let suffix = if semicolon_for_stmt(context, stmt, is_last_expr) { ";" } else { @@ -126,6 +126,16 @@ fn format_stmt( let shape = shape.sub_width(suffix.len())?; format_expr(ex, expr_type, context, shape).map(|s| s + suffix) } + ast::StmtKind::Expr(ref ex) => { + let suffix = if semicolon_for_expr(context, ex) { + ";" + } else { + "" + }; + + let shape = shape.sub_width(suffix.len())?; + format_expr(ex, expr_type, context, shape).map(|s| s + suffix) + } ast::StmtKind::MacCall(..) | ast::StmtKind::Item(..) | ast::StmtKind::Empty => None, }; result.and_then(|res| recover_comment_removed(res, stmt.span(), context)) diff --git a/src/utils.rs b/src/utils.rs index 26d867a1761..56016425f9a 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -290,6 +290,31 @@ pub(crate) fn semicolon_for_expr(context: &RewriteContext<'_>, expr: &ast::Expr) } } +/// Previously, we used to have `trailing_semicolon = always` enabled, and due to +/// a bug between `format_stmt` and `format_expr`, we used to subtract the size of +/// `;` *TWICE* from the shape. This means that an expr that would fit onto a line +/// of, e.g. 99 (limit 100) after subtracting one for the semicolon would still be +/// wrapped. +/// +/// This function reimplements the old heuristic of double counting the "phantom" +/// semicolon that should have already been accounted for, to not break existing +/// formatting with the `trailing_semicolon = preserve` behavior. +#[inline] +pub(crate) fn semicolon_for_expr_extra_hacky_double_counted_spacing( + context: &RewriteContext<'_>, + expr: &ast::Expr, +) -> bool { + match expr.kind { + ast::ExprKind::Ret(..) | ast::ExprKind::Continue(..) | ast::ExprKind::Break(..) => { + match context.config.trailing_semicolon() { + TrailingSemicolon::Always | TrailingSemicolon::Preserve => true, + TrailingSemicolon::Never => false, + } + } + _ => false, + } +} + #[inline] pub(crate) fn semicolon_for_stmt( context: &RewriteContext<'_>, diff --git a/tests/target/expr.rs b/tests/target/expr.rs index 8ce3546f72a..8237c22c533 100644 --- a/tests/target/expr.rs +++ b/tests/target/expr.rs @@ -536,14 +536,18 @@ fn issue3226() { { { { - return Err(ErrorKind::ManagementInterfaceError("Server exited unexpectedly").into()) + return Err( + ErrorKind::ManagementInterfaceError("Server exited unexpectedly").into(), + ) } } } { { { - break Err(ErrorKind::ManagementInterfaceError("Server exited unexpectedlyy").into()) + break Err( + ErrorKind::ManagementInterfaceError("Server exited unexpectedlyy").into(), + ) } } } From 3126770aa4d1c0e381f01e8c1fa73b854e786ad9 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 17 Apr 2024 21:08:24 -0400 Subject: [PATCH 4/6] Apply review comments --- Configurations.md | 19 +++++++++---------- src/expr.rs | 2 +- src/utils.rs | 4 ++-- tests/source/return-expr-version-one.rs | 5 +++++ .../configs/trailing_semicolon/always.rs | 2 +- .../configs/trailing_semicolon/never.rs | 2 +- .../configs/trailing_semicolon/preserve.rs | 2 +- .../issue-5797/retain_trailing_semicolon.rs | 2 +- tests/target/return-expr-version-one.rs | 7 +++++++ tests/target/return-expr-version-two.rs | 5 +++++ 10 files changed, 33 insertions(+), 17 deletions(-) create mode 100644 tests/source/return-expr-version-one.rs create mode 100644 tests/target/return-expr-version-one.rs create mode 100644 tests/target/return-expr-version-two.rs diff --git a/Configurations.md b/Configurations.md index 6d858fb5da5..392e8d42bf8 100644 --- a/Configurations.md +++ b/Configurations.md @@ -2780,32 +2780,31 @@ See also: [`match_block_trailing_comma`](#match_block_trailing_comma). Add trailing semicolon after break, continue and return -- **Default value**: `preserve` -- **Possible values**: `always`, `never`, `preserve` +- **Default value**: `Preserve` +- **Possible values**: `Preserve`, `Always`, `Never` - **Stable**: No (tracking issue: [#3378](https://github.com/rust-lang/rustfmt/issues/3378)) -#### `always` (default): +#### `Preserve` (default): ```rust fn foo() -> usize { return 0; } -``` -#### `never`: -```rust -fn foo() -> usize { +fn bar() -> usize { return 0 } ``` - -#### `preserve`: +#### `Always`: ```rust fn foo() -> usize { return 0; } +``` -fn bar() -> usize { +#### `Never`: +```rust +fn foo() -> usize { return 0 } ``` diff --git a/src/expr.rs b/src/expr.rs index ce93a3c7b5c..a46cad28157 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -64,7 +64,7 @@ pub(crate) fn format_expr( if contains_skip(&*expr.attrs) { return Some(context.snippet(expr.span()).to_owned()); } - let shape = if expr_type == ExprType::Statement + let shape = if context.config.version() == Version::One && expr_type == ExprType::Statement && semicolon_for_expr_extra_hacky_double_counted_spacing(context, expr) { shape.sub_width(1)? diff --git a/src/utils.rs b/src/utils.rs index 56016425f9a..63d374ee278 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -290,7 +290,7 @@ pub(crate) fn semicolon_for_expr(context: &RewriteContext<'_>, expr: &ast::Expr) } } -/// Previously, we used to have `trailing_semicolon = always` enabled, and due to +/// Previously, we used to have `trailing_semicolon = Always` enabled, and due to /// a bug between `format_stmt` and `format_expr`, we used to subtract the size of /// `;` *TWICE* from the shape. This means that an expr that would fit onto a line /// of, e.g. 99 (limit 100) after subtracting one for the semicolon would still be @@ -298,7 +298,7 @@ pub(crate) fn semicolon_for_expr(context: &RewriteContext<'_>, expr: &ast::Expr) /// /// This function reimplements the old heuristic of double counting the "phantom" /// semicolon that should have already been accounted for, to not break existing -/// formatting with the `trailing_semicolon = preserve` behavior. +/// formatting with the `trailing_semicolon = Preserve` behavior for Version = One. #[inline] pub(crate) fn semicolon_for_expr_extra_hacky_double_counted_spacing( context: &RewriteContext<'_>, diff --git a/tests/source/return-expr-version-one.rs b/tests/source/return-expr-version-one.rs new file mode 100644 index 00000000000..ee2e0d355d8 --- /dev/null +++ b/tests/source/return-expr-version-one.rs @@ -0,0 +1,5 @@ +// rustfmt-version: One + +fn main() { + return hellooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo(arg); +} diff --git a/tests/target/configs/trailing_semicolon/always.rs b/tests/target/configs/trailing_semicolon/always.rs index 764ef8792bc..2f260b8e8bf 100644 --- a/tests/target/configs/trailing_semicolon/always.rs +++ b/tests/target/configs/trailing_semicolon/always.rs @@ -1,4 +1,4 @@ -// rustfmt-trailing_semicolon: always +// rustfmt-trailing_semicolon: Always #![feature(loop_break_value)] diff --git a/tests/target/configs/trailing_semicolon/never.rs b/tests/target/configs/trailing_semicolon/never.rs index f4fce216f1a..b1a0e55b81a 100644 --- a/tests/target/configs/trailing_semicolon/never.rs +++ b/tests/target/configs/trailing_semicolon/never.rs @@ -1,4 +1,4 @@ -// rustfmt-trailing_semicolon: never +// rustfmt-trailing_semicolon: Never #![feature(loop_break_value)] diff --git a/tests/target/configs/trailing_semicolon/preserve.rs b/tests/target/configs/trailing_semicolon/preserve.rs index 7804f230a81..b7074b877fd 100644 --- a/tests/target/configs/trailing_semicolon/preserve.rs +++ b/tests/target/configs/trailing_semicolon/preserve.rs @@ -1,4 +1,4 @@ -// rustfmt-trailing_semicolon: never +// rustfmt-trailing_semicolon: Never #![feature(loop_break_value)] diff --git a/tests/target/issue-5797/retain_trailing_semicolon.rs b/tests/target/issue-5797/retain_trailing_semicolon.rs index 6eaf426cf5f..d01925505b6 100644 --- a/tests/target/issue-5797/retain_trailing_semicolon.rs +++ b/tests/target/issue-5797/retain_trailing_semicolon.rs @@ -1,4 +1,4 @@ -// rustfmt-trailing_semicolon: never +// rustfmt-trailing_semicolon: Never fn foo() {} fn main() { diff --git a/tests/target/return-expr-version-one.rs b/tests/target/return-expr-version-one.rs new file mode 100644 index 00000000000..6bd8a52441f --- /dev/null +++ b/tests/target/return-expr-version-one.rs @@ -0,0 +1,7 @@ +// rustfmt-version: One + +fn main() { + return hellooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo( + arg, + ); +} diff --git a/tests/target/return-expr-version-two.rs b/tests/target/return-expr-version-two.rs new file mode 100644 index 00000000000..66fc5105d3f --- /dev/null +++ b/tests/target/return-expr-version-two.rs @@ -0,0 +1,5 @@ +// rustfmt-version: Two + +fn main() { + return hellooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo(arg); +} From 922e8bad0a33586de16fe9410c167ac18df5055e Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 17 Apr 2024 21:08:28 -0400 Subject: [PATCH 5/6] Self format rustfmt --- src/closures.rs | 30 +++++++++++++----------------- src/expr.rs | 3 ++- src/macros.rs | 7 +------ 3 files changed, 16 insertions(+), 24 deletions(-) diff --git a/src/closures.rs b/src/closures.rs index 5bf29441b54..52f17a2f09c 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -391,26 +391,22 @@ pub(crate) fn rewrite_last_closure( // We force to use block for the body of the closure for certain kinds of expressions. if is_block_closure_forced(context, body) { - return rewrite_closure_with_block(body, &prefix, context, body_shape).map( - |body_str| { - match fn_decl.output { - ast::FnRetTy::Default(..) if body_str.lines().count() <= 7 => { - // If the expression can fit in a single line, we need not force block - // closure. However, if the closure has a return type, then we must - // keep the blocks. - match rewrite_closure_expr(body, &prefix, context, shape) { - Some(single_line_body_str) - if !single_line_body_str.contains('\n') => - { - single_line_body_str - } - _ => body_str, + return rewrite_closure_with_block(body, &prefix, context, body_shape).map(|body_str| { + match fn_decl.output { + ast::FnRetTy::Default(..) if body_str.lines().count() <= 7 => { + // If the expression can fit in a single line, we need not force block + // closure. However, if the closure has a return type, then we must + // keep the blocks. + match rewrite_closure_expr(body, &prefix, context, shape) { + Some(single_line_body_str) if !single_line_body_str.contains('\n') => { + single_line_body_str } + _ => body_str, } - _ => body_str, } - }, - ); + _ => body_str, + } + }); } // When overflowing the closure which consists of a single control flow expression, diff --git a/src/expr.rs b/src/expr.rs index a46cad28157..2a15ac08b2b 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -64,7 +64,8 @@ pub(crate) fn format_expr( if contains_skip(&*expr.attrs) { return Some(context.snippet(expr.span()).to_owned()); } - let shape = if context.config.version() == Version::One && expr_type == ExprType::Statement + let shape = if context.config.version() == Version::One + && expr_type == ExprType::Statement && semicolon_for_expr_extra_hacky_double_counted_spacing(context, expr) { shape.sub_width(1)? diff --git a/src/macros.rs b/src/macros.rs index 6e114c76f26..3329f9a2db6 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -240,12 +240,7 @@ fn rewrite_macro_inner( } = match parse_macro_args(context, ts, style, is_forced_bracket) { Some(args) => args, None => { - return return_macro_parse_failure_fallback( - context, - shape.indent, - position, - mac.span(), - ); + return return_macro_parse_failure_fallback(context, shape.indent, position, mac.span()); } }; From 1973b90ba986ef109326cd4f10956f21c169d3ee Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 17 Apr 2024 21:14:54 -0400 Subject: [PATCH 6/6] Comments --- src/config/options.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/config/options.rs b/src/config/options.rs index 04c78ffc39a..d3902e49cab 100644 --- a/src/config/options.rs +++ b/src/config/options.rs @@ -50,9 +50,15 @@ pub enum ControlBraceStyle { } #[config_type] +/// How to treat the semicolon that is optional for final diverging expressions +/// (`return`/`break`/`continue`). pub enum TrailingSemicolon { + /// Always rewrite `return;` and `break;` expressions to have a trailing semicolon, + /// unless the block is a single-line block, e.g. `let PAT = e else { return }`. Always, + /// Always return `return` and `break` expressions to remove the trailing semicolon. Never, + /// Preserve an existing trailing semicolon if it exists. Preserve, }