Parens: more fixes (#16901)

* Fix a few more paren corner cases * Match-like exprs in `if` exprs, `while` exprs, and `for` exprs. Also `let` exprs. * Nested, dangling `as` patterns. * Outlaw `match` exprs (where the first `|` is leftward of the `m` in `match) * Single-line comments (`//`, `///`). Multiline comments (`(*…*)`) would be… rather more difficult to handle. * Double-parenthesized tuples in method applications, since we can't tell purely syntactically whether the tuple might be the first parameter of a method whose next parameter is an implied outref parameter: `x.TryGetValue ((y, z))` i.e., `x.TryGetValue ((y, z), &value)` * Multiline tuple patterns in `let`-bindings. These need parens if the bound expression starts on the same column. * Handle typed pats in getters & setters * Double parens oddities * Sometimes we can't tell just by looking at the untyped AST whether we need parens, since their necessity may be dictated by type information located elsewhere. Compare, e.g., #16254, which probably has a similar underlying cause. * Keep parens for parenzed app preceded by prefix op * Keep parens around tuple in interp string * More nested match fun * No space when expr would reparse as prefix op * No space when expr would reparse as prefix op * No space when expr would reparse as prefix op * Update release notes * Remove unfinished multiline comment stuff * Keep parens around dot-get that would be adjacent * E.g., removing parens in place from ```fsharp Debug.Assert((xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel) ``` would result in the the argument to `Assert` becoming `(xT.DeclaringType :?> ProvidedTypeDefinition)`. The `.BelongsToTargetModel` would then be parsed as a get on the return value of _that_ call. * Fantomas
dotnet · Mar 21, 2024 · c7fcbf6 · c7fcbf6
1 parent 1be52aa
commit c7fcbf6
Show file tree

Hide file tree

Showing 6 changed files with 486 additions and 46 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
@@ -6,7 +6,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))
+* 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))
 * 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))
+* 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))
 
 ### Changed
 

diff --git a/src/Compiler/Service/SynExpr.fs b/src/Compiler/Service/SynExpr.fs
@@ -344,6 +344,7 @@ module SynExpr =
         | InfixApp(prec, side) -> ValueSome(prec, side)
         | SynExpr.App(argExpr = SynExpr.ComputationExpr _) -> ValueSome(UnaryPrefix, Left)
         | SynExpr.App(funcExpr = SynExpr.Paren(expr = SynExpr.App _)) -> ValueSome(Apply, Left)
+        | SynExpr.App(flag = ExprAtomicFlag.Atomic) -> ValueSome(Dot, Non)
         | SynExpr.App _ -> ValueSome(Apply, Non)
         | SynExpr.DotSet(targetExpr = SynExpr.Paren(expr = Is inner)) -> ValueSome(Dot, Left)
         | SynExpr.DotSet(rhsExpr = SynExpr.Paren(expr = Is inner)) -> ValueSome(Set, Right)
@@ -437,6 +438,14 @@ module SynExpr =
                 | SynExpr.IfThenElse _ as expr -> Some expr
                 | _ -> None)
 
+        /// Matches a dangling let or use construct.
+        [<return: Struct>]
+        let (|LetOrUse|_|) =
+            dangling (function
+                | SynExpr.LetOrUse _
+                | SynExpr.LetOrUseBang _ as expr -> Some expr
+                | _ -> None)
+
         /// Matches a dangling sequential expression.
         [<return: Struct>]
         let (|Sequential|_|) =
@@ -610,13 +619,26 @@ module SynExpr =
         //
         //     o.M((x = y))
         //     o.N((x = y), z)
+        //
+        // Likewise, double parens must stay around a tuple, since we don't know whether
+        // the method being invoked might have a signature like
+        //
+        //     val TryGetValue : 'Key * outref<'Value> -> bool
+        //
+        // where 'Key is 'a * 'b, in which case the double parens are required.
         | SynExpr.Paren(expr = InfixApp(Relational(OriginalNotation "="), _)),
-          SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _)) :: _
+          SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
         | InfixApp(Relational(OriginalNotation "="), _),
-          SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _)) :: _
+          SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(
+              funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
         | InfixApp(Relational(OriginalNotation "="), _),
           SyntaxNode.SynExpr(SynExpr.Tuple(isStruct = false)) :: SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(
-              funcExpr = SynExpr.LongIdent _)) :: _ -> true
+              funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
+        | SynExpr.Paren(expr = SynExpr.Tuple(isStruct = false)),
+          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
 
         // Already parenthesized.
         | _, SyntaxNode.SynExpr(SynExpr.Paren _) :: _ -> false
@@ -676,6 +698,15 @@ module SynExpr =
           SyntaxNode.SynExpr(SynExpr.App _) :: SyntaxNode.SynExpr(HighPrecedenceApp | SynExpr.Assert _ | SynExpr.InferredUpcast _ | SynExpr.InferredDowncast _) :: _ ->
             true
 
