Keep extra parens around unit & tuples in arg pats (#17618)

* Keep extra parens around unit & tuples in arg pats * There are several scenarios where it is impossible to know whether the parentheses are required, or whethery they may affect compilation, by looking only at the syntax.
dotnet · Aug 28, 2024 · 99c200f · 99c200f
1 parent 673bc48
commit 99c200f
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 26 deletions.
diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md
@@ -12,6 +12,7 @@
 * MethodAccessException on equality comparison of a type private to module. ([Issue #17541](https://github.com/dotnet/fsharp/issues/17541), [PR #17548](https://github.com/dotnet/fsharp/pull/17548))
 * Fixed checking failure when `global` namespace is involved with enabled GraphBasedChecking ([PR #17553](https://github.com/dotnet/fsharp/pull/17553))
 * Add missing byte chars notations, enforce limits in decimal notation in byte char & string (Issues [#15867](https://github.com/dotnet/fsharp/issues/15867), [#15868](https://github.com/dotnet/fsharp/issues/15868), [#15869](https://github.com/dotnet/fsharp/issues/15869), [PR #15898](https://github.com/dotnet/fsharp/pull/15898))
+* Parentheses analysis: keep extra parentheses around unit & tuples in method definitions. ([PR #17618](https://github.com/dotnet/fsharp/pull/17618))
 
 ### Added
 

diff --git a/src/Compiler/Service/SynPat.fs b/src/Compiler/Service/SynPat.fs
@@ -28,23 +28,6 @@ module SynPat =
         else
             ValueNone
 
-    /// Matches if any member in the given list is an inherit
-    /// or implementation of an interface with generic type args.
-    [<return: Struct>]
-    let (|AnyGenericInheritOrInterfaceImpl|_|) members =
-        if
-            members
-            |> List.exists (function
-                | SynMemberDefn.ImplicitInherit(inheritType = SynType.App(typeArgs = _ :: _))
-                | SynMemberDefn.ImplicitInherit(inheritType = SynType.LongIdentApp(typeArgs = _ :: _))
-                | SynMemberDefn.Interface(interfaceType = SynType.App(typeArgs = _ :: _))
-                | SynMemberDefn.Interface(interfaceType = SynType.LongIdentApp(typeArgs = _ :: _)) -> true
-                | _ -> false)
-        then
-            ValueSome AnyGenericInheritOrInterfaceImpl
-        else
-            ValueNone
-
     /// Matches the rightmost potentially dangling nested pattern.
     let rec (|Rightmost|) pat =
         match pat with
@@ -182,15 +165,20 @@ module SynPat =
         //     type C<'T> = abstract M : 'T -> unit
         //     let _ = { new C<unit> with override _.M (()) = () }
         //     let _ = { new C<int * int> with override _.M ((x, y)) = () }
+        //
+        // Single versus double parens are also compiled differently in cases like these:
+        //
+        //     type T =
+        //         static member M ()                   = () // .method public static void M()
+        //         static member M (())                 = () // .method public static void M(class [FSharp.Core]Microsoft.FSharp.Core.Unit _arg1)
+        //         static member M (_ : int, _ : int)   = () // .method public static void M(int32 _arg1, int32 _arg2)
+        //         static member M ((_ : int, _ : int)) = () // .method public static void M(class [System.Runtime]System.Tuple`2<int32, int32> _arg1)
         | 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 = _ :: _))) :: _
+          SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: _
         | 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 = _ :: _))) :: _
+          SyntaxNode.SynPat(SynPat.Paren _) :: SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: _
         | 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
+          SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: _ -> true
 
         // Not required:
         //

diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs
@@ -2801,7 +2801,15 @@ module Patterns =
 
                 expectFix code expected
 
-            let bareAtomics = fmtAllAsMemberData bareAtomics
+            let bareAtomics =
+                bareAtomics
+                |> List.map (function
+                    // We can't actually reliably remove the extra parens in argument patterns,
+                    // since they affect compilation.
+                    // See https://github.com/dotnet/fsharp/issues/17611, https://github.com/dotnet/fsharp/issues/16254, etc.
+                    | SynPat.Paren(SynPat.Const "()") as doubleParen, _ -> doubleParen, doubleParen
+                    | pats -> pats)
+                |> fmtAllAsMemberData
 
             let bareNonAtomics =
                 patterns
@@ -2825,7 +2833,15 @@ module Patterns =
                 let expected = $"type T () = member _.M %s{expected} = Unchecked.defaultof<_>"
                 expectFix code expected
 
-            let bareAtomics = fmtAllAsMemberData bareAtomics
+            let bareAtomics =
+                bareAtomics
+                |> List.map (function
+                    // We can't actually reliably remove the extra parens in argument patterns,
+                    // since they affect compilation.
+                    // See https://github.com/dotnet/fsharp/issues/17611, https://github.com/dotnet/fsharp/issues/16254, etc.
+                    | SynPat.Paren(SynPat.Const "()") as doubleParen, _ -> doubleParen, doubleParen
+                    | pats -> pats)
+                |> fmtAllAsMemberData
 
             let bareNonAtomics =
                 patterns
@@ -2959,7 +2975,7 @@ module Patterns =
             ",
             "
             type C = abstract M : unit -> unit
-            let _ = { new C with override _.M () = () }
+            let _ = { new C with override _.M (()) = () }
             "
 
             // See https://github.com/dotnet/fsharp/issues/16254.