Fixup Bidirectional Map

Sources/SwiftDriver/IncrementalCompilation/BidirectionalMap.swift

@@ -11,50 +11,63 @@
 //===----------------------------------------------------------------------===//
 
 /// 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] = [:]
 
   public init() {}
 
+  /// Accesses the value associated with the given key for reading and writing.
+  ///
+  /// - Parameter key: The key to find in the dictionary.
+  /// - Returns: The value associated with key if key is in the bidirectional
+  /// map; otherwise, `nil`.
   public subscript(_ key: T1) -> T2? {
     get {
-      guard let value = map1[key]
-      else {
-        return nil
-      }
-      return value
+      return self.map1[key]
     }
     set {
-      if let someNewValue = newValue {
-        map1[key] = someNewValue
-        map2[someNewValue] = key
-        return
+      // First, strike any existing mappings.
+      if let oldTarget = self.map1.removeValue(forKey: key) {
+        self.map2.removeValue(forKey: oldTarget)
       }
-      if let oldValue = map1.removeValue(forKey: key) {
-        map2.removeValue(forKey: oldValue)
+      // Then construct the forward mapping (or removal).
+      self.map1[key] = newValue
+      if let newValue = newValue {
+        // And finally, the backwards mapping (or removal).
+        self.map2[newValue] = key
       }
     }
   }
+
+  /// Accesses the value associated with the given key for reading and writing.
+  ///
+  /// - Parameter key: The key to find in the dictionary.
+  /// - Returns: The value associated with key if key is in the bidirectional
+  /// map; otherwise, `nil`.
   public subscript(_ key: T2) -> T1? {
     get {
-      guard let value = map2[key]
-      else {
-        return nil
-      }
-      return value
+      return self.map2[key]
     }
     set {
-      if let someNewValue = newValue {
-        map2[key] = someNewValue
-        map1[someNewValue] = key
-        return
+      // First, strike any existing mappings.
+      if let oldSource = self.map2.removeValue(forKey: key) {
+        self.map1.removeValue(forKey: oldSource)
       }
-      if let oldValue = map2.removeValue(forKey: key) {
-        map1.removeValue(forKey: oldValue)
+      // Then construct the reverse mapping (or removal).
+      self.map2[key] = newValue
+      if let newValue = newValue {
+        // And finally the forwards mapping (or removal).
+        self.map1[newValue] = key
       }
     }
   }
+
   public func contains(key: T1) -> Bool {
     map1.keys.contains(key)
   }

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -23,7 +23,19 @@ import SwiftOptions
 
   @_spi(Testing) public var nodeFinder = NodeFinder()
 
-  /// Maps input files (e.g. .swift) to and from the DependencySource object
+  /// 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>()
 
   // The set of paths to external dependencies known to be in the graph

Tests/SwiftDriverTests/BidirectionalMapTests.swift

@@ -15,7 +15,7 @@ import SwiftDriver
 
 class BidirectionalMapTests: XCTestCase {
 
-  func testTwoDMap() {
+  func testBiDiMap() {
     func test(_ biMapToTest: BidirectionalMap<Int, String>) {
       zip(biMapToTest.map{$0}.sorted {$0.0 < $1.0}, testContents).forEach {
         XCTAssertEqual($0.0, $1.0)
@@ -49,4 +49,21 @@ class BidirectionalMapTests: XCTestCase {
     test(biMap)
     test(biMap2)
   }
+
+  func testDirectionality() {
+    var biMap = BidirectionalMap<Int, String>()
+    biMap[1] = "Hello"
+    XCTAssertEqual(biMap["Hello"], 1)
+    XCTAssertEqual(biMap[1], "Hello")
+    biMap[2] = "World"
+    XCTAssertEqual(biMap["World"], 2)
+    XCTAssertEqual(biMap[2], "World")
+
+    biMap["World"] = 3
+    XCTAssertEqual(biMap["World"], 3)
+    XCTAssertEqual(biMap[3], "World")
+    biMap["Hello"] = 4
+    XCTAssertEqual(biMap["Hello"], 4)
+    XCTAssertEqual(biMap[4], "Hello")
+  }
 }

Tests/SwiftDriverTests/IncrementalCompilationTests.swift

@@ -926,7 +926,8 @@ extension IncrementalCompilationTests {
 class CrossModuleIncrementalBuildTests: XCTestCase {
   func makeOutputFileMap(
     in workingDirectory: AbsolutePath,
-    for files: [AbsolutePath]
+    for files: [AbsolutePath],
+    outputTransform transform: (String) -> String = { $0 }
   ) -> String {
     """
     {
@@ -937,15 +938,68 @@ class CrossModuleIncrementalBuildTests: XCTestCase {
       """
       ,
       "\(file)": {
-        "dependencies": "\(file.basenameWithoutExt + ".d")",
-        "object": "\(file.pathString + ".o")",
-        "swiftmodule": "\(file.basenameWithoutExt + "~partial.swiftmodule")",
-        "swift-dependencies": "\(file.basenameWithoutExt + ".swiftdeps")"
+        "dependencies": "\(transform(file.basenameWithoutExt) + ".d")",
+        "object": "\(transform(file.pathString) + ".o")",
+        "swiftmodule": "\(transform(file.basenameWithoutExt) + "~partial.swiftmodule")",
+        "swift-dependencies": "\(transform(file.basenameWithoutExt) + ".swiftdeps")"
         }
       """
     }.joined(separator: "\n").appending("\n}"))
   }
-
+
+  func testChangingOutputFileMap() throws {
+    try withTemporaryDirectory { path in
+      try localFileSystem.changeCurrentWorkingDirectory(to: path)
+      let magic = path.appending(component: "magic.swift")
+      try localFileSystem.writeFileContents(magic) {
+        $0 <<< "public func castASpell() {}"
+      }
+
+      let ofm = path.appending(component: "ofm.json")
+      try localFileSystem.writeFileContents(ofm) {
+        $0 <<< self.makeOutputFileMap(in: path, for: [ magic ]) {
+          $0 + "-some_suffix"
+        }
+      }
+
+      do {
+        var driver = try Driver(args: [
+          "swiftc",
+          "-incremental",
+          "-emit-module",
+          "-output-file-map", ofm.pathString,
+          "-module-name", "MagicKit",
+          "-working-directory", path.pathString,
+          "-c",
+          magic.pathString,
+        ])
+        let jobs = try driver.planBuild()
+        try driver.run(jobs: jobs)
+      }
+
+      try localFileSystem.writeFileContents(ofm) {
+        $0 <<< self.makeOutputFileMap(in: path, for: [ magic ]) {
+          $0 + "-some_other_suffix"
+        }
+      }
+
+      do {
+        var driver = try Driver(args: [
+          "swiftc",
+          "-incremental",
+          "-emit-module",
+          "-output-file-map", ofm.pathString,
+          "-module-name", "MagicKit",
+          "-working-directory", path.pathString,
+          "-c",
+          magic.pathString,
+        ])
+        let jobs = try driver.planBuild()
+        try driver.run(jobs: jobs)
+      }
+    }
+  }
+
   func testEmbeddedModuleDependencies() throws {
     try withTemporaryDirectory { path in
       try localFileSystem.changeCurrentWorkingDirectory(to: path)