Merge pull request #664 from davidungar/replace-bijection-on-vacation

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

Author: David Ungar

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,22 +54,20 @@ 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 source(requiredFor 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 {
+
+  @_spi(Testing) public func input(neededFor source: DependencySource) -> TypedVirtualPath? {
+    guard let input = inputDependencySourceMap.input(ifKnownFor: 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
     }
@@ -155,7 +141,7 @@ extension ModuleDependencyGraph {
       return TransitivelyInvalidatedInputSet()
     }
     return collectInputsRequiringCompilationAfterProcessing(
-      dependencySource: getSource(for: input))
+      dependencySource: source(requiredFor: input))
   }
 }
 
@@ -177,17 +163,17 @@ extension ModuleDependencyGraph {
   /// speculatively scheduled in the first wave.
   func collectInputsInvalidatedBy(input: TypedVirtualPath
   ) -> TransitivelyInvalidatedInputArray {
-    let changedSource = getSource(for: input)
+    let changedSource = source(requiredFor: input)
     let allDependencySourcesToRecompile =
       collectSwiftDepsUsing(dependencySource: changedSource)
 
     return allDependencySourcesToRecompile.compactMap {
       depedencySource in
       guard depedencySource != changedSource else {return nil}
-      let dependentSource = inputDependencySourceMap[depedencySource]
+      let dependentInput = inputDependencySourceMap.input(ifKnownFor: depedencySource)
       info.reporter?.report(
-        "Found dependent of \(input.file.basename):", dependentSource)
-      return dependentSource
+        "Found dependent of \(input.file.basename):", dependentInput)
+      return dependentInput
     }
   }
 
@@ -204,7 +190,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)
@@ -214,17 +200,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.
@@ -234,7 +249,7 @@ extension ModuleDependencyGraph {
   func collectInputsRequiringCompilation(byCompiling input: TypedVirtualPath
   ) -> TransitivelyInvalidatedInputSet? {
     precondition(input.type == .swift)
-    let dependencySource = getSource(for: input)
+    let dependencySource = source(requiredFor: input)
     return collectInputsRequiringCompilationAfterProcessing(
       dependencySource: dependencySource)
   }
@@ -329,7 +344,7 @@ extension ModuleDependencyGraph {
   ) -> TransitivelyInvalidatedInputSet? {
     var invalidatedInputs = TransitivelyInvalidatedInputSet()
     for invalidatedSwiftDeps in collectSwiftDepsUsingInvalidated(nodes: directlyInvalidatedNodes) {
-      guard let invalidatedInput = getInput(for: invalidatedSwiftDeps) else {
+      guard let invalidatedInput = input(neededFor: invalidatedSwiftDeps) else {
         return nil
       }
       invalidatedInputs.insert(invalidatedInput)
@@ -406,27 +421,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 {
 
@@ -551,10 +545,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
       }
 
@@ -850,7 +845,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)
       }
@@ -995,7 +990,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))
@@ -1113,7 +1109,7 @@ extension Set where Element == ModuleDependencyGraph.Node {
   }
 }
 
-extension BidirectionalMap where T1 == TypedVirtualPath, T2 == DependencySource {
+extension InputDependencySourceMap {
   fileprivate func matches(_ other: Self) -> Bool {
     self == other
   }
@@ -1131,6 +1127,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,93 @@
+//===------------- 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
+
+/// Maps input source files to- and from- `DependencySource`s containing the swiftdeps paths.
+/// Deliberately at the level of specific intention, for clarity and to facilitate restructuring later.
+@_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.
+
+  /// Holds the mapping for now. To be replaced later.
+  public typealias BiMap = BidirectionalMap<TypedVirtualPath, DependencySource>
+  @_spi(Testing) public var biMap = BiMap()
+}
+
+// MARK: - Accessing
+extension InputDependencySourceMap {
+  /// Find where the swiftdeps are stored for a given source file.
+  ///
+  /// - Parameter input: A source file path
+  /// - Returns: the corresponding `DependencySource`, or nil if none known.
+  @_spi(Testing) public func sourceIfKnown(for input: TypedVirtualPath) -> DependencySource? {
+    biMap[input]
+  }
+
+  /// Find where the source file is for a given swiftdeps file.
+  ///
+  /// - Parameter source: A `DependencySource` (containing a swiftdeps file)
+  /// - Returns: the corresponding input source file, or nil if none known.
+  @_spi(Testing) public func input(ifKnownFor source: DependencySource) -> TypedVirtualPath? {
+    biMap[source]
+  }
+
+  /// Enumerate the input <-> dependency source pairs to be serialized
+  ///
+  /// - Parameter eachFn: a function to be called for each pair
+  @_spi(Testing) public func enumerateToSerializePriors(
+    _ eachFn: (TypedVirtualPath, DependencySource) -> Void
+  ) {
+    biMap.forEach(eachFn)
+  }
+}
+
+// MARK: - Populating
+extension InputDependencySourceMap {
+  /// For structural modifications-to-come, reify the various reasons to add an entry.
+  public enum AdditionPurpose {
+    /// For unit testing
+    case mocking
+
+    /// When building a graph from swiftDeps files without priors
+    case buildingFromSwiftDeps
+
+    /// Deserializing the map stored with the priors
+    case readingPriors
+
+    /// After reading the priors, used to add entries for any inputs that might not have been in the priors.
+    case inputsAddedSincePriors
+  }
+
+  /// Add a mapping to & from input source file to swiftDeps file path
+  ///
+  /// - Parameter input: the source file path
+  /// - Parameter dependencySource: the dependency source (holding the swiftdeps path)
+  /// - Parameter why: the purpose for this addition. Will be used for future restructuring.
+  @_spi(Testing) public mutating func addEntry(_ input: TypedVirtualPath,
+                                               _ dependencySource: DependencySource,
+                                               for why: 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.input(ifKnownFor: source).map {
+              input in
               "\(node.key) in \(input.file.basename)"
             }
             ?? "\(node.key)"