Merge #876

876: Fix join_lines not adding a comma after join_single_expr_block with match arm r=matklad a=vipentti

Fixes #868 

Co-authored-by: Ville Penttinen <[email protected]>

4 files changed, 143 insertions, 10 deletions

diff --git a/crates/ra_assists/src/flip_comma.rs b/crates/ra_assists/src/flip_comma.rs
index 08644d720..0d4a789fc 100644
--- a/crates/ra_assists/src/flip_comma.rs
+++ b/crates/ra_assists/src/flip_comma.rs
@@ -2,9 +2,10 @@ use hir::db::HirDatabase;
 use ra_syntax::{
     Direction,
     SyntaxKind::COMMA,
+    algo::non_trivia_sibling,
 };
 
-use crate::{AssistCtx, Assist, non_trivia_sibling};
+use crate::{AssistCtx, Assist};
 
 pub(crate) fn flip_comma(mut ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
     let comma = ctx.leaf_at_offset().find(|leaf| leaf.kind() == COMMA)?;
diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs
index 7bd9b5ae6..e1e899edc 100644
--- a/crates/ra_assists/src/lib.rs
+++ b/crates/ra_assists/src/lib.rs
@@ -10,7 +10,7 @@ mod assist_ctx;
 use itertools::Itertools;
 
 use ra_text_edit::TextEdit;
-use ra_syntax::{TextRange, TextUnit, SyntaxNode, Direction};
+use ra_syntax::{TextRange, TextUnit};
 use ra_db::FileRange;
 use hir::db::HirDatabase;
 
@@ -104,10 +104,6 @@ fn all_assists<DB: HirDatabase>() -> &'static [fn(AssistCtx<DB>) -> Option<Assis
     ]
 }
 
-fn non_trivia_sibling(node: &SyntaxNode, direction: Direction) -> Option<&SyntaxNode> {
-    node.siblings(direction).skip(1).find(|node| !node.kind().is_trivia())
-}
-
 #[cfg(test)]
 mod helpers {
     use hir::mock::MockDatabase;
diff --git a/crates/ra_ide_api_light/src/join_lines.rs b/crates/ra_ide_api_light/src/join_lines.rs
index 949ee1544..b5bcd62fb 100644
--- a/crates/ra_ide_api_light/src/join_lines.rs
+++ b/crates/ra_ide_api_light/src/join_lines.rs
@@ -2,8 +2,9 @@ use itertools::Itertools;
 use ra_syntax::{
     SourceFile, TextRange, TextUnit, AstNode, SyntaxNode,
     SyntaxKind::{self, WHITESPACE, COMMA, R_CURLY, R_PAREN, R_BRACK},
-    algo::find_covering_node,
+    algo::{find_covering_node, non_trivia_sibling},
     ast,
+    Direction,
 };
 use ra_fmt::{
     compute_ws, extract_trivial_expression
@@ -120,11 +121,30 @@ fn remove_newline(
     }
 }
 
+fn has_comma_after(node: &SyntaxNode) -> bool {
+    match non_trivia_sibling(node, Direction::Next) {
+        Some(n) => n.kind() == COMMA,
+        _ => false,
+    }
+}
+
 fn join_single_expr_block(edit: &mut TextEditBuilder, node: &SyntaxNode) -> Option<()> {
     let block = ast::Block::cast(node.parent()?)?;
     let block_expr = ast::BlockExpr::cast(block.syntax().parent()?)?;
     let expr = extract_trivial_expression(block)?;
-    edit.replace(block_expr.syntax().range(), expr.syntax().text().to_string());
+
+    let block_range = block_expr.syntax().range();
+    let mut buf = expr.syntax().text().to_string();
+
+    // Match block needs to have a comma after the block
+    if let Some(match_arm) = block_expr.syntax().parent().and_then(ast::MatchArm::cast) {
+        if !has_comma_after(match_arm.syntax()) {
+            buf.push(',');
+        }
+    }
+
+    edit.replace(block_range, buf);
+
     Some(())
 }
 
@@ -208,7 +228,6 @@ fn foo() {
     }
 
     #[test]
-    #[ignore] // FIXME: https://github.com/rust-analyzer/rust-analyzer/issues/868
     fn join_lines_adds_comma_for_block_in_match_arm() {
         check_join_lines(
             r"
@@ -231,6 +250,118 @@ fn foo(e: Result<U, V>) {
     }
 
     #[test]
+    fn join_lines_keeps_comma_for_block_in_match_arm() {
+        // We already have a comma
+        check_join_lines(
+            r"
+fn foo(e: Result<U, V>) {
+    match e {
+        Ok(u) => <|>{
+            u.foo()
+        },
+        Err(v) => v,
+    }
+}",
+            r"
+fn foo(e: Result<U, V>) {
+    match e {
+        Ok(u) => <|>u.foo(),
+        Err(v) => v,
+    }
+}",
+        );
+
+        // comma with whitespace between brace and ,
+        check_join_lines(
+            r"
+fn foo(e: Result<U, V>) {
+    match e {
+        Ok(u) => <|>{
+            u.foo()
+        }    ,
+        Err(v) => v,
+    }
+}",
+            r"
+fn foo(e: Result<U, V>) {
+    match e {
+        Ok(u) => <|>u.foo()    ,
+        Err(v) => v,
+    }
+}",
+        );
+
+        // comma with newline between brace and ,
+        check_join_lines(
+            r"
+fn foo(e: Result<U, V>) {
+    match e {
+        Ok(u) => <|>{
+            u.foo()
+        }
+        ,
+        Err(v) => v,
+    }
+}",
+            r"
+fn foo(e: Result<U, V>) {
+    match e {
+        Ok(u) => <|>u.foo()
+        ,
+        Err(v) => v,
+    }
+}",
+        );
+    }
+
+    #[test]
+    fn join_lines_keeps_comma_with_single_arg_tuple() {
+        // A single arg tuple
+        check_join_lines(
+            r"
+fn foo() {
+    let x = (<|>{
+       4
+    },);
+}",
+            r"
+fn foo() {
+    let x = (<|>4,);
+}",
+        );
+
+        // single arg tuple with whitespace between brace and comma
+        check_join_lines(
+            r"
+fn foo() {
+    let x = (<|>{
+       4
+    }   ,);
+}",
+            r"
+fn foo() {
+    let x = (<|>4   ,);
+}",
+        );
+
+        // single arg tuple with newline between brace and comma
+        check_join_lines(
+            r"
+fn foo() {
+    let x = (<|>{
+       4
+    }
+    ,);
+}",
+            r"
+fn foo() {
+    let x = (<|>4
+    ,);
+}",
+        );
+    }
+
+    #[test]
     fn test_join_lines_use_items_left() {
         // No space after the '{'
         check_join_lines(
diff --git a/crates/ra_syntax/src/algo.rs b/crates/ra_syntax/src/algo.rs
index e8cf0d4b5..e2b4f0388 100644
--- a/crates/ra_syntax/src/algo.rs
+++ b/crates/ra_syntax/src/algo.rs
@@ -2,7 +2,7 @@ pub mod visit;
 
 use rowan::TransparentNewType;
 
-use crate::{SyntaxNode, TextRange, TextUnit, AstNode};
+use crate::{SyntaxNode, TextRange, TextUnit, AstNode, Direction};
 
 pub use rowan::LeafAtOffset;
 
@@ -29,6 +29,11 @@ pub fn find_node_at_offset<N: AstNode>(syntax: &SyntaxNode, offset: TextUnit) ->
     find_leaf_at_offset(syntax, offset).find_map(|leaf| leaf.ancestors().find_map(N::cast))
 }
 
+/// Finds the first sibling in the given direction which is not `trivia`
+pub fn non_trivia_sibling(node: &SyntaxNode, direction: Direction) -> Option<&SyntaxNode> {
+    node.siblings(direction).skip(1).find(|node| !node.kind().is_trivia())
+}
+
 pub fn find_covering_node(root: &SyntaxNode, range: TextRange) -> &SyntaxNode {
     SyntaxNode::from_repr(root.0.covering_node(range))
 }