From acaba55fcd203cb3152d70a4ae9a21aae9037b60 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 27 Aug 2024 13:57:51 -0400 Subject: [PATCH 1/2] 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. --- src/Compiler/Service/SynPat.fs | 34 ++++++------------- .../RemoveUnnecessaryParenthesesTests.fs | 22 ++++++++++-- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/Compiler/Service/SynPat.fs b/src/Compiler/Service/SynPat.fs index 8ed9cd69ff1..c94448ce318 100644 --- 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. - [] - 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 with override _.M (()) = () } // let _ = { new C 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 _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 index 1994c5a7dac..b9b95bc44ac 100644 --- 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. From 10c006c7c1bf727870bca9ad735dd94bf91cab08 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 27 Aug 2024 14:11:18 -0400 Subject: [PATCH 2/2] Update release notes --- docs/release-notes/.FSharp.Compiler.Service/9.0.100.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md index d3e795bf8b7..12adae55598 100644 --- 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