Implement trailing_semicolon = "Preserve"

Configurations.md

@@ -2780,18 +2780,29 @@ 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**: `Preserve`, `Always`, `Never`
 - **Stable**: No (tracking issue: [#3378](https://github.com/rust-lang/rustfmt/issues/3378))
 
-#### `true` (default):
+#### `Preserve` (default):
+```rust
+fn foo() -> usize {
+    return 0;
+}
+
+fn bar() -> usize {
+    return 0
+}
+```
+
+#### `Always`:
 ```rust
 fn foo() -> usize {
     return 0;
 }
 ```
 
-#### `false`:
+#### `Never`:
 ```rust
 fn foo() -> usize {
     return 0

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,

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

src/config/options.rs

@@ -49,6 +49,19 @@ pub enum ControlBraceStyle {
     AlwaysNextLine,
 }
 
+#[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,
+}
+
 #[config_type]
 /// How to indent.
 pub enum IndentStyle {

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,10 @@ 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 context.config.version() == Version::One
+        && expr_type == ExprType::Statement
+        && semicolon_for_expr_extra_hacky_double_counted_spacing(context, expr)
+    {
         shape.sub_width(1)?
     } else {
         shape

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

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

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,15 +273,43 @@ 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;
     }
 
     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,
+    }
+}
+
+/// 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 for Version = One.
+#[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,
     }
@@ -301,7 +329,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,
         },

tests/source/return-expr-version-one.rs

@@ -0,0 +1,5 @@
+// rustfmt-version: One
+
+fn main() {
+    return hellooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo(arg);
+}

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,23 +26,23 @@ 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
         .attrs
         .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 (..) { .. })`",
-        ));
+        ))
     };
 }

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,23 +28,23 @@ 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
         .attrs
         .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 (..) { .. })`",
-        ));
+        ))
     };
 }