do not mutate

Author: David Ungar

Sources/SwiftDriver/IncrementalCompilation/BidirectionalMap.swift

@@ -12,10 +12,6 @@
 
 /// Like a two-way dictionary
 ///
-// FIXME: The current use of this abstraction in the driver is
-// fundamentally broken. This data structure should be retired ASAP.
-// See the extended FIXME in its use in
-// `ModuleDependencyGraph.inputDependencySourceMap`
 public struct BidirectionalMap<T1: Hashable, T2: Hashable>: Equatable, Sequence {
   private var map1: [T1: T2] = [:]
   private var map2: [T2: T1] = [:]
@@ -74,6 +70,20 @@ public struct BidirectionalMap<T1: Hashable, T2: Hashable>: Equatable, Sequence
   public func contains(key: T2) -> Bool {
     map2.keys.contains(key)
   }
+
+  public mutating func updateValue(_ newValue: T2, forKey key: T1) -> T2? {
+    let oldValue = map1.updateValue(newValue, forKey: key)
+    _ = oldValue.map {map2.removeValue(forKey: $0)}
+    map2[newValue] = key
+    return oldValue
+  }
+  public mutating func updateValue(_ newValue: T1, forKey key: T2) -> T1? {
+    let oldValue = map2.updateValue(newValue, forKey: key)
+   _ = oldValue.map {map1.removeValue(forKey: $0)}
+    map1[newValue] = key
+    return oldValue
+  }
+
   public mutating func removeValue(forKey t1: T1) {
     if let t2 = map1[t1] {
       map2.removeValue(forKey: t2)

Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift

@@ -227,9 +227,6 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
     else {
       return buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals()
     }
-    guard graph.populateInputDependencySourceMap(for: .inputsAddedSincePriors) else {
-      return nil
-    }
     graph.dotFileWriter?.write(graph)
 
     // Any externals not already in graph must be additions which should trigger
@@ -255,13 +252,11 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
   private func buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals()
   -> (ModuleDependencyGraph, TransitivelyInvalidatedInputSet)?
   {
-    let graph = ModuleDependencyGraph(self, .buildingWithoutAPrior)
-    assert(outputFileMap.onlySourceFilesHaveSwiftDeps())
-    
-    guard graph.populateInputDependencySourceMap(for: .buildingFromSwiftDeps) else {
+    guard let graph = ModuleDependencyGraph(self, .buildingWithoutAPrior)
+    else {
       return nil
     }
-
+    
     var inputsInvalidatedByChangedExternals = TransitivelyInvalidatedInputSet()
     for input in sourceFiles.currentInOrder {
        guard let invalidatedInputs =

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -24,7 +24,7 @@ import SwiftOptions
   @_spi(Testing) public var nodeFinder = NodeFinder()
 
   /// Maps input files (e.g. .swift) to and from the DependencySource object.
-  @_spi(Testing) public private(set) var inputDependencySourceMap = InputDependencySourceMap()
+  @_spi(Testing) public let inputDependencySourceMap: InputDependencySourceMap
 
   // The set of paths to external dependencies known to be in the graph
   public internal(set) var fingerprintedExternalDependencies = Set<FingerprintedExternalDependency>()
@@ -43,7 +43,7 @@ import SwiftOptions
   /// Minimize the number of file system modification-time queries.
   private var externalDependencyModTimeCache = [ExternalDependency: Bool]()
 
-  public init(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup,
+  public init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup,
               _ phase: Phase
   ) {
     self.info = info
@@ -52,6 +52,11 @@ import SwiftOptions
     : nil
     self.phase = phase
     self.creationPhase = phase
+    guard let inputDependencySourceMap = InputDependencySourceMap(info)
+    else {
+      return nil
+    }
+    self.inputDependencySourceMap = inputDependencySourceMap
   }
 
   @_spi(Testing) public func sourceRequired(for input: TypedVirtualPath,
@@ -188,46 +193,9 @@ extension ModuleDependencyGraph {
     return nodeFinder.findNodes(for: source).map {!$0.isEmpty}
       ?? false
   }
-  
-  /// Returns: false on error
-  func populateInputDependencySourceMap(
-    `for` purpose: InputDependencySourceMap.AdditionPurpose
-  ) -> Bool {
-    let ofm = info.outputFileMap
-    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 in the output file map")
-      )
-      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.
@@ -566,9 +534,13 @@ extension ModuleDependencyGraph {
       private var inputDependencySourceMap: [(TypedVirtualPath, DependencySource)] = []
       public private(set) var allNodes: [Node] = []
 
-      init(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) {
+      init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) {
         self.fileSystem = info.fileSystem
-        self.graph = ModuleDependencyGraph(info, .updatingFromAPrior)
+        guard let graph = ModuleDependencyGraph(info, .updatingFromAPrior)
+        else {
+          return nil
+        }
+        self.graph = graph
       }
 
       func finalizeGraph() -> ModuleDependencyGraph {
@@ -577,11 +549,6 @@ extension ModuleDependencyGraph {
             .record(def: dependencyKey, use: self.allNodes[useID])
           assert(isNewUse, "Duplicate use def-use arc in graph?")
         }
-        for (input, dependencySource) in inputDependencySourceMap {
-          graph.inputDependencySourceMap.addEntry(input,
-                                                  dependencySource,
-                                                  for: .readingPriors)
-        }
         return self.graph
       }
 
@@ -717,7 +684,9 @@ extension ModuleDependencyGraph {
       }
     }
 
-    var visitor = Visitor(info)
+    guard var visitor = Visitor(info) else {
+      return nil
+    }
     try Bitcode.read(bytes: data, using: &visitor)
     guard let major = visitor.majorVersion,
           let minor = visitor.minorVersion,
@@ -1152,13 +1121,3 @@ extension Set where Element == FingerprintedExternalDependency {
     self == other
   }
 }
-
-/// This should be in a test file, but addMapEntry should be private.
-extension ModuleDependencyGraph {
-  @_spi(Testing) public func mockMapEntry(
-    _ mockInput: TypedVirtualPath,
-    _ mockDependencySource: DependencySource
-  ) {
-    inputDependencySourceMap.addEntry(mockInput, mockDependencySource, for: .mocking)
-  }
-}

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

@@ -15,20 +15,46 @@ import TSCBasic
 
   /// 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.
+  // This map caches the same information as in the `OutputFileMap`, but it
+  // optimizes the reverse lookup, and includes path interning via `DependencySource`.
+  // Once created, it does not change.
 
   public typealias BiMap = BidirectionalMap<TypedVirtualPath, DependencySource>
-  @_spi(Testing) public var biMap = BiMap()
+  @_spi(Testing) public let biMap: BiMap
+
+  /// Create the reverse map to map swiftdeps -> input (source) file from the `OutputFileMap`
+   ///
+   /// - Returns: the map, or nil if error
+  init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) {
+    let outputFileMap = info.outputFileMap
+    let diagnosticEngine = info.diagnosticEngine
+
+    assert(outputFileMap.onlySourceFilesHaveSwiftDeps())
+    var hadError = false
+    self.biMap = info.inputFiles.reduce(into: BiMap()) { biMap, input in
+      guard input.type == .swift else {return}
+      guard let dependencySource = outputFileMap.getDependencySource(for: input)
+       else {
+         // The legacy driver fails silently here.
+         diagnosticEngine.emit(
+           .remarkDisabled("\(input.file.basename) has no swiftDeps file")
+         )
+         hadError = true
+         // Don't stop at the first problem.
+         return
+       }
+       if let sameSourceForInput = biMap.updateValue(dependencySource, forKey: input) {
+         diagnosticEngine.emit(
+           .remarkDisabled(
+             "\(dependencySource) and \(sameSourceForInput) have the same input file in the output file map: \(input)")
+         )
+         hadError = true
+       }
+     }
+     if hadError {
+       return nil
+     }
+   }
 }
 
 // MARK: - Accessing
@@ -45,20 +71,6 @@ extension InputDependencySourceMap {
     _ eachFn: (TypedVirtualPath, DependencySource) -> Void
   ) {
     biMap.forEach(eachFn)
-
-extension OutputFileMap {
-  @_spi(Testing) public func getDependencySource(
-    for sourceFile: TypedVirtualPath
-  ) -> DependencySource? {
-    assert(sourceFile.type == FileType.swift)
-    guard let swiftDepsPath = existingOutput(inputFile: sourceFile.fileHandle,
-                                             outputType: .swiftDeps)
-    else {
-      return nil
-   }
-    assert(VirtualPath.lookup(swiftDepsPath).extension == FileType.swiftDeps.rawValue)
-    let typedSwiftDepsFile = TypedVirtualPath(file: swiftDepsPath, type: .swiftDeps)
-    return DependencySource(typedSwiftDepsFile)
   }
 }
 
@@ -77,18 +89,3 @@ extension OutputFileMap {
     return DependencySource(typedSwiftDepsFile)
   }
 }
-
-// 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
-  }
-}

Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift

@@ -161,7 +161,7 @@ struct MockModuleDependencyGraphCreator {
   }
 
   func mockUpAGraph() -> ModuleDependencyGraph {
-    ModuleDependencyGraph(info, .buildingWithoutAPrior)
+    ModuleDependencyGraph(info, .buildingWithoutAPrior)!
   }
 }