From 9bba4550d268ae0b002dacd5a8a971dca85e16f2 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 7 Nov 2023 19:21:04 -0500 Subject: [PATCH 1/5] =?UTF-8?q?Handle=20special=20parsing=20of=20`new=20T?= =?UTF-8?q?=E2=80=A6`=20&=20`inherit=20T=E2=80=A6`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Ctor applications are handled differently in the parser after the `new` and `inherit` keywords than they are elsewhere: only a subset of expressions are allowed as an argument (as specified in `atomicExprAfterType` in pars.fsy). This means that any expression outside of that blessed subset must be parenthesized, even if the exact same ctor application would not require parentheses without `new` or `inherit`. --- src/Compiler/Service/ServiceAnalysis.fs | 37 ++++++++++++++ .../RemoveUnnecessaryParenthesesTests.fs | 48 +++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 89caef4642e..948654773b3 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -692,6 +692,22 @@ module UnnecessaryParentheses = module SynExpr = open FSharp.Compiler.SyntaxTrivia + /// See atomicExprAfterType in pars.fsy. + [] + let (|AtomicExprAfterType|_|) expr = + match expr with + | SynExpr.Paren _ + | SynExpr.Quote _ + | SynExpr.Const _ + | SynExpr.Tuple(isStruct = true) + | SynExpr.Record _ + | SynExpr.AnonRecd _ + | SynExpr.InterpolatedString _ + | SynExpr.Null _ + | SynExpr.ArrayOrList _ + | SynExpr.ArrayOrListComputed _ -> ValueSome AtomicExprAfterType + | _ -> ValueNone + /// Matches if the given expression represents a high-precedence /// function application, e.g., /// @@ -1142,6 +1158,19 @@ module UnnecessaryParentheses = | SynExpr.Paren(expr = SynExpr.App _), SyntaxNode.SynExpr (SynExpr.App _) :: SyntaxNode.SynExpr (SynExpr.JoinIn _) :: _ -> ValueNone + // Parens are not required around a few anointed expressions after inherit: + // + // inherit T(3) + // inherit T(null) + // inherit T("") + // … + | SynExpr.Paren(expr = AtomicExprAfterType; range = range), SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> + ValueSome range + + // Parens are otherwise required in inherit T(x), etc. (see atomicExprAfterType in pars.fsy). + | SynExpr.Paren _, SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> + ValueNone + // We can't remove parens when they're required for fluent calls: // // x.M(y).N z @@ -1284,6 +1313,14 @@ module UnnecessaryParentheses = | SynExpr.AddressOf _, SynExpr.Typed _ | SynExpr.JoinIn _, SynExpr.Typed _ -> ValueNone + // new T(expr) + | SynExpr.New _, AtomicExprAfterType -> ValueSome range + | SynExpr.New _, _ -> ValueNone + + // { inherit T(expr); … } + | SynExpr.Record (baseInfo = Some (_, SynExpr.Paren (expr = Is inner), _, _, _)), AtomicExprAfterType -> ValueSome range + | SynExpr.Record (baseInfo = Some (_, SynExpr.Paren (expr = Is inner), _, _, _)), _ -> ValueNone + | _, SynExpr.Paren _ | _, SynExpr.Quote _ | _, SynExpr.Const _ diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index d0d1dd9bf5c..a08f05bdc31 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -127,6 +127,8 @@ let _ = // New "new exn(null)", "new exn null" "new exn (null)", "new exn null" + "new ResizeArray(3)", "new ResizeArray 3" + "let x = 3 in new ResizeArray(x)", "let x = 3 in new ResizeArray(x)" // Unless the rules for ctor args in `new` exprs are ever relaxed (e.g., atomicExprAfterType → argExpr in pars.fsy). // ObjExpr "{ new System.IDisposable with member _.Dispose () = (ignore 3) }", @@ -1091,6 +1093,52 @@ let builder = Builder () let (+) _ _ = builder let _ = (2 + 2) { return 5 } " + + """ + type E (message : string) = + inherit exn ($"{message}") + """, + """ + type E (message : string) = + inherit exn $"{message}" + """ + + // Unless the rules for ctor args in `inherit` exprs are ever relaxed (e.g., atomicExprAfterType → argExpr in pars.fsy). + " + type E (message : string) = + inherit exn (message) + ", + " + type E (message : string) = + inherit exn (message) + " + + """ + type E = + inherit exn + val Message2 : string + new (str1, str2) = { inherit exn ($"{str1}"); Message2 = str2 } + """, + """ + type E = + inherit exn + val Message2 : string + new (str1, str2) = { inherit exn $"{str1}"; Message2 = str2 } + """ + + // Unless the rules for ctor args in `inherit` exprs are ever relaxed (e.g., atomicExprAfterType → argExpr in pars.fsy). + " + type E = + inherit exn + val Message2 : string + new (str1, str2) = { inherit exn (str1); Message2 = str2 } + ", + " + type E = + inherit exn + val Message2 : string + new (str1, str2) = { inherit exn (str1); Message2 = str2 } + " } [] From e79cbbe24b46ae254fb2fb10ec88ee8bdf22ee66 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 7 Nov 2023 19:56:05 -0500 Subject: [PATCH 2/5] Fantomas --- src/Compiler/Service/ServiceAnalysis.fs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 948654773b3..46dec0ce8a1 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -1164,12 +1164,11 @@ module UnnecessaryParentheses = // inherit T(null) // inherit T("") // … - | SynExpr.Paren(expr = AtomicExprAfterType; range = range), SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> + | SynExpr.Paren (expr = AtomicExprAfterType; range = range), SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> ValueSome range // Parens are otherwise required in inherit T(x), etc. (see atomicExprAfterType in pars.fsy). - | SynExpr.Paren _, SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> - ValueNone + | SynExpr.Paren _, SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> ValueNone // We can't remove parens when they're required for fluent calls: // @@ -1318,8 +1317,8 @@ module UnnecessaryParentheses = | SynExpr.New _, _ -> ValueNone // { inherit T(expr); … } - | SynExpr.Record (baseInfo = Some (_, SynExpr.Paren (expr = Is inner), _, _, _)), AtomicExprAfterType -> ValueSome range - | SynExpr.Record (baseInfo = Some (_, SynExpr.Paren (expr = Is inner), _, _, _)), _ -> ValueNone + | SynExpr.Record(baseInfo = Some (_, SynExpr.Paren(expr = Is inner), _, _, _)), AtomicExprAfterType -> ValueSome range + | SynExpr.Record(baseInfo = Some (_, SynExpr.Paren(expr = Is inner), _, _, _)), _ -> ValueNone | _, SynExpr.Paren _ | _, SynExpr.Quote _ From 24a1167213bb4616e35882dcf770bdbf98c64f84 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 7 Nov 2023 21:16:19 -0500 Subject: [PATCH 3/5] Arrays only --- src/Compiler/Service/ServiceAnalysis.fs | 4 ++-- .../CodeFixes/RemoveUnnecessaryParenthesesTests.fs | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 46dec0ce8a1..e804fabed28 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -704,8 +704,8 @@ module UnnecessaryParentheses = | SynExpr.AnonRecd _ | SynExpr.InterpolatedString _ | SynExpr.Null _ - | SynExpr.ArrayOrList _ - | SynExpr.ArrayOrListComputed _ -> ValueSome AtomicExprAfterType + | SynExpr.ArrayOrList (isArray = true) + | SynExpr.ArrayOrListComputed (isArray = true) -> ValueSome AtomicExprAfterType | _ -> ValueNone /// Matches if the given expression represents a high-precedence diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index a08f05bdc31..992d4e25d98 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -129,6 +129,10 @@ let _ = "new exn (null)", "new exn null" "new ResizeArray(3)", "new ResizeArray 3" "let x = 3 in new ResizeArray(x)", "let x = 3 in new ResizeArray(x)" // Unless the rules for ctor args in `new` exprs are ever relaxed (e.g., atomicExprAfterType → argExpr in pars.fsy). + "ResizeArray([3])", "ResizeArray [3]" + "new ResizeArray([3])", "new ResizeArray([3])" + "ResizeArray([|3|])", "ResizeArray [|3|]" + "new ResizeArray([|3|])", "new ResizeArray [|3|]" // ObjExpr "{ new System.IDisposable with member _.Dispose () = (ignore 3) }", From 83fa32ccdd883eb48f6029d96461ef49c55e8ccc Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 7 Nov 2023 21:22:21 -0500 Subject: [PATCH 4/5] Fantomas --- src/Compiler/Service/ServiceAnalysis.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index e804fabed28..134fb44f55b 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -704,8 +704,8 @@ module UnnecessaryParentheses = | SynExpr.AnonRecd _ | SynExpr.InterpolatedString _ | SynExpr.Null _ - | SynExpr.ArrayOrList (isArray = true) - | SynExpr.ArrayOrListComputed (isArray = true) -> ValueSome AtomicExprAfterType + | SynExpr.ArrayOrList(isArray = true) + | SynExpr.ArrayOrListComputed(isArray = true) -> ValueSome AtomicExprAfterType | _ -> ValueNone /// Matches if the given expression represents a high-precedence From 6ace27d0e1c667d33acdee0bd82a84cfc57d4f97 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Wed, 8 Nov 2023 08:10:19 -0500 Subject: [PATCH 5/5] Clean up comments --- src/Compiler/Service/ServiceAnalysis.fs | 2 +- .../CodeFixes/RemoveUnnecessaryParenthesesTests.fs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 134fb44f55b..f168884644c 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -1167,7 +1167,7 @@ module UnnecessaryParentheses = | SynExpr.Paren (expr = AtomicExprAfterType; range = range), SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> ValueSome range - // Parens are otherwise required in inherit T(x), etc. (see atomicExprAfterType in pars.fsy). + // Parens are otherwise required in inherit T(x), etc. | SynExpr.Paren _, SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitInherit _) :: _ -> ValueNone // We can't remove parens when they're required for fluent calls: diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 992d4e25d98..551cfef9f68 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -128,7 +128,7 @@ let _ = "new exn(null)", "new exn null" "new exn (null)", "new exn null" "new ResizeArray(3)", "new ResizeArray 3" - "let x = 3 in new ResizeArray(x)", "let x = 3 in new ResizeArray(x)" // Unless the rules for ctor args in `new` exprs are ever relaxed (e.g., atomicExprAfterType → argExpr in pars.fsy). + "let x = 3 in new ResizeArray(x)", "let x = 3 in new ResizeArray(x)" "ResizeArray([3])", "ResizeArray [3]" "new ResizeArray([3])", "new ResizeArray([3])" "ResizeArray([|3|])", "ResizeArray [|3|]" @@ -1107,7 +1107,6 @@ let _ = (2 + 2) { return 5 } inherit exn $"{message}" """ - // Unless the rules for ctor args in `inherit` exprs are ever relaxed (e.g., atomicExprAfterType → argExpr in pars.fsy). " type E (message : string) = inherit exn (message) @@ -1130,7 +1129,6 @@ let _ = (2 + 2) { return 5 } new (str1, str2) = { inherit exn $"{str1}"; Message2 = str2 } """ - // Unless the rules for ctor args in `inherit` exprs are ever relaxed (e.g., atomicExprAfterType → argExpr in pars.fsy). " type E = inherit exn