Parens: some more (#17012)

* Keep parens for prefix & infix apps in copy exprs * Better handling of nested arrow-sensitive constructs * Can't have bare typed exprs in records * Keep parens around unit in func/method invocation * We can't know purely from the syntax whether we need the double parens. * Handle app chains depending on pseudo-dot prec * Add space around underscores * Keep parens around hanging tuples * Handle some simple cases of #16999 * Update release notes * Update release notes
dotnet · Apr 10, 2024 · 253c6a6 · 253c6a6
1 parent 0993409
commit 253c6a6
Show file tree

Hide file tree

Showing 6 changed files with 435 additions and 33 deletions.
diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
@@ -8,7 +8,7 @@
 * Code generated files with > 64K methods and generated symbols crash when loaded. Use infered sequence points for debugging. ([Issue #16399](https://github.com/dotnet/fsharp/issues/16399), [#PR 16514](https://github.com/dotnet/fsharp/pull/16514))
 * `nameof Module` expressions and patterns are processed to link files in `--test:GraphBasedChecking`. ([PR #16550](https://github.com/dotnet/fsharp/pull/16550), [PR #16743](https://github.com/dotnet/fsharp/pull/16743))
 * Graph Based Checking doesn't throw on invalid parsed input so it can be used for IDE scenarios ([PR #16575](https://github.com/dotnet/fsharp/pull/16575), [PR #16588](https://github.com/dotnet/fsharp/pull/16588), [PR #16643](https://github.com/dotnet/fsharp/pull/16643))
-* Various parenthesization API fixes. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666), [PR #16901](https://github.com/dotnet/fsharp/pull/16901), [PR #16973](https://github.com/dotnet/fsharp/pull/16973))
+* Various parenthesization API fixes. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666), [PR #16901](https://github.com/dotnet/fsharp/pull/16901), [PR #16973](https://github.com/dotnet/fsharp/pull/16973), [PR #17012](https://github.com/dotnet/fsharp/pull/17012))
 * Keep parens for problematic exprs (`if`, `match`, etc.) in `$"{(…):N0}"`, `$"{(…),-3}"`, etc. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578))
 * Fix crash in DOTNET_SYSTEM_GLOBALIZATION_INVARIANT mode [#PR 16471](https://github.com/dotnet/fsharp/pull/16471))
 * Fix16572 - Fixed the preview feature enabling Is properties for union case did not work correctly with let .rec and .fsi files ([PR #16657](https://github.com/dotnet/fsharp/pull/16657))

diff --git a/docs/release-notes/.VisualStudio/17.10.md b/docs/release-notes/.VisualStudio/17.10.md
@@ -1,7 +1,7 @@
 ### Fixed
 
 * Show signature help mid-pipeline in more scenarios. ([PR #16462](https://github.com/dotnet/fsharp/pull/16462))
-* Various unneeded parentheses code fix improvements. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666), [PR #16789](https://github.com/dotnet/fsharp/pull/16789), [PR #16901](https://github.com/dotnet/fsharp/pull/16901))
+* Various unneeded parentheses code fix improvements. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666), [PR #16789](https://github.com/dotnet/fsharp/pull/16789), [PR #16901](https://github.com/dotnet/fsharp/pull/16901), [PR #17012](https://github.com/dotnet/fsharp/pull/17012))
 
 ### Changed
 

diff --git a/src/Compiler/Service/SynExpr.fs b/src/Compiler/Service/SynExpr.fs
@@ -472,6 +472,21 @@ module SynExpr =
                 | SynExpr.Lambda _ as expr -> Some expr
                 | _ -> None)
 
+        /// Matches a dangling arrow-sensitive construct.
+        [<return: Struct>]
+        let (|ArrowSensitive|_|) =
+            dangling (function
+                | SynExpr.Match _
+                | SynExpr.MatchBang _
+                | SynExpr.MatchLambda _
+                | SynExpr.TryWith _
+                | SynExpr.Lambda _
+                | SynExpr.Typed _
+                | SynExpr.TypeTest _
+                | SynExpr.Upcast _
+                | SynExpr.Downcast _ as expr -> Some expr
+                | _ -> None)
+
         /// Matches a nested dangling construct that could become problematic
         /// if the surrounding parens were removed.
         [<return: Struct>]
@@ -543,14 +558,14 @@ module SynExpr =
         let shouldBeParenthesizedInContext = shouldBeParenthesizedInContext getSourceLineStr
         let containsSensitiveIndentation = containsSensitiveIndentation getSourceLineStr
 
+        let (|StartsWith|) (s: string) = s[0]
+
         // Matches if the given expression starts with a symbol, e.g., <@ … @>, $"…", @"…", +1, -1…
         let (|StartsWithSymbol|_|) =
             let (|TextStartsWith|) (m: range) =
                 let line = getSourceLineStr m.StartLine
                 line[m.StartColumn]
 
-            let (|StartsWith|) (s: string) = s[0]
-
             function
             | SynExpr.Quote _
             | SynExpr.InterpolatedString _
@@ -638,7 +653,9 @@ module SynExpr =
           SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
         | SynExpr.Tuple(isStruct = false),
           SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(
-              funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _ -> true
+              funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
+        | SynExpr.Const(SynConst.Unit, _),
+          SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _ -> true
 
         // Already parenthesized.
         | _, SyntaxNode.SynExpr(SynExpr.Paren _) :: _ -> false
@@ -693,6 +710,28 @@ module SynExpr =
             ->
             true
 
+        // Hanging tuples:
+        //
+        //     let _ =
+        //         (
+        //             1, 2,
+        //           3, 4
+        //         )
+        //
+        // or
+        //
+        //     [
+        //         1, 2,
+        //         3, 4
+        //         (1, 2,
+        //        3, 4)
+        //     ]
+        | SynExpr.Tuple(isStruct = false; exprs = exprs; range = range), _ when
+            range.StartLine <> range.EndLine
+            && exprs |> List.exists (fun e -> e.Range.StartColumn < range.StartColumn)
+            ->
+            true
+
         // Check for nested matches, e.g.,
         //
         //     match … with … -> (…, match … with … -> … | … -> …) | … -> …
@@ -781,10 +820,26 @@ module SynExpr =
         //     (f x)[z]
         //     (f(x))[z]
         //     x.M(y)[z]
-        | _, SyntaxNode.SynExpr(SynExpr.App _) :: SyntaxNode.SynExpr(SynExpr.DotGet _ | SynExpr.DotIndexedGet _ | SynExpr.DotLambda _) :: _
-        | SynExpr.App _, SyntaxNode.SynExpr(SynExpr.App(argExpr = SynExpr.ArrayOrListComputed(isArray = false))) :: _
-        | _,
-          SyntaxNode.SynExpr(SynExpr.App _) :: SyntaxNode.SynExpr(SynExpr.App(argExpr = SynExpr.ArrayOrListComputed(isArray = false))) :: _ ->
+        //     M(x).N <- y
+        | SynExpr.App _, SyntaxNode.SynExpr(SynExpr.App(argExpr = SynExpr.ArrayOrListComputed(isArray = false))) :: _ -> true
+
+        | _, SyntaxNode.SynExpr(SynExpr.App _) :: path
+        | _, SyntaxNode.SynExpr(OuterBinaryExpr expr (Dot, _)) :: SyntaxNode.SynExpr(SynExpr.App _) :: path when
+            let rec appChainDependsOnDotOrPseudoDotPrecedence path =
+                match path with
+                | SyntaxNode.SynExpr(SynExpr.DotGet _) :: _
+                | SyntaxNode.SynExpr(SynExpr.DotLambda _) :: _
+                | SyntaxNode.SynExpr(SynExpr.DotIndexedGet _) :: _
+                | SyntaxNode.SynExpr(SynExpr.Set _) :: _
+                | SyntaxNode.SynExpr(SynExpr.DotSet _) :: _
+                | SyntaxNode.SynExpr(SynExpr.DotIndexedSet _) :: _
+                | SyntaxNode.SynExpr(SynExpr.DotNamedIndexedPropertySet _) :: _
+                | SyntaxNode.SynExpr(SynExpr.App(argExpr = SynExpr.ArrayOrListComputed(isArray = false))) :: _ -> true
+                | SyntaxNode.SynExpr(SynExpr.App _) :: path -> appChainDependsOnDotOrPseudoDotPrecedence path
+                | _ -> false
+
+            appChainDependsOnDotOrPseudoDotPrecedence path
+            ->
             true
 
         // The :: operator is parsed differently from other symbolic infix operators,
@@ -873,23 +928,22 @@ module SynExpr =
 
             | SynExpr.TryFinally(trivia = trivia), Dangling.Try tryExpr when problematic tryExpr.Range trivia.FinallyKeyword -> true
 
-            | SynExpr.Match(clauses = clauses; trivia = { WithKeyword = withKeyword }), Dangling.Match matchOrTry when
-                problematic matchOrTry.Range withKeyword
-                || anyProblematic matchOrTry.Range clauses
+            | SynExpr.Match(clauses = clauses; trivia = { WithKeyword = withKeyword }), Dangling.ArrowSensitive dangling when
+                problematic dangling.Range withKeyword || anyProblematic dangling.Range clauses
                 ->
                 true
 
-            | SynExpr.MatchBang(clauses = clauses; trivia = { WithKeyword = withKeyword }), Dangling.Match matchOrTry when
-                problematic matchOrTry.Range withKeyword
-                || anyProblematic matchOrTry.Range clauses
+            | SynExpr.MatchBang(clauses = clauses; trivia = { WithKeyword = withKeyword }), Dangling.ArrowSensitive dangling when
+                problematic dangling.Range withKeyword || anyProblematic dangling.Range clauses
                 ->
                 true
 
-            | SynExpr.MatchLambda(matchClauses = clauses), Dangling.Match matchOrTry when anyProblematic matchOrTry.Range clauses -> true
+            | SynExpr.MatchLambda(matchClauses = clauses), Dangling.ArrowSensitive dangling when anyProblematic dangling.Range clauses ->
+                true
 
-            | SynExpr.TryWith(withCases = clauses; trivia = trivia), Dangling.Match matchOrTry when
-                problematic matchOrTry.Range trivia.WithKeyword
-                || anyProblematic matchOrTry.Range clauses
+            | SynExpr.TryWith(withCases = clauses; trivia = trivia), Dangling.ArrowSensitive dangling when
+                problematic dangling.Range trivia.WithKeyword
+                || anyProblematic dangling.Range clauses
                 ->
                 true
 
@@ -903,12 +957,36 @@ module SynExpr =
             //          match x with
             //          | 3 | _ -> y) -> ()
             //     | _ -> ()
-            | _, Dangling.Match matchOrTry when
-                let line = getSourceLineStr matchOrTry.Range.EndLine
-                let endCol = matchOrTry.Range.EndColumn
-
-                line.Length > endCol + 1
-                && line.AsSpan(endCol + 1).TrimStart(' ').StartsWith("->".AsSpan())
+            | _, Dangling.ArrowSensitive dangling when
+                let rec ancestralTrailingArrow path =
+                    match path with
+                    | SyntaxNode.SynMatchClause _ :: _ -> shouldBeParenthesizedInContext path dangling
+
+                    | SyntaxNode.SynExpr(SynExpr.Tuple _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.App _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.IfThenElse _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.IfThenElse _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.Sequential _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.YieldOrReturn _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.YieldOrReturnFrom _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.Set _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.DotSet _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.DotNamedIndexedPropertySet _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.DotIndexedSet _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.LongIdentSet _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.LetOrUse _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.Lambda _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.Match _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.MatchLambda _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.MatchBang _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.TryWith _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.TryFinally _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.Do _) :: path
+                    | SyntaxNode.SynExpr(SynExpr.DoBang _) :: path -> ancestralTrailingArrow path
+
+                    | _ -> false
+
+                ancestralTrailingArrow outerPath
                 ->
                 true
 
@@ -939,8 +1017,19 @@ module SynExpr =
                     | SynInterpolatedStringPart.FillExpr(qualifiers = Some _) -> true
                     | _ -> false)
 
-            | SynExpr.Record(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)), Dangling.Problematic _
-            | SynExpr.AnonRecd(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)), Dangling.Problematic _ -> true
+            // { (!x) with … }
+            | SynExpr.Record(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)),
+              SynExpr.App(isInfix = false; funcExpr = FuncExpr.SymbolicOperator(StartsWith('!' | '~')))
+            | SynExpr.AnonRecd(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)),
+              SynExpr.App(isInfix = false; funcExpr = FuncExpr.SymbolicOperator(StartsWith('!' | '~'))) -> false
+
+            // { (+x) with … }
+            // { (x + y) with … }
+            // { (x |> f) with … }
+            // { (printfn "…"; x) with … }
+            | SynExpr.Record(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)), (PrefixApp _ | InfixApp _ | Dangling.Problematic _)
+            | SynExpr.AnonRecd(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)), (PrefixApp _ | InfixApp _ | Dangling.Problematic _) ->
+                true
 
             | SynExpr.Record(recordFields = recordFields), Dangling.Problematic _ ->
                 let rec loop recordFields =
@@ -964,8 +1053,6 @@ module SynExpr =
 
             | SynExpr.Paren _, SynExpr.Typed _
             | SynExpr.Quote _, SynExpr.Typed _
-            | SynExpr.AnonRecd _, SynExpr.Typed _
-            | SynExpr.Record _, SynExpr.Typed _
             | SynExpr.While(doExpr = SynExpr.Paren(expr = Is inner)), SynExpr.Typed _
             | SynExpr.WhileBang(doExpr = SynExpr.Paren(expr = Is inner)), SynExpr.Typed _
             | SynExpr.For(doBody = Is inner), SynExpr.Typed _

diff --git a/src/Compiler/Service/SynPat.fs b/src/Compiler/Service/SynPat.fs
@@ -127,6 +127,52 @@ module SynPat =
                                          MemberKind = SynMemberKind.PropertyGetSet | SynMemberKind.PropertyGet | SynMemberKind.PropertySet
                                      }))) :: _ -> true
 
