Fix unused opens analyzer on rec modules and optimize it (#5005)

* filter out duplicates in GetOpenDeclarations

* Add rec module to unused opens test

* add unused opens tests for rec modules

* fix unused opens for rec modules

* filter out symbol uses that lays above the open statement

* filter out union cases definitions

* fix ItemsAreEffectivelyEqualHash for Item.ModuleOrNamespaces

* use Dictionary to lookup symbol uses by declaring entity

* formatting

* cleanup

* use Dictionary to filter out already processed modules

* add BagAdd and BagExistsValueForKey Dictionary extensions

src/absil/illib.fs

@@ -5,14 +5,12 @@ module public Microsoft.FSharp.Compiler.AbstractIL.Internal.Library
 
 
 open System
-open System.Collections
 open System.Collections.Generic
 open System.Diagnostics
 open System.IO
 open System.Reflection
-open System.Text
 open System.Threading
-open Internal.Utilities
+open System.Runtime.CompilerServices
 
 #if FX_RESHAPED_REFLECTION
 open Microsoft.FSharp.Core.ReflectionAdapters
@@ -561,10 +559,23 @@ module String =
             // http://stackoverflow.com/questions/19365404/stringreader-omits-trailing-linebreak
             yield String.Empty
         |]
-module Dictionary = 
 
+module Dictionary = 
     let inline newWithSize (size: int) = Dictionary<_,_>(size, HashIdentity.Structural)
-        
+
+[<Extension>]
+type DictionaryExtensions() =
+    [<Extension>]
+    static member inline BagAdd(dic: Dictionary<'key, 'value list>, key: 'key, value: 'value) =
+        match dic.TryGetValue key with
+        | true, values -> dic.[key] <- value :: values
+        | _ -> dic.[key] <- [value]
+
+    [<Extension>]
+    static member inline BagExistsValueForKey(dic: Dictionary<'key, 'value list>, key: 'key, f: 'value -> bool) =
+        match dic.TryGetValue key with
+        | true, values -> values |> List.exists f
+        | _ -> false
 
 module Lazy = 
     let force (x: Lazy<'T>) = x.Force()

src/fsharp/NameResolution.fs

@@ -1437,7 +1437,7 @@ let ItemsAreEffectivelyEqualHash (g: TcGlobals) orig =
     | UnionCaseUse ucase ->  hash ucase.CaseName
     | RecordFieldUse (name, _) -> hash name
     | EventUse einfo -> einfo.ComputeHashCode()
-    | Item.ModuleOrNamespaces _ -> 100013
+    | Item.ModuleOrNamespaces (mref :: _) -> hash mref.DefinitionRange
     | _ -> 389329
 
 [<System.Diagnostics.DebuggerDisplay("{DebugToString()}")>]
@@ -1518,7 +1518,7 @@ type TcResultsSinkImpl(g, ?source: string) =
                     member __.Equals ((m1, item1), (m2, item2)) = m1 = m2 && ItemsAreEffectivelyEqual g item1 item2 } )
 
     let capturedMethodGroupResolutions = ResizeArray<_>()
-    let capturedOpenDeclarations = ResizeArray<_>()
+    let capturedOpenDeclarations = ResizeArray<OpenDeclaration>()
     let allowedRange (m:range) = not m.IsSynthetic       
 
     member this.GetResolutions() = 
@@ -1527,7 +1527,8 @@ type TcResultsSinkImpl(g, ?source: string) =
     member this.GetSymbolUses() = 
         TcSymbolUses(g, capturedNameResolutions, capturedFormatSpecifierLocations.ToArray())
 
-    member this.GetOpenDeclarations() = capturedOpenDeclarations.ToArray()
+    member this.GetOpenDeclarations() = 
+        capturedOpenDeclarations |> Seq.distinctBy (fun x -> x.Range, x.AppliedScope, x.IsOwnNamespace) |> Seq.toArray
 
     interface ITypecheckResultsSink with
         member sink.NotifyEnvWithScope(m,nenv,ad) = 

src/fsharp/service/ServiceAnalysis.fs

@@ -8,6 +8,7 @@ open Microsoft.FSharp.Compiler.Range
 open Microsoft.FSharp.Compiler.PrettyNaming
 open System.Collections.Generic
 open System.Runtime.CompilerServices
+open Microsoft.FSharp.Compiler.AbstractIL.Internal.Library
 
 module UnusedOpens =
 
@@ -101,10 +102,15 @@ module UnusedOpens =
              | :? FSharpMemberOrFunctionOrValue as fv when not fv.IsModuleValueOrMember -> 
                 // Local values can be ignored
                 false
+             | :? FSharpMemberOrFunctionOrValue when su.IsFromDefinition -> 
+                // Value definitions should be ignored
+                false
              | :? FSharpGenericParameter -> 
                 // Generic parameters can be ignored, they never come into scope via 'open'
                 false
-             | _ -> 
+             | :? FSharpUnionCase when su.IsFromDefinition -> 
+                false
+             | _ ->
                 // For the rest of symbols we pick only those which are the first part of a long ident, because it's they which are
                 // contained in opened namespaces / modules. For example, we pick `IO` from long ident `IO.File.OpenWrite` because
                 // it's `open System` which really brings it into scope.
@@ -122,16 +128,12 @@ module UnusedOpens =
                 | _ -> false
             | _ -> false)
 
-    /// Represents intermediate tracking data used to track the modules which are known to have been used so far
-    type UsedModule =
-        { Module: FSharpEntity
-          AppliedScope: range }
-
     /// Given an 'open' statement, find fresh modules/namespaces referred to by that statement where there is some use of a revealed symbol
     /// in the scope of the 'open' is from that module.
     ///
     /// Performance will be roughly NumberOfOpenStatements x NumberOfSymbolUses
-    let getUsedModules (symbolUses1: FSharpSymbolUse[], symbolUses2: FSharpSymbolUse[]) (usedModules: UsedModule list) (openStatement: OpenStatement) =
+    let isOpenStatementUsed (symbolUses2: FSharpSymbolUse[]) (symbolUsesRangesByDeclaringEntity: Dictionary<FSharpEntity, range list>) 
+                            (usedModules: Dictionary<FSharpEntity, range list>) (openStatement: OpenStatement) =
 
         // Don't re-check modules whose symbols are already known to have been used
         let openedGroupsToExamine =
@@ -139,10 +141,7 @@ module UnusedOpens =
                 let openedEntitiesToExamine =
                     openedGroup.OpenedModules 
                     |> List.filter (fun openedEntity ->
-                        not (usedModules
-                            |> List.exists (fun used ->
-                                rangeContainsRange used.AppliedScope openStatement.AppliedScope &&
-                                used.Module.IsEffectivelySameAs openedEntity.Entity)))
+                        not (usedModules.BagExistsValueForKey(openedEntity.Entity, fun scope -> rangeContainsRange scope openStatement.AppliedScope)))
                              
                 match openedEntitiesToExamine with
                 | [] -> None
@@ -153,45 +152,59 @@ module UnusedOpens =
         let newlyUsedOpenedGroups = 
             openedGroupsToExamine |> List.filter (fun openedGroup ->
                 openedGroup.OpenedModules |> List.exists (fun openedEntity ->
-
-                    symbolUses1 |> Array.exists (fun symbolUse ->
-                        rangeContainsRange openStatement.AppliedScope symbolUse.RangeAlternate &&
-                        match symbolUse.Symbol with
-                        | :? FSharpMemberOrFunctionOrValue as f ->
-                            match f.DeclaringEntity with
-                            | Some ent when ent.IsNamespace || ent.IsFSharpModule -> ent.IsEffectivelySameAs openedEntity.Entity
-                            | _ -> false
-                        | _ -> false) || 
-
+                    symbolUsesRangesByDeclaringEntity.BagExistsValueForKey(openedEntity.Entity, fun symbolUseRange ->
+                        rangeContainsRange openStatement.AppliedScope symbolUseRange &&
+                        Range.posGt symbolUseRange.Start openStatement.Range.End) || 
+                    
                     symbolUses2 |> Array.exists (fun symbolUse ->
                         rangeContainsRange openStatement.AppliedScope symbolUse.RangeAlternate &&
+                        Range.posGt symbolUse.RangeAlternate.Start openStatement.Range.End &&
                         openedEntity.RevealedSymbolsContains symbolUse.Symbol)))
 
         // Return them as interim used entities
-        newlyUsedOpenedGroups |> List.collect (fun openedGroup -> 
-                openedGroup.OpenedModules |> List.map (fun x -> { Module = x.Entity; AppliedScope = openStatement.AppliedScope }))
+        let newlyOpenedModules = newlyUsedOpenedGroups |> List.collect (fun openedGroup -> openedGroup.OpenedModules)
+        for openedModule in newlyOpenedModules do
+            let scopes =
+                match usedModules.TryGetValue openedModule.Entity with
+                | true, scopes -> openStatement.AppliedScope :: scopes
+                | _ -> [openStatement.AppliedScope]
+            usedModules.[openedModule.Entity] <- scopes
+        newlyOpenedModules.Length > 0
                                           
     /// Incrementally filter out the open statements one by one. Filter those whose contents are referred to somewhere in the symbol uses.
     /// Async to allow cancellation.
-    let rec filterOpenStatementsIncremental symbolUses (openStatements: OpenStatement list) (usedModules: UsedModule list) acc = 
+    let rec filterOpenStatementsIncremental symbolUses2 (symbolUsesRangesByDeclaringEntity: Dictionary<FSharpEntity, range list>) (openStatements: OpenStatement list)
+                                            (usedModules: Dictionary<FSharpEntity, range list>) acc = 
         async { 
             match openStatements with
             | openStatement :: rest ->
-                match getUsedModules symbolUses usedModules openStatement with
-                | [] -> 
+                if isOpenStatementUsed symbolUses2 symbolUsesRangesByDeclaringEntity usedModules openStatement then
+                    return! filterOpenStatementsIncremental symbolUses2 symbolUsesRangesByDeclaringEntity rest usedModules acc
+                else
                     // The open statement has not been used, include it in the results
-                    return! filterOpenStatementsIncremental symbolUses rest usedModules (openStatement :: acc)
-                | moreUsedModules -> 
-                    // The open statement has been used, add the modules which are already known to be used to the list of things we don't need to re-check
-                    return! filterOpenStatementsIncremental symbolUses rest (moreUsedModules @ usedModules) acc
+                    return! filterOpenStatementsIncremental symbolUses2 symbolUsesRangesByDeclaringEntity rest usedModules (openStatement :: acc)
             | [] -> return List.rev acc
         }
 
+    let entityHash = HashIdentity.FromFunctions (fun (x: FSharpEntity) -> x.GetEffectivelySameAsHash()) (fun x y -> x.IsEffectivelySameAs(y))
+
     /// Filter out the open statements whose contents are referred to somewhere in the symbol uses.
     /// Async to allow cancellation.
-    let filterOpenStatements symbolUses openStatements =
+    let filterOpenStatements (symbolUses1: FSharpSymbolUse[], symbolUses2: FSharpSymbolUse[]) openStatements =
         async {
-            let! results = filterOpenStatementsIncremental symbolUses (List.ofArray openStatements) [] []
+            // the key is a namespace or module, the value is a list of FSharpSymbolUse range of symbols defined in the 
+            // namespace or module. So, it's just symbol uses ranges groupped by namespace or module where they are _defined_. 
+            let symbolUsesRangesByDeclaringEntity = Dictionary<FSharpEntity, range list>(entityHash)
+            for symbolUse in symbolUses1 do
+                match symbolUse.Symbol with
+                | :? FSharpMemberOrFunctionOrValue as f ->
+                    match f.DeclaringEntity with
+                    | Some entity when entity.IsNamespace || entity.IsFSharpModule -> 
+                        symbolUsesRangesByDeclaringEntity.BagAdd(entity, symbolUse.RangeAlternate)
+                    | _ -> ()
+                | _ -> ()
+
+            let! results = filterOpenStatementsIncremental symbolUses2 symbolUsesRangesByDeclaringEntity (List.ofArray openStatements) (Dictionary(entityHash)) []
             return results |> List.map (fun os -> os.Range)
         }
 

tests/service/ProjectAnalysisTests.fs

@@ -5281,13 +5281,15 @@ let ``#4030, Incremental builder creation warnings`` (args, errorSeverities) =
 //------------------------------------------------------
 
 [<Test>]
-let ``Unused opens smoke test 1``() =
+let ``Unused opens in rec module smoke test 1``() =
 
     let fileName1 = Path.ChangeExtension(Path.GetTempFileName(), ".fs")
     let base2 = Path.GetTempFileName()
     let dllName = Path.ChangeExtension(base2, ".dll")
     let projFileName = Path.ChangeExtension(base2, ".fsproj")
     let fileSource1 = """
+module rec Module
+
 open System.Collections // unused
 open System.Collections.Generic // used, should not appear
 open FSharp.Control // unused
@@ -5343,11 +5345,83 @@ type UseTheThings(i:int) =
     let unusedOpens = UnusedOpens.getUnusedOpens (fileCheckResults, (fun i -> lines.[i-1])) |> Async.RunSynchronously
     let unusedOpensData = [ for uo in unusedOpens -> tups uo, lines.[uo.StartLine-1] ]
     let expected = 
-          [(((2, 5), (2, 23)), "open System.Collections // unused");
-           (((4, 5), (4, 19)), "open FSharp.Control // unused");
-           (((5, 5), (5, 16)), "open FSharp.Data // unused");
-           (((6, 5), (6, 25)), "open System.Globalization // unused");
-           (((23, 5), (23, 21)), "open SomeUnusedModule")]
+          [(((4, 5), (4, 23)), "open System.Collections // unused");
+           (((6, 5), (6, 19)), "open FSharp.Control // unused");
+           (((7, 5), (7, 16)), "open FSharp.Data // unused");
+           (((8, 5), (8, 25)), "open System.Globalization // unused");
+           (((25, 5), (25, 21)), "open SomeUnusedModule")]
+    unusedOpensData |> shouldEqual expected
+
+[<Test>]
+let ``Unused opens in non rec module smoke test 1``() =
+
+    let fileName1 = Path.ChangeExtension(Path.GetTempFileName(), ".fs")
+    let base2 = Path.GetTempFileName()
+    let dllName = Path.ChangeExtension(base2, ".dll")
+    let projFileName = Path.ChangeExtension(base2, ".fsproj")
+    let fileSource1 = """
+module Module
+
+open System.Collections // unused
+open System.Collections.Generic // used, should not appear
+open FSharp.Control // unused
+open FSharp.Data // unused
+open System.Globalization // unused
+
+module SomeUnusedModule = 
+    let f x  = x
+
+module SomeUsedModuleContainingFunction = 
+    let g x  = x
+
+module SomeUsedModuleContainingActivePattern = 
+    let (|ActivePattern|) x  = x
+
+module SomeUsedModuleContainingExtensionMember = 
+    type System.Int32 with member x.Q = 1
+
+module SomeUsedModuleContainingUnion = 
+    type Q = A | B
+
+open SomeUnusedModule
+open SomeUsedModuleContainingFunction
+open SomeUsedModuleContainingExtensionMember
+open SomeUsedModuleContainingActivePattern
+open SomeUsedModuleContainingUnion
+
+type UseTheThings(i:int) =
+    member x.Value = Dictionary<int,int>() // use something from System.Collections.Generic, as a constructor
+    member x.UseSomeUsedModuleContainingFunction() = g 3
+    member x.UseSomeUsedModuleContainingActivePattern(ActivePattern g) = g
+    member x.UseSomeUsedModuleContainingExtensionMember() = (3).Q
+    member x.UseSomeUsedModuleContainingUnion() = A
+"""
+    File.WriteAllText(fileName1, fileSource1)
+
+    let fileNames = [fileName1]
+    let args = mkProjectCommandLineArgs (dllName, fileNames)
+    let keepAssemblyContentsChecker = FSharpChecker.Create(keepAssemblyContents=true)
+    let options =  keepAssemblyContentsChecker.GetProjectOptionsFromCommandLineArgs (projFileName, args)
+    
+    let fileCheckResults = 
+        keepAssemblyContentsChecker.ParseAndCheckFileInProject(fileName1, 0, fileSource1, options)  |> Async.RunSynchronously
+        |> function 
+            | _, FSharpCheckFileAnswer.Succeeded(res) -> res
+            | _ -> failwithf "Parsing aborted unexpectedly..."
+    //let symbolUses = fileCheckResults.GetAllUsesOfAllSymbolsInFile() |> Async.RunSynchronously |> Array.indexed 
+    // Fragments used to check hash codes:
+    //(snd symbolUses.[42]).Symbol.IsEffectivelySameAs((snd symbolUses.[37]).Symbol)
+    //(snd symbolUses.[42]).Symbol.GetEffectivelySameAsHash()
+    //(snd symbolUses.[37]).Symbol.GetEffectivelySameAsHash()
+    let lines = File.ReadAllLines(fileName1)
+    let unusedOpens = UnusedOpens.getUnusedOpens (fileCheckResults, (fun i -> lines.[i-1])) |> Async.RunSynchronously
+    let unusedOpensData = [ for uo in unusedOpens -> tups uo, lines.[uo.StartLine-1] ]
+    let expected = 
+          [(((4, 5), (4, 23)), "open System.Collections // unused");
+           (((6, 5), (6, 19)), "open FSharp.Control // unused");
+           (((7, 5), (7, 16)), "open FSharp.Data // unused");
+           (((8, 5), (8, 25)), "open System.Globalization // unused");
+           (((25, 5), (25, 21)), "open SomeUnusedModule")]
     unusedOpensData |> shouldEqual expected
 
 [<Test>]

vsintegration/tests/UnitTests/UnusedOpensTests.fs

@@ -753,3 +753,56 @@ module M2 =
     let _ = T()
 """
     => []
+
+[<Test>]
+let ``unused open declaration in top level rec module``() =
+    """
+module rec TopModule
+open System
+open System.IO
+let _ = DateTime.Now
+"""
+    => [ 4, (5, 14) ]
+
+[<Test>]
+let ``unused open declaration in rec namespace``() =
+    """
+namespace rec TopNamespace
+open System
+open System.IO
+module Nested =
+    let _ = DateTime.Now
+"""
+    => [ 4, (5, 14) ]
+
+[<Test>]
+let ``unused inner module open declaration in rec module``() =
+    """
+module rec TopModule
+
+module Nested =
+    let x = 1
+    let f x = x
+    type T() = class end
+    type R = { F: int }
+
+open Nested
+"""
+    => [ 10, (5, 11) ]
+
+[<Test>]
+let ``used inner module open declaration in rec module``() =
+    """
+module rec TopModule
+
+module Nested =
+    let x = 1
+    let f x = x
+    type T() = class end
+    type R = { F: int }
+
+open Nested
+
+let _ = f 1
+"""
+    => []
\ No newline at end of file