From ab30ce1ba92b89af8bd8ea0e36e191e8825e566f Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Sun, 3 Dec 2023 14:22:35 -0500 Subject: [PATCH 1/3] Fix some some parenthesization oddities in records * Keep parens around problematic expressions (sequential, try-with/finally, match, lambdas, etc.) in record copy expressions. * Keep parens around dangling problematic expressions in record field assigments when there are subsequent record fields on the same line. --- src/Compiler/Service/ServiceAnalysis.fs | 25 ++++++ .../RemoveUnnecessaryParenthesesTests.fs | 76 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 09dc977ec81..b37ee356feb 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -1325,6 +1325,31 @@ module UnnecessaryParentheses = -> ValueNone + | SynExpr.Record(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)), Dangling.Problematic _ + | SynExpr.AnonRecd(copyInfo = Some(SynExpr.Paren(expr = Is inner), _)), Dangling.Problematic _ -> ValueNone + + | SynExpr.Record(recordFields = recordFields), Dangling.Problematic _ -> + let rec loop recordFields = + match recordFields with + | [] -> ValueSome range + | SynExprRecordField(expr = Some(SynExpr.Paren(expr = Is inner)); blockSeparator = Some _) :: SynExprRecordField( + fieldName = SynLongIdent(id = id :: _), _) :: _ when problematic inner.Range id.idRange -> ValueNone + | _ :: recordFields -> loop recordFields + + loop recordFields + + | SynExpr.AnonRecd(recordFields = recordFields), Dangling.Problematic _ -> + let rec loop recordFields = + match recordFields with + | [] -> ValueSome range + | (_, Some _blockSeparator, SynExpr.Paren(expr = Is inner)) :: (SynLongIdent(id = id :: _), _, _) :: _ when + problematic inner.Range id.idRange + -> + ValueNone + | _ :: recordFields -> loop recordFields + + loop recordFields + | SynExpr.Paren _, SynExpr.Typed _ | SynExpr.Quote _, SynExpr.Typed _ | SynExpr.AnonRecd _, SynExpr.Typed _ diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 27bce12bb2d..521f150764e 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -919,6 +919,39 @@ in x // AnonRecd "id ({||})", "id {||}" + "{| A = (fun () -> ()) |}", "{| A = fun () -> () |}" + "{| A = (fun () -> ()); B = 3 |}", "{| A = (fun () -> ()); B = 3 |}" + "{| A = (let x = 3 in x); B = 3 |}", "{| A = (let x = 3 in x); B = 3 |}" + "{| (try {||} with _ -> reraise ()) with A = 4 |}", "{| (try {||} with _ -> reraise ()) with A = 4 |}" + + " + {| A = (fun () -> ()) + B = 3 |} + ", + " + {| A = fun () -> () + B = 3 |} + " + + " + {| A = (let x = 3 in x) + B = 3 |} + ", + " + {| A = let x = 3 in x + B = 3 |} + " + + " + {| (try {||} with _ -> reraise ()) + with + A = 4 |} + ", + " + {| (try {||} with _ -> reraise ()) + with + A = 4 |} + " // ArrayOrList "id ([])", "id []" @@ -928,6 +961,49 @@ in x // Record "id ({ A = x })", "id { A = x }" + "{ A = (fun () -> ()) }", "{ A = fun () -> () }" + "{ A = (fun () -> ()); B = 3 }", "{ A = (fun () -> ()); B = 3 }" + "{ A = (let x = 3 in x); B = 3 }", "{ A = (let x = 3 in x); B = 3 }" + "{ A.B.C.D.X = (match () with () -> ()); A.B.C.D.Y = 3 }", "{ A.B.C.D.X = (match () with () -> ()); A.B.C.D.Y = 3 }" + "{ (try { A = 3 } with _ -> reraise ()) with A = 4 }", "{ (try { A = 3 } with _ -> reraise ()) with A = 4 }" + + " + { A = (fun () -> ()) + B = 3 } + ", + " + { A = fun () -> () + B = 3 } + " + + " + { A = (let x = 3 in x) + B = 3 } + ", + " + { A = let x = 3 in x + B = 3 } + " + + " + { A.B.C.D.X = (match () with () -> ()) + A.B.C.D.Y = 3 } + ", + " + { A.B.C.D.X = match () with () -> () + A.B.C.D.Y = 3 } + " + + " + { (try { A = 3 } with _ -> reraise ()) + with + A = 4 } + ", + " + { (try { A = 3 } with _ -> reraise ()) + with + A = 4 } + " // New "id (new obj())", "id (new obj())" From 186736f9a6b0e23ade4f250a2bd62ab7c31e7c96 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Sun, 3 Dec 2023 14:47:03 -0500 Subject: [PATCH 2/3] Add entry to FCS release notes --- docs/release-notes/FSharp.Compiler.Service/8.0.200.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/release-notes/FSharp.Compiler.Service/8.0.200.md b/docs/release-notes/FSharp.Compiler.Service/8.0.200.md index c0d199a7864..e42b66576bf 100644 --- a/docs/release-notes/FSharp.Compiler.Service/8.0.200.md +++ b/docs/release-notes/FSharp.Compiler.Service/8.0.200.md @@ -1,2 +1,3 @@ -- Miscellaneous fixes to parens analysis - https://github.com/dotnet/fsharp/pull/16262 -- Fixes #16359 - correctly handle imports with 0 length public key tokens - https://github.com/dotnet/fsharp/pull/16363 \ No newline at end of file +- Parens analysis: miscellaneous fixes - https://github.com/dotnet/fsharp/pull/16262 +- Parens analysis: fix some parenthesization corner-cases in record expressions - https://github.com/dotnet/fsharp/pull/16370 +- Fixes #16359 - correctly handle imports with 0 length public key tokens - https://github.com/dotnet/fsharp/pull/16363 From ccf4b641699d56292a2c049077e2c43034dc89c3 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Mon, 4 Dec 2023 10:09:50 -0500 Subject: [PATCH 3/3] Short-circuit when containing record field found --- src/Compiler/Service/ServiceAnalysis.fs | 15 ++++++++++----- .../RemoveUnnecessaryParenthesesTests.fs | 9 +++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index b37ee356feb..08852434016 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -1333,7 +1333,11 @@ module UnnecessaryParentheses = match recordFields with | [] -> ValueSome range | SynExprRecordField(expr = Some(SynExpr.Paren(expr = Is inner)); blockSeparator = Some _) :: SynExprRecordField( - fieldName = SynLongIdent(id = id :: _), _) :: _ when problematic inner.Range id.idRange -> ValueNone + fieldName = SynLongIdent(id = id :: _), _) :: _ -> + if problematic inner.Range id.idRange then + ValueNone + else + ValueSome range | _ :: recordFields -> loop recordFields loop recordFields @@ -1342,10 +1346,11 @@ module UnnecessaryParentheses = let rec loop recordFields = match recordFields with | [] -> ValueSome range - | (_, Some _blockSeparator, SynExpr.Paren(expr = Is inner)) :: (SynLongIdent(id = id :: _), _, _) :: _ when - problematic inner.Range id.idRange - -> - ValueNone + | (_, Some _blockSeparator, SynExpr.Paren(expr = Is inner)) :: (SynLongIdent(id = id :: _), _, _) :: _ -> + if problematic inner.Range id.idRange then + ValueNone + else + ValueSome range | _ :: recordFields -> loop recordFields loop recordFields diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 521f150764e..71338a6895b 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -933,6 +933,15 @@ in x B = 3 |} " + " + {| A = (fun () -> ()); B = (fun () -> ()) + C = 3 |} + ", + " + {| A = (fun () -> ()); B = fun () -> () + C = 3 |} + " + " {| A = (let x = 3 in x) B = 3 |}