Fix UnusedDeclarationsAnalyzer (#2901)

* fixed: UnusedDeclarationsAnalyzer returns false positives for symbols sharing same range (implicit ctor and class itself, for example)

* fix UnusedDeclarationsAnalyzer for active patterns

* fix test

src/fsharp/NameResolution.fs

@@ -3180,7 +3180,15 @@ let ResolveLongIdentAsExprAndComputeRange (sink:TcResultsSink) (ncenv:NameResolv
 
     let callSink refinedItem =
         if not isFakeIdents then
-            CallNameResolutionSink sink (itemRange, nenv, refinedItem, item, ItemOccurence.Use, nenv.DisplayEnv, ad)
+            let occurence = 
+                match item with
+                // It's r.h.s. `Case1` in `let (|Case1|Case1|) _ = if true then Case1 else Case2`
+                // We return `Binding` for it because it's actually not usage, but definition. If we did not
+                // it confuses detecting unused definitions.
+                | Item.ActivePatternResult _ -> ItemOccurence.Binding 
+                | _ -> ItemOccurence.Use
+
+            CallNameResolutionSink sink (itemRange, nenv, refinedItem, item, occurence, nenv.DisplayEnv, ad)
     let afterOverloadResolution =
         match sink.CurrentSink with
         |   None -> AfterOverloadResolution.DoNothing

tests/service/ProjectAnalysisTests.fs

@@ -1471,8 +1471,8 @@ let ``Test project 5 all symbols`` () =
             ("val op_Modulus", "Microsoft.FSharp.Core.Operators.( % )", "file1",
              ((4, 34), (4, 35)), []);
             ("val input", "input", "file1", ((4, 28), (4, 33)), []);
-            ("symbol ", "Even", "file1", ((4, 47), (4, 51)), []);
-            ("symbol ", "Odd", "file1", ((4, 57), (4, 60)), []);
+            ("symbol ", "Even", "file1", ((4, 47), (4, 51)), ["defn"]);
+            ("symbol ", "Odd", "file1", ((4, 57), (4, 60)), ["defn"]);
             ("val |Even|Odd|", "ActivePatterns.( |Even|Odd| )", "file1",
              ((4, 5), (4, 15)), ["defn"]);
             ("val input", "input", "file1", ((7, 15), (7, 20)), ["defn"]);

vsintegration/src/FSharp.Editor/Completion/SignatureHelp.fs

@@ -35,7 +35,7 @@ type internal FSharpSignatureHelpProvider
     static let oneColAfter (lp: LinePosition) = LinePosition(lp.Line,lp.Character+1)
     static let oneColBefore (lp: LinePosition) = LinePosition(lp.Line,max 0 (lp.Character-1))
 
-    // Unit-testable core rutine
+    // Unit-testable core routine
     static member internal ProvideMethodsAsyncAux(checker: FSharpChecker, documentationBuilder: IDocumentationBuilder, sourceText: SourceText, caretPosition: int, options: FSharpProjectOptions, triggerIsTypedChar: char option, filePath: string, textVersionHash: int) = async {
         let! parseResults, checkFileAnswer = checker.ParseAndCheckFileInProject(filePath, textVersionHash, sourceText.ToString(), options)
         match checkFileAnswer with
@@ -130,7 +130,7 @@ type internal FSharpSignatureHelpProvider
                 
             match (computedTextSpans|> Array.tryFindIndex (fun t -> t.Contains(caretPosition))) with 
             | None -> 
-                // Because 'TextSpan.Contains' only succeeeds if 'TextSpan.Start <= caretPosition < TextSpan.End' is true,
+                // Because 'TextSpan.Contains' only succeeds if 'TextSpan.Start <= caretPosition < TextSpan.End' is true,
                 // we need to check if the caret is at the very last position in the TextSpan.
                 //
                 // We default to 0, which is the first argument, if the caret position was nowhere to be found.
@@ -225,7 +225,6 @@ type internal FSharpSignatureHelpProvider
 
 open System.ComponentModel.Composition
 open Microsoft.VisualStudio.Utilities
-open Microsoft.VisualStudio.Text
 open Microsoft.VisualStudio.Text.Classification
 open Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.SignatureHelp.Presentation
 

vsintegration/src/FSharp.Editor/Diagnostics/DocumentDiagnosticAnalyzer.fs

@@ -3,7 +3,6 @@
 namespace Microsoft.VisualStudio.FSharp.Editor
 
 open System
-open System.Composition
 open System.Collections.Immutable
 open System.Collections.Generic
 open System.Threading
@@ -11,15 +10,11 @@ open System.Threading.Tasks
 
 open Microsoft.CodeAnalysis
 open Microsoft.CodeAnalysis.Diagnostics
-open Microsoft.CodeAnalysis.Host.Mef
 open Microsoft.CodeAnalysis.Text
-open Microsoft.CodeAnalysis.SolutionCrawler
 
 open Microsoft.FSharp.Compiler
 open Microsoft.FSharp.Compiler.SourceCodeServices
-open Microsoft.FSharp.Compiler.Range
 
-open Microsoft.VisualStudio.FSharp.LanguageService
 
 [<RequireQualifiedAccess>]
 type internal DiagnosticsType =

vsintegration/src/FSharp.Editor/Diagnostics/UnusedDeclarationsAnalyzer.fs

@@ -31,8 +31,15 @@ type internal UnusedDeclarationsAnalyzer() =
     
     let symbolUseComparer =
         { new IEqualityComparer<FSharpSymbolUse> with
-            member __.Equals (x, y) = x.Symbol.IsEffectivelySameAs y.Symbol
-            member __.GetHashCode x = x.Symbol.GetHashCode() }
+            member __.Equals (x, y) = 
+                if x.IsFromDefinition && y.IsFromDefinition then
+                    x.RangeAlternate = y.RangeAlternate
+                else
+                    x.Symbol.DeclarationLocation = y.Symbol.DeclarationLocation || x.Symbol.IsEffectivelySameAs y.Symbol
+            member __.GetHashCode x = 
+                x.Symbol.DeclarationLocation 
+                |> Option.orElseWith (fun _ -> x.Symbol.ImplementationLocation) 
+                |> hash }
 
     let countSymbolsUses (symbolsUses: FSharpSymbolUse[]) =
         let result = Dictionary<FSharpSymbolUse, int>(symbolUseComparer)
@@ -44,23 +51,39 @@ type internal UnusedDeclarationsAnalyzer() =
         result
 
     let getSingleDeclarations (symbolsUses: FSharpSymbolUse[]) (isScript: bool) =
+        let symbolUsesCounts = countSymbolsUses symbolsUses
+        
         let declarations =
-            countSymbolsUses symbolsUses
-            |> Seq.choose (fun (KeyValue(symbolUse, count)) ->
-                match symbolUse.Symbol with
-                // Determining that a record, DU or module is used anywhere requires inspecting all their enclosed entities (fields, cases and func / vals)
-                // for usages, which is too expensive to do. Hence we never gray them out.
-                | :? FSharpEntity as e when e.IsFSharpRecord || e.IsFSharpUnion || e.IsInterface || e.IsFSharpModule || e.IsClass -> None
-                // FCS returns inconsistent results for override members; we're skipping these symbols.
-                | :? FSharpMemberOrFunctionOrValue as f when 
-                        f.IsOverrideOrExplicitInterfaceImplementation ||
-                        f.IsConstructorThisValue ||
-                        f.IsBaseValue ||
-                        f.IsConstructor -> None
-                // Usage of DU case parameters does not give any meaningful feedback; we never gray them out.
-                | :? FSharpParameter when symbolUse.IsFromDefinition -> None
-                | _ when count = 1 && symbolUse.IsFromDefinition && (isScript || symbolUse.IsPrivateToFile) -> Some symbolUse
-                | _ -> None)
+            symbolUsesCounts
+            |> Seq.map (fun (KeyValue(su, count)) -> su, count)
+            |> Seq.groupBy (fun (su, _) -> su.RangeAlternate)
+            |> Seq.collect (fun (_, symbolUsesWithSameRange) ->
+                let singleDeclarationSymbolUses =
+                    symbolUsesWithSameRange
+                    |> Seq.choose(fun (symbolUse, count) ->
+                        match symbolUse.Symbol with
+                        // Determining that a record, DU or module is used anywhere requires inspecting all their enclosed entities (fields, cases and func / vals)
+                        // for usages, which is too expensive to do. Hence we never gray them out.
+                        | :? FSharpEntity as e when e.IsFSharpRecord || e.IsFSharpUnion || e.IsInterface || e.IsFSharpModule || e.IsClass -> None
+                        // FCS returns inconsistent results for override members; we're skipping these symbols.
+                        | :? FSharpMemberOrFunctionOrValue as f when 
+                                f.IsOverrideOrExplicitInterfaceImplementation ||
+                                //f.IsConstructorThisValue ||
+                                f.IsBaseValue ||
+                                f.IsConstructor -> None
+                        // Usage of DU case parameters does not give any meaningful feedback; we never gray them out.
+                        | :? FSharpParameter when symbolUse.IsFromDefinition -> None
+                        | _ when count = 1 && symbolUse.IsFromDefinition && (isScript || symbolUse.IsPrivateToFile) -> Some symbolUse
+                        | _ -> None)
+                    |> Seq.toList
+
+                // Only if *all* `SymbolUse`s are single declarations, return them. If there is at least one that should not be marked as unused, return nothing.
+                if Seq.length symbolUsesWithSameRange = List.length singleDeclarationSymbolUses then
+                    singleDeclarationSymbolUses
+                else []
+               )
+            |> Seq.toList
+
         HashSet(declarations, symbolUseComparer)
 
     override __.SupportedDiagnostics = ImmutableArray.Create Descriptor
@@ -76,7 +99,7 @@ type internal UnusedDeclarationsAnalyzer() =
                 let! _, _, checkResults = checker.ParseAndCheckDocument(document, options, sourceText = sourceText, allowStaleResults = true)
                 let! allSymbolUsesInFile = checkResults.GetAllUsesOfAllSymbolsInFile() |> liftAsync
                 let unusedDeclarations = getSingleDeclarations allSymbolUsesInFile (isScriptFile document.FilePath)
-                return 
+                return
                     unusedDeclarations
                     |> Seq.filter (fun symbolUse -> not (symbolUse.Symbol.DisplayName.StartsWith "_"))
                     |> Seq.map (fun symbolUse -> Diagnostic.Create(Descriptor, RoslynHelpers.RangeToLocation(symbolUse.RangeAlternate, sourceText, document.FilePath)))