Skip to content

Commit

Permalink
Parens: tweak sensitive indentation handling (#16248)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@brianrourkeboll
brianrourkeboll authored Nov 14, 2023
1 parent 995a182 commit d16430f
Show file tree
Hide file tree
Showing 4 changed files with 260 additions and 80 deletions.
159 changes: 104 additions & 55 deletions vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>] () =
Expand Down Expand Up @@ -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) =
Expand All @@ -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
Expand Down
38 changes: 38 additions & 0 deletions vsintegration/src/FSharp.Editor/Common/Extensions.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
2 changes: 1 addition & 1 deletion vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
Loading

0 comments on commit d16430f

Please sign in to comment.