Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shift multiline paren contents less aggressively #16789

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Shift multiline paren contents less aggressively #16789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43110c7
Shift paren contents less aggressively
brianrourkeboll Feb 29, 2024
7b69233
Update release notes
brianrourkeboll Feb 29, 2024
96bf8fc
Merge branch 'main' into parens-another-one
KevinRansom Mar 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/release-notes/.VisualStudio/17.10.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Expand Down
132 changes: 68 additions & 64 deletions vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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◌
[<return: Struct>]
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()
[<NoEquality; NoComparison>]
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

[<ExportCodeFixProvider(FSharpConstants.FSharpLanguageName, Name = CodeFix.RemoveUnnecessaryParentheses); Shared; Sealed>]
type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConstructor>] () =
Expand Down Expand Up @@ -147,22 +106,67 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst

match firstChar, lastChar with
| '(', ')' ->
/// 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
Expand All @@ -178,8 +182,8 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst
| _ -> None

let (|ShouldPutSpaceAfter|_|) (s: string) =
// "(……)…"
// ↑ ↑
// (……)…
// ↑ ↑
match s[s.Length - 1], sourceText[min context.Span.End (sourceText.Length - 1)] with
| '>', ('|' | ']') -> Some ShouldPutSpaceAfter
| _, (')' | ']' | '[' | '}' | '.' | ';' | ',' | '|') -> None
Expand Down
18 changes: 12 additions & 6 deletions vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

/// <summary>
/// Expects no code fix to be applied to the given code.
Expand Down
Loading
Loading