From 43110c72c397876e44e063ecdfcb2e36f28391ec Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 29 Feb 2024 11:14:51 -0500 Subject: [PATCH 1/2] Shift paren contents less aggressively * The logic in #16666 addressed some multiline parentheses removal issues, but it was slightly too aggressive and sometimes shifted lines farther than was necessary or safe. --- .../CodeFixes/RemoveUnnecessaryParentheses.fs | 132 ++++++------- .../CodeFixes/CodeFixTestFramework.fs | 18 +- .../RemoveUnnecessaryParenthesesTests.fs | 173 ++++++++++++++++-- 3 files changed, 240 insertions(+), 83 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs index 60cbf8ccd76..b55fbb26139 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs @@ -46,60 +46,19 @@ module private Patterns = else ValueNone - /// Trim only spaces from the start if there is something else - /// before the open paren on the same line (or else we could move - /// the whole inner expression up a line); otherwise trim all whitespace - // from start and end. - let (|Trim|) (span: TextSpan) (sourceText: SourceText) = - let linePosition = sourceText.Lines.GetLinePosition span.Start - let line = (sourceText.Lines.GetLineFromPosition span.Start).ToString() - - if line.AsSpan(0, linePosition.Character).LastIndexOfAnyExcept(' ', '(') >= 0 then - fun (s: string) -> s.TrimEnd().TrimStart ' ' - else - fun (s: string) -> s.Trim() - - /// Returns the offsides diff 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 (|OffsidesDiff|_|) (span: TextSpan) (sourceText: SourceText) = - let startLinePosition = sourceText.Lines.GetLinePosition span.Start - let endLinePosition = sourceText.Lines.GetLinePosition span.End - let startLineNo = startLinePosition.Line - let endLineNo = endLinePosition.Line - - if startLineNo = endLineNo then - ValueNone - else - let rec loop innerOffsides lineNo startCol = - if lineNo <= endLineNo then - let line = sourceText.Lines[lineNo].ToString() +[] +type private InnerOffsides = + /// We haven't found an inner construct yet. + | NoneYet - match line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') with - | -1 -> loop innerOffsides (lineNo + 1) 0 - | i -> loop (i + startCol) (lineNo + 1) 0 - else - ValueSome(startLinePosition.Character - innerOffsides) + /// The start column of the first inner construct we find. + /// This may not be on the same line as the open paren. + | FirstLine of col: int - loop startLinePosition.Character startLineNo (startLinePosition.Character + 1) - - let (|ShiftLeft|NoShift|ShiftRight|) n = - if n < 0 then ShiftLeft -n - elif n = 0 then NoShift - else ShiftRight n + /// The leftmost start column of an inner construct on a line + /// following the first inner construct we found. + /// We keep the first column of the first inner construct for comparison at the end. + | FollowingLine of firstLine: int * followingLine: int [] type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [] () = @@ -147,22 +106,67 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [ + /// Trim only spaces from the start if there is something else + /// before the open paren on the same line (or else we could move + /// the whole inner expression up a line); otherwise trim all whitespace + /// from start and end. + let (|Trim|) (sourceText: SourceText) = + let linePosition = sourceText.Lines.GetLinePosition context.Span.Start + let line = (sourceText.Lines.GetLineFromPosition context.Span.Start).ToString() + + if line.AsSpan(0, linePosition.Character).LastIndexOfAnyExcept(' ', '(') >= 0 then + fun (s: string) -> s.TrimEnd().TrimStart ' ' + else + fun (s: string) -> s.Trim() + + let (|ShiftLeft|NoShift|ShiftRight|) (sourceText: SourceText) = + let startLinePosition = sourceText.Lines.GetLinePosition context.Span.Start + let endLinePosition = sourceText.Lines.GetLinePosition context.Span.End + let startLineNo = startLinePosition.Line + let endLineNo = endLinePosition.Line + + if startLineNo = endLineNo then + NoShift + else + let outerOffsides = startLinePosition.Character + + let rec loop innerOffsides lineNo startCol = + if lineNo <= endLineNo then + let line = sourceText.Lines[lineNo].ToString() + + match line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') with + | -1 -> loop innerOffsides (lineNo + 1) 0 + | i -> + match innerOffsides with + | NoneYet -> loop (FirstLine(i + startCol)) (lineNo + 1) 0 + | FirstLine innerOffsides -> loop (FollowingLine(innerOffsides, i + startCol)) (lineNo + 1) 0 + | FollowingLine(firstLine, innerOffsides) -> + loop (FollowingLine(firstLine, min innerOffsides (i + startCol))) (lineNo + 1) 0 + else + innerOffsides + + match loop NoneYet startLineNo (startLinePosition.Character + 1) with + | NoneYet -> NoShift + | FirstLine innerOffsides when innerOffsides < outerOffsides -> ShiftRight(outerOffsides - innerOffsides) + | FirstLine innerOffsides -> ShiftLeft(innerOffsides - outerOffsides) + | FollowingLine(firstLine, followingLine) -> + match firstLine - outerOffsides with + | 0 -> NoShift + | 1 when firstLine < followingLine -> NoShift + | primaryOffset when primaryOffset < 0 -> ShiftRight -primaryOffset + | primaryOffset -> ShiftLeft primaryOffset + let adjusted = match sourceText with | TrailingOpen context.Span -> txt[1 .. txt.Length - 2].TrimEnd() - - | Trim context.Span trim & OffsidesDiff context.Span spaces -> - match spaces with - | NoShift -> trim txt[1 .. txt.Length - 2] - | ShiftLeft spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n" + String(' ', spaces), "\n")) - | ShiftRight spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n", "\n" + String(' ', spaces))) - - | _ -> txt[1 .. txt.Length - 2].Trim() + | Trim trim & NoShift -> trim txt[1 .. txt.Length - 2] + | Trim trim & ShiftLeft spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n" + String(' ', spaces), "\n")) + | Trim trim & ShiftRight spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n", "\n" + String(' ', spaces))) let newText = let (|ShouldPutSpaceBefore|_|) (s: string) = - // "……(……)" - // ↑↑ ↑ + // ……(……) + // ↑↑ ↑ match sourceText[max (context.Span.Start - 2) 0], sourceText[max (context.Span.Start - 1) 0], s[0] with | _, _, ('\n' | '\r') -> None | '[', '|', (Punctuation | LetterOrDigit) -> None @@ -178,8 +182,8 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [ None let (|ShouldPutSpaceAfter|_|) (s: string) = - // "(……)…" - // ↑ ↑ + // (……)… + // ↑ ↑ match s[s.Length - 1], sourceText[min context.Span.End (sourceText.Length - 1)] with | '>', ('|' | ']') -> Some ShouldPutSpaceAfter | _, (')' | ']' | '[' | '}' | '.' | ';' | ',' | '|') -> None diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs index ca8866cda7a..1fb8d27c3a8 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs @@ -205,13 +205,19 @@ module Xunit = let split (s: string) = s.Split([| Environment.NewLine |], StringSplitOptions.RemoveEmptyEntries) - let expected = split expected - let actual = split actual + let expectedLines = split expected + let actualLines = split actual - try - 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] + if expectedLines.Length <> actualLines.Length then + Assert.Equal(expected, actual) + else + try + Assert.All( + (expectedLines, actualLines) ||> 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] /// /// Expects no code fix to be applied to the given code. diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index ff57ffac6d8..8808871f497 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -311,6 +311,152 @@ let _ = "(x) :: []", "x :: []" "x :: ([])", "x :: []" + """ + let longVarName1 = 1 + let longVarName2 = 2 + (longFunctionName + longVarName1 + longVarName2) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 + longFunctionName + longVarName1 + longVarName2 + """ + + """ + let longVarName1 = 1 + let longVarName2 = 2 + (longFunctionName + longVarName1 + longVarName2 + ) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 + longFunctionName + longVarName1 + longVarName2 + """ + + """ + let longVarName1 = 1 + let longVarName2 = 2 + (longFunctionName + longVarName1 + longVarName2 + ) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 + longFunctionName + longVarName1 + longVarName2 + """ + + """ + let longVarName1 = 1 + let longVarName2 = 2 + ( + longFunctionName + longVarName1 + longVarName2 + ) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 + longFunctionName + longVarName1 + longVarName2 + """ + + // We could remove here, but we'd need to update + // the "sensitive indentation" logic to differentiate between + // an outer offsides column established by an open paren (as here) + // and an outer offsides column established by, say, the leftmost + // column of a binding, e.g.: + // + // let _ = ( + // ↑ + // ) + // + // static member M () = ( + // ↑ + // ) + """ + let longVarName1 = 1 + let longVarName2 = 2 + ( + longFunctionName + longVarName1 + longVarName2 + ) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 + ( + longFunctionName + longVarName1 + longVarName2 + ) + """ + + """ + let longVarName1 = 1 + let longVarName2 = 2 + ( + longFunctionName + longVarName1 + longVarName2 + ) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 + ( + longFunctionName + longVarName1 + longVarName2 + ) + """ + + """ + let longVarName1 = 1 + let longVarName2 = 2 + (+longFunctionName + longVarName1 + longVarName2) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 + +longFunctionName + longVarName1 + longVarName2 + """ + + """ + let longVarName1 = 1 + let longVarName2 = 2 in ( + longFunctionName + longVarName1 + longVarName2 + ) + """, + """ + let longVarName1 = 1 + let longVarName2 = 2 in + longFunctionName + longVarName1 + longVarName2 + """ + """ let x = (printfn $"{y}" 2) @@ -342,10 +488,11 @@ let _ = " let x = 2 - + 2 + + 2 in x " + // The extra spaces in this test are intentional. " let x = @@ -361,9 +508,9 @@ let _ = 2 - - - + 2 + + + + 2 in x " @@ -398,7 +545,7 @@ let _ = ", " let x = 2 - + 2 + + 2 in x " @@ -409,7 +556,7 @@ let _ = ", " let x = 2 - + 2 + + 2 in x " @@ -420,7 +567,7 @@ let _ = ", " let x = x - +y + +y in x " @@ -442,7 +589,7 @@ let _ = ", " let x = 2 - <<< 2 + <<< 2 in x " @@ -455,7 +602,7 @@ in x " let (<<<<<<<<) = (<<<) let x = 2 - <<<<<<<< 2 +<<<<<<<< 2 in x " @@ -505,7 +652,7 @@ in x let y = 2 - + 2 + + 2 in x + y " @@ -532,7 +679,7 @@ in x " x < 2 - + 3 + + 3 " // LetOrUse @@ -569,7 +716,7 @@ in x let x = () 2 - ++++++ 2 + ++++++ 2 in x " @@ -806,7 +953,7 @@ in x """ let mutable x = 3 x <- 3 - <<< 3 + <<< 3 """ // DotIndexedGet From 7b6923352943d3983018980419184ea7ddafc8a1 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 29 Feb 2024 11:58:12 -0500 Subject: [PATCH 2/2] Update release notes --- docs/release-notes/.VisualStudio/17.10.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/release-notes/.VisualStudio/17.10.md b/docs/release-notes/.VisualStudio/17.10.md index a4b0a8a9f0d..6c27a8fcfd1 100644 --- a/docs/release-notes/.VisualStudio/17.10.md +++ b/docs/release-notes/.VisualStudio/17.10.md @@ -1,7 +1,7 @@ ### Fixed * Show signature help mid-pipeline in more scenarios. ([PR #16462](https://github.com/dotnet/fsharp/pull/16462)) -* Various unneeded parentheses code fix improvements. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666)) +* Various unneeded parentheses code fix improvements. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666), [PR #16789](https://github.com/dotnet/fsharp/pull/16789)) ### Changed