From d16430f204da31492411cad4a5320a2b29429efa Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> Date: Tue, 14 Nov 2023 08:34:31 -0500 Subject: [PATCH] Parens: tweak sensitive indentation handling (#16248) * 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 | 159 ++++++++++++------ .../src/FSharp.Editor/Common/Extensions.fs | 38 +++++ .../CodeFixes/CodeFixTestFramework.fs | 2 +- .../RemoveUnnecessaryParenthesesTests.fs | 141 +++++++++++++--- 4 files changed, 260 insertions(+), 80 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs index 6ee9fa87edd..6a5e34823e3 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs @@ -15,61 +15,98 @@ open CancellableTasks module private Patterns = let inline toPat f x = if f x then ValueSome() else ValueNone - [<return: Struct>] - let inline (|LetterOrDigit|_|) c = toPat Char.IsLetterOrDigit c - - [<return: Struct>] - let inline (|Punctuation|_|) c = toPat Char.IsPunctuation c - - [<return: Struct>] - 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 = + [<AutoOpen>] + module Char = + [<return: Struct>] + let inline (|LetterOrDigit|_|) c = toPat Char.IsLetterOrDigit c + + [<return: Struct>] + let inline (|Punctuation|_|) c = toPat Char.IsPunctuation c + + [<return: Struct>] + let inline (|Symbol|_|) c = toPat Char.IsSymbol c + + [<AutoOpen>] + 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 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 <= endLine then - let line = sourceText.Lines[ lineNo ].ToString() + let line = sourceText.Lines[ lineNo ].ToString().AsSpan() + let i = line.IndexOfAnyExcept("*/%-+:^@><=!|0$.?) ".AsSpan()) + i > offsides || loop (lineNo + 1) + else + false - match offsides with - | ValueNone -> - let i = line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') + loop (startLine + 1) - if i >= 0 then - loop (ValueSome(i + startCol)) (lineNo + 1) 0 - else - loop offsides (lineNo + 1) 0 + [<return: Struct>] + let (|ContainsSensitiveIndentation|_|) span sourceText = + toPat (containsSensitiveIndentation span) sourceText - | ValueSome offsidesCol -> - let i = line.AsSpan(0, min offsidesCol line.Length).IndexOfAnyExcept(' ', ')') - i <= offsidesCol || loop offsides (lineNo + 1) 0 - else - false + [<return: Struct>] + let (|HasPrecedingConstructOnSameLine|_|) span sourceText = + toPat (hasPrecedingConstructOnSameLine span) sourceText - loop ValueNone startLine startCol + [<return: Struct>] + let (|FollowingLineMovesOffsidesRightward|_|) span sourceText = + toPat (followingLineMovesOffsidesRightward span) sourceText [<ExportCodeFixProvider(FSharpConstants.FSharpLanguageName, Name = CodeFix.RemoveUnnecessaryParentheses); Shared; Sealed>] type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConstructor>] () = @@ -130,7 +167,6 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst | _, LetterOrDigit, '(' -> 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 +178,24 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst | LetterOrDigit, LetterOrDigit -> 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 | 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] return diff --git a/vsintegration/src/FSharp.Editor/Common/Extensions.fs b/vsintegration/src/FSharp.Editor/Common/Extensions.fs index 76b6ff0644f..0c70a078675 100644 --- a/vsintegration/src/FSharp.Editor/Common/Extensions.fs +++ b/vsintegration/src/FSharp.Editor/Common/Extensions.fs @@ -578,6 +578,16 @@ open System.Runtime.CompilerServices [<Sealed; AbstractClass; Extension>] type ReadOnlySpanExtensions = + [<Extension>] + static member IndexOfAnyExcept(span: ReadOnlySpan<char>, 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 + [<Extension>] static member IndexOfAnyExcept(span: ReadOnlySpan<char>, value0: char, value1: char) = let mutable i = 0 @@ -592,4 +602,32 @@ type ReadOnlySpanExtensions = i <- i + 1 if found then i else -1 + + [<Extension>] + static member IndexOfAnyExcept(span: ReadOnlySpan<char>, values: ReadOnlySpan<char>) = + 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 + + [<Extension>] + static member LastIndexOfAnyExcept(span: ReadOnlySpan<char>, 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/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] diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 36a39c1b92f..ca780c76db7 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -105,6 +105,23 @@ let _ = "(fun x -> x), y", "(fun x -> x), y" "3, (null: string)", "3, (null: string)" + " + 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 |}" @@ -243,9 +260,9 @@ let _ = | _ -> 3) ", " - 3 > match x with - | 1 - | _ -> 3 + 3 > match x with + | 1 + | _ -> 3 " // Do @@ -272,8 +289,8 @@ let _ = in x """, """ - let x = printfn $"{y}" - 2 + let x = printfn $"{y}" + 2 in x """ @@ -288,14 +305,61 @@ 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 + in x + " + " let x = (2 + 2) in x ", " - let x = 2 - + 2 + let x = 2 + + 2 in x " @@ -305,8 +369,8 @@ let _ = in x ", " - let x = 2 - + 2 + let x = 2 + + 2 in x " @@ -316,8 +380,8 @@ let _ = in x ", " - let x = 2 - + 2 + let x = 2 + + 2 in x " @@ -327,8 +391,8 @@ let _ = in x ", " - let x = x - +y + let x = x + +y in x " @@ -338,8 +402,8 @@ let _ = in x ", " - let x = 2 - +2 + let x = 2 + +2 in x " @@ -349,11 +413,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 @@ -399,8 +476,8 @@ let _ = ) let y = - 2 - + 2 + 2 + + 2 in x + y " @@ -426,9 +503,9 @@ let _ = ", " x < - 2 - + 3 - + 2 + + 3 + " // LetOrUse @@ -547,8 +624,8 @@ let _ = """, """ let mutable x = 3 - x <- 3 - <<< 3 + x <- 3 + <<< 3 """ // DotIndexedGet @@ -702,6 +779,22 @@ let _ = // LibraryOnlyILAssembly """(# "ldlen.multi 2 0" array : int #)""", """(# "ldlen.multi 2 0" array : int #)""" + + // Miscellaneous + " + (match x with + | 1 -> () + | _ -> ()) + + y + ", + " + match x with + | 1 -> () + | _ -> () + + y + " } [<Theory; MemberData(nameof exprs)>]