Merge pull request rust-lang#3102 from nrc/arm-guard-newline

Only put `{` on a newline in a match arm where necessary
rust-lang-ci · Oct 15, 2018 · 5f02be6 · 5f02be6
2 parents bc4414e + e2be62c
commit 5f02be6
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 68 deletions.
diff --git a/src/chains.rs b/src/chains.rs
@@ -74,7 +74,7 @@ use rewrite::{Rewrite, RewriteContext};
 use shape::Shape;
 use source_map::SpanUtils;
 use utils::{
-    first_line_width, last_line_extendable, last_line_width, mk_sp, rewrite_ident,
+    self, first_line_width, last_line_extendable, last_line_width, mk_sp, rewrite_ident,
     trimmed_last_line_width, wrap_str,
 };
 
@@ -130,7 +130,7 @@ enum ChainItemKind {
 impl ChainItemKind {
     fn is_block_like(&self, context: &RewriteContext, reps: &str) -> bool {
         match self {
-            ChainItemKind::Parent(ref expr) => is_block_expr(context, expr, reps),
+            ChainItemKind::Parent(ref expr) => utils::is_block_expr(context, expr, reps),
             ChainItemKind::MethodCall(..)
             | ChainItemKind::StructField(..)
             | ChainItemKind::TupleField(..)
@@ -845,38 +845,6 @@ impl<'a> ChainFormatter for ChainFormatterVisual<'a> {
     }
 }
 
-// States whether an expression's last line exclusively consists of closing
-// parens, braces, and brackets in its idiomatic formatting.
-fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool {
-    match expr.node {
-        ast::ExprKind::Mac(..)
-        | ast::ExprKind::Call(..)
-        | ast::ExprKind::MethodCall(..)
-        | ast::ExprKind::Array(..)
-        | ast::ExprKind::Struct(..)
-        | ast::ExprKind::While(..)
-        | ast::ExprKind::WhileLet(..)
-        | ast::ExprKind::If(..)
-        | ast::ExprKind::IfLet(..)
-        | ast::ExprKind::Block(..)
-        | ast::ExprKind::Loop(..)
-        | ast::ExprKind::ForLoop(..)
-        | ast::ExprKind::Match(..) => repr.contains('\n'),
-        ast::ExprKind::Paren(ref expr)
-        | ast::ExprKind::Binary(_, _, ref expr)
-        | ast::ExprKind::Index(_, ref expr)
-        | ast::ExprKind::Unary(_, ref expr)
-        | ast::ExprKind::Closure(_, _, _, _, ref expr, _)
-        | ast::ExprKind::Try(ref expr)
-        | ast::ExprKind::Yield(Some(ref expr)) => is_block_expr(context, expr, repr),
-        // This can only be a string lit
-        ast::ExprKind::Lit(_) => {
-            repr.contains('\n') && trimmed_last_line_width(repr) <= context.config.tab_spaces()
-        }
-        _ => false,
-    }
-}
-
 /// Remove try operators (`?`s) that appear in the given string. If removing
 /// them leaves an empty line, remove that line as well unless it is the first
 /// line (we need the first newline for detecting pre/post comment).

diff --git a/src/matches.rs b/src/matches.rs
@@ -247,52 +247,42 @@ fn rewrite_match_arm(
     } else {
         (mk_sp(arm.span().lo(), arm.span().lo()), String::new())
     };
-    let pats_str =
-        rewrite_match_pattern(context, &ptr_vec_to_ref_vec(&arm.pats), &arm.guard, shape)
-            .and_then(|pats_str| {
-                combine_strs_with_missing_comments(
-                    context,
-                    &attrs_str,
-                    &pats_str,
-                    missing_span,
-                    shape,
-                    false,
-                )
-            })?;
 
-    let arrow_span = mk_sp(arm.pats.last().unwrap().span.hi(), arm.body.span.lo());
-    rewrite_match_body(
-        context,
-        &arm.body,
-        &pats_str,
-        shape,
-        arm.guard.is_some(),
-        arrow_span,
-        is_last,
-    )
-}
-
-fn rewrite_match_pattern(
-    context: &RewriteContext,
-    pats: &[&ast::Pat],
-    guard: &Option<ast::Guard>,
-    shape: Shape,
-) -> Option<String> {
     // Patterns
     // 5 = ` => {`
     let pat_shape = shape.sub_width(5)?;
-    let pats_str = rewrite_multiple_patterns(context, pats, pat_shape)?;
+    let pats_str = rewrite_multiple_patterns(context, &ptr_vec_to_ref_vec(&arm.pats), pat_shape)?;
 
     // Guard
+    let block_like_pat = trimmed_last_line_width(&pats_str) <= context.config.tab_spaces();
+    let new_line_guard = pats_str.contains('\n') && !block_like_pat;
     let guard_str = rewrite_guard(
         context,
-        guard,
+        &arm.guard,
         shape,
         trimmed_last_line_width(&pats_str),
-        pats_str.contains('\n'),
+        new_line_guard,
     )?;
 
-    Some(format!("{}{}", pats_str, guard_str))
+    let lhs_str = combine_strs_with_missing_comments(
+        context,
+        &attrs_str,
+        &format!("{}{}", pats_str, guard_str),
+        missing_span,
+        shape,
+        false,
+    )?;
+
+    let arrow_span = mk_sp(arm.pats.last().unwrap().span.hi(), arm.body.span.lo());
+    rewrite_match_body(
+        context,
+        &arm.body,
+        &lhs_str,
+        shape,
+        guard_str.contains('\n'),
+        arrow_span,
+        is_last,
+    )
 }
 
 fn block_can_be_flattened<'a>(

diff --git a/src/utils.rs b/src/utils.rs
@@ -420,3 +420,35 @@ pub fn starts_with_newline(s: &str) -> bool {
 pub fn first_line_ends_with(s: &str, c: char) -> bool {
     s.lines().next().map_or(false, |l| l.ends_with(c))
 }
+
+// States whether an expression's last line exclusively consists of closing
+// parens, braces, and brackets in its idiomatic formatting.
+pub fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool {
+    match expr.node {
+        ast::ExprKind::Mac(..)
+        | ast::ExprKind::Call(..)
+        | ast::ExprKind::MethodCall(..)
+        | ast::ExprKind::Array(..)
+        | ast::ExprKind::Struct(..)
+        | ast::ExprKind::While(..)
+        | ast::ExprKind::WhileLet(..)
+        | ast::ExprKind::If(..)
+        | ast::ExprKind::IfLet(..)
+        | ast::ExprKind::Block(..)
+        | ast::ExprKind::Loop(..)
+        | ast::ExprKind::ForLoop(..)
+        | ast::ExprKind::Match(..) => repr.contains('\n'),
+        ast::ExprKind::Paren(ref expr)
+        | ast::ExprKind::Binary(_, _, ref expr)
+        | ast::ExprKind::Index(_, ref expr)
+        | ast::ExprKind::Unary(_, ref expr)
+        | ast::ExprKind::Closure(_, _, _, _, ref expr, _)
+        | ast::ExprKind::Try(ref expr)
+        | ast::ExprKind::Yield(Some(ref expr)) => is_block_expr(context, expr, repr),
+        // This can only be a string lit
+        ast::ExprKind::Lit(_) => {
+            repr.contains('\n') && trimmed_last_line_width(repr) <= context.config.tab_spaces()
+        }
+        _ => false,
+    }
+}
diff --git a/tests/source/match.rs b/tests/source/match.rs
@@ -536,3 +536,16 @@ fn issue_3030() {
             }
     }
 }
+
+fn issue_3005() {
+            match *token {
+                 Token::Dimension {
+                     value, ref unit, ..
+                 } if num_context.is_ok(context.parsing_mode, value) =>
+                 {
+                     return NoCalcLength::parse_dimension(context, value, unit)
+                         .map(LengthOrPercentage::Length)
+                         .map_err(|()| location.new_unexpected_token_error(token.clone()))
+                 },
+             }
+}
diff --git a/tests/target/match.rs b/tests/target/match.rs
@@ -564,3 +564,15 @@ fn issue_3030() {
             ) => {}
     }
 }
+
+fn issue_3005() {
+    match *token {
+        Token::Dimension {
+            value, ref unit, ..
+        } if num_context.is_ok(context.parsing_mode, value) => {
+            return NoCalcLength::parse_dimension(context, value, unit)
+                .map(LengthOrPercentage::Length)
+                .map_err(|()| location.new_unexpected_token_error(token.clone()))
+        }
+    }
+}