Skip to content

Commit

Permalink
Some parens fixes (#1286)
Browse files Browse the repository at this point in the history
See dotnet/fsharp#16901

* Keep parens around outlaw `match` exprs (where the first `|` is
  leftward of the start of the `match` keyword).

* Ignore single-line comments when determining offsides lines.

* Don't add a space when removing parens when doing so would result in
  reparsing an infix op as a prefix op.
  • Loading branch information
brianrourkeboll authored May 8, 2024
1 parent e9ad5eb commit 22d9876
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 33 deletions.
13 changes: 13 additions & 0 deletions src/FsAutoComplete.Core/Utils.fs
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,19 @@ type ReadOnlySpanExtensions =

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
Expand Down
3 changes: 3 additions & 0 deletions src/FsAutoComplete.Core/Utils.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ type ReadOnlySpanExtensions =
[<Extension>]
static member IndexOfAnyExcept: span: ReadOnlySpan<char> * value0: char * value1: char -> int

[<Extension>]
static member IndexOfAnyExcept: span: ReadOnlySpan<char> * values: ReadOnlySpan<char> -> int

[<Extension>]
static member LastIndexOfAnyExcept: span: ReadOnlySpan<char> * value0: char * value1: char -> int
#endif
Expand Down
116 changes: 85 additions & 31 deletions src/FsAutoComplete/CodeFixes/RemoveUnnecessaryParentheses.fs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,35 @@ open FsAutoComplete.CodeFix.Types
open FsToolkit.ErrorHandling
open FsAutoComplete
open FsAutoComplete.LspHelpers
open FSharp.Compiler.Text

let title = "Remove unnecessary parentheses"

[<AutoOpen>]
module private Patterns =
let inline toPat f x = if f x then ValueSome() else ValueNone

/// Starts with //.
[<return: Struct>]
let (|StartsWithSingleLineComment|_|) (s: string) =
if s.AsSpan().TrimStart(' ').StartsWith("//".AsSpan()) then
ValueSome StartsWithSingleLineComment
else
ValueNone

/// Starts with match, e.g.,
///
/// (match … with
/// | … -> …)
[<return: Struct>]
let (|StartsWithMatch|_|) (s: string) =
let s = s.AsSpan().TrimStart ' '

if s.StartsWith("match".AsSpan()) && (s.Length = 5 || s[5] = ' ') then
ValueSome StartsWithMatch
else
ValueNone

[<AutoOpen>]
module Char =
[<return: Struct>]
Expand Down Expand Up @@ -90,8 +112,8 @@ let fix (getFileLines: GetFileLines) : CodeFix =
| None -> id

let (|ShiftLeft|NoShift|ShiftRight|) (sourceText: IFSACSourceText) =
let startLineNo = range.StartLine
let endLineNo = range.EndLine
let startLineNo = Line.toZ range.StartLine
let endLineNo = Line.toZ range.EndLine

if startLineNo = endLineNo then
NoShift
Expand All @@ -105,11 +127,17 @@ let fix (getFileLines: GetFileLines) : CodeFix =
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
match line[i + startCol ..] with
| StartsWithMatch
| StartsWithSingleLineComment -> loop innerOffsides (lineNo + 1) 0
| _ ->
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

Expand All @@ -133,24 +161,27 @@ let fix (getFileLines: GetFileLines) : CodeFix =

let newText =
let (|ShouldPutSpaceBefore|_|) (s: string) =
// ……(……)
// ↑↑ ↑
(sourceText.TryGetChar(range.Start.IncColumn -1), sourceText.TryGetChar range.Start)
||> Option.map2 (fun twoBefore oneBefore ->
match twoBefore, oneBefore, s[0] with
| _, _, ('\n' | '\r') -> None
| '[', '|', (Punctuation | LetterOrDigit) -> None
| _, '[', '<' -> Some ShouldPutSpaceBefore
| _, ('(' | '[' | '{'), _ -> None
| _, '>', _ -> Some ShouldPutSpaceBefore
| ' ', '=', _ -> Some ShouldPutSpaceBefore
| _, '=', ('(' | '[' | '{') -> None
| _, '=', (Punctuation | Symbol) -> Some ShouldPutSpaceBefore
| _, LetterOrDigit, '(' -> None
| _, (LetterOrDigit | '`'), _ -> Some ShouldPutSpaceBefore
| _, (Punctuation | Symbol), (Punctuation | Symbol) -> Some ShouldPutSpaceBefore
| _ -> None)
|> Option.flatten
match s with
| StartsWithMatch -> None
| _ ->
// ……(……)
// ↑↑ ↑
(sourceText.TryGetChar(range.Start.IncColumn -1), sourceText.TryGetChar range.Start)
||> Option.map2 (fun twoBefore oneBefore ->
match twoBefore, oneBefore, s[0] with
| _, _, ('\n' | '\r') -> None
| '[', '|', (Punctuation | LetterOrDigit) -> None
| _, '[', '<' -> Some ShouldPutSpaceBefore
| _, ('(' | '[' | '{'), _ -> None
| _, '>', _ -> Some ShouldPutSpaceBefore
| ' ', '=', _ -> Some ShouldPutSpaceBefore
| _, '=', ('(' | '[' | '{') -> None
| _, '=', (Punctuation | Symbol) -> Some ShouldPutSpaceBefore
| _, LetterOrDigit, '(' -> None
| _, (LetterOrDigit | '`'), _ -> Some ShouldPutSpaceBefore
| _, (Punctuation | Symbol), (Punctuation | Symbol) -> Some ShouldPutSpaceBefore
| _ -> None)
|> Option.flatten

let (|ShouldPutSpaceAfter|_|) (s: string) =
// (……)…
Expand All @@ -160,22 +191,45 @@ let fix (getFileLines: GetFileLines) : CodeFix =
match s[s.Length - 1], endChar with
| '>', ('|' | ']') -> Some ShouldPutSpaceAfter
| _, (')' | ']' | '[' | '}' | '.' | ';' | ',' | '|') -> None
| _, ('+' | '-' | '%' | '&' | '!' | '~') -> None
| (Punctuation | Symbol), (Punctuation | Symbol | LetterOrDigit) -> Some ShouldPutSpaceAfter
| LetterOrDigit, LetterOrDigit -> Some ShouldPutSpaceAfter
| _ -> None)

