[Incremental] Fix fingerprint integration bug, add a test, and add debugging affordances. (#505)

* Fix fingerprint integration bug, add a test, and add debugging affordances.

* Add a warning

Author: David Ungar

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -307,7 +307,8 @@ extension ModuleDependencyGraph {
     fed.incrementalDependencySource?
       .read(in: info.fileSystem, reporter: info.reporter)
       .map { unserializedDepGraph in
-        Integrator.integrate(
+        info.reporter?.report("Integrating changes from", fed.externalDependency.file)
+        return Integrator.integrate(
           from: unserializedDepGraph,
           into: self,
           includeAddedExternals: includeAddedExternals)

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Integrator.swift

@@ -136,6 +136,7 @@ extension ModuleDependencyGraph.Integrator {
     // Node was and still is. Do not remove it.
     disappearedNodes.removeValue(forKey: matchHere.key)
     if matchHere.fingerprint != integrand.fingerprint {
+      matchHere.setFingerprint(integrand.fingerprint)
       results.allInvalidatedNodes.insert(matchHere)
     }
     return matchHere

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Node.swift

@@ -23,16 +23,21 @@ extension ModuleDependencyGraph {
   /// (Cargo-culted and modified from the legacy driver.)
   ///
   /// Use a class, not a struct because otherwise it would be duplicated for each thing it uses
+  ///
+  /// Neither the `fingerprint`, nor the `isTraced` value is part of the node's identity.
+  /// Neither of these must be considered for equality testing or hashing because their
+  /// value is subject to change during integration and tracing.
 
   public final class Node {
 
     /*@_spi(Testing)*/ public typealias Graph = ModuleDependencyGraph
 
     /// Hold these where an invariant can be checked.
-    let keyAndFingerprint: KeyAndFingerprintHolder
+    /// Must be able to change the fingerprint
+    private(set) var keyAndFingerprint: KeyAndFingerprintHolder
 
     var key: DependencyKey { keyAndFingerprint.key }
-    var fingerprint: String? { keyAndFingerprint.fingerprint }
+    /*@_spi(Testing)*/ public var fingerprint: String? { keyAndFingerprint.fingerprint }
 
     /// The dependencySource file that holds this entity iff the entities .swiftdeps (or in future, .swiftmodule) is known.
     /// If more than one source file has the same DependencyKey, then there
@@ -59,23 +64,28 @@ extension ModuleDependencyGraph {
   }
 }
 
+// MARK: - Setting fingerprint
+extension ModuleDependencyGraph.Node {
+  func setFingerprint(_ newFP: String?) {
+    keyAndFingerprint = try! KeyAndFingerprintHolder(key, newFP)
+  }
+}
+
 // MARK: - trace status
 extension ModuleDependencyGraph.Node {
   var isUntraced: Bool { !isTraced }
   func setTraced() { isTraced = true }
-  func setUntraced() { isTraced = false }
+  @_spi(Testing) public func setUntraced() { isTraced = false }
 }
 
 // MARK: - comparing, hashing
 extension ModuleDependencyGraph.Node: Equatable, Hashable {
-  public static func == (lhs: Graph.Node, rhs: Graph.Node) -> Bool {
-    lhs.key == rhs.key && lhs.fingerprint == rhs.fingerprint
-      && lhs.dependencySource == rhs.dependencySource
+  public static func ==(lhs: ModuleDependencyGraph.Node, rhs: ModuleDependencyGraph.Node) -> Bool {
+    lhs.keyAndFingerprint.key == rhs.keyAndFingerprint.key &&
+    lhs.dependencySource == rhs.dependencySource
   }
-  
   public func hash(into hasher: inout Hasher) {
-    hasher.combine(key)
-    hasher.combine(fingerprint)
+    hasher.combine(keyAndFingerprint.key)
     hasher.combine(dependencySource)
   }
 }

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/NodeFinder.swift

@@ -173,6 +173,7 @@ extension ModuleDependencyGraph.NodeFinder {
     let replacement = Graph.Node(key: original.key,
                                  fingerprint: newFingerprint,
                                  dependencySource: newDependencySource)
+    assert(original.isExpat, "Would have to search every use in usesByDef if original could be a use.")
     if usesByDef.removeValue(original, forKey: original.key) != nil {
       usesByDef.insertValue(replacement, forKey: original.key)
     }

Sources/SwiftDriver/IncrementalCompilation/SourceFileDependencyGraph.swift

@@ -58,7 +58,7 @@ import TSCUtility
 }
 
 extension SourceFileDependencyGraph {
-  public struct Node {
+  public struct Node: CustomStringConvertible {
     public let keyAndFingerprint: KeyAndFingerprintHolder
     public var key: DependencyKey { keyAndFingerprint.key }
     public var fingerprint: String? { keyAndFingerprint.fingerprint }
@@ -92,6 +92,17 @@ extension SourceFileDependencyGraph {
         }
       }
     }
+
+    public var description: String {
+      [
+        key.description,
+        fingerprint.map {"fingerprint: \($0.description)"},
+        isProvides ? "provides" : "depends",
+        defsIDependUpon.isEmpty ? nil : "depends on \(defsIDependUpon.count)"
+      ]
+        .compactMap{$0}
+        .joined(separator: ", ")
+    }
   }
 }
 

Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift

@@ -29,6 +29,24 @@ extension ModuleDependencyGraph {
     }
     return false
   }
+
+  func setUntraced() {
+    nodeFinder.forEachNode {
+      $0.setUntraced()
+    }
+  }
+
+  func ensureIsSerializable() {
+    var nodeIDs = Set<Node>()
+    nodeFinder.forEachNode { node in
+      nodeIDs.insert(node)
+    }
+    for key in nodeFinder.usesByDef.keys {
+      for use in nodeFinder.usesByDef[key, default: []] {
+        XCTAssertTrue(nodeIDs.contains(use), "Node ID was not registered! \(use), \(String(describing: use.fingerprint))")
+      }
+    }
+  }
 }
 
 // MARK: - mocking

Tests/SwiftDriverTests/ModuleDependencyGraphTests.swift

@@ -752,7 +752,7 @@ class ModuleDependencyGraphTests: XCTestCase {
 
   func testLoadPassesWithFingerprint() {
     let graph = ModuleDependencyGraph(mock: de)
-    _ = graph.getInvalidatedSourcesForSimulatedLoad(
+    _ = graph.getInvalidatedNodesForSimulatedLoad(
       0,
       [MockDependencyKind.nominal: ["A@1"]],
       includeAddedExternals: false)
@@ -781,6 +781,35 @@ class ModuleDependencyGraphTests: XCTestCase {
     }
   }
 
+  func testUseFingerprintsPingPong() {
+    let graph = ModuleDependencyGraph(mock: de)
+    // Because of the cross-type dependency, A->B,
+    // when A changes, only B is dirtied in 1.
+
+    graph.simulateLoad(0, [.nominal: ["A@1"]])
+    graph.simulateLoad(1, [.nominal: ["B", "A->B"]])
+    graph.ensureIsSerializable()
+
+    do {
+      let swiftDeps = graph.simulateReload(0, [.nominal: ["A@2"]]).sorted()
+      XCTAssertEqual(swiftDeps, [0, 1])
+      graph.ensureIsSerializable()
+    }
+
+    do {
+      // In the real world, we would have a graph with untraced nodes from the
+      // priors, remembering the fingerprints from before.
+      graph.setUntraced()
+      // When the driver integrates a node that preexists but has a new fingerprint
+      // it must change that fingerprint in the node in the graph.
+      // If it does not, and subsequently integrates the old fingerprint,
+      // the change will be missed.
+      let swiftDeps = graph.simulateReload(0, [.nominal: ["A@1"]]).sorted()
+      XCTAssertEqual(swiftDeps, [0, 1])
+      graph.ensureIsSerializable()
+    }
+  }
+
   func testCrossTypeDependencyBaseline() {
     let graph = ModuleDependencyGraph(mock: de)
     graph.simulateLoad(0, [.nominal: ["A"]])
@@ -894,7 +923,7 @@ extension ModuleDependencyGraph {
     includePrivateDeps: Bool = true,
     hadCompilationError: Bool = false)
   {
-    _ = getInvalidatedSourcesForSimulatedLoad(
+    _ = getInvalidatedNodesForSimulatedLoad(
       swiftDepsIndex, dependencyDescriptions,
       includeAddedExternals: false,
       interfaceHash,
@@ -909,7 +938,7 @@ extension ModuleDependencyGraph {
                       hadCompilationError: Bool = false)
   -> [Int]
   {
-    let invalidatedNodes = getInvalidatedSourcesForSimulatedLoad(
+    let invalidatedNodes = getInvalidatedNodesForSimulatedLoad(
       swiftDepsIndex,
       dependencyDescriptions,
       includeAddedExternals: true,
@@ -922,7 +951,7 @@ extension ModuleDependencyGraph {
   }
 
 
-  func getInvalidatedSourcesForSimulatedLoad(
+  func getInvalidatedNodesForSimulatedLoad(
     _ swiftDepsIndex: Int,
     _ dependencyDescriptions: [MockDependencyKind: [String]],
     includeAddedExternals: Bool,