+        // Parens must be kept when there is a multiline expression
+        // to the right whose offsides line would be shifted if the
+        // parentheses were removed from a leading pattern on the same line, e.g.,
+        //
+        //     match maybe with
+        //     | Some(x) -> let y = x * 2
+        //                  let z = 99
+        //                  x + y + z
+        //     | None -> 3
+        //
+        // or
+        //
+        //     let (x) = printfn "…"
+        //               printfn "…"
+        | _ when
+            // This is arbitrary and will result in some false positives.
+            let maxBacktracking = 10
+
+            let rec wouldMoveRhsOffsides n pat path =
+                if n = maxBacktracking then
+                    true
+                else
+                    // This does not thoroughly search the trailing
+                    // expression — nor does it go up the expression
+                    // tree and search farther rightward, or look at record bindings,
+                    // etc., etc., etc. — and will result in some false negatives.
+                    match path with
+                    // Expand the range to that of the outer pattern, since
+                    // the parens may extend beyond the inner pat
+                    | SyntaxNode.SynPat outer :: path when n = 1 -> wouldMoveRhsOffsides (n + 1) outer path
+                    | SyntaxNode.SynPat _ :: path -> wouldMoveRhsOffsides (n + 1) pat path
+
+                    | SyntaxNode.SynExpr(SynExpr.Lambda(body = rhs)) :: _
+                    | SyntaxNode.SynExpr(SynExpr.LetOrUse(body = rhs)) :: _
+                    | SyntaxNode.SynExpr(SynExpr.LetOrUseBang(body = rhs)) :: _
+                    | SyntaxNode.SynBinding(SynBinding(expr = rhs)) :: _
+                    | SyntaxNode.SynMatchClause(SynMatchClause(resultExpr = rhs)) :: _ ->
+                        let rhsRange = rhs.Range
+                        rhsRange.StartLine <> rhsRange.EndLine && pat.Range.EndLine = rhsRange.StartLine
+
+                    | _ -> false
+
+            wouldMoveRhsOffsides 1 pat path
+            ->
+            true
+
         // () is parsed as this.
         | SynPat.Const(SynConst.Unit, _), _ -> true
 

diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs
@@ -206,8 +206,8 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst
                                 | ' ', '=', _ -> Some ShouldPutSpaceBefore
                                 | _, '=', ('(' | '[' | '{') -> None
                                 | _, '=', (Punctuation | Symbol) -> Some ShouldPutSpaceBefore
-                                | _, LetterOrDigit, '(' -> None
-                                | _, (LetterOrDigit | '`'), _ -> Some ShouldPutSpaceBefore
+                                | _, ('_' | LetterOrDigit), '(' -> None
+                                | _, ('_' | LetterOrDigit | '`'), _ -> Some ShouldPutSpaceBefore
                                 | _, (Punctuation | Symbol), (Punctuation | Symbol) -> Some ShouldPutSpaceBefore
                                 | _ -> None
 
@@ -219,7 +219,7 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst
                             | _, (')' | ']' | '[' | '}' | '.' | ';' | ',' | '|') -> None
                             | _, ('+' | '-' | '%' | '&' | '!' | '~') -> None
                             | (Punctuation | Symbol), (Punctuation | Symbol | LetterOrDigit) -> Some ShouldPutSpaceAfter
-                            | LetterOrDigit, LetterOrDigit -> Some ShouldPutSpaceAfter
+                            | ('_' | LetterOrDigit), ('_' | LetterOrDigit) -> Some ShouldPutSpaceAfter
                             | _ -> None
 
                         let (|WouldTurnInfixIntoPrefix|_|) (s: string) =