Skip to content

Commit

Permalink
Add unnecessary parentheses analyzer & code fix (#1235)
Browse files Browse the repository at this point in the history
* Add unnecessary parens analyzer & codefix

This uses the API exposed in FSC in
dotnet/fsharp#16079.

* Comments

* Use nicer APIs

* Fix

* Better there

* Fmt

* Disambiguate AsSpan overload

* Remove debug fail

* Remove redundant diag filter
  • Loading branch information
brianrourkeboll authored Feb 19, 2024
1 parent c6de3f8 commit c3c0c14
Show file tree
Hide file tree
Showing 11 changed files with 355 additions and 12 deletions.
1 change: 1 addition & 0 deletions src/FsAutoComplete.Core/Commands.fs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ type NotificationEvent =
// | Lint of file: string<LocalPath> * warningsWithCodes: Lint.EnrichedLintWarning list
| UnusedDeclarations of file: string<LocalPath> * decls: range[] * version: int
| SimplifyNames of file: string<LocalPath> * names: SimplifyNames.SimplifiableRange[] * version: int
| UnnecessaryParentheses of file: string<LocalPath> * ranges: range[] * version: int
| Canceled of errorMessage: string
| FileParsed of string<LocalPath>
| TestDetected of file: string<LocalPath> * tests: TestAdapter.TestAdapterEntry<range>[]
Expand Down
32 changes: 32 additions & 0 deletions src/FsAutoComplete.Core/Utils.fs
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,38 @@ type ReadOnlySpanExtensions =

line

#if !NET7_0_OR_GREATER
[<Extension>]
static member IndexOfAnyExcept(span: ReadOnlySpan<char>, value0: char, value1: char) =
let mutable i = 0
let mutable found = false

while not found && i < span.Length do
let c = span[i]

if c <> value0 && c <> value1 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

type ConcurrentDictionary<'key, 'value> with

member x.TryFind key =
Expand Down
8 changes: 8 additions & 0 deletions src/FsAutoComplete.Core/Utils.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,14 @@ type ReadOnlySpanExtensions =
[<Extension>]
static member LastLine: text: ReadOnlySpan<char> -> ReadOnlySpan<char>

#if !NET7_0_OR_GREATER
[<Extension>]
static member IndexOfAnyExcept: span: ReadOnlySpan<char> * value0: char * value1: char -> int

[<Extension>]
static member LastIndexOfAnyExcept: span: ReadOnlySpan<char> * value0: char * value1: char -> int
#endif

type ConcurrentDictionary<'key, 'value> with

member TryFind: key: 'key -> 'value option
Expand Down
166 changes: 166 additions & 0 deletions src/FsAutoComplete/CodeFixes/RemoveUnnecessaryParentheses.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
module FsAutoComplete.CodeFix.RemoveUnnecessaryParentheses

open System
open Ionide.LanguageServerProtocol.Types
open FsAutoComplete.CodeFix
open FsAutoComplete.CodeFix.Types
open FsToolkit.ErrorHandling
open FsAutoComplete
open FsAutoComplete.LspHelpers

let title = "Remove unnecessary parentheses"

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

[<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 =
/// E.g., something like:
///
/// let … = (␤
/// …
/// )
[<return: Struct>]
let (|TrailingOpen|_|) (range: FcsRange) (sourceText: IFSACSourceText) =
match sourceText.GetLine range.Start with
| Some line ->
if
line.AsSpan(0, range.Start.Column).LastIndexOfAnyExcept(' ', '(') >= 0
&& line.AsSpan(range.Start.Column).IndexOfAnyExcept('(', ' ') < 0
then
ValueSome TrailingOpen
else
ValueNone

| None -> 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|) (range: FcsRange) (sourceText: IFSACSourceText) =
match sourceText.GetLine range.Start with
| Some line ->
if line.AsSpan(0, range.Start.Column).LastIndexOfAnyExcept(' ', '(') >= 0 then
fun (s: string) -> s.TrimEnd().TrimStart ' '
else
fun (s: string) -> s.Trim()

| None -> id

/// 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).
[<return: Struct>]
let (|OffsidesDiff|_|) (range: FcsRange) (sourceText: IFSACSourceText) =
let startLineNo = range.StartLine
let endLineNo = range.EndLine

if startLineNo = endLineNo then
ValueNone
else
let rec loop innerOffsides (pos: FcsPos) (startCol: int) =
if pos.Line <= endLineNo then
match sourceText.GetLine pos with
| None -> ValueNone
| Some line ->
match line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') with
| -1 -> loop innerOffsides (pos.IncLine()) 0
| i -> loop (i + startCol) (pos.IncLine()) 0
else
ValueSome(range.StartColumn - innerOffsides)

loop range.StartColumn range.Start (range.StartColumn + 1)

let (|ShiftLeft|NoShift|ShiftRight|) n =
if n < 0 then ShiftLeft -n
elif n = 0 then NoShift
else ShiftRight n

/// A codefix that removes unnecessary parentheses from the source.
let fix (getFileLines: GetFileLines) : CodeFix =
Run.ifDiagnosticByCode (Set.singleton "FSAC0004") (fun d codeActionParams ->
asyncResult {
let fileName = codeActionParams.TextDocument.GetFilePath() |> normalizePath
let range = protocolRangeToRange (string fileName) d.Range

let! sourceText = getFileLines fileName
let! txt = sourceText.GetText range

let firstChar = txt[0]
let lastChar = txt[txt.Length - 1]

match firstChar, lastChar with
| '(', ')' ->
let adjusted =
match sourceText with
| TrailingOpen range -> txt[1 .. txt.Length - 2].TrimEnd()

| Trim range trim & OffsidesDiff range 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()

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

let (|ShouldPutSpaceAfter|_|) (s: string) =
// (……)…
// ↑ ↑
sourceText.TryGetChar(range.End.IncColumn 1)
|> Option.bind (fun endChar ->
match s[s.Length - 1], endChar with
| '>', ('|' | ']') -> Some ShouldPutSpaceAfter
| _, (')' | ']' | '[' | '}' | '.' | ';' | ',' | '|') -> None
| (Punctuation | Symbol), (Punctuation | Symbol | LetterOrDigit) -> Some ShouldPutSpaceAfter
| LetterOrDigit, LetterOrDigit -> Some ShouldPutSpaceAfter
| _ -> None)

match adjusted with
| ShouldPutSpaceBefore & ShouldPutSpaceAfter -> " " + adjusted + " "
| ShouldPutSpaceBefore -> " " + adjusted
| ShouldPutSpaceAfter -> adjusted + " "
| adjusted -> adjusted

return
[ { Edits = [| { Range = d.Range; NewText = newText } |]
File = codeActionParams.TextDocument
Title = title
SourceDiagnostic = Some d
Kind = FixKind.Fix } ]

| _notParens -> return []
})
5 changes: 5 additions & 0 deletions src/FsAutoComplete/LspHelpers.fs
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,7 @@ type FSharpConfigDto =
UnusedDeclarationsAnalyzerExclusions: string array option
SimplifyNameAnalyzer: bool option
SimplifyNameAnalyzerExclusions: string array option
UnnecessaryParenthesesAnalyzer: bool option
ResolveNamespaces: bool option
EnableReferenceCodeLens: bool option
EnableAnalyzers: bool option
Expand Down Expand Up @@ -798,6 +799,7 @@ type FSharpConfig =
UnusedDeclarationsAnalyzerExclusions: Regex array
SimplifyNameAnalyzer: bool
SimplifyNameAnalyzerExclusions: Regex array
UnnecessaryParenthesesAnalyzer: bool
ResolveNamespaces: bool
EnableReferenceCodeLens: bool
EnableAnalyzers: bool
Expand Down Expand Up @@ -846,6 +848,7 @@ type FSharpConfig =
UnusedDeclarationsAnalyzerExclusions = [||]
SimplifyNameAnalyzer = false
SimplifyNameAnalyzerExclusions = [||]
UnnecessaryParenthesesAnalyzer = false
ResolveNamespaces = false
EnableReferenceCodeLens = false
EnableAnalyzers = false
Expand Down Expand Up @@ -896,6 +899,7 @@ type FSharpConfig =
SimplifyNameAnalyzerExclusions =
defaultArg dto.SimplifyNameAnalyzerExclusions [||]
|> Array.choose tryCreateRegex
UnnecessaryParenthesesAnalyzer = defaultArg dto.UnnecessaryParenthesesAnalyzer false
ResolveNamespaces = defaultArg dto.ResolveNamespaces false
EnableReferenceCodeLens = defaultArg dto.EnableReferenceCodeLens false
EnableAnalyzers = defaultArg dto.EnableAnalyzers false
Expand Down Expand Up @@ -1003,6 +1007,7 @@ type FSharpConfig =
defaultArg
(dto.SimplifyNameAnalyzerExclusions |> Option.map (Array.choose tryCreateRegex))
x.SimplifyNameAnalyzerExclusions
UnnecessaryParenthesesAnalyzer = defaultArg dto.UnnecessaryParenthesesAnalyzer x.UnnecessaryParenthesesAnalyzer
ResolveNamespaces = defaultArg dto.ResolveNamespaces x.ResolveNamespaces
EnableReferenceCodeLens = defaultArg dto.EnableReferenceCodeLens x.EnableReferenceCodeLens
EnableAnalyzers = defaultArg dto.EnableAnalyzers x.EnableAnalyzers
Expand Down
2 changes: 2 additions & 0 deletions src/FsAutoComplete/LspHelpers.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ type FSharpConfigDto =
UnusedDeclarationsAnalyzerExclusions: string array option
SimplifyNameAnalyzer: bool option
SimplifyNameAnalyzerExclusions: string array option
UnnecessaryParenthesesAnalyzer: bool option
ResolveNamespaces: bool option
EnableReferenceCodeLens: bool option
EnableAnalyzers: bool option
Expand Down Expand Up @@ -387,6 +388,7 @@ type FSharpConfig =
UnusedDeclarationsAnalyzerExclusions: System.Text.RegularExpressions.Regex array
SimplifyNameAnalyzer: bool
SimplifyNameAnalyzerExclusions: System.Text.RegularExpressions.Regex array
UnnecessaryParenthesesAnalyzer: bool
ResolveNamespaces: bool
EnableReferenceCodeLens: bool
EnableAnalyzers: bool
Expand Down
43 changes: 41 additions & 2 deletions src/FsAutoComplete/LspServers/AdaptiveServerState.fs
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,24 @@ type AdaptiveState(lspClient: FSharpLspClient, sourceTextFactory: ISourceTextFac
logger.error (Log.setMessage "checkSimplifiedNames failed" >> Log.addExn e)
}

let checkUnnecessaryParentheses =
async {
try
use progress = new ServerProgressReport(lspClient)
do! progress.Begin($"Checking for unnecessary parentheses {fileName}...", message = filePathUntag)

let! unnecessaryParentheses = UnnecessaryParentheses.getUnnecessaryParentheses getSourceLine tyRes.GetAST

let! ct = Async.CancellationToken

notifications.Trigger(
NotificationEvent.UnnecessaryParentheses(filePath, Array.ofSeq unnecessaryParentheses, file.Version),
ct
)
with e ->
logger.error (Log.setMessage "checkUnnecessaryParentheses failed" >> Log.addExn e)
}

let inline isNotExcluded (exclusions: Regex array) =
exclusions |> Array.exists (fun r -> r.IsMatch filePathUntag) |> not

Expand All @@ -366,7 +384,9 @@ type AdaptiveState(lspClient: FSharpLspClient, sourceTextFactory: ISourceTextFac
config.SimplifyNameAnalyzer
&& isNotExcluded config.SimplifyNameAnalyzerExclusions
then
checkSimplifiedNames ]
checkSimplifiedNames
if config.UnnecessaryParenthesesAnalyzer then
checkUnnecessaryParentheses ]

