From c3c0c14147a5906ce69465b94bdacc7ca54211d3 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Mon, 19 Feb 2024 10:04:55 -0500 Subject: [PATCH] Add unnecessary parentheses analyzer & code fix (#1235) * Add unnecessary parens analyzer & codefix This uses the API exposed in FSC in https://github.com/dotnet/fsharp/pull/16079. * Comments * Use nicer APIs * Fix * Better there * Fmt * Disambiguate AsSpan overload * Remove debug fail * Remove redundant diag filter --- src/FsAutoComplete.Core/Commands.fs | 1 + src/FsAutoComplete.Core/Utils.fs | 32 ++++ src/FsAutoComplete.Core/Utils.fsi | 8 + .../CodeFixes/RemoveUnnecessaryParentheses.fs | 166 ++++++++++++++++++ src/FsAutoComplete/LspHelpers.fs | 5 + src/FsAutoComplete/LspHelpers.fsi | 2 + .../LspServers/AdaptiveServerState.fs | 43 ++++- .../CodeFixTests/Tests.fs | 84 ++++++++- .../FindReferencesTests.fs | 15 +- test/FsAutoComplete.Tests.Lsp/Helpers.fs | 1 + .../InlayHintTests.fs | 10 +- 11 files changed, 355 insertions(+), 12 deletions(-) create mode 100644 src/FsAutoComplete/CodeFixes/RemoveUnnecessaryParentheses.fs diff --git a/src/FsAutoComplete.Core/Commands.fs b/src/FsAutoComplete.Core/Commands.fs index 580599657..f25d996f5 100644 --- a/src/FsAutoComplete.Core/Commands.fs +++ b/src/FsAutoComplete.Core/Commands.fs @@ -80,6 +80,7 @@ type NotificationEvent = // | Lint of file: string * warningsWithCodes: Lint.EnrichedLintWarning list | UnusedDeclarations of file: string * decls: range[] * version: int | SimplifyNames of file: string * names: SimplifyNames.SimplifiableRange[] * version: int + | UnnecessaryParentheses of file: string * ranges: range[] * version: int | Canceled of errorMessage: string | FileParsed of string | TestDetected of file: string * tests: TestAdapter.TestAdapterEntry[] diff --git a/src/FsAutoComplete.Core/Utils.fs b/src/FsAutoComplete.Core/Utils.fs index f5bb4d3c9..0fad06fb8 100644 --- a/src/FsAutoComplete.Core/Utils.fs +++ b/src/FsAutoComplete.Core/Utils.fs @@ -499,6 +499,38 @@ type ReadOnlySpanExtensions = line +#if !NET7_0_OR_GREATER + [] + static member IndexOfAnyExcept(span: ReadOnlySpan, 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 + + [] + static member LastIndexOfAnyExcept(span: ReadOnlySpan, 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 = diff --git a/src/FsAutoComplete.Core/Utils.fsi b/src/FsAutoComplete.Core/Utils.fsi index 99e3829f8..2d2bd94bc 100644 --- a/src/FsAutoComplete.Core/Utils.fsi +++ b/src/FsAutoComplete.Core/Utils.fsi @@ -181,6 +181,14 @@ type ReadOnlySpanExtensions = [] static member LastLine: text: ReadOnlySpan -> ReadOnlySpan +#if !NET7_0_OR_GREATER + [] + static member IndexOfAnyExcept: span: ReadOnlySpan * value0: char * value1: char -> int + + [] + static member LastIndexOfAnyExcept: span: ReadOnlySpan * value0: char * value1: char -> int +#endif + type ConcurrentDictionary<'key, 'value> with member TryFind: key: 'key -> 'value option diff --git a/src/FsAutoComplete/CodeFixes/RemoveUnnecessaryParentheses.fs b/src/FsAutoComplete/CodeFixes/RemoveUnnecessaryParentheses.fs new file mode 100644 index 000000000..56f3e091a --- /dev/null +++ b/src/FsAutoComplete/CodeFixes/RemoveUnnecessaryParentheses.fs @@ -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" + +[] +module private Patterns = + let inline toPat f x = if f x then ValueSome() else ValueNone + + [] + module Char = + [] + let inline (|LetterOrDigit|_|) c = toPat Char.IsLetterOrDigit c + + [] + let inline (|Punctuation|_|) c = toPat Char.IsPunctuation c + + [] + let inline (|Symbol|_|) c = toPat Char.IsSymbol c + + [] + module SourceText = + /// E.g., something like: + /// + /// let … = (␤ + /// … + /// ) + [] + 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). + [] + 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 [] + }) diff --git a/src/FsAutoComplete/LspHelpers.fs b/src/FsAutoComplete/LspHelpers.fs index c72b6b6de..52b7f2e56 100644 --- a/src/FsAutoComplete/LspHelpers.fs +++ b/src/FsAutoComplete/LspHelpers.fs @@ -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 @@ -798,6 +799,7 @@ type FSharpConfig = UnusedDeclarationsAnalyzerExclusions: Regex array SimplifyNameAnalyzer: bool SimplifyNameAnalyzerExclusions: Regex array + UnnecessaryParenthesesAnalyzer: bool ResolveNamespaces: bool EnableReferenceCodeLens: bool EnableAnalyzers: bool @@ -846,6 +848,7 @@ type FSharpConfig = UnusedDeclarationsAnalyzerExclusions = [||] SimplifyNameAnalyzer = false SimplifyNameAnalyzerExclusions = [||] + UnnecessaryParenthesesAnalyzer = false ResolveNamespaces = false EnableReferenceCodeLens = false EnableAnalyzers = false @@ -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 @@ -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 diff --git a/src/FsAutoComplete/LspHelpers.fsi b/src/FsAutoComplete/LspHelpers.fsi index e6bd689ab..782fc48ed 100644 --- a/src/FsAutoComplete/LspHelpers.fsi +++ b/src/FsAutoComplete/LspHelpers.fsi @@ -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 @@ -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 diff --git a/src/FsAutoComplete/LspServers/AdaptiveServerState.fs b/src/FsAutoComplete/LspServers/AdaptiveServerState.fs index cf399f7c3..b34bde74a 100644 --- a/src/FsAutoComplete/LspServers/AdaptiveServerState.fs +++ b/src/FsAutoComplete/LspServers/AdaptiveServerState.fs @@ -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 @@ -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 @@ -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 = @@ -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 { diff --git a/test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs b/test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs index dfd80ea5a..df903fe4c 100644 --- a/test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs +++ b/test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs @@ -2794,7 +2794,8 @@ let private resolveNamespaceTests state = ResolveNamespaces = Some true } serverTestList (nameof ResolveNamespace) state config None (fun server -> - [ let selectCodeFix = CodeFix.matching (fun ca -> ca.Title.StartsWith("open", StringComparison.Ordinal)) + [ let selectCodeFix = + CodeFix.matching (fun ca -> ca.Title.StartsWith("open", StringComparison.Ordinal)) testCaseAsync "doesn't fail when target not in last line" <| CodeFix.checkApplicable @@ -3304,6 +3305,84 @@ let private removePatternArgumentTests state = let (None) = None """ ]) +let private removeUnnecessaryParenthesesTests state = + let config = + { defaultConfigDto with + UnnecessaryParenthesesAnalyzer = Some true } + + serverTestList (nameof RemoveUnnecessaryParentheses) state config None (fun server -> + [ let selector = + CodeFix.ofKind "quickfix" + >> CodeFix.withTitle RemoveUnnecessaryParentheses.title + + testCaseAsync "Can remove unnecessary parentheses" + <| CodeFix.check + server + """ + let x = $0(3) + """ + (Diagnostics.expectCode "FSAC0004") + selector + """ + let x = 3 + """ + + testCaseAsync "Adds space after" + <| CodeFix.check + server + """ + $0(int)3.14 + """ + (Diagnostics.expectCode "FSAC0004") + selector + """ + int 3.14 + """ + + testCaseAsync "Adds space before" + <| CodeFix.check + server + """ + int$0(3.14) + """ + (Diagnostics.expectCode "FSAC0004") + selector + """ + int 3.14 + """ + + testCaseAsync "Adds space before and after" + <| CodeFix.check + server + """ + [|$0(<@ 1 @>)|] + """ + (Diagnostics.expectCode "FSAC0004") + selector + """ + [| <@ 1 @> |] + """ + + testCaseAsync "Handles multiline expr well" + <| CodeFix.check + server + """ + let _ = + let x = 3 + ( + let y = 99 + y - x + )$0 + """ + (Diagnostics.expectCode "FSAC0004") + selector + """ + let _ = + let x = 3 + let y = 99 + y - x + """ ]) + let tests textFactory state = testList "CodeFix-tests" @@ -3352,4 +3431,5 @@ let tests textFactory state = wrapExpressionInParenthesesTests state removeRedundantAttributeSuffixTests state removePatternArgumentTests state - UpdateValueInSignatureFileTests.tests state ] + UpdateValueInSignatureFileTests.tests state + removeUnnecessaryParenthesesTests state ] diff --git a/test/FsAutoComplete.Tests.Lsp/FindReferencesTests.fs b/test/FsAutoComplete.Tests.Lsp/FindReferencesTests.fs index 98b12a822..3bbf90b3b 100644 --- a/test/FsAutoComplete.Tests.Lsp/FindReferencesTests.fs +++ b/test/FsAutoComplete.Tests.Lsp/FindReferencesTests.fs @@ -378,27 +378,27 @@ let private untitledTests state = open System open System.Threading.Tasks let _ = task { - do! Task.$$ (TimeSpan.MaxValue) - do! Task.$<``Delay``>$ (TimeSpan.MaxValue) - do! System.Threading.Tasks.Task.$$ (TimeSpan.MaxValue) + do! Task.$$ TimeSpan.MaxValue + do! Task.$<``Delay``>$ TimeSpan.MaxValue + do! System.Threading.Tasks.Task.$$ TimeSpan.MaxValue do! System .Threading .Tasks .Task - .$$ (TimeSpan.MaxValue) + .$$ TimeSpan.MaxValue } """ """ open System open System.Threading.Tasks let _ = task { - do! Task.$$ (TimeSpan.MaxValue) + do! Task.$$ TimeSpan.MaxValue printfn "..." - do! Threading.Tasks.Task.$$ (TimeSpan.MaxValue) + do! Threading.Tasks.Task.$$ TimeSpan.MaxValue } let _ = task { - do! Task.$$ (TimeSpan.MaxValue) + do! Task.$$ TimeSpan.MaxValue } """ // No Task.Delay @@ -458,6 +458,7 @@ let private rangeTests state = async { let (source, cursors) = sourceWithCursors |> extractRanges let! (doc, diags) = server |> Server.createUntitledDocument source + use doc = doc Expect.hasLength diags 0 "There should be no diags" diff --git a/test/FsAutoComplete.Tests.Lsp/Helpers.fs b/test/FsAutoComplete.Tests.Lsp/Helpers.fs index 2b0ee4c6b..366a21d30 100644 --- a/test/FsAutoComplete.Tests.Lsp/Helpers.fs +++ b/test/FsAutoComplete.Tests.Lsp/Helpers.fs @@ -226,6 +226,7 @@ let defaultConfigDto: FSharpConfigDto = UnusedDeclarationsAnalyzerExclusions = None SimplifyNameAnalyzer = None SimplifyNameAnalyzerExclusions = None + UnnecessaryParenthesesAnalyzer = None ResolveNamespaces = None EnableReferenceCodeLens = None EnableAnalyzers = None diff --git a/test/FsAutoComplete.Tests.Lsp/InlayHintTests.fs b/test/FsAutoComplete.Tests.Lsp/InlayHintTests.fs index 433757289..55321d0a0 100644 --- a/test/FsAutoComplete.Tests.Lsp/InlayHintTests.fs +++ b/test/FsAutoComplete.Tests.Lsp/InlayHintTests.fs @@ -151,8 +151,16 @@ module private LspInlayHints = = async { let! (doc, diags) = server |> Server.createUntitledDocument text + + let pertinentDiags = + diags + |> Array.filter (function + // Ignore extra parens diagnostics here. + | { Code = Some "FSAC0004" } -> false + | _ -> true) + use doc = doc - Expect.hasLength diags 0 "Should not have had check errors" + Expect.hasLength pertinentDiags 0 "Should not have had check errors" let! hints = doc |> Document.inlayHintsAt range let hints = hints |> Array.sortBy (fun h -> h.Position)