Merge pull request #740 from davidungar/july-cull

[Incremental] When reading priors, don't use nodes for removed inputs.

Author: David Ungar

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -129,6 +129,22 @@ extension ModuleDependencyGraph {
       invalidatedNodes.formUnion(self.integrateExternal(.known(fed)))
     }
   }
+
+  /// Determine whether (deserialized) node was for a definition in a source file that is no longer part of the build.
+  ///
+  /// If the priors were read from an invocation containing a subsuequently removed input,
+  /// the nodes defining decls from that input must be culled.
+  ///
+  /// - Parameter node: The (deserialized) node to test.
+  /// - Returns: true iff the node corresponds to a definition on a removed source file.
+  fileprivate func isForRemovedInput(_ node: Node) -> Bool {
+    guard let fileWithDeps = node.dependencySource?.typedFile,
+          fileWithDeps.type == .swift // e.g., could be a .swiftdeps file
+    else {
+      return false
+    }
+    return !isPartOfBuild(fileWithDeps)
+  }
 }
 
 // MARK: - Scheduling the first wave
@@ -532,7 +548,13 @@ extension ModuleDependencyGraph {
       private var identifiers: [String] = [""]
       private var currentDefKey: DependencyKey? = nil
       private var nodeUses: [(DependencyKey, Int)] = []
-      public private(set) var allNodes: [Node] = []
+
+      /// Deserialized nodes, in order appearing in the priors file. If `nil`, the node is for a removed source file.
+      ///
+      /// Since the def-use relationship is serialized according the index of the node in the priors file, this
+      /// `Array` supports the deserialization of the def-use links by mapping index to node.
+      /// The optionality of the contents lets the ``ModuleDependencyGraph/isForRemovedInput`` check to be cached.
+      public private(set) var potentiallyUsedNodes: [Node?] = []
 
       init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) {
         self.fileSystem = info.fileSystem
@@ -545,8 +567,12 @@ extension ModuleDependencyGraph {
 
       func finalizeGraph() -> ModuleDependencyGraph {
         for (dependencyKey, useID) in self.nodeUses {
+          guard let use = self.potentiallyUsedNodes[useID] else {
+            // Don't record uses of defs of removed files.
+            continue
+          }
           let isNewUse = self.graph.nodeFinder
-            .record(def: dependencyKey, use: self.allNodes[useID])
+            .record(def: dependencyKey, use: use)
           assert(isNewUse, "Duplicate use def-use arc in graph?")
         }
         return self.graph
@@ -565,7 +591,12 @@ extension ModuleDependencyGraph {
       mutating func didExitBlock() throws {}
 
       private mutating func finalize(node newNode: Node) {
-        self.allNodes.append(newNode)
+        if graph.isForRemovedInput(newNode) {
+          // Preserve the mapping of Int to Node for reconstructing def-use links with a placeholder.
+          self.potentiallyUsedNodes.append(nil)
+          return
+        }
+        self.potentiallyUsedNodes.append(newNode)
         let oldNode = self.graph.nodeFinder.insert(newNode)
         assert(oldNode == nil,
                "Integrated the same node twice: \(oldNode!), \(newNode)")

Tests/SwiftDriverTests/DependencyGraphSerializationTests.swift

@@ -16,7 +16,8 @@ import TSCBasic
 import TSCUtility
 
 class DependencyGraphSerializationTests: XCTestCase, ModuleDependencyGraphMocker {
-  static let mockGraphCreator = MockModuleDependencyGraphCreator(maxIndex: 12)
+  static let maxIndex = 12
+  static let mockGraphCreator = MockModuleDependencyGraphCreator(maxIndex: maxIndex)
 
   /// Unit test of the `ModuleDependencyGraph` serialization
   ///
@@ -33,8 +34,9 @@ class DependencyGraphSerializationTests: XCTestCase, ModuleDependencyGraphMocker
       compilerVersion: "Swift 99",
       mockSerializedGraphVersion: alteredVersion)
     do {
+      let outputFileMap = OutputFileMap.mock(maxIndex: Self.maxIndex)
       _ = try ModuleDependencyGraph.read(from: mockPath,
-                                         info: .mock(fileSystem: fs))
+                                         info: .mock(outputFileMap: outputFileMap, fileSystem: fs))
       XCTFail("Should have thrown an exception")
     }
     catch let ModuleDependencyGraph.ReadError.mismatchedSerializedGraphVersion(expected, read) {
@@ -51,8 +53,9 @@ class DependencyGraphSerializationTests: XCTestCase, ModuleDependencyGraphMocker
     let fs = InMemoryFileSystem()
     try graph.write(to: mockPath, on: fs, compilerVersion: "Swift 99")
 
+    let outputFileMap = OutputFileMap.mock(maxIndex: Self.maxIndex)
     let deserializedGraph = try ModuleDependencyGraph.read(from: mockPath,
-                                                           info: .mock(fileSystem: fs))!
+                                                           info: .mock(outputFileMap: outputFileMap, fileSystem: fs))!
     var originalNodes = Set<ModuleDependencyGraph.Node>()
     graph.nodeFinder.forEachNode {
       originalNodes.insert($0)

Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift

@@ -129,14 +129,18 @@ extension BuildRecordInfo {
 extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
   static func mock(
     options: IncrementalCompilationState.Options = [.verifyDependencyGraphAfterEveryImport],
-    outputFileMap: OutputFileMap = OutputFileMap(),
+    outputFileMap: OutputFileMap,
     fileSystem: FileSystem = localFileSystem,
     diagnosticEngine: DiagnosticsEngine = DiagnosticsEngine()
   ) -> Self {
     let diagnosticsEngine = DiagnosticsEngine()
-    return Self(options, outputFileMap,
+    // Must set input files in order to avoid graph deserialization from culling
+    let inputFiles = outputFileMap.entries.keys
+      .filter {VirtualPath.lookup($0).extension == FileType.swift.rawValue }
+      .map {TypedVirtualPath(file: $0, type: .swift)}
+     return Self(options, outputFileMap,
                 BuildRecordInfo.mock(diagnosticsEngine, outputFileMap),
-                nil, nil, [], fileSystem,
+                nil, nil, inputFiles, fileSystem,
                 diagnosticsEngine)
   }
 }
@@ -176,7 +180,7 @@ struct MockModuleDependencyGraphCreator {
 
 
 extension OutputFileMap {
-  fileprivate static func mock(maxIndex: Int) -> Self {
+  static func mock(maxIndex: Int) -> Self {
     OutputFileMap( entries: (0...maxIndex) .reduce(into: [:]) {
       entries, index in
       let inputHandle = TypedVirtualPath(mockInput: index).file.intern()

Tests/SwiftDriverTests/IncrementalCompilationTests.swift

@@ -798,7 +798,7 @@ extension IncrementalCompilationTests {
         fingerprintChanged(.interface, "main")
         fingerprintChanged(.implementation, "main")
 
-        let affectedInputs = removeInputFromInvocation && !afterRestoringBadPriors
+        let affectedInputs = removeInputFromInvocation
         ? ["other"]
         : [removedInput, "other"]
         for input in affectedInputs {
@@ -810,10 +810,6 @@ extension IncrementalCompilationTests {
                       ? nil : input)
           }
         }
-        if removeInputFromInvocation && afterRestoringBadPriors {
-          failedToFindSource(removedInput)
-          failedToReadSomeSource(compiling: "main")
-        }
         let affectedInputsInBuild = affectedInputs.filter(inputs.contains)
         queuingLater(affectedInputsInBuild)
         schedulingInvalidated(affectedInputsInBuild)
@@ -826,27 +822,6 @@ extension IncrementalCompilationTests {
     let graph = try driver.moduleDependencyGraph()
     graph.verifyGraph()
     if removeInputFromInvocation {
-      if afterRestoringBadPriors {
-        // FIXME: Fix the driver
-        // If you incrementally compile with a.swift and b.swift,
-        // at the end, the driver saves a serialized `ModuleDependencyGraph`
-        // contains nodes for declarations defined in both files.
-        // If you then later remove b.swift and recompile, the driver will
-        // see that a file was removed (via comparisons with the saved `BuildRecord`
-        // and will delete the saved priors. However, if for some reason the
-        // saved priors are not deleted, the driver will read saved priors
-        // containing entries for the deleted file. This test simulates that
-        // condition by restoring the deleted priors. The driver ought to be fixed
-        // to cull any entries for removed files from the deserialized priors.
-        //
-        // There is a wrinkle: How can a node for a decl in a removed file be
-        // distinguished from a node for a decl in an incrementally-imported
-        // external dependency? In both cases the node's `dependencySource`
-        // is for a file that is not in the `inputFiles`.
-        print("*** WARNING: skipping checks, driver fails to cleaned out the graph ***",
-              to: &stderrStream); stderrStream.flush()
-        return graph
-      }
       graph.ensureOmits(sourceBasenameWithoutExt: removedInput)
       graph.ensureOmits(name: topLevelName)
     }