let (|WouldTurnInfixIntoPrefix|_|) (s: string) =
// (……)…
// ↑ ↑
sourceText.TryGetChar(range.End.IncColumn 1)
|> Option.bind (fun endChar ->
match s[s.Length - 1], endChar with
| (Punctuation | Symbol), ('+' | '-' | '%' | '&' | '!' | '~') ->
match sourceText.GetLine range.End with
| None -> None
| Some line ->
// (……)+…
// ↑
match line.AsSpan(range.EndColumn).IndexOfAnyExcept("*/%-+:^@><=!|$.?".AsSpan()) with
| -1 -> None
| i when line[range.EndColumn + i] <> ' ' -> Some WouldTurnInfixIntoPrefix
| _ -> None
| _ -> None)

match adjusted with
| ShouldPutSpaceBefore & ShouldPutSpaceAfter -> " " + adjusted + " "
| ShouldPutSpaceBefore -> " " + adjusted
| ShouldPutSpaceAfter -> adjusted + " "
| adjusted -> adjusted
| WouldTurnInfixIntoPrefix -> ValueNone
| ShouldPutSpaceBefore & ShouldPutSpaceAfter -> ValueSome(" " + adjusted + " ")
| ShouldPutSpaceBefore -> ValueSome(" " + adjusted)
| ShouldPutSpaceAfter -> ValueSome(adjusted + " ")
| adjusted -> ValueSome adjusted

return
[ { Edits = [| { Range = d.Range; NewText = newText } |]
newText
|> ValueOption.map (fun newText ->
{ Edits = [| { Range = d.Range; NewText = newText } |]
File = codeActionParams.TextDocument
Title = title
SourceDiagnostic = Some d
Kind = FixKind.Fix } ]
Kind = FixKind.Fix })
|> ValueOption.toList

| _notParens -> return []
})
65 changes: 63 additions & 2 deletions test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -1606,7 +1606,7 @@ let private generateXmlDocumentationTests state =
/// <summary></summary>
type MyRecord = { Foo: int }
"""

testCaseAsync "documentation for record type with multiple attribute lists"
<| CodeFix.check
server
Expand Down Expand Up @@ -3436,7 +3436,68 @@ let private removeUnnecessaryParenthesesTests state =
longFunctionName
longVarName1
longVarName2
""" ])
"""

testCaseAsync "Handles outlaw match exprs"
<| CodeFix.check
server
"""
3 > (match x with
| 1
| _ -> 3)$0
"""
(Diagnostics.expectCode "FSAC0004")
selector
"""
3 > match x with
| 1
| _ -> 3
"""

testCaseAsync "Handles even more outlaw match exprs"
<| CodeFix.check
server
"""
3 > ( match x with
| 1
| _ -> 3)$0
"""
(Diagnostics.expectCode "FSAC0004")
selector
"""
3 > match x with
| 1
| _ -> 3
"""

testCaseAsync "Handles single-line comments"
<| CodeFix.check
server
"""
3 > (match x with
// Lol.
| 1
| _ -> 3)$0
"""
(Diagnostics.expectCode "FSAC0004")
selector
"""
3 > match x with
// Lol.
| 1
| _ -> 3
"""

testCaseAsync "Keep parens when removal would cause reparse of infix as prefix"
<| CodeFix.checkNotApplicable
server
"""
""+(Unchecked.defaultof<string>)$0+""
"""
(Diagnostics.expectCode "FSAC0004")
selector

])

let tests textFactory state =
testList
Expand Down

0 comments on commit 22d9876

Please sign in to comment.