Merge pull request #685 from davidungar/again3

[NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing

Author: David Ungar

Sources/SwiftDriver/CMakeLists.txt

@@ -33,6 +33,7 @@ add_library(SwiftDriver
   Execution/ParsableOutput.swift
   Execution/ProcessProtocol.swift
 
+  "IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift"
   "IncrementalCompilation/ModuleDependencyGraphParts/Integrator.swift"
   "IncrementalCompilation/ModuleDependencyGraphParts/Node.swift"
   "IncrementalCompilation/ModuleDependencyGraphParts/NodeFinder.swift"

Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift

@@ -226,7 +226,7 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
     else {
       return buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals()
     }
-    guard graph.populateInputDependencySourceMap() else {
+    guard graph.populateInputDependencySourceMap(for: .inputsAddedSincePriors) else {
       return nil
     }
     graph.dotFileWriter?.write(graph)
@@ -256,7 +256,8 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
   {
     let graph = ModuleDependencyGraph(self, .buildingWithoutAPrior)
     assert(outputFileMap.onlySourceFilesHaveSwiftDeps())
-    guard graph.populateInputDependencySourceMap() else {
+    
+    guard graph.populateInputDependencySourceMap(for: .buildingFromSwiftDeps) else {
       return nil
     }
 

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -24,19 +24,7 @@ import SwiftOptions
   @_spi(Testing) public var nodeFinder = NodeFinder()
 
   /// Maps input files (e.g. .swift) to and from the DependencySource object.
-  ///
-  // FIXME: The map between swiftdeps and swift files is absolutely *not*
-  // a bijection. In particular, more than one swiftdeps file can be encountered
-  // in the course of deserializing priors *and* reading the output file map
-  // *and* re-reading swiftdeps files after frontends complete
-  // that correspond to the same swift file. These cause two problems:
-  // - overwrites in this data structure that lose data and
-  // - cache misses in `getInput(for:)` that cause the incremental build to
-  // turn over when e.g. entries in the output file map change. This should be
-  // replaced by a multi-map from swift files to dependency sources,
-  // and a regular map from dependency sources to swift files -
-  // since that direction really is one-to-one.
-  @_spi(Testing) public private(set) var inputDependencySourceMap = BidirectionalMap<TypedVirtualPath, DependencySource>()
+  @_spi(Testing) public private(set) var inputDependencySourceMap = InputDependencySourceMap()
 
   // The set of paths to external dependencies known to be in the graph
   public internal(set) var fingerprintedExternalDependencies = Set<FingerprintedExternalDependency>()
@@ -66,27 +54,16 @@ import SwiftOptions
     self.creationPhase = phase
   }
 
-  private func addMapEntry(_ input: TypedVirtualPath, _ dependencySource: DependencySource) {
-    assert(input.type == .swift && dependencySource.typedFile.type == .swiftDeps)
-    inputDependencySourceMap[input] = dependencySource
-  }
-
-  @_spi(Testing) public func getSource(for input: TypedVirtualPath,
-                                       function: String = #function,
-                                       file: String = #file,
-                                       line: Int = #line) -> DependencySource {
-    guard let source = inputDependencySourceMap[input] else {
+  @_spi(Testing) public func sourceRequired(for input: TypedVirtualPath,
+                                            function: String = #function,
+                                            file: String = #file,
+                                            line: Int = #line) -> DependencySource {
+    guard let source = inputDependencySourceMap.sourceIfKnown(for: input)
+    else {
       fatalError("\(input.file.basename) not found in inputDependencySourceMap, \(file):\(line) in \(function)")
     }
     return source
   }
-  @_spi(Testing) public func getInput(for source: DependencySource) -> TypedVirtualPath? {
-    guard let input = inputDependencySourceMap[source] else {
-      info.diagnosticEngine.emit(warning: "Failed to find source file for '\(source.file.basename)', recovering with a full rebuild. Next build will be incremental.")
-      return nil
-    }
-    return input
-  }
 }
 
 extension ModuleDependencyGraph {
@@ -155,7 +132,7 @@ extension ModuleDependencyGraph {
       return TransitivelyInvalidatedInputSet()
     }
     return collectInputsRequiringCompilationAfterProcessing(
-      dependencySource: getSource(for: input))
+      dependencySource: sourceRequired(for: input))
   }
 }
 
@@ -175,17 +152,16 @@ extension ModuleDependencyGraph {
   /// speculatively scheduled in the first wave.
   func collectInputsInvalidatedBy(input: TypedVirtualPath
   ) -> TransitivelyInvalidatedInputArray {
-    let changedSource = getSource(for: input)
+    let changedSource = sourceRequired(for: input)
     let allDependencySourcesToRecompile =
       collectSwiftDepsUsing(dependencySource: changedSource)
 
     return allDependencySourcesToRecompile.compactMap {
       dependencySource in
       guard dependencySource != changedSource else {return nil}
-      let dependentSource = inputDependencySourceMap[dependencySource]
-      info.reporter?.report(
-        "Found dependent of \(input.file.basename):", dependentSource)
-      return dependentSource
+      let inputToRecompile = inputDependencySourceMap.inputIfKnown(for: dependencySource)
+      info.reporter?.report("Found dependent of \(input.file.basename):", inputToRecompile)
+      return inputToRecompile
     }
   }
 
@@ -202,7 +178,7 @@ extension ModuleDependencyGraph {
   /// Does the graph contain any dependency nodes for a given source-code file?
   func containsNodes(forSourceFile file: TypedVirtualPath) -> Bool {
     precondition(file.type == .swift)
-    guard let source = inputDependencySourceMap[file] else {
+    guard let source = inputDependencySourceMap.sourceIfKnown(for: file) else {
       return false
     }
     return containsNodes(forDependencySource: source)
@@ -212,17 +188,46 @@ extension ModuleDependencyGraph {
     return nodeFinder.findNodes(for: source).map {!$0.isEmpty}
       ?? false
   }
-
-  /// Return true on success
-  func populateInputDependencySourceMap() -> Bool {
+  
+  /// Returns: false on error
+  func populateInputDependencySourceMap(
+    `for` purpose: InputDependencySourceMap.AdditionPurpose
+  ) -> Bool {
     let ofm = info.outputFileMap
-    let de = info.diagnosticEngine
-    return info.inputFiles.reduce(true) { okSoFar, input in
-      ofm.getDependencySource(for: input, diagnosticEngine: de)
-        .map {source in addMapEntry(input, source); return okSoFar } ?? false
+    let diags = info.diagnosticEngine
+    var allFound = true
+    for input in info.inputFiles {
+      if let source = ofm.dependencySource(for: input, diagnosticEngine: diags) {
+        inputDependencySourceMap.addEntry(input, source, for: purpose)
+      } else {
+        // Don't break in order to report all failures.
+        allFound = false
+      }
     }
+    return allFound
   }
 }
+extension OutputFileMap {
+  fileprivate func dependencySource(
+    for sourceFile: TypedVirtualPath,
+    diagnosticEngine: DiagnosticsEngine
+  ) -> DependencySource? {
+    assert(sourceFile.type == FileType.swift)
+    guard let swiftDepsPath = existingOutput(inputFile: sourceFile.fileHandle,
+                                             outputType: .swiftDeps)
+    else {
+      // The legacy driver fails silently here.
+      diagnosticEngine.emit(
+        .remarkDisabled("\(sourceFile.file.basename) has no swiftDeps file")
+      )
+      return nil
+    }
+    assert(VirtualPath.lookup(swiftDepsPath).extension == FileType.swiftDeps.rawValue)
+    let typedSwiftDepsFile = TypedVirtualPath(file: swiftDepsPath, type: .swiftDeps)
+    return DependencySource(typedSwiftDepsFile)
+  }
+}
+
 // MARK: - Scheduling the 2nd wave
 extension ModuleDependencyGraph {
   /// After `source` has been compiled, figure out what other source files need compiling.
@@ -232,7 +237,7 @@ extension ModuleDependencyGraph {
   func collectInputsRequiringCompilation(byCompiling input: TypedVirtualPath
   ) -> TransitivelyInvalidatedInputSet? {
     precondition(input.type == .swift)
-    let dependencySource = getSource(for: input)
+    let dependencySource = sourceRequired(for: input)
     return collectInputsRequiringCompilationAfterProcessing(
       dependencySource: dependencySource)
   }
@@ -327,7 +332,10 @@ extension ModuleDependencyGraph {
   ) -> TransitivelyInvalidatedInputSet? {
     var invalidatedInputs = TransitivelyInvalidatedInputSet()
     for invalidatedSwiftDeps in collectSwiftDepsUsingInvalidated(nodes: directlyInvalidatedNodes) {
-      guard let invalidatedInput = getInput(for: invalidatedSwiftDeps) else {
+      guard let invalidatedInput = inputDependencySourceMap.inputIfKnown(for: invalidatedSwiftDeps)
+      else {
+        info.diagnosticEngine.emit(
+          warning: "Failed to find source file for '\(invalidatedSwiftDeps.file.basename)', recovering with a full rebuild. Next build will be incremental.")
         return nil
       }
       invalidatedInputs.insert(invalidatedInput)
@@ -451,27 +459,6 @@ extension ModuleDependencyGraph {
   }
 }
 
-extension OutputFileMap {
-  fileprivate func getDependencySource(
-    for sourceFile: TypedVirtualPath,
-    diagnosticEngine: DiagnosticsEngine
-  ) -> DependencySource? {
-    assert(sourceFile.type == FileType.swift)
-    guard let swiftDepsPath = existingOutput(inputFile: sourceFile.fileHandle,
-                                             outputType: .swiftDeps)
-    else {
-      // The legacy driver fails silently here.
-      diagnosticEngine.emit(
-        .remarkDisabled("\(sourceFile.file.basename) has no swiftDeps file")
-      )
-      return nil
-    }
-    assert(VirtualPath.lookup(swiftDepsPath).extension == FileType.swiftDeps.rawValue)
-    let typedSwiftDepsFile = TypedVirtualPath(file: swiftDepsPath, type: .swiftDeps)
-    return DependencySource(typedSwiftDepsFile)
-  }
-}
-
 // MARK: - tracking traced nodes
 extension ModuleDependencyGraph {
 
@@ -596,10 +583,11 @@ extension ModuleDependencyGraph {
             .record(def: dependencyKey, use: self.allNodes[useID])
           assert(isNewUse, "Duplicate use def-use arc in graph?")
         }
-        for (input, source) in inputDependencySourceMap {
-          graph.addMapEntry(input, source)
+        for (input, dependencySource) in inputDependencySourceMap {
+          graph.inputDependencySourceMap.addEntry(input,
+                                                  dependencySource,
+                                                  for: .readingPriors)
         }
-
         return self.graph
       }
 
@@ -895,7 +883,7 @@ extension ModuleDependencyGraph {
         }
       }
 
-      for (input, dependencySource) in graph.inputDependencySourceMap {
+      graph.inputDependencySourceMap.enumerateToSerializePriors { input, dependencySource in
         self.addIdentifier(input.file.name)
         self.addIdentifier(dependencySource.file.name)
       }
@@ -1040,7 +1028,8 @@ extension ModuleDependencyGraph {
             }
           }
         }
-        for (input, dependencySource) in graph.inputDependencySourceMap {
+        graph.inputDependencySourceMap.enumerateToSerializePriors {
+          input, dependencySource in
           serializer.stream.writeRecord(serializer.abbreviations[.mapNode]!) {
             $0.append(RecordID.mapNode)
             $0.append(serializer.lookupIdentifierCode(for: input.file.name))
@@ -1158,7 +1147,7 @@ extension Set where Element == ModuleDependencyGraph.Node {
   }
 }
 
-extension BidirectionalMap where T1 == TypedVirtualPath, T2 == DependencySource {
+extension InputDependencySourceMap {
   fileprivate func matches(_ other: Self) -> Bool {
     self == other
   }
@@ -1176,6 +1165,6 @@ extension ModuleDependencyGraph {
     _ mockInput: TypedVirtualPath,
     _ mockDependencySource: DependencySource
   ) {
-    addMapEntry(mockInput, mockDependencySource)
+    inputDependencySourceMap.addEntry(mockInput, mockDependencySource, for: .mocking)
   }
 }

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

@@ -0,0 +1,64 @@
+//===------------- InputDependencySourceMap.swift ---------------- --------===//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2021 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+import Foundation
+import TSCBasic
+
+@_spi(Testing) public struct InputDependencySourceMap: Equatable {
+  
+  /// Maps input files (e.g. .swift) to and from the DependencySource object.
+  ///
+  // FIXME: The map between swiftdeps and swift files is absolutely *not*
+  // a bijection. In particular, more than one swiftdeps file can be encountered
+  // in the course of deserializing priors *and* reading the output file map
+  // *and* re-reading swiftdeps files after frontends complete
+  // that correspond to the same swift file. These cause two problems:
+  // - overwrites in this data structure that lose data and
+  // - cache misses in `getInput(for:)` that cause the incremental build to
+  // turn over when e.g. entries in the output file map change. This should be
+  // replaced by a multi-map from swift files to dependency sources,
+  // and a regular map from dependency sources to swift files -
+  // since that direction really is one-to-one.
+  
+  public typealias BiMap = BidirectionalMap<TypedVirtualPath, DependencySource>
+  @_spi(Testing) public var biMap = BiMap()
+}
+
+// MARK: - Accessing
+extension InputDependencySourceMap {
+  @_spi(Testing) public func sourceIfKnown(for input: TypedVirtualPath) -> DependencySource? {
+    biMap[input]
+  }
+
+  @_spi(Testing) public func inputIfKnown(for source: DependencySource) -> TypedVirtualPath? {
+    biMap[source]
+  }
+
+  @_spi(Testing) public func enumerateToSerializePriors(
+    _ eachFn: (TypedVirtualPath, DependencySource) -> Void
+  ) {
+    biMap.forEach(eachFn)
+  }
+}
+
+// MARK: - Populating
+extension InputDependencySourceMap {
+  public enum AdditionPurpose {
+    case mocking,
+         buildingFromSwiftDeps,
+         readingPriors,
+         inputsAddedSincePriors }
+  @_spi(Testing) public mutating func addEntry(_ input: TypedVirtualPath,
+                                               _ dependencySource: DependencySource,
+                                               `for` _ : AdditionPurpose) {
+    assert(input.type == .swift && dependencySource.typedFile.type == .swiftDeps)
+    biMap[input] = dependencySource
+  }
+}

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Tracer.swift

@@ -121,7 +121,8 @@ extension ModuleDependencyGraph.Tracer {
         path.compactMap { node in
           node.dependencySource.map {
             source in
-            graph.inputDependencySourceMap[source].map { input in
+            graph.inputDependencySourceMap.inputIfKnown(for: source).map {
+              input in
               "\(node.key) in \(input.file.basename)"
             }
             ?? "\(node.key)"