+        // Parens must be kept in a scenario like
+        //
+        //     !x.M(y)
+        //     ~~~x.M(y)
+        //
+        // since prefix ! or ~~~ (with no space) have higher
+        // precedence than regular function application.
+        | _, SyntaxNode.SynExpr(SynExpr.App _) :: SyntaxNode.SynExpr(PrefixApp High) :: _ -> true
+
         // Parens are never required around suffixed or infixed numeric literals, e.g.,
         //
         //     (1l).ToString()
@@ -794,13 +825,17 @@ module SynExpr =
             match outer, inner with
             | ConfusableWithTypeApp, _ -> true
 
-            | SynExpr.IfThenElse _, Dangling.Sequential _ -> true
+            | SynExpr.IfThenElse(trivia = trivia), Dangling.LetOrUse letOrUse ->
+                Position.posLt letOrUse.Range.Start trivia.ThenKeyword.Start
 
-            | SynExpr.IfThenElse(trivia = trivia), Dangling.IfThen ifThenElse when
-                problematic ifThenElse.Range trivia.ThenKeyword
-                || trivia.ElseKeyword |> Option.exists (problematic ifThenElse.Range)
-                ->
-                true
+            | SynExpr.IfThenElse(trivia = trivia), Dangling.IfThen dangling
+            | SynExpr.IfThenElse(trivia = trivia), Dangling.Match dangling ->
+                problematic dangling.Range trivia.ThenKeyword
+                || trivia.ElseKeyword |> Option.exists (problematic dangling.Range)
+
+            | SynExpr.IfThenElse(ifExpr = expr), Dangling.Sequential dangling
+            | SynExpr.While(whileExpr = expr), Dangling.Problematic dangling
+            | SynExpr.ForEach(enumExpr = expr), Dangling.Problematic dangling -> Range.rangeContainsRange expr.Range dangling.Range
 
             | SynExpr.TryFinally(trivia = trivia), Dangling.Try tryExpr when problematic tryExpr.Range trivia.FinallyKeyword -> true
 
@@ -824,6 +859,25 @@ module SynExpr =
                 ->
                 true
 
+            // A match-like construct could be problematically nested like this:
+            //
+            //     match () with
+            //     | _ when
+            //         p &&
+            //         let x = f ()
+            //         (let y = z
+            //          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())
+                ->
+                true
+
             | SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is inner); expr2 = expr2), Dangling.Problematic _ when
                 problematic inner.Range expr2.Range
                 ->
@@ -842,9 +896,10 @@ module SynExpr =
             | SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is inner); expr2 = expr2), _ when innerBindingsWouldShadowOuter inner expr2 ->
                 true
 
-            | SynExpr.InterpolatedString _, SynExpr.Sequential _ -> true
+            | SynExpr.InterpolatedString _, SynExpr.Sequential _
+            | SynExpr.InterpolatedString _, SynExpr.Tuple(isStruct = false) -> true
 
