From 16d1195970f865b3dec6dd21d8995bc52898e61b Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 9 Nov 2023 08:51:37 -0500 Subject: [PATCH 1/6] Add test for nested sensitive indentation --- .../RemoveUnnecessaryParenthesesTests.fs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 551cfef9f68..4ca11e4e8e0 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -104,6 +104,23 @@ let _ = "struct (1, (1))", "struct (1, 1)" "(fun x -> x), y", "(fun x -> x), y" + " + let _ = + 1, + (true + || false + || true), + 1 + ", + " + let _ = + 1, + (true + || false + || true), + 1 + " + // AnonymousRecord "{| A = (1) |}", "{| A = 1 |}" "{| A = (1); B = 2 |}", "{| A = 1; B = 2 |}" From eac2ddcb11766800a0a0dbee0bc955ad130a5965 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 9 Nov 2023 14:48:29 -0500 Subject: [PATCH 2/6] Dedent by one space in certain scenarios * There are certain scenarios where we must consider both the context outside of a pair of parentheses as well as the inside in order to reintegrate the inner construct into the outer context after removing the parentheses. --- .../CodeFixes/RemoveUnnecessaryParentheses.fs | 166 ++++++++++++------ .../src/FSharp.Editor/Common/Extensions.fs | 41 +++++ .../RemoveUnnecessaryParenthesesTests.fs | 83 ++++++--- 3 files changed, 211 insertions(+), 79 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs index 6ee9fa87edd..8b4ad35cc8a 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs @@ -15,61 +15,101 @@ open CancellableTasks module private Patterns = let inline toPat f x = if f x then ValueSome() else ValueNone - [] - let inline (|LetterOrDigit|_|) c = toPat Char.IsLetterOrDigit c - - [] - let inline (|Punctuation|_|) c = toPat Char.IsPunctuation c - - [] - let inline (|Symbol|_|) c = toPat Char.IsSymbol c - -module private SourceText = - /// Returns true if the given span contains an expression - /// whose indentation would be made invalid if the open paren - /// were removed (because the offside line would be shifted), e.g., - /// - /// // Valid. - /// (let x = 2 - /// x) - /// - /// // Invalid. - /// ←let x = 2 - /// x◌ - /// - /// // Valid. - /// ◌let x = 2 - /// x◌ - let containsSensitiveIndentation (span: TextSpan) (sourceText: SourceText) = - let startLinePosition = sourceText.Lines.GetLinePosition span.Start - let endLinePosition = sourceText.Lines.GetLinePosition span.End - let startLine = startLinePosition.Line - let startCol = startLinePosition.Character - let endLine = endLinePosition.Line - - if startLine = endLine then - false - else - let rec loop offsides lineNo startCol = - if lineNo <= endLine then - let line = sourceText.Lines[ lineNo ].ToString() - - match offsides with - | ValueNone -> - let i = line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') - - if i >= 0 then - loop (ValueSome(i + startCol)) (lineNo + 1) 0 - else - loop offsides (lineNo + 1) 0 - - | ValueSome offsidesCol -> - let i = line.AsSpan(0, min offsidesCol line.Length).IndexOfAnyExcept(' ', ')') - i <= offsidesCol || loop offsides (lineNo + 1) 0 + [] + module Char = + [] + let inline (|LetterOrDigit|_|) c = toPat Char.IsLetterOrDigit c + + [] + let inline (|Punctuation|_|) c = toPat Char.IsPunctuation c + + [] + let inline (|Symbol|_|) c = toPat Char.IsSymbol c + + [] + module SourceText = + /// Returns true if the given span contains an expression + /// whose indentation would be made invalid if the open paren + /// were removed (because the offside line would be shifted), e.g., + /// + /// // Valid. + /// (let x = 2 + /// x) + /// + /// // Invalid. + /// ←let x = 2 + /// x◌ + /// + /// // Valid. + /// ◌let x = 2 + /// x◌ + let containsSensitiveIndentation (span: TextSpan) (sourceText: SourceText) = + let startLinePosition = sourceText.Lines.GetLinePosition span.Start + let endLinePosition = sourceText.Lines.GetLinePosition span.End + let startLine = startLinePosition.Line + let startCol = startLinePosition.Character + let endLine = endLinePosition.Line + + if startLine = endLine then + false + else + let rec loop offsides lineNo startCol = + if lineNo <= endLine then + let line = sourceText.Lines[ lineNo ].ToString() + + match offsides with + | ValueNone -> + let i = line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') + + if i >= 0 then + loop (ValueSome(i + startCol)) (lineNo + 1) 0 + else + loop offsides (lineNo + 1) 0 + + | ValueSome offsidesCol -> + let i = line.AsSpan(0, min offsidesCol line.Length).IndexOfAnyExcept(' ', ')') + i <= offsidesCol || loop offsides (lineNo + 1) 0 + else + false + + loop ValueNone startLine startCol + + let hasPrecedingConstructOnSameLine (span: TextSpan) (sourceText: SourceText) = + let linePosition = sourceText.Lines.GetLinePosition span.Start + let line = (sourceText.Lines.GetLineFromPosition span.Start).ToString() + line.AsSpan(0, linePosition.Character).LastIndexOfAnyExcept(' ', '(') >= 0 + + let precedingLineHasSameOffsides (span: TextSpan) (sourceText: SourceText) = + let startLinePosition = sourceText.Lines.GetLinePosition span.Start + let startLine = startLinePosition.Line + let offsides = startLinePosition.Character + + let rec loop lineNo = + if lineNo >= 0 then + let line = sourceText.Lines[ lineNo ].ToString().AsSpan() + let i = line.IndexOfAnyExcept("*/%-+:^@><=!|0$.? ".AsSpan()) + + if i >= 0 && i <> offsides then + false + else + let lo = if line[i] = ' ' then i else 0 + lo <= offsides || loop (lineNo - 1) else false - loop ValueNone startLine startCol + loop (startLine - 1) + + [] + let (|ContainsSensitiveIndentation|_|) span sourceText = + toPat (containsSensitiveIndentation span) sourceText + + [] + let (|HasPrecedingConstructOnSameLine|_|) span sourceText = + toPat (hasPrecedingConstructOnSameLine span) sourceText + + [] + let (|PrecedingLineHasSameOffsides|_|) span sourceText = + toPat (precedingLineHasSameOffsides span) sourceText [] type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [] () = @@ -130,7 +170,6 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [ None | _, (LetterOrDigit | '`'), _ -> Some ShouldPutSpaceBefore | _, (Punctuation | Symbol), (Punctuation | Symbol) -> Some ShouldPutSpaceBefore - | _ when SourceText.containsSensitiveIndentation context.Span sourceText -> Some ShouldPutSpaceBefore | _ -> None let (|ShouldPutSpaceAfter|_|) (s: string) = @@ -142,11 +181,24 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [ Some ShouldPutSpaceAfter | _ -> None + let (|NewOffsidesOnFirstLine|_|) (s: string) = + let s = s.AsSpan 1 // (… + let newline = s.IndexOfAny('\n', '\r') + + if newline < 0 || s.Slice(0, newline).IndexOfAnyExcept(@"\r\n ".AsSpan()) >= 0 then + Some NewOffsidesOnFirstLine + else + None + let newText = - match txt with - | ShouldPutSpaceBefore & ShouldPutSpaceAfter -> " " + txt[1 .. txt.Length - 2] + " " - | ShouldPutSpaceBefore -> " " + txt[1 .. txt.Length - 2] - | ShouldPutSpaceAfter -> txt[1 .. txt.Length - 2] + " " + match txt, sourceText with + | ShouldPutSpaceBefore & ShouldPutSpaceAfter, _ -> " " + txt[1 .. txt.Length - 2] + " " + | ShouldPutSpaceBefore, _ -> " " + txt[1 .. txt.Length - 2] + | ShouldPutSpaceAfter, _ -> txt[1 .. txt.Length - 2] + " " + | NewOffsidesOnFirstLine, ContainsSensitiveIndentation context.Span & HasPrecedingConstructOnSameLine context.Span + | NewOffsidesOnFirstLine, ContainsSensitiveIndentation context.Span & PrecedingLineHasSameOffsides context.Span -> + txt[ 1 .. txt.Length - 2 ].Replace("\n ", "\n") + | NewOffsidesOnFirstLine, ContainsSensitiveIndentation context.Span -> " " + txt[1 .. txt.Length - 2] | _ -> txt[1 .. txt.Length - 2] return diff --git a/vsintegration/src/FSharp.Editor/Common/Extensions.fs b/vsintegration/src/FSharp.Editor/Common/Extensions.fs index 76b6ff0644f..80dfd34ab5d 100644 --- a/vsintegration/src/FSharp.Editor/Common/Extensions.fs +++ b/vsintegration/src/FSharp.Editor/Common/Extensions.fs @@ -578,6 +578,19 @@ open System.Runtime.CompilerServices [] type ReadOnlySpanExtensions = + [] + static member IndexOfAnyExcept(span: ReadOnlySpan, value: char) = + let mutable i = 0 + let mutable found = false + + while not found && i < span.Length do + if span[i] <> value then + found <- true + else + i <- i + 1 + + if found then i else -1 + [] static member IndexOfAnyExcept(span: ReadOnlySpan, value0: char, value1: char) = let mutable i = 0 @@ -592,4 +605,32 @@ type ReadOnlySpanExtensions = i <- i + 1 if found then i else -1 + + [] + static member IndexOfAnyExcept(span: ReadOnlySpan, values: ReadOnlySpan) = + let mutable i = 0 + let mutable found = false + + while not found && i < span.Length do + if values.IndexOf span[i] < 0 then + found <- true + else + i <- i + 1 + + if found then i else -1 + + [] + static member LastIndexOfAnyExcept(span: ReadOnlySpan, value0: char, value1: char) = + let mutable i = span.Length - 1 + let mutable found = false + + while not found && i >= 0 do + let c = span[i] + + if c <> value0 && c <> value1 then + found <- true + else + i <- i - 1 + + if found then i else -1 #endif diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 4ca11e4e8e0..659c02507f5 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -115,9 +115,9 @@ let _ = " let _ = 1, - (true - || false - || true), + true + || false + || true, 1 " @@ -245,9 +245,9 @@ let _ = | _ -> 3) ", " - 3 > match x with - | 1 - | _ -> 3 + 3 > match x with + | 1 + | _ -> 3 " // Do @@ -274,8 +274,8 @@ let _ = in x """, """ - let x = printfn $"{y}" - 2 + let x = printfn $"{y}" + 2 in x """ @@ -290,14 +290,40 @@ let _ = in x " + " + let x = + (2 + + 2) + in x + ", + " + let x = + 2 + + 2 + in x + " + + " + let x = ( + 2 + + 2) + in x + ", + " + let x = + 2 + + 2 + in x + " + " let x = (2 + 2) in x ", " - let x = 2 - + 2 + let x = 2 + + 2 in x " @@ -307,8 +333,8 @@ let _ = in x ", " - let x = 2 - + 2 + let x = 2 + + 2 in x " @@ -318,8 +344,8 @@ let _ = in x ", " - let x = 2 - + 2 + let x = 2 + + 2 in x " @@ -329,8 +355,8 @@ let _ = in x ", " - let x = x - +y + let x = x + +y in x " @@ -340,8 +366,8 @@ let _ = in x ", " - let x = 2 - +2 + let x = 2 + +2 in x " @@ -351,11 +377,24 @@ let _ = in x ", " - let x = 2 - <<< 2 + let x = 2 + <<< 2 in x " + " +let (<<<<<<<<) = (<<<) +let x = (2 +<<<<<<<< 2) +in x + ", + " +let (<<<<<<<<) = (<<<) +let x = 2 +<<<<<<<< 2 +in x + " + " let x = ( 2 @@ -533,8 +572,8 @@ let _ = """, """ let mutable x = 3 - x <- 3 - <<< 3 + x <- 3 + <<< 3 """ // DotIndexedGet From 8e06ba79145442b1a9a4ed251be93b7d6310857d Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 9 Nov 2023 14:54:13 -0500 Subject: [PATCH 3/6] Reverse the lines for easier reading on failure --- .../tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs index 7ff151e6040..0a0ca440236 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs @@ -209,7 +209,7 @@ module Xunit = let actual = split actual try - Assert.All(Array.zip expected actual, (fun (expected, actual) -> Assert.Equal(expected, actual))) + Assert.All((expected, actual) ||> Array.zip |> Array.rev, (fun (expected, actual) -> Assert.Equal(expected, actual))) with :? Xunit.Sdk.AllException as all when all.Failures.Count = 1 -> raise all.Failures[0] From a787de9c6780b417b30c2d4cadf36a48ab4d92cb Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 9 Nov 2023 15:13:58 -0500 Subject: [PATCH 4/6] Fantomas --- vsintegration/src/FSharp.Editor/Common/Extensions.fs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Common/Extensions.fs b/vsintegration/src/FSharp.Editor/Common/Extensions.fs index 80dfd34ab5d..0c70a078675 100644 --- a/vsintegration/src/FSharp.Editor/Common/Extensions.fs +++ b/vsintegration/src/FSharp.Editor/Common/Extensions.fs @@ -584,10 +584,7 @@ type ReadOnlySpanExtensions = let mutable found = false while not found && i < span.Length do - if span[i] <> value then - found <- true - else - i <- i + 1 + if span[i] <> value then found <- true else i <- i + 1 if found then i else -1 From 19d6af153a7ab72571dfcf46f0fb9a7325a3ceca Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Fri, 10 Nov 2023 09:23:20 -0500 Subject: [PATCH 5/6] Handle empty lines correctly --- .../CodeFixes/RemoveUnnecessaryParentheses.fs | 5 ++--- .../RemoveUnnecessaryParenthesesTests.fs | 21 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs index 8b4ad35cc8a..b008e45cdf2 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs @@ -89,11 +89,10 @@ module private Patterns = let line = sourceText.Lines[ lineNo ].ToString().AsSpan() let i = line.IndexOfAnyExcept("*/%-+:^@><=!|0$.? ".AsSpan()) - if i >= 0 && i <> offsides then + if i < offsides then false else - let lo = if line[i] = ' ' then i else 0 - lo <= offsides || loop (lineNo - 1) + i = offsides || loop (lineNo - 1) else false diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index c77378df104..8c616b6d668 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -303,6 +303,27 @@ let _ = in x " + " + let x = + + + (2 + + + + 2) + in x + ", + " + let x = + + + 2 + + + + 2 + in x + " + " let x = ( 2 From db4adac3fd5bfee8f656a8b5f33d96f3b500f7a7 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Fri, 10 Nov 2023 09:55:35 -0500 Subject: [PATCH 6/6] We actually need the more general thing --- .../CodeFixes/RemoveUnnecessaryParentheses.fs | 24 ++++++------- .../RemoveUnnecessaryParenthesesTests.fs | 36 +++++++++++++------ 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs index b008e45cdf2..6a5e34823e3 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs @@ -79,24 +79,22 @@ module private Patterns = let line = (sourceText.Lines.GetLineFromPosition span.Start).ToString() line.AsSpan(0, linePosition.Character).LastIndexOfAnyExcept(' ', '(') >= 0 - let precedingLineHasSameOffsides (span: TextSpan) (sourceText: SourceText) = + let followingLineMovesOffsidesRightward (span: TextSpan) (sourceText: SourceText) = let startLinePosition = sourceText.Lines.GetLinePosition span.Start let startLine = startLinePosition.Line + let endLinePosition = sourceText.Lines.GetLinePosition span.End + let endLine = endLinePosition.Line let offsides = startLinePosition.Character let rec loop lineNo = - if lineNo >= 0 then + if lineNo <= endLine then let line = sourceText.Lines[ lineNo ].ToString().AsSpan() - let i = line.IndexOfAnyExcept("*/%-+:^@><=!|0$.? ".AsSpan()) - - if i < offsides then - false - else - i = offsides || loop (lineNo - 1) + let i = line.IndexOfAnyExcept("*/%-+:^@><=!|0$.?) ".AsSpan()) + i > offsides || loop (lineNo + 1) else false - loop (startLine - 1) + loop (startLine + 1) [] let (|ContainsSensitiveIndentation|_|) span sourceText = @@ -107,8 +105,8 @@ module private Patterns = toPat (hasPrecedingConstructOnSameLine span) sourceText [] - let (|PrecedingLineHasSameOffsides|_|) span sourceText = - toPat (precedingLineHasSameOffsides span) sourceText + let (|FollowingLineMovesOffsidesRightward|_|) span sourceText = + toPat (followingLineMovesOffsidesRightward span) sourceText [] type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [] () = @@ -194,8 +192,8 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [ " " + txt[1 .. txt.Length - 2] + " " | ShouldPutSpaceBefore, _ -> " " + txt[1 .. txt.Length - 2] | ShouldPutSpaceAfter, _ -> txt[1 .. txt.Length - 2] + " " - | NewOffsidesOnFirstLine, ContainsSensitiveIndentation context.Span & HasPrecedingConstructOnSameLine context.Span - | NewOffsidesOnFirstLine, ContainsSensitiveIndentation context.Span & PrecedingLineHasSameOffsides context.Span -> + | NewOffsidesOnFirstLine, + ContainsSensitiveIndentation context.Span & (HasPrecedingConstructOnSameLine context.Span | FollowingLineMovesOffsidesRightward context.Span) -> txt[ 1 .. txt.Length - 2 ].Replace("\n ", "\n") | NewOffsidesOnFirstLine, ContainsSensitiveIndentation context.Span -> " " + txt[1 .. txt.Length - 2] | _ -> txt[1 .. txt.Length - 2] diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 8c616b6d668..65a3d7ec11a 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -298,8 +298,8 @@ let _ = ", " let x = - 2 - + 2 + 2 + + 2 in x " @@ -317,10 +317,10 @@ let _ = let x = - 2 - + 2 + - + 2 + + 2 in x " @@ -461,8 +461,8 @@ in x ) let y = - 2 - + 2 + 2 + + 2 in x + y " @@ -488,9 +488,9 @@ in x ", " x < - 2 - + 3 - + 2 + + 3 + " // LetOrUse @@ -748,6 +748,22 @@ in x // LibraryOnlyILAssembly """(# "ldlen.multi 2 0" array : int #)""", """(# "ldlen.multi 2 0" array : int #)""" + + // Miscellaneous + " + (match x with + | 1 -> () + | _ -> ()) + + y + ", + " + match x with + | 1 -> () + | _ -> () + + y + " } []