Get Unused Declarations from FCS (#998)

Remove custom `UnusedDeclarationAnalyzer`

Change: Unused Diags now always contain Code `FSAC0003`

Fix: don't trigger Prefix with `_` CodeFix for backticks
Fix: don't replace private variable with `_`

src/FsAutoComplete.Core/Commands.fs

@@ -80,7 +80,7 @@ type NotificationEvent =
   | AnalyzerMessage of messages: FSharp.Analyzers.SDK.Message[] * file: string<LocalPath>
   | UnusedOpens of file: string<LocalPath> * opens: Range[]
   // | Lint of file: string<LocalPath> * warningsWithCodes: Lint.EnrichedLintWarning list
-  | UnusedDeclarations of file: string<LocalPath> * decls: (range * bool)[]
+  | UnusedDeclarations of file: string<LocalPath> * decls: range[]
   | SimplifyNames of file: string<LocalPath> * names: SimplifyNames.SimplifiableRange[]
   | Canceled of errorMessage: string
   | FileParsed of string<LocalPath>
@@ -1513,11 +1513,8 @@ type Commands(checker: FSharpCompilerServiceChecker, state: State, hasAnalyzers:
       match tyResOpt with
       | None -> ()
       | Some tyRes ->
-        let allUses = tyRes.GetCheckResults.GetAllUsesOfAllSymbolsInFile()
-
-        let unused =
-          UnusedDeclarationsAnalyzer.getUnusedDeclarationRanges allUses isScript
-          |> Seq.toArray
+        let! unused = UnusedDeclarations.getUnusedDeclarations (tyRes.GetCheckResults, isScript)
+        let unused = unused |> Seq.toArray
 
         notify.Trigger(NotificationEvent.UnusedDeclarations(file, unused))
     }

src/FsAutoComplete.Core/FsAutoComplete.Core.fsproj

@@ -26,7 +26,6 @@
     <Compile Include="DocumentationFormatter.fs" />
     <Compile Include="FileSystem.fs" />
     <Compile Include="KeywordList.fs" />
-    <Compile Include="UnusedDeclarationsAnalyzer.fs" />
     <Compile Include="Decompiler.fs" />
     <Compile Include="Sourcelink.fs" />
     <Compile Include="InteractiveDirectives.fs" />

src/FsAutoComplete/CodeFixes/RenameUnusedValue.fs

@@ -5,44 +5,86 @@ open FsAutoComplete.CodeFix
 open FsAutoComplete.CodeFix.Types
 open FsAutoComplete
 open FsAutoComplete.LspHelpers
+open FSharp.Compiler.Symbols
+open FSharp.UMX
+open FSharp.Compiler.Syntax
+open FSharp.Compiler.Text
 
 let titleReplace = "Replace with _"
 let titlePrefix = "Prefix with _"
 
+/// `let private foo = ...`: `foo` cannot be `_`: `private` not allowed -> must be replaced too
+///
+/// But current FCS version doesn't include range for `SynAccess`
+/// -> no (easy) way to get range with accessibility
+/// -> instead of range to replace, just if there's accessibility
+let private variableHasAccessibility (ast: ParsedInput) (pos: Position) =
+  SyntaxTraversal.Traverse(
+    pos,
+    ast,
+    { new SyntaxVisitorBase<_>() with
+        member _.VisitPat(_, defaultTraverse, pat) =
+          match pat with
+          | SynPat.Named (accessibility = Some _; range = range) when Range.rangeContainsPos range pos ->
+            // `SynAccess` in FCS version currently used in FSAC doesn't contain its range
+            // -> no easy way to get range with accessibility
+            // -> instead of returning range with accessibility, just info if there's accessibility
+            // TODO: return range with accessibility once available (https://github.com/dotnet/fsharp/pull/13304)
+            Some true
+          | _ -> defaultTraverse pat }
+  )
+  |> Option.defaultValue false
+
 /// a codefix that suggests prepending a _ to unused values
-let fix (getRangeText: GetRangeText) =
-  Run.ifDiagnosticByMessage "is unused" (fun diagnostic codeActionParams ->
+let fix (getParseResultsForFile: GetParseResultsForFile) =
+  Run.ifDiagnosticByCode (Set.ofList [ "FSAC0003" ]) (fun diagnostic codeActionParams ->
     asyncResult {
-      match getRangeText (codeActionParams.TextDocument.GetFilePath() |> normalizePath) diagnostic.Range with
-      | Ok unusedExpression ->
-        match diagnostic.Code with
-        | Some _ ->
-          return
-            [ { SourceDiagnostic = Some diagnostic
-                File = codeActionParams.TextDocument
-                Title = titleReplace
-                Edits =
-                  [| { Range = diagnostic.Range
-                       NewText = "_" } |]
-                Kind = FixKind.Refactor } ]
-        | None ->
-          let replaceSuggestion = "_"
-          let prefixSuggestion = $"_%s{unusedExpression}"
-
-          return
-            [ { SourceDiagnostic = Some diagnostic
-                File = codeActionParams.TextDocument
-                Title = titleReplace
-                Edits =
-                  [| { Range = diagnostic.Range
-                       NewText = replaceSuggestion } |]
-                Kind = FixKind.Refactor }
-              { SourceDiagnostic = Some diagnostic
-                File = codeActionParams.TextDocument
-                Title = titlePrefix
-                Edits =
-                  [| { Range = diagnostic.Range
-                       NewText = prefixSuggestion } |]
-                Kind = FixKind.Refactor } ]
-      | Error _ -> return []
+      let fileName = codeActionParams.TextDocument.GetFilePath() |> Utils.normalizePath
+      let startPos = protocolPosToPos codeActionParams.Range.Start
+      let! (tyRes, line, lines) = getParseResultsForFile fileName startPos
+
+      let mkFix title range =
+        { SourceDiagnostic = Some diagnostic
+          File = codeActionParams.TextDocument
+          Title = title
+          Edits = [| { Range = range; NewText = "_" } |]
+          Kind = FixKind.Refactor }
+
+      let mkReplaceFix = mkFix titleReplace
+
+      let tryMkPrefixFix range =
+        match lines.GetText(protocolRangeToRange (UMX.untag fileName) range) with
+        //              cannot prefix backticks -> exclude
+        | Ok ident when PrettyNaming.IsIdentifierName ident ->
+          mkFix titlePrefix (protocolPosToRange range.Start) |> Some
+        | _ -> None
+
+      let tryMkValueReplaceFix (range: Ionide.LanguageServerProtocol.Types.Range) =
+        // // `let private foo = ...` -> `private` must be removed (`let private _ = ...` is not valid)
+        if variableHasAccessibility tyRes.GetAST (protocolPosToPos range.Start) then
+          None
+        else
+          mkReplaceFix range |> Some
+
+      // CodeFixes:
+      // * Replace with _
+      //  * variable
+      //  * this
+      // * Prefix with _
+      //  * variable
+      // * Otherwise: neither
+      match tyRes.TryGetSymbolUse startPos line with
+      | None -> return []
+      | Some symbolUse ->
+        match symbolUse.Symbol with
+        | :? FSharpMemberOrFunctionOrValue as mfv ->
+          if mfv.IsMemberThisValue then
+            return [ mkReplaceFix diagnostic.Range ]
+          elif mfv.IsValue then
+            return
+              [ tryMkValueReplaceFix diagnostic.Range; tryMkPrefixFix diagnostic.Range ]
+              |> List.choose id
+          else
+            return []
+        | _ -> return []
     })

src/FsAutoComplete/FsAutoComplete.Lsp.fs

@@ -267,6 +267,7 @@ type FSharpLspServer(state: State, lspClient: FSharpLspClient) =
       let doc = p.TextDocument
       let filePath = doc.GetFilePath() |> Utils.normalizePath
       let version = doc.Version.Value // this always has a value, despite the Option type
+
       match state.TryGetFileSource(filePath) with
       | Ok content ->
 
@@ -360,9 +361,9 @@ type FSharpLspServer(state: State, lspClient: FSharpLspClient) =
 
         let diags =
           decls
-          |> Array.map (fun (n, t) ->
+          |> Array.map (fun n ->
             { Range = fcsRangeToLsp n
-              Code = (if t then Some "FSAC0003" else None)
+              Code = Some "FSAC0003"
               Severity = Some DiagnosticSeverity.Hint
               Source = "FSAC"
               Message = "This value is unused"
@@ -801,7 +802,7 @@ type FSharpLspServer(state: State, lspClient: FSharpLspClient) =
              (ResolveNamespace.fix tryGetParseResultsForFile commands.GetNamespaceSuggestions)
            ReplaceWithSuggestion.fix
            RemoveRedundantQualifier.fix
-           Run.ifEnabled (fun _ -> config.UnusedDeclarationsAnalyzer) (RenameUnusedValue.fix getRangeText)
+           Run.ifEnabled (fun _ -> config.UnusedDeclarationsAnalyzer) (RenameUnusedValue.fix tryGetParseResultsForFile)
            AddNewKeywordToDisposableConstructorInvocation.fix getRangeText
            Run.ifEnabled
              (fun _ -> config.UnionCaseStubGeneration)

test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs

@@ -1263,11 +1263,106 @@ let private renameUnusedValue state =
         """
         let add one two $0three = one + two
         """
-        (Diagnostics.log >> Diagnostics.acceptAll)
-        (CodeFix.log >> selectPrefix)
+        (Diagnostics.acceptAll)
+        selectPrefix
         """
         let add one two _three = one + two
         """
+
+    testCaseAsync "doesn't replace function with _" <|
+      CodeFix.checkNotApplicable server
+        """
+        let $0f _ = ()
+        """
+        (Diagnostics.acceptAll)
+        selectReplace
+    testCaseAsync "doesn't prefix function with _" <|
+      CodeFix.checkNotApplicable server
+        """
+        let $0f _ = ()
+        """
+        (Diagnostics.acceptAll)
+        selectPrefix
+
+    ptestCaseAsync "replacing private variable with _ replaces private too" <|
+      CodeFix.check server
+        """
+        let private $0value = 42
+        """
+        (Diagnostics.acceptAll)
+        selectReplace
+        """
+        let _ = 42
+        """
+    //TODO: remove this test and enable prev test once FCS includes range for `SynAccess`
+    testCaseAsync "private variable cannot be replaces with _" <|
+      CodeFix.checkNotApplicable server
+        """
+        let private $0value = 42
+        """
+        (Diagnostics.acceptAll)
+        selectReplace
+
+    testCaseAsync "prefixing private variable with _ keeps private" <|
+      CodeFix.check server
+        """
+        let private $0value = 42
+        """
+        (Diagnostics.acceptAll)
+        selectPrefix
+        """
+        let private _value = 42
+        """
+
+    testCaseAsync "can replace backticks with _" <|
+      CodeFix.check server
+        """
+        let $0``hello world`` = 42
+        """
+        (Diagnostics.acceptAll)
+        selectReplace
+        """
+        let _ = 42
+        """
+    testCaseAsync "cannot prefix backticks with _" <|
+      CodeFix.checkNotApplicable server
+        """
+        let $0``hello world`` = 42
+        """
+        (Diagnostics.acceptAll)
+        selectPrefix
+
+    testCaseAsync "replace doesn't trigger for function" <|
+      CodeFix.checkNotApplicable server
+        """
+        let $0f _ = ()
+        """
+        (Diagnostics.acceptAll)
+        selectReplace
+    testCaseAsync "prefix doesn't trigger for function" <|
+      CodeFix.checkNotApplicable server
+        """
+        let $0f _ = ()
+        """
+        (Diagnostics.acceptAll)
+        selectPrefix
+
+    testCaseAsync "replace doesn't trigger for member" <|
+      CodeFix.checkNotApplicable server
+        """
+        type T() =
+          member _.$0F () = ()
+        """
+        (Diagnostics.acceptAll)
+        selectReplace
+    testCaseAsync "prefix doesn't trigger for member" <|
+      CodeFix.checkNotApplicable server
+        """
+        type T() =
+          member _.$0F () = ()
+        """
+        (Diagnostics.acceptAll)
+        selectPrefix
   ])
 
 let private replaceWithSuggestionTests state =

test/FsAutoComplete.Tests.Lsp/Program.fs

@@ -86,6 +86,7 @@ let lspTests =
             XmlDocumentationGeneration.tests state
             InlayHintTests.tests state
             DependentFileChecking.tests state
+            UnusedDeclarationsTests.tests state
           ]
     ]
 

test/FsAutoComplete.Tests.Lsp/TestCases/UnusedDeclarations/Library.fs

@@ -0,0 +1 @@
+

test/FsAutoComplete.Tests.Lsp/TestCases/UnusedDeclarations/UnusedDeclarations.fsproj

@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFramework>net6.0</TargetFramework>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <Compile Include="Library.fs" />
+  </ItemGroup>
+
+</Project>