-            | SynExpr.InterpolatedString(contents = contents), (SynExpr.Tuple(isStruct = false) | Dangling.Problematic _) ->
+            | SynExpr.InterpolatedString(contents = contents), Dangling.Problematic _ ->
                 contents
                 |> List.exists (function
                     | SynInterpolatedStringPart.FillExpr(qualifiers = Some _) -> true

diff --git a/src/Compiler/Service/SynPat.fs b/src/Compiler/Service/SynPat.fs
@@ -55,6 +55,24 @@ module SynPat =
         | SynPat.Tuple(isStruct = false; elementPats = Last(Rightmost pat)) -> pat
         | pat -> pat
 
+    /// Matches a nested as pattern.
+    [<return: Struct>]
+    let rec (|DanglingAs|_|) pat =
+        let (|AnyDanglingAs|_|) =
+            List.tryPick (function
+                | DanglingAs -> Some()
+                | _ -> None)
+
+        match pat with
+        | SynPat.As _ -> ValueSome()
+        | SynPat.Or(lhsPat = DanglingAs)
+        | SynPat.Or(rhsPat = DanglingAs)
+        | SynPat.ListCons(lhsPat = DanglingAs)
+        | SynPat.ListCons(rhsPat = DanglingAs)
+        | SynPat.Ands(pats = AnyDanglingAs)
+        | SynPat.Tuple(isStruct = false; elementPats = AnyDanglingAs) -> ValueSome()
+        | _ -> ValueNone
+
     /// Matches if the given pattern is atomic.
     [<return: Struct>]
     let (|Atomic|_|) pat =
@@ -88,6 +106,7 @@ module SynPat =
         //     fun (x, y, …) -> …
         //     fun (x: …) -> …
         //     fun (Pattern …) -> …
+        //     set (x: …, y: …) = …
         | SynPat.Typed _, SyntaxNode.SynPat(Rightmost(SynPat.Paren(Is pat, _))) :: SyntaxNode.SynMatchClause _ :: _
         | Rightmost(SynPat.Typed _), SyntaxNode.SynMatchClause _ :: _
         | SynPat.Typed _, SyntaxNode.SynExpr(SynExpr.LetOrUseBang _) :: _
@@ -98,23 +117,62 @@ module SynPat =
         | SynPat.LongIdent(argPats = SynArgPats.Pats(_ :: _)), SyntaxNode.SynBinding _ :: _
         | SynPat.LongIdent(argPats = SynArgPats.Pats(_ :: _)), SyntaxNode.SynExpr(SynExpr.Lambda _) :: _
         | SynPat.Tuple(isStruct = false), SyntaxNode.SynExpr(SynExpr.Lambda(parsedData = Some _)) :: _
-        | SynPat.Typed _, SyntaxNode.SynExpr(SynExpr.Lambda(parsedData = Some _)) :: _ -> true
+        | SynPat.Typed _, SyntaxNode.SynExpr(SynExpr.Lambda(parsedData = Some _)) :: _
+        | SynPat.Typed _,
+          SyntaxNode.SynPat(SynPat.Tuple(isStruct = false)) :: SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn(SynMemberDefn.GetSetMember _) :: _
+        | SynPat.Typed _,
+          SyntaxNode.SynPat(SynPat.Tuple(isStruct = false)) :: SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding(SynBinding(
+              valData = SynValData(
+                  memberFlags = Some {
+                                         MemberKind = SynMemberKind.PropertyGetSet | SynMemberKind.PropertyGet | SynMemberKind.PropertySet
+                                     }))) :: _ -> true
 
         // () is parsed as this.
         | SynPat.Const(SynConst.Unit, _), _ -> true
 
         // (()) is required when overriding a generic member
-        // where unit is the generic type argument:
+        // where unit or a tuple type is the generic type argument:
         //
         //     type C<'T> = abstract M : 'T -> unit
         //     let _ = { new C<unit> with override _.M (()) = () }
-        | SynPat.Paren(SynPat.Const(SynConst.Unit, _), _),
+        //     let _ = { new C<int * int> with override _.M ((x, y)) = () }
+        | SynPat.Paren((SynPat.Const(SynConst.Unit, _) | SynPat.Tuple(isStruct = false)), _),
           SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynExpr(SynExpr.ObjExpr(
               objType = SynType.App(typeArgs = _ :: _) | SynType.LongIdentApp(typeArgs = _ :: _))) :: _
-        | SynPat.Paren(SynPat.Const(SynConst.Unit, _), _),
+        | SynPat.Tuple(isStruct = false),
+          SyntaxNode.SynPat(SynPat.Paren _) :: SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynExpr(SynExpr.ObjExpr(
+              objType = SynType.App(typeArgs = _ :: _) | SynType.LongIdentApp(typeArgs = _ :: _))) :: _
+        | SynPat.Paren((SynPat.Const(SynConst.Unit, _) | SynPat.Tuple(isStruct = false)), _),
           SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn(SynTypeDefn(
               typeRepr = SynTypeDefnRepr.ObjectModel(members = AnyGenericInheritOrInterfaceImpl))) :: _ -> true
 
+        // Not required:
+        //
+        //     let (a,
+        //          b,
+        //          c) = …
+        //
+        // Required:
+        //
+        //     let (a,
+        //          b,
+        //          c) =
+        //         …
+        | SynPat.Tuple(isStruct = false; range = innerRange), SyntaxNode.SynBinding(SynBinding(expr = body)) :: _ ->
+            innerRange.StartLine <> innerRange.EndLine
+            && innerRange.StartLine < body.Range.StartLine
+            && body.Range.StartColumn <= innerRange.StartColumn
+
+        // The parens could be required by a signature file like this:
+        //
+        //     type SemanticClassificationItem =
+        //         new: (range * SemanticClassificationType) -> SemanticClassificationItem
+        | SynPat.Paren(SynPat.Tuple(isStruct = false), _),
+          SyntaxNode.SynPat(SynPat.LongIdent(longDotId = SynLongIdent(id = [ Ident "new" ]))) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn _ :: _
+        | SynPat.Tuple(isStruct = false),
+          SyntaxNode.SynPat(SynPat.Paren _) :: SyntaxNode.SynPat(SynPat.LongIdent(longDotId = SynLongIdent(id = [ Ident "new" ]))) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn _ :: _ ->
+            true
+
         // Parens are required around the atomic argument of
         // any additional `new` constructor that is not the last.
         //
@@ -204,9 +262,9 @@ module SynPat =
             // A | (B as C)
             // A & (B as C)
             // A, (B as C)
-            | SynPat.Or _, SynPat.As _
-            | SynPat.Ands _, SynPat.As _
-            | SynPat.Tuple _, SynPat.As _
+            | SynPat.Or _, (SynPat.As _ | DanglingAs)
+            | SynPat.Ands _, (SynPat.As _ | DanglingAs)
+            | SynPat.Tuple _, (SynPat.As _ | DanglingAs)
 
             // x, (y, z)
             // x & (y, z)