From 702ef0338f3f112a55e02aabf8caa2873af7797f Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 2 Apr 2024 10:15:49 -0400 Subject: [PATCH 1/4] Handle some undentation oddities in sequentials --- src/Compiler/Service/SynExpr.fs | 65 +++++++++++ .../RemoveUnnecessaryParenthesesTests.fs | 105 ++++++++++++++++++ 2 files changed, 170 insertions(+) diff --git a/src/Compiler/Service/SynExpr.fs b/src/Compiler/Service/SynExpr.fs index 46bf646a213..92cdb606941 100644 --- a/src/Compiler/Service/SynExpr.fs +++ b/src/Compiler/Service/SynExpr.fs @@ -539,6 +539,21 @@ module SynExpr = loop ValueNone startLine range.StartColumn + /// Matches constructs that are sensitive to + /// certain kinds of undentation in sequential expressions. + [] + let (|UndentationSensitive|_|) expr = + match expr with + | SynExpr.TryWith _ + | SynExpr.TryFinally _ + | SynExpr.For _ + | SynExpr.ForEach _ + | SynExpr.IfThenElse _ + | SynExpr.Match _ + | SynExpr.While _ + | SynExpr.Do _ -> ValueSome UndentationSensitive + | _ -> ValueNone + let rec shouldBeParenthesizedInContext (getSourceLineStr: int -> string) path expr : bool = let shouldBeParenthesizedInContext = shouldBeParenthesizedInContext getSourceLineStr let containsSensitiveIndentation = containsSensitiveIndentation getSourceLineStr @@ -693,6 +708,56 @@ module SynExpr = -> true + // There are certain constructs whose indentation in + // a sequential expression is valid when parenthesized + // (and separated from the following expression by a semicolon), + // but where the parsing of the outer expression would change if + // the parentheses were removed in place, e.g., + // + // [ + // (if p then q else r); // Cannot remove in place because of the indentation of y below. + // y + // ] + // + // or + // + // [ + // x; + // (if p then q else r); // Cannot remove in place because of the indentation of x above. + // z + // ] + // + // This analysis is imperfect in that it sometimes requires parens when they + // may not be required, but the only way to know for sure in such cases would be to walk up + // an unknown number of ancestral sequential expressions to check for problematic + // indentation (or to keep track of the offsides line throughout the AST traversal): + // + // [ + // x; // This line's indentation means we cannot remove below. + // (…); + // (…); + // (* 𝑛 more such lines. *) + // (…); + // (…); + // (if p then q else r); // Can no longer remove here because of the indentation of x above. + // z + // ] + | UndentationSensitive, + SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr))) :: SyntaxNode.SynExpr(SynExpr.Sequential( + expr1 = SynExpr.Paren(expr = other) | other)) :: _ + | UndentationSensitive, + SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.Sequential _) :: _ + | UndentationSensitive, + SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.ArrayOrListComputed _) :: _ + | UndentationSensitive, + SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.ArrayOrList _) :: _ + | UndentationSensitive, + SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.ComputationExpr _) :: _ when + expr.Range.StartLine <> other.Range.StartLine + && expr.Range.StartColumn <= other.Range.StartColumn + -> + true + // Check for nested matches, e.g., // // match … with … -> (…, match … with … -> … | … -> …) | … -> … diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 13227d8eb42..05d377d4f0a 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -886,6 +886,111 @@ in x """ printfn "1"; (printfn "2") """, """ printfn "1"; printfn "2" """ "let x = 3; (5) in x", "let x = 3; 5 in x" + // Technically we could remove some parens in some of these, + // but additional exprs above or below can suddenly move the offsides line, + // and we don't want to do unbounded lookahead or lookbehind. + + " + let _ = + [ + (if p then q else r); + y + ] + ", + " + let _ = + [ + (if p then q else r); + y + ] + " + + " + let _ = + [ + (if p then q else r); + y + ] + ", + " + let _ = + [ + if p then q else r; + y + ] + " + + " + let _ = + [ + x; + (if p then q else r); + (if foo then bar else baz); + ] + ", + " + let _ = + [ + x; + (if p then q else r); + if foo then bar else baz; + ] + " + + " + let _ = + [ + (if p then q else r); + y; + (if foo then bar else baz); + ] + ", + " + let _ = + [ + (if p then q else r); + y; + if foo then bar else baz; + ] + " + + " + let _ = + [ + (if p then q else r); + (if foo then bar else baz); + z + ] + ", + " + let _ = + [ + if p then q else r; + (if foo then bar else baz); + z + ] + " + + " + let _ = + [ + x; + (if a then b else c); + (if p then q else r); + (if foo then bar else baz); + z + ] + ", + " + let _ = + [ + x; + (if a then b else c); + (if p then q else r); + (if foo then bar else baz); + z + ] + " " let _ = let y = 100 From 99060d5a784e2383326dd5e2629baa0cf3db08af Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 2 Apr 2024 10:16:35 -0400 Subject: [PATCH 2/4] Keep parens around nested sequentials * This is technically only necessary if the outer sequential is itself nested inside of a list/array/sequence/computation expression where each node of the "sequential" expression is actually an implicit yield, but there is currently no easy, inexpensive way of knowing that. --- src/Compiler/Service/SynExpr.fs | 2 ++ .../RemoveUnnecessaryParenthesesTests.fs | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/Compiler/Service/SynExpr.fs b/src/Compiler/Service/SynExpr.fs index 92cdb606941..ceff207f094 100644 --- a/src/Compiler/Service/SynExpr.fs +++ b/src/Compiler/Service/SynExpr.fs @@ -977,6 +977,8 @@ module SynExpr = -> true + | SynExpr.Sequential _, Dangling.Problematic(SynExpr.Sequential _) -> true + | SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is inner); expr2 = expr2), Dangling.Problematic _ when problematic inner.Range expr2.Range -> diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 05d377d4f0a..d45985caffa 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -886,6 +886,21 @@ in x """ printfn "1"; (printfn "2") """, """ printfn "1"; printfn "2" """ "let x = 3; (5) in x", "let x = 3; 5 in x" + """ + [ + () + (printfn "1"; ()) + () + ] + """, + """ + [ + () + (printfn "1"; ()) + () + ] + """ + // Technically we could remove some parens in some of these, // but additional exprs above or below can suddenly move the offsides line, // and we don't want to do unbounded lookahead or lookbehind. @@ -991,6 +1006,7 @@ in x z ] " + " let _ = let y = 100 From faa5b863ca32c158551db229cfaccdfd568617d1 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 9 Apr 2024 07:06:39 -0400 Subject: [PATCH 3/4] Update release notes --- docs/release-notes/.FSharp.Compiler.Service/8.0.300.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index d01125b6b9d..02bc8222c0e 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -8,7 +8,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), [PR #16901](https://github.com/dotnet/fsharp/pull/16901), [PR #16973](https://github.com/dotnet/fsharp/pull/16973)) +* 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), [PR #16973](https://github.com/dotnet/fsharp/pull/16973), [PR #16977](https://github.com/dotnet/fsharp/pull/16977)) * 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)) From 7ab693a3150c5cddb156ea39ab4133f60b46126d Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 11 Apr 2024 18:12:53 -0400 Subject: [PATCH 4/4] Move release notes --- docs/release-notes/.FSharp.Compiler.Service/8.0.300.md | 2 +- docs/release-notes/.FSharp.Compiler.Service/8.0.400.md | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 docs/release-notes/.FSharp.Compiler.Service/8.0.400.md diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index 7a2c273b0bd..57bc1578db3 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -8,7 +8,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), [PR #16901](https://github.com/dotnet/fsharp/pull/16901), [PR #16973](https://github.com/dotnet/fsharp/pull/16973), [PR #17012](https://github.com/dotnet/fsharp/pull/17012), [PR #16977](https://github.com/dotnet/fsharp/pull/16977)) +* 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), [PR #16973](https://github.com/dotnet/fsharp/pull/16973), [PR #17012](https://github.com/dotnet/fsharp/pull/17012)) * 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/.FSharp.Compiler.Service/8.0.400.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.400.md new file mode 100644 index 00000000000..9ecdf18ee3e --- /dev/null +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.400.md @@ -0,0 +1,3 @@ +### Fixed + +Various parenthesization API fixes. ([PR #16977](https://github.com/dotnet/fsharp/pull/16977))