async {
do! analyzers |> Async.parallel75 |> Async.Ignore<unit[]>
Expand Down Expand Up @@ -525,6 +545,24 @@ type AdaptiveState(lspClient: FSharpLspClient, sourceTextFactory: ISourceTextFac

diagnosticCollections.SetFor(uri, "F# simplify names", version, diags)

| NotificationEvent.UnnecessaryParentheses(file, ranges, version) ->
let uri = Path.LocalPathToUri file

let diags =
ranges
|> Array.map (fun range ->
{ Diagnostic.Range = fcsRangeToLsp range
Code = Some "FSAC0004"
Severity = Some DiagnosticSeverity.Hint
Source = Some "FSAC"
Message = "Parentheses can be removed"
RelatedInformation = Some [||]
Tags = Some [| DiagnosticTag.Unnecessary |]
Data = None
CodeDescription = None })

diagnosticCollections.SetFor(uri, "F# unnecessary parentheses", version, diags)

// | NotificationEvent.Lint (file, warnings) ->
// let uri = Path.LocalPathToUri file
// // let fs =
Expand Down Expand Up @@ -1848,7 +1886,8 @@ type AdaptiveState(lspClient: FSharpLspClient, sourceTextFactory: ISourceTextFac
RemovePatternArgument.fix tryGetParseAndCheckResultsForFile
ToInterpolatedString.fix tryGetParseAndCheckResultsForFile getLanguageVersion
AdjustConstant.fix tryGetParseAndCheckResultsForFile
UpdateValueInSignatureFile.fix tryGetParseAndCheckResultsForFile |])
UpdateValueInSignatureFile.fix tryGetParseAndCheckResultsForFile
RemoveUnnecessaryParentheses.fix forceFindSourceText |])

let forgetDocument (uri: DocumentUri) =
async {
Expand Down
Loading

0 comments on commit c3c0c14

Please sign in to comment.