Removed reference counting from IncrementalBuilder; now relies on GC. (#6863)

* TcImports doesn't explicitly have to be disposed, relies on GC. Removed reference counting in incremental builder. Builders can only be invalidated by a type provider, not anything else.

* Removing any notion of being incremental builder reference counting and being alive

* Added ITypeProviderThread

* Fixed declaration list item

* Changed EnqueueWorkAndWait to just EnqueueWork that's not blocking

* Minor cleanup

* Forgot to check disposed

* Quick update on comment

* Update FCS project

* Renamed ITypeProviderThread to ICompilationThread

* Changes from feedback and added a test

Author: TIHan

fcs/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj

@@ -2,6 +2,7 @@
   <PropertyGroup>
     <FSharpSourcesRoot>$(MSBuildProjectDirectory)\..\..\src</FSharpSourcesRoot>
   </PropertyGroup>
+<Project Sdk="Microsoft.NET.Sdk">
   <Import Project="..\netfx.props" />
   <Import Project="..\..\src\buildtools\buildtools.targets" />
   <PropertyGroup>
@@ -546,18 +547,18 @@
     <Compile Include="$(FSharpSourcesRoot)\fsharp\symbols\SymbolPatterns.fs">
       <Link>Symbols/SymbolPatterns.fs</Link>
     </Compile>
-    <Compile Include="$(FSharpSourcesRoot)\fsharp\service\IncrementalBuild.fsi">
-      <Link>Service/IncrementalBuild.fsi</Link>
-    </Compile>
-    <Compile Include="$(FSharpSourcesRoot)\fsharp\service\IncrementalBuild.fs">
-      <Link>Service/IncrementalBuild.fs</Link>
-    </Compile>
     <Compile Include="$(FSharpSourcesRoot)\fsharp\service\Reactor.fsi">
       <Link>Service/Reactor.fsi</Link>
     </Compile>
     <Compile Include="$(FSharpSourcesRoot)\fsharp\service\Reactor.fs">
       <Link>Service/Reactor.fs</Link>
     </Compile>
+    <Compile Include="$(FSharpSourcesRoot)\fsharp\service\IncrementalBuild.fsi">
+      <Link>Service/IncrementalBuild.fsi</Link>
+    </Compile>
+    <Compile Include="$(FSharpSourcesRoot)\fsharp\service\IncrementalBuild.fs">
+      <Link>Service/IncrementalBuild.fs</Link>
+    </Compile>
     <Compile Include="$(FSharpSourcesRoot)\fsharp\service\ServiceConstants.fs">
       <Link>Service/ServiceConstants.fs</Link>
     </Compile>

src/fsharp/CompileOps.fs

@@ -209,6 +209,14 @@ type UnresolvedAssemblyReference = UnresolvedAssemblyReference of string * Assem
 type ResolvedExtensionReference = ResolvedExtensionReference of string * AssemblyReference list * Tainted<ITypeProvider> list
 #endif
 
+/// The thread in which compilation calls will be enqueued and done work on.
+/// Note: This is currently only used when disposing of type providers and will be extended to all the other type provider calls when compilations can be done in parallel.
+///       Right now all calls in FCS to type providers are single-threaded through use of the reactor thread. 
+type ICompilationThread =
+
+    /// Enqueue work to be done on a compilation thread.
+    abstract EnqueueWork: (CompilationThreadToken -> unit) -> unit
+
 [<RequireQualifiedAccess>]
 type CompilerTarget = 
     | WinExe 
@@ -351,6 +359,7 @@ type TcConfigBuilder =
 #if !NO_EXTENSIONTYPING
       mutable showExtensionTypeMessages: bool
 #endif
+      mutable compilationThread: ICompilationThread
       mutable pause: bool 
       mutable alwaysCallVirt: bool
       mutable noDebugData: bool
@@ -510,6 +519,7 @@ type TcConfig =
 #if !NO_EXTENSIONTYPING
     member showExtensionTypeMessages: bool
 #endif
+    member compilationThread: ICompilationThread
     member pause: bool 
     member alwaysCallVirt: bool
     member noDebugData: bool
@@ -591,6 +601,8 @@ type TcAssemblyResolutions =
     static member BuildFromPriorResolutions    : CompilationThreadToken * TcConfig * AssemblyResolution list * UnresolvedAssemblyReference list -> TcAssemblyResolutions 
 
 /// Represents a table of imported assemblies with their resolutions.
+/// Is a disposable object, but it is recommended not to explicitly call Dispose unless you absolutely know nothing will be using its contents after the disposal.
+/// Otherwise, simply allow the GC to collect this and it will properly call Dispose from the finalizer.
 [<Sealed>] 
 type TcImports =
     interface System.IDisposable

src/fsharp/CompileOps.fsi

@@ -1211,11 +1211,14 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
                         keepAssemblyContents, keepAllBackgroundResolutions, maxTimeShareMilliseconds) =
 
     let tcConfigP = TcConfigProvider.Constant tcConfig
-    let importsInvalidated = new Event<string>()
     let fileParsed = new Event<string>()
     let beforeFileChecked = new Event<string>()
     let fileChecked = new Event<string>()
     let projectChecked = new Event<unit>()
+#if !NO_EXTENSIONTYPING
+    let importsInvalidatedByTypeProvider = new Event<string>()
+#endif
+    let mutable currentTcImportsOpt = None
 
     // Check for the existence of loaded sources and prepend them to the sources list if present.
     let sourceFiles = tcConfig.GetAvailableLoadedSources() @ (sourceFiles |>List.map (fun s -> rangeStartup, s))
@@ -1242,42 +1245,19 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
            for (_, f, _) in sourceFiles do
                 yield f |]
 
-    // The IncrementalBuilder needs to hold up to one item that needs to be disposed, which is the tcImports for the incremental
-    // build. 
-    let mutable cleanupItem = None: TcImports option
-    let disposeCleanupItem() =
-            match cleanupItem with 
-            | None -> ()
-            | Some item -> 
-                cleanupItem <- None
-                dispose item 
-
-    let setCleanupItem x = 
-            assert cleanupItem.IsNone
-            cleanupItem <- Some x
-
-    let mutable disposed = false
-    let assertNotDisposed() =
-        if disposed then  
-            System.Diagnostics.Debug.Assert(false, "IncrementalBuild object has already been disposed!")
-
-    let mutable referenceCount = 0
-
     //----------------------------------------------------
     // START OF BUILD TASK FUNCTIONS 
 
     /// This is a build task function that gets placed into the build rules as the computation for a VectorStamp
     ///
     /// Get the timestamp of the given file name.
     let StampFileNameTask (cache: TimeStampCache) _ctok (_m: range, filename: string, _isLastCompiland) =
-        assertNotDisposed()
         cache.GetFileTimeStamp filename
 
     /// This is a build task function that gets placed into the build rules as the computation for a VectorMap
     ///
     /// Parse the given file and return the given input.
     let ParseTask ctok (sourceRange: range, filename: string, isLastCompiland) =
-        assertNotDisposed()
         DoesNotRequireCompilerThreadTokenAndCouldPossiblyBeMadeConcurrent  ctok
 
         let errorLogger = CompilationErrorLogger("ParseTask", tcConfig.errorSeverityOptions)
@@ -1300,7 +1280,6 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
     ///
     /// Timestamps of referenced assemblies are taken from the file's timestamp.
     let StampReferencedAssemblyTask (cache: TimeStampCache) ctok (_ref, timeStamper) =
-        assertNotDisposed()
         timeStamper cache ctok
 
 
@@ -1309,18 +1288,13 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
     // Link all the assemblies together and produce the input typecheck accumulator               
     let CombineImportedAssembliesTask ctok _ : Cancellable<TypeCheckAccumulator> =
       cancellable {
-        assertNotDisposed()
         let errorLogger = CompilationErrorLogger("CombineImportedAssembliesTask", tcConfig.errorSeverityOptions)
         // Return the disposable object that cleans up
         use _holder = new CompilationGlobalsScope(errorLogger, BuildPhase.Parameter)
 
         let! tcImports = 
           cancellable {
             try
-                // We dispose any previous tcImports, for the case where a dependency changed which caused this part
-                // of the partial build to be re-evaluated.
-                disposeCleanupItem()
-
                 let! tcImports = TcImports.BuildNonFrameworkTcImports(ctok, tcConfigP, tcGlobals, frameworkTcImports, nonFrameworkResolutions, unresolvedReferences)  
 #if !NO_EXTENSIONTYPING
                 tcImports.GetCcusExcludingBase() |> Seq.iter (fun ccu -> 
@@ -1338,19 +1312,13 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
                     //
                     // In the invalidation handler we use a weak reference to allow the IncrementalBuilder to 
                     // be collected if, for some reason, a TP instance is not disposed or not GC'd.
-                    let capturedImportsInvalidated = WeakReference<_>(importsInvalidated)
+                    let capturedImportsInvalidated = WeakReference<_>(importsInvalidatedByTypeProvider)
                     ccu.Deref.InvalidateEvent.Add(fun msg -> 
                         match capturedImportsInvalidated.TryGetTarget() with 
                         | true, tg -> tg.Trigger msg
-                        | _ -> ()))
+                        | _ -> ()))  
 #endif
-                    
-                    
-                // The tcImports must be cleaned up if this builder ever gets disposed. We also dispose any previous
-                // tcImports should we be re-creating an entry because a dependency changed which caused this part
-                // of the partial build to be re-evaluated.
-                setCleanupItem tcImports
-
+                currentTcImportsOpt <- Some tcImports
                 return tcImports
             with e -> 
                 System.Diagnostics.Debug.Assert(false, sprintf "Could not BuildAllReferencedDllTcImports %A" e)
@@ -1390,7 +1358,6 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
     ///
     /// Type check all files.     
     let TypeCheckTask ctok (tcAcc: TypeCheckAccumulator) input: Eventually<TypeCheckAccumulator> =    
-        assertNotDisposed()
         match input with 
         | Some input, _sourceRange, filename, parseErrors->
             IncrementalBuilderEventTesting.MRU.Add(IncrementalBuilderEventTesting.IBETypechecked filename)
@@ -1462,7 +1429,6 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
     /// Finish up the typechecking to produce outputs for the rest of the compilation process
     let FinalizeTypeCheckTask ctok (tcStates: TypeCheckAccumulator[]) = 
       cancellable {
-        assertNotDisposed()
         DoesNotRequireCompilerThreadTokenAndCouldPossiblyBeMadeConcurrent  ctok
 
         let errorLogger = CompilationErrorLogger("CombineImportedAssembliesTask", tcConfig.errorSeverityOptions)
@@ -1581,21 +1547,7 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
         partialBuild <- b
 
     let MaxTimeStampInDependencies cache (ctok: CompilationThreadToken) (output: INode) = 
-        IncrementalBuild.MaxTimeStampInDependencies cache ctok output.Name partialBuild 
-
-    member this.IncrementUsageCount() = 
-        assertNotDisposed() 
-        System.Threading.Interlocked.Increment(&referenceCount) |> ignore
-        { new System.IDisposable with member __.Dispose() = this.DecrementUsageCount() }
-
-    member __.DecrementUsageCount() = 
-        assertNotDisposed()
-        let currentValue =  System.Threading.Interlocked.Decrement(&referenceCount)
-        if currentValue = 0 then 
-                disposed <- true
-                disposeCleanupItem()
-
-    member __.IsAlive = referenceCount > 0
+        IncrementalBuild.MaxTimeStampInDependencies cache ctok output.Name partialBuild
 
     member __.TcConfig = tcConfig
 
@@ -1607,21 +1559,14 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
 
     member __.ProjectChecked = projectChecked.Publish
 
-    member __.ImportedCcusInvalidated = importsInvalidated.Publish
-
-    member __.AllDependenciesDeprecated = allDependencies
-
 #if !NO_EXTENSIONTYPING
-    member __.ThereAreLiveTypeProviders = 
-        let liveTPs =
-            match cleanupItem with 
-            | None -> []
-            | Some tcImports -> [for ia in tcImports.GetImportedAssemblies() do yield! ia.TypeProviders]
-        match liveTPs with
-        | [] -> false
-        | _ -> true                
+    member __.ImportsInvalidatedByTypeProvider = importsInvalidatedByTypeProvider.Publish
 #endif
 
+    member __.TryGetCurrentTcImports () = currentTcImportsOpt
+
+    member __.AllDependenciesDeprecated = allDependencies
+
     member __.Step (ctok: CompilationThreadToken) =  
       cancellable {
         let cache = TimeStampCache defaultTimeStamp // One per step
@@ -1808,6 +1753,14 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
                 // Never open PDB files for the language service, even if --standalone is specified
                 tcConfigB.openDebugInformationForLaterStaticLinking <- false
 
+                tcConfigB.compilationThread <- 
+                    { new ICompilationThread with 
+                        member __.EnqueueWork work = 
+                            Reactor.Singleton.EnqueueOp ("Unknown", "ICompilationThread.EnqueueWork", "work", fun ctok ->
+                                work ctok
+                            )
+                    }
+
                 tcConfigB, sourceFilesNew
 
             match loadClosureOpt with
@@ -1887,11 +1840,3 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
 
         return builderOpt, diagnostics
       }
-
-    static member KeepBuilderAlive (builderOpt: IncrementalBuilder option) = 
-        match builderOpt with 
-        | Some builder -> builder.IncrementUsageCount() 
-        | None -> { new System.IDisposable with member __.Dispose() = () }
-
-    member __.IsBeingKeptAliveApartFromCacheEntry = (referenceCount >= 2)
-

src/fsharp/service/IncrementalBuild.fs

@@ -80,9 +80,6 @@ type internal PartialCheckResults =
 [<Class>]
 type internal IncrementalBuilder = 
 
-      /// Check if the builder is not disposed
-      member IsAlive : bool
-
       /// The TcConfig passed in to the builder creation.
       member TcConfig : TcConfig
 
@@ -104,15 +101,17 @@ type internal IncrementalBuilder =
       /// overall analysis results for the project will be quick.
       member ProjectChecked : IEvent<unit>
 
+#if !NO_EXTENSIONTYPING
       /// Raised when a type provider invalidates the build.
-      member ImportedCcusInvalidated : IEvent<string>
+      member ImportsInvalidatedByTypeProvider : IEvent<string>
+#endif
+
+      /// Tries to get the current successful TcImports. This is only used in testing. Do not use it for other stuff.
+      member TryGetCurrentTcImports : unit -> TcImports option
 
       /// The list of files the build depends on
       member AllDependenciesDeprecated : string[]
-#if !NO_EXTENSIONTYPING
-      /// Whether there are any 'live' type providers that may need a refresh when a project is Cleaned
-      member ThereAreLiveTypeProviders : bool
-#endif
+
       /// Perform one step in the F# build. Return true if the background work is finished.
       member Step : CompilationThreadToken -> Cancellable<bool>
 
@@ -164,13 +163,6 @@ type internal IncrementalBuilder =
 
       static member TryCreateBackgroundBuilderForProjectOptions : CompilationThreadToken * ReferenceResolver.Resolver * defaultFSharpBinariesDir: string * FrameworkImportsCache * scriptClosureOptions:LoadClosure option * sourceFiles:string list * commandLineArgs:string list * projectReferences: IProjectReference list * projectDirectory:string * useScriptResolutionRules:bool * keepAssemblyContents: bool * keepAllBackgroundResolutions: bool * maxTimeShareMilliseconds: int64 * tryGetMetadataSnapshot: ILBinaryReader.ILReaderTryGetMetadataSnapshot * suggestNamesForErrors: bool -> Cancellable<IncrementalBuilder option * FSharpErrorInfo[]>
 
-      /// Increment the usage count on the IncrementalBuilder by 1. This initial usage count is 0 so immediately after creation 
-      /// a call to KeepBuilderAlive should be made. The returns an IDisposable which will 
-      /// decrement the usage count and dispose if the usage count goes to zero
-      static member KeepBuilderAlive : IncrementalBuilder option -> IDisposable
-
-      member IsBeingKeptAliveApartFromCacheEntry : bool
-
 /// Generalized Incremental Builder. This is exposed only for unittesting purposes.
 module internal IncrementalBuild =
     type INode = 

src/fsharp/service/IncrementalBuild.fsi

@@ -492,19 +492,12 @@ type FSharpDeclarationListItem(name: string, nameInCode: string, fullName: strin
     member __.StructuredDescriptionTextAsync = 
         let userOpName = "ToolTip"
         match info with
-        | Choice1Of2 (items: CompletionItem list, infoReader, m, denv, reactor:IReactorOperations, checkAlive) -> 
+        | Choice1Of2 (items: CompletionItem list, infoReader, m, denv, reactor:IReactorOperations) -> 
             // reactor causes the lambda to execute on the background compiler thread, through the Reactor
             reactor.EnqueueAndAwaitOpAsync (userOpName, "StructuredDescriptionTextAsync", name, fun ctok -> 
                 RequireCompilationThread ctok
-                // This is where we do some work which may touch TAST data structures owned by the IncrementalBuilder - infoReader, item etc. 
-                // It is written to be robust to a disposal of an IncrementalBuilder, in which case it will just return the empty string. 
-                // It is best to think of this as a "weak reference" to the IncrementalBuilder, i.e. this code is written to be robust to its
-                // disposal. Yes, you are right to scratch your head here, but this is ok.
-                cancellable.Return(
-                    if checkAlive() then 
-                        FSharpToolTipText(items |> List.map (fun x -> SymbolHelpers.FormatStructuredDescriptionOfItem true infoReader m denv x.ItemWithInst))
-                    else 
-                        FSharpToolTipText [ FSharpStructuredToolTipElement.Single(wordL (tagText (FSComp.SR.descriptionUnavailable())), FSharpXmlDoc.None) ]))
+                cancellable.Return(FSharpToolTipText(items |> List.map (fun x -> SymbolHelpers.FormatStructuredDescriptionOfItem true infoReader m denv x.ItemWithInst)))
+            )
             | Choice2Of2 result -> 
                 async.Return result
 
@@ -559,7 +552,7 @@ type FSharpDeclarationListInfo(declarations: FSharpDeclarationListItem[], isForT
     member __.IsError = isError
 
     // Make a 'Declarations' object for a set of selected items
-    static member Create(infoReader:InfoReader, m, denv, getAccessibility, items: CompletionItem list, reactor, currentNamespaceOrModule: string[] option, isAttributeApplicationContext: bool, checkAlive) = 
+    static member Create(infoReader:InfoReader, m: range, denv, getAccessibility, items: CompletionItem list, reactor, currentNamespaceOrModule: string[] option, isAttributeApplicationContext: bool) = 
         let g = infoReader.g
         let isForType = items |> List.exists (fun x -> x.Type.IsSome)
         let items = items |> SymbolHelpers.RemoveExplicitlySuppressedCompletionItems g
@@ -697,7 +690,7 @@ type FSharpDeclarationListInfo(declarations: FSharpDeclarationListItem[], isForT
                             | ns -> Some (System.String.Join(".", ns)))
 
                     FSharpDeclarationListItem(
-                        name, nameInCode, fullName, glyph, Choice1Of2 (items, infoReader, m, denv, reactor, checkAlive), getAccessibility item.Item,
+                        name, nameInCode, fullName, glyph, Choice1Of2 (items, infoReader, m, denv, reactor), getAccessibility item.Item,
                         item.Kind, item.IsOwnMember, item.MinorPriority, item.Unresolved.IsNone, namespaceToOpen))
 
         new FSharpDeclarationListInfo(Array.ofList decls, isForType, false)

src/fsharp/service/ServiceDeclarationLists.fs

@@ -67,7 +67,7 @@ type public FSharpDeclarationListInfo =
     member IsError : bool
 
     // Implementation details used by other code in the compiler    
-    static member internal Create : infoReader:InfoReader * m:range * denv:DisplayEnv * getAccessibility:(Item -> FSharpAccessibility option) * items:CompletionItem list * reactor:IReactorOperations * currentNamespace:string[] option * isAttributeApplicationContex:bool * checkAlive:(unit -> bool) -> FSharpDeclarationListInfo
+    static member internal Create : infoReader:InfoReader * m:range * denv:DisplayEnv * getAccessibility:(Item -> FSharpAccessibility option) * items:CompletionItem list * reactor:IReactorOperations * currentNamespace:string[] option * isAttributeApplicationContex:bool -> FSharpDeclarationListInfo
 
     static member internal Error : message:string -> FSharpDeclarationListInfo