Use comma-leading layout for tuples with open-ended elements

Replace parenthesis wrapping with comma-leading multiline layout when a
non-last tuple element is open-ended (lambda, if-then-else, match,
try-with, try-finally). This removes wrapInfixAppRhsInParenIfNeeded
and the ad-hoc containsLambdaOrMatchExpr check in genTupleMultiline,
replacing both with requiresMultilineToPreserveSemantics.

Also extends isOpenEndedExpression to include match, function,
try-with, and try-finally expressions.

Author: nojaf

src/Fantomas.Core.Tests/ControlStructureTests.fs

@@ -417,7 +417,7 @@ elif true then ()
 """
 
 [<Test>]
-let ``multiline if in tuple`` () =
+let ``multiline if in tuple uses comma-leading layout`` () =
     formatSourceString
         """
 (if true then 1 else 2
@@ -428,7 +428,8 @@ let ``multiline if in tuple`` () =
     |> should
         equal
         """
-((if true then 1 else 2), 3)
+(if true then 1 else 2
+ , 3)
 """
 
 // https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-if-expressions

src/Fantomas.Core.Tests/RequiresMultilineToPreserveSemanticsTests.fs

@@ -141,3 +141,91 @@ x = fun y -> y
 |> g
 |> h
 """
+
+[<Test>]
+let ``open-ended expression in middle of chained pipe stays multiline`` () =
+    formatSourceString
+        """
+a
+|> fun x -> x + 1
+|> h
+"""
+        config
+    |> prepend newline
+    |> should
+        equal
+        """
+a
+|> fun x -> x + 1
+|> h
+"""
+
+// Expr.Tuple (open-ended non-last element)
+
+[<Test>]
+let ``lambda as non-last tuple element stays multiline`` () =
+    formatSourceString
+        """
+fun x -> x
+, y
+"""
+        config
+    |> prepend newline
+    |> should
+        equal
+        """
+fun x -> x
+, y
+"""
+
+[<Test>]
+let ``nested open-ended as non-last tuple element stays multiline`` () =
+    formatSourceString
+        """
+x = fun y -> y
+, z
+"""
+        config
+    |> prepend newline
+    |> should
+        equal
+        """
+x = fun y -> y
+, z
+"""
+
+[<Test>]
+let ``if-then-else as non-last tuple element stays multiline`` () =
+    formatSourceString
+        """
+if a then b else c
+, y
+"""
+        config
+    |> prepend newline
+    |> should
+        equal
+        """
+if a then b else c
+, y
+"""
+
+[<Test>]
+let ``match as non-last tuple element stays multiline`` () =
+    formatSourceString
+        """
+match x with
+| true -> 1
+| false -> 2
+, y
+"""
+        config
+    |> prepend newline
+    |> should
+        equal
+        """
+match x with
+| true -> 1
+| false -> 2
+, y
+"""

src/Fantomas.Core.Tests/TupleTests.fs

@@ -6,7 +6,7 @@ open FsUnit
 open Fantomas.Core.Tests.TestHelpers
 
 [<Test>]
-let ``tuple with lambda should add parenthesis`` () =
+let ``tuple with lambda stays multiline to preserve semantics`` () =
     formatSourceString
         """
 let private carouselSample =
@@ -19,7 +19,10 @@ let private carouselSample =
     |> should
         equal
         """let private carouselSample =
-    FunctionComponent.Of<obj>((fun _ -> fragment [] []), "CarouselSample")
+    FunctionComponent.Of<obj>(
+        fun _ -> fragment [] []
+        , "CarouselSample"
+    )
 """
 
 [<Test>]
@@ -231,7 +234,7 @@ let (var1withAVeryLongLongLongLongLongLongName,
 """
 
 [<Test>]
-let ``tuple with if/then/else, 1319`` () =
+let ``tuple with if/then/else uses comma-leading layout, 1319`` () =
     formatSourceString
         """
 let y =
@@ -248,18 +251,18 @@ let y =
         equal
         """
 let y =
-    (if String.IsNullOrWhiteSpace(args) then
-         ""
-     elif args.StartsWith("(") then
-         args
-     elif
-         v.CurriedParameterGroups.Count > 1
-         && (not verboseMode)
-     then
-         " " + args
-     else
-         sprintf "(%s)" args),
-    namesWithIndices
+    if String.IsNullOrWhiteSpace(args) then
+        ""
+    elif args.StartsWith("(") then
+        args
+    elif
+        v.CurriedParameterGroups.Count > 1
+        && (not verboseMode)
+    then
+        " " + args
+    else
+        sprintf "(%s)" args
+    , namesWithIndices
 """
 
 [<Test>]
@@ -406,7 +409,7 @@ let shiftTimes localDate (start: Utc, duration) =
 """
 
 [<Test>]
-let ``if then expression inside object instantiation breaks when formatted, 2819`` () =
+let ``if then expression in object instantiation uses comma-leading layout, 2819`` () =
     formatSourceString
         """
 type Chapter() =
@@ -433,11 +436,15 @@ type Chapter() =
     member val Title: string option = Unchecked.defaultof<_> with get, set
     member val Url: string = Unchecked.defaultof<_> with get, set
 
-let c = Chapter(Title = (if true then Some "" else None), Url = "")
+let c =
+    Chapter(
+        Title = if true then Some "" else None
+        , Url = ""
+    )
 """
 
 [<Test>]
-let ``object instantiation with ifthenelse in tuple`` () =
+let ``object instantiation with ifthenelse in tuple uses comma-leading layout`` () =
     formatSourceString
         """
 type Chapter() =
@@ -463,11 +470,15 @@ type Chapter() =
     member val Title: string option = Unchecked.defaultof<_> with get, set
     member val Url: string = Unchecked.defaultof<_> with get, set
 
-let c = Chapter((if true then Some "" else None), "")
+let c =
+    Chapter(
+        if true then Some "" else None
+        , ""
+    )
 """
 
 [<Test>]
-let ``infixapp with ifthen rhs in tuple`` () =
+let ``infixapp with ifthen rhs in tuple uses comma-leading layout to preserve semantics`` () =
     formatSourceString
         """
 let a = 0
@@ -495,13 +506,14 @@ let d = 2
 
 let _ =
     try
-        a <> (if b then c else d), b
+        a <> if b then c else d
+        , b
     with ex ->
         false, false
 """
 
 [<Test>]
-let ``infixapp with lambda rhs in tuple`` () =
+let ``infixapp with lambda rhs in tuple uses comma-leading layout to preserve semantics`` () =
     formatSourceString
         """
 a |> fun b -> if b then 0 else 1
@@ -513,7 +525,8 @@ a |> fun b -> if b then 0 else 1
     |> should
         equal
         """
-a |> (fun b -> if b then 0 else 1), 2
+a |> fun b -> if b then 0 else 1
+, 2
 """
 
 [<Test>]

src/Fantomas.Core/CodePrinter.fs

@@ -363,7 +363,12 @@ let rec isOpenEndedExpression (e: Expr) =
     | Expr.Lambda _
     | Expr.IfThen _
     | Expr.IfThenElse _
-    | Expr.IfThenElif _ -> true
+    | Expr.IfThenElif _
+    | Expr.Match _
+    | Expr.MatchLambda _
+    | Expr.TryWith _
+    | Expr.TryWithSingleClause _
+    | Expr.TryFinally _ -> true
     | Expr.InfixApp node -> isOpenEndedExpression node.RightHandSide
     | Expr.SameInfixApps node ->
         match List.tryLast node.SubsequentExpressions with
@@ -1940,81 +1945,37 @@ let genMultilineFunctionApplicationArguments (argExpr: Expr) =
     | _ -> genExpr argExpr
 
 let genTupleExpr (node: ExprTupleNode) =
-    // if a tuple element is an InfixApp with a lambda or if-then-else expression on the rhs,
-    // we need to wrap the rhs in parenthesis to avoid a parse error caused by the higher precedence of "," over the rhs expression.
-    // see 2819
-    let wrapInfixAppRhsInParenIfNeeded expr =
-        match expr with
-        | Expr.InfixApp exprInfixAppNode ->
-            match exprInfixAppNode.RightHandSide with
-            | IsLambdaOrIfThenElse e ->
-                let parenNode =
-                    mkExprParenNode
-                        (SingleTextNode("(", Fantomas.FCS.Text.Range.range0))
-                        e
-                        (SingleTextNode(")", Fantomas.FCS.Text.Range.range0))
-                        Fantomas.FCS.Text.Range.range0
-
-                ExprInfixAppNode(
-                    exprInfixAppNode.LeftHandSide,
-                    exprInfixAppNode.Operator,
-                    parenNode,
-                    Fantomas.FCS.Text.Range.range0
-                )
-                |> Expr.InfixApp
-            | _ -> expr
-        | _ -> expr
-
     let shortExpression =
-        let lastIndex = Array.length node.Children - 1
-
-        coli sepNone node.Items (fun i c ->
+        coli sepNone node.Items (fun _i c ->
             match c with
-            | Choice1Of2 e ->
-                match e with
-                | IsLambdaOrIfThenElse e when i <> lastIndex -> sepOpenT +> genExpr e +> sepCloseT
-                | e -> genExpr (wrapInfixAppRhsInParenIfNeeded e)
+            | Choice1Of2 e -> genExpr e
             | Choice2Of2 comma -> genSingleTextNode comma +> addSpaceIfSpaceAfterComma)
 
     let longExpression = genTupleMultiline node
 
-    atCurrentColumn (expressionFitsOnRestOfLine shortExpression longExpression)
-    |> genNode node
+    let exprs = node.Items |> List.choose (function Choice1Of2 e -> Some e | _ -> None)
+
+    if requiresMultilineToPreserveSemantics exprs then
+        atCurrentColumn longExpression |> genNode node
+    else
+        atCurrentColumn (expressionFitsOnRestOfLine shortExpression longExpression)
+        |> genNode node
 
 let genTupleMultiline (node: ExprTupleNode) =
-    let containsLambdaOrMatchExpr =
-        // If the any items (expect the last) is a match/lambda
-        node.Items
-        |> List.chunkBySize 2
-        |> List.exists (fun pair ->
-            match pair with
-            | [ Choice1Of2 e; Choice2Of2 _ ] ->
-                match e with
-                | Expr.Match _
-                | Expr.Lambda _ -> true
-                | Expr.InfixApp node ->
-                    match node.RightHandSide with
-                    | Expr.Match _
-                    | Expr.Lambda _ -> true
-                    | _ -> false
-                | Expr.SameInfixApps node ->
-                    match List.last node.SubsequentExpressions with
-                    | _, Expr.Lambda _ -> true
-                    | _ -> false
-                | _ -> false
-            | _ -> false)
+    let exprs = node.Items |> List.choose (function Choice1Of2 e -> Some e | _ -> None)
 
-    let lastIndex = List.length node.Items - 1
+    // When a non-last element is open-ended (lambda, if-then-else, match, ...),
+    // the comma must start a new line so that it isn't swallowed by the preceding expression.
+    let commaLeading = requiresMultilineToPreserveSemantics exprs
 
-    let genItem idx =
+    let genItem _idx =
         function
         | Choice1Of2 e ->
             match e with
-            | IsIfThenElse _ when (idx < lastIndex) -> autoParenthesisIfExpressionExceedsPageWidth (genExpr e)
             | Expr.InfixApp node when (node.Operator.Text = "=") -> genNamedArgumentExpr node
             | _ -> genExpr e
         | Choice2Of2 comma ->
-            if containsLambdaOrMatchExpr then
+            if commaLeading then
                 sepNln +> genSingleTextNode comma +> sepSpace
             else
                 genSingleTextNode comma +> sepNln