Improve detection and replacement logic for 'remove binding' codefix

.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"
             ]
         },
         {

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 } ]
     })

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)

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
 ]

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<LanguageServerProtocol.Types.PublishDiagnosticsParams> "textDocument/publishDiagnostics"

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
+  ()