diff --git a/.vscode/launch.json b/.vscode/launch.json index c1aa80bd1..93e678ad8 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -39,7 +39,7 @@ "enableStepFiltering": false, "args": [ "--filter", - "lsp.Ionide WorkspaceLoader.Go to definition tests.GoTo Tests.Go-to-type-definition on first char of identifier" + "lsp.Ionide WorkspaceLoader.codefix tests.remove unused binding" ] }, { diff --git a/src/FsAutoComplete/CodeFixes/RemoveUnusedBinding.fs b/src/FsAutoComplete/CodeFixes/RemoveUnusedBinding.fs index 3a658c6a5..6aa47bff6 100644 --- a/src/FsAutoComplete/CodeFixes/RemoveUnusedBinding.fs +++ b/src/FsAutoComplete/CodeFixes/RemoveUnusedBinding.fs @@ -16,28 +16,53 @@ let posBetween (range: Range) tester = Pos.posGeq tester range.Start // positions on this one are flipped to simulate Pos.posLte, because that doesn't exist && Pos.posGeq range.End tester +type private ReplacmentRangeResult = +| FullBinding of bindingRange: Range +| Pattern of patternRange: Range + type FSharpParseFileResults with - member this.TryRangeOfBindingWithHeadPatternWithPos pos = + member private this.TryRangeOfBindingWithHeadPatternWithPos (diagnosticRange: range) = this.ParseTree |> Option.bind (fun input -> - AstTraversal.Traverse(pos, input, { new AstTraversal.AstVisitorBase<_>() with + AstTraversal.Traverse(diagnosticRange.Start, input, { new AstTraversal.AstVisitorBase<_>() with member _.VisitExpr(_, _, defaultTraverse, expr) = defaultTraverse expr + override _.VisitPat(defaultTraverse, pat: SynPat) = + // if the diagnostic was for this specific pattern in its entirety, then we're don + if Range.equals pat.Range diagnosticRange then Some (Pattern diagnosticRange) + else + match pat with + | SynPat.Paren (inner, m) -> + // otherwise if the pattern inside a parens + if Range.rangeContainsRange m diagnosticRange + then + // explicitly matches + if Range.equals inner.Range diagnosticRange + // then return the range of the parens, so the entire pattern gets removed + then Some (Pattern m) + else defaultTraverse inner + else defaultTraverse inner + | pat -> defaultTraverse pat override _.VisitBinding(defaultTraverse, binding) = match binding with | SynBinding.Binding(_, SynBindingKind.NormalBinding, _, _, _, _, _, pat, _, _, _, _) as binding -> - if posBetween binding.RangeOfHeadPat pos then - Some binding.RangeOfBindingAndRhs - else - // Check if it's an operator - match pat with - | SynPat.LongIdent(LongIdentWithDots([id], _), _, _, _, _, _) when id.idText.StartsWith("op_") -> - if posBetween id.idRange pos then - Some binding.RangeOfBindingAndRhs - else - defaultTraverse binding - | _ -> defaultTraverse binding + // walk the patterns in the binding first, to allow the parameter traversal a chance to fire + match defaultTraverse binding with + | None -> + // otherwise if the diagnostic was in this binding's head pattern then do teh replacement + if Range.rangeContainsRange binding.RangeOfHeadPat diagnosticRange then + Some (FullBinding binding.RangeOfBindingAndRhs) + else + // Check if it's an operator + match pat with + | SynPat.LongIdent(LongIdentWithDots([id], _), _, _, _, _, _) when id.idText.StartsWith("op_") -> + if Range.rangeContainsRange id.idRange diagnosticRange then + Some (FullBinding binding.RangeOfBindingAndRhs) + else + defaultTraverse binding + | _ -> defaultTraverse binding + | Some range -> Some range | _ -> defaultTraverse binding }) ) @@ -48,21 +73,36 @@ let fix (getParseResults: GetParseResultsForFile): CodeFix = let fileName = codeActionParams.TextDocument.GetFilePath() |> Utils.normalizePath let fcsRange = protocolRangeToRange (codeActionParams.TextDocument.GetFilePath()) diagnostic.Range let! tyres, line, lines = getParseResults fileName fcsRange.Start - let! rangeOfBinding = tyres.GetParseResults.TryRangeOfBindingWithHeadPatternWithPos(fcsRange.Start) |> Result.ofOption (fun () -> "no binding range found") + let! rangeOfBinding = tyres.GetParseResults.TryRangeOfBindingWithHeadPatternWithPos(fcsRange) |> Result.ofOption (fun () -> "no binding range found") - let protocolRange = fcsRangeToLsp rangeOfBinding + match rangeOfBinding with + | Pattern rangeOfPattern -> + let protocolRange = fcsRangeToLsp rangeOfPattern - // the pos at the end of the keyword - let! endOfPrecedingKeyword = - Navigation.walkBackUntilCondition lines (dec lines protocolRange.Start) (System.Char.IsWhiteSpace) - |> Result.ofOption (fun _ -> "failed to walk") + // the pos at the end of the previous token + let! endOfPrecedingToken = + Navigation.walkBackUntilCondition lines protocolRange.Start (System.Char.IsWhiteSpace >> not) + |> Result.ofOption (fun _ -> "failed to walk") + // replace from there to the end of the pattern's range + let replacementRange = { Start = endOfPrecedingToken; End = protocolRange.End } + return [ { Title = "Remove unused parameter" + Edits = [| { Range = replacementRange; NewText = "" } |] + File = codeActionParams.TextDocument + SourceDiagnostic = Some diagnostic + Kind = FixKind.Refactor } ] + | FullBinding bindingRangeWithPats -> + let protocolRange = fcsRangeToLsp bindingRangeWithPats + // the pos at the end of the previous keyword + let! endOfPrecedingKeyword = + Navigation.walkBackUntilCondition lines (dec lines protocolRange.Start) (System.Char.IsWhiteSpace >> not) + |> Result.ofOption (fun _ -> "failed to walk") - // walk back to the start of the keyword, which is always `let` or `use` - let keywordStartColumn = decMany lines endOfPrecedingKeyword 3 - let replacementRange = { Start = keywordStartColumn; End = protocolRange.End } - return [ { Title = "Remove unused binding" - Edits = [| { Range = replacementRange; NewText = "" } |] - File = codeActionParams.TextDocument - SourceDiagnostic = Some diagnostic - Kind = FixKind.Refactor } ] + // walk back to the start of the keyword, which is always `let` or `use` + let keywordStartColumn = decMany lines endOfPrecedingKeyword 3 + let replacementRange = { Start = keywordStartColumn; End = protocolRange.End } + return [ { Title = "Remove unused binding" + Edits = [| { Range = replacementRange; NewText = "" } |] + File = codeActionParams.TextDocument + SourceDiagnostic = Some diagnostic + Kind = FixKind.Refactor } ] }) diff --git a/src/FsAutoComplete/FsAutoComplete.Lsp.fs b/src/FsAutoComplete/FsAutoComplete.Lsp.fs index 53fec3081..0394e11de 100644 --- a/src/FsAutoComplete/FsAutoComplete.Lsp.fs +++ b/src/FsAutoComplete/FsAutoComplete.Lsp.fs @@ -614,7 +614,7 @@ type FSharpLspServer(backgroundServiceEnabled: bool, state: State, lspClient: FS Run.ifEnabled (fun _ -> config.ResolveNamespaces) (ResolveNamespace.fix tryGetParseResultsForFile commands.GetNamespaceSuggestions) SuggestedIdentifier.fix RedundantQualifier.fix - UnusedValue.fix getRangeText + Run.ifEnabled (fun _ -> config.UnusedDeclarationsAnalyzer) (UnusedValue.fix getRangeText) NewWithDisposables.fix getRangeText Run.ifEnabled (fun _ -> config.UnionCaseStubGeneration) (GenerateUnionCases.fix getFileLines tryGetParseResultsForFile commands.GetUnionPatternMatchCases getUnionCaseStubReplacements) diff --git a/test/FsAutoComplete.Tests.Lsp/CodeFixTests.fs b/test/FsAutoComplete.Tests.Lsp/CodeFixTests.fs index 1458b176a..c2f7f424e 100644 --- a/test/FsAutoComplete.Tests.Lsp/CodeFixTests.fs +++ b/test/FsAutoComplete.Tests.Lsp/CodeFixTests.fs @@ -6,6 +6,9 @@ open Helpers open LanguageServerProtocol.Types open FsAutoComplete.Utils +let pos (line, character) = { Line = line; Character = character } +let range st e = { Start = pos st; End = pos e } + let (|Refactor|_|) title newText action = match action with | { Title = title' @@ -17,6 +20,13 @@ let (|Refactor|_|) title newText action = when title' = title && newText' = newText -> Some () | _ -> None +let (|AtRange|_|) range (action: CodeAction) = + match action with + | { Edit = { DocumentChanges = Some [| + { Edits = [| { Range = range' } |] } + |] } } when range = range' -> Some () + | _ -> None + let abstractClassGenerationTests state = let server = async { @@ -307,6 +317,97 @@ let unusedValueTests state = canReplaceUnusedParameter ] +let removeUnusedBindingTests state = + let (|RemoveBinding|_|) = (|Refactor|_|) "Remove unused binding" "" + let (|RemoveParameter|_|) = (|Refactor|_|) "Remove unused parameter" "" + + let server = + async { + let path = Path.Combine(__SOURCE_DIRECTORY__, "TestCases", "RemoveUnusedBinding") + let cfg = { defaultConfigDto with FSIExtraParameters = Some [| "--warnon:1182" |] } + let! (server, events) = serverInitialize path cfg state + do! waitForWorkspaceFinishedParsing events + let path = Path.Combine(path, "Script.fsx") + let tdop : DidOpenTextDocumentParams = { TextDocument = loadDocument path } + do! server.TextDocumentDidOpen tdop + let! diagnostics = + events + |> waitForCompilerDiagnosticsForFile "Script.fsx" + |> AsyncResult.bimap (fun _ -> failtest "Should have had errors") id + return (server, path, diagnostics) + } + |> Async.Cache + + let canRemoveUnusedSingleCharacterFunctionParameter = testCaseAsync "can remove unused single character function parameter" (async { + let! server, file, diagnostics = server + let targetRange = range (0, 9) (0, 10) + let diagnostic = + diagnostics + |> Array.tryFind (fun d -> d.Range = targetRange && d.Code = Some "1182") + |> Option.defaultWith (fun () -> failwith "could not find diagnostic with expected range and code") + let detected = { + CodeActionParams.TextDocument = { Uri = Path.FilePathToUri file } + Range = diagnostic.Range + Context = { Diagnostics = [| diagnostic |] } + } + let replacementRange = range (0, 8) (0, 10) + match! server.TextDocumentCodeAction detected with + | Ok (Some (TextDocumentCodeActionResult.CodeActions [| RemoveParameter & AtRange replacementRange ; _ (* explicit type annotation codefix *) |])) -> () + | Ok other -> + failtestf $"Should have generated _, but instead generated %A{other}" + | Error reason -> + failtestf $"Should have succeeded, but failed with %A{reason}" + }) + + let canRemoveUnusedSingleCharacterFunctionParameterInParens = testCaseAsync "can remove unused single character function parameter in parens" (async { + let! server, file, diagnostics = server + let targetRange = range (2, 11) (2, 12) + let diagnostic = + diagnostics + |> Array.tryFind (fun d -> d.Range = targetRange && d.Code = Some "1182") + |> Option.defaultWith (fun () -> failwith "could not find diagnostic with expected range and code") + let detected = { + CodeActionParams.TextDocument = { Uri = Path.FilePathToUri file } + Range = diagnostic.Range + Context = { Diagnostics = [| diagnostic |] } + } + let replacementRange = range (2, 9) (2, 13) + match! server.TextDocumentCodeAction detected with + | Ok (Some (TextDocumentCodeActionResult.CodeActions [| RemoveParameter & AtRange replacementRange ; _ (* explicit type annotation codefix *) |])) -> () + | Ok other -> + failtestf $"Should have generated _, but instead generated %A{other}" + | Error reason -> + failtestf $"Should have succeeded, but failed with %A{reason}" + }) + + let canRemoveUnusedBindingInsideTopLevel = testCaseAsync "can remove unused binding inside top level" (async { + let! server, file, diagnostics = server + let targetRange = range (5, 6) (5, 10) + let diagnostic = + diagnostics + |> Array.tryFind (fun d -> d.Range = targetRange && d.Code = Some "1182") + |> Option.defaultWith (fun () -> failwith "could not find diagnostic with expected range and code") + let detected = { + CodeActionParams.TextDocument = { Uri = Path.FilePathToUri file } + Range = diagnostic.Range + Context = { Diagnostics = [| diagnostic |] } + } + let replacementRange = range (5,2) (5,16) // span of whole `let incr...` binding + match! server.TextDocumentCodeAction detected with + | Ok (Some (TextDocumentCodeActionResult.CodeActions [| RemoveBinding & AtRange replacementRange |])) -> () + | Ok other -> + failtestf $"Should have generated _, but instead generated %A{other}" + | Error reason -> + failtestf $"Should have succeeded, but failed with %A{reason}" + }) + + + testList "remove unused binding" [ + canRemoveUnusedSingleCharacterFunctionParameter + canRemoveUnusedSingleCharacterFunctionParameterInParens + canRemoveUnusedBindingInsideTopLevel + ] + let addExplicitTypeAnnotationTests state = let server = async { @@ -354,4 +455,5 @@ let tests state = testList "codefix tests" [ missingInstanceMemberTests state unusedValueTests state addExplicitTypeAnnotationTests state + removeUnusedBindingTests state ] diff --git a/test/FsAutoComplete.Tests.Lsp/Helpers.fs b/test/FsAutoComplete.Tests.Lsp/Helpers.fs index af77bfc2a..de054a77d 100644 --- a/test/FsAutoComplete.Tests.Lsp/Helpers.fs +++ b/test/FsAutoComplete.Tests.Lsp/Helpers.fs @@ -413,6 +413,10 @@ let fsacDiagnostics file = fileDiagnostics file >> diagnosticsFromSource "FSAC" +let compilerDiagnostics file = + fileDiagnostics file + >> diagnosticsFromSource "F# Compiler" + let diagnosticsToResult = Observable.map (function | [||] -> Ok () | diags -> Core.Error diags) @@ -426,6 +430,11 @@ let waitForFsacDiagnosticsForFile file = >> diagnosticsToResult >> Async.AwaitObservable +let waitForCompilerDiagnosticsForFile file = + compilerDiagnostics file + >> diagnosticsToResult + >> Async.AwaitObservable + let waitForParsedScript (event: ClientEvents) = event |> typedEvents "textDocument/publishDiagnostics" diff --git a/test/FsAutoComplete.Tests.Lsp/TestCases/RemoveUnusedBinding/Script.fsx b/test/FsAutoComplete.Tests.Lsp/TestCases/RemoveUnusedBinding/Script.fsx new file mode 100644 index 000000000..ae7e48670 --- /dev/null +++ b/test/FsAutoComplete.Tests.Lsp/TestCases/RemoveUnusedBinding/Script.fsx @@ -0,0 +1,7 @@ +let incr i x = 2 + +let incr2 (i) = 2 + +let container () = + let incr x = 2 + ()