[Explicit Module Builds][Incremental Builds] Unify logic used to check if a prior inter-module dep graph is up-to-date with a check to decide which modules to re-build

Using the same 'computeInvalidatedModuleDependencies' routine, which is more thorough and checks module inputs to each dependency as well.

Author: artemcm

Sources/SwiftDriver/ExplicitModuleBuilds/InterModuleDependencies/CommonDependencyOperations.swift

@@ -294,7 +294,6 @@ internal extension InterModuleDependencyGraph {
     let sourceModuleInfo = try moduleInfo(of: sourceModuleId)
     // Visit the module's dependencies
     var hasOutOfDateModuleDependency = false
-    var mostRecentlyUpdatedDependencyOutput: TimePoint = .zero
     for dependencyId in sourceModuleInfo.directDependencies ?? [] {
       // If we have not already visited this module, recurse.
       if !visited.contains(dependencyId) {
@@ -304,32 +303,101 @@ internal extension InterModuleDependencyGraph {
       }
       // Even if we're not revisiting a dependency, we must check if it's already known to be out of date.
       hasOutOfDateModuleDependency = hasOutOfDateModuleDependency || modulesRequiringRebuild.contains(dependencyId)
-
-      // Keep track of dependencies' output file time stamp to determine if it is newer than the current module.
-      if let depOutputTimeStamp = try? fileSystem.lastModificationTime(for: VirtualPath.lookup(moduleInfo(of: dependencyId).modulePath.path)),
-         depOutputTimeStamp > mostRecentlyUpdatedDependencyOutput {
-        mostRecentlyUpdatedDependencyOutput = depOutputTimeStamp
-      }
     }
 
     if hasOutOfDateModuleDependency {
       reporter?.reportExplicitDependencyWillBeReBuilt(sourceModuleId.moduleNameForDiagnostic, reason: "Invalidated by downstream dependency")
       modulesRequiringRebuild.insert(sourceModuleId)
-    } else if try !IncrementalCompilationState.IncrementalDependencyAndInputSetup.verifyModuleDependencyUpToDate(moduleID: sourceModuleId, moduleInfo: sourceModuleInfo,
-                                                                                                                 fileSystem: fileSystem, reporter: reporter) {
+    } else if try !verifyModuleDependencyUpToDate(moduleID: sourceModuleId, fileSystem: fileSystem, reporter: reporter) {
       reporter?.reportExplicitDependencyWillBeReBuilt(sourceModuleId.moduleNameForDiagnostic, reason: "Out-of-date")
       modulesRequiringRebuild.insert(sourceModuleId)
-    } else if let outputModTime = try? fileSystem.lastModificationTime(for: VirtualPath.lookup(sourceModuleInfo.modulePath.path)),
-              outputModTime < mostRecentlyUpdatedDependencyOutput {
-      // If a prior variant of this module dependnecy exists, and is older than any of its direct or transitive
-      // module dependency outputs, it must also be re-built.
-      reporter?.reportExplicitDependencyWillBeReBuilt(sourceModuleId.moduleNameForDiagnostic, reason: "Has newer module dependency inputs")
-      modulesRequiringRebuild.insert(sourceModuleId)
     }
 
     // Now that we've determined if this module must be rebuilt, mark it as visited.
     visited.insert(sourceModuleId)
   }
+
+  func verifyModuleDependencyUpToDate(moduleID: ModuleDependencyId,
+                                      fileSystem: FileSystem,
+                                      reporter: IncrementalCompilationState.Reporter?) throws -> Bool {
+    let checkedModuleInfo = try moduleInfo(of: moduleID)
+    // Verify that the specified input exists and is older than the specified output
+    let verifyInputOlderThanOutputModTime: (String, VirtualPath, TimePoint) -> Bool =
+    { moduleName, inputPath, outputModTime in
+      guard let inputModTime =
+              try? fileSystem.lastModificationTime(for: inputPath) else {
+        reporter?.report("Unable to 'stat' \(inputPath.description)")
+        return false
+      }
+      if inputModTime > outputModTime {
+        reporter?.reportExplicitDependencyOutOfDate(moduleName,
+                                                    inputPath: inputPath.description)
+        return false
+      }
+      return true
+    }
+
+    // Check if the output file exists
+    guard let outputModTime = try? fileSystem.lastModificationTime(for: VirtualPath.lookup(checkedModuleInfo.modulePath.path)) else {
+      reporter?.report("Module output not found: '\(moduleID.moduleNameForDiagnostic)'")
+      return false
+    }
+
+    // Check if a dependency of this module has a newer output than this module
+    for dependencyId in checkedModuleInfo.directDependencies ?? [] {
+      let dependencyInfo = try moduleInfo(of: dependencyId)
+      if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
+                                            VirtualPath.lookup(dependencyInfo.modulePath.path),
+                                            outputModTime) {
+        return false
+      }
+    }
+
+    // Check if any of the textual sources of this module are newer than this module
+    switch checkedModuleInfo.details {
+    case .swift(let swiftDetails):
+      if let moduleInterfacePath = swiftDetails.moduleInterfacePath {
+        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
+                                              VirtualPath.lookup(moduleInterfacePath.path),
+                                              outputModTime) {
+          return false
+        }
+      }
+      if let bridgingHeaderPath = swiftDetails.bridgingHeaderPath {
+        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
+                                              VirtualPath.lookup(bridgingHeaderPath.path),
+                                              outputModTime) {
+          return false
+        }
+      }
+      for bridgingSourceFile in swiftDetails.bridgingSourceFiles ?? [] {
+        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
+                                              VirtualPath.lookup(bridgingSourceFile.path),
+                                              outputModTime) {
+          return false
+        }
+      }
+    case .clang(_):
+      for inputSourceFile in checkedModuleInfo.sourceFiles ?? [] {
+        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
+                                              try VirtualPath(path: inputSourceFile),
+                                              outputModTime) {
+          return false
+        }
+      }
+    case .swiftPrebuiltExternal(_):
+      // TODO: We have to give-up here until we have a way to verify the timestamp of the binary module.
+      // We can do better here by knowing if this module hasn't changed - which would allows us to not
+      // invalidate any of the dependencies that depend on it.
+      reporter?.report("Unable to verify binary module dependency up-to-date: \(moduleID.moduleNameForDiagnostic)")
+      return false;
+    case .swiftPlaceholder(_):
+      // TODO: This should never ever happen. Hard error?
+      return false;
+    }
+
+    return true
+  }
 }
 
 internal extension InterModuleDependencyGraph {

Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift

@@ -155,7 +155,8 @@ extension IncrementalCompilationState.FirstWaveComputer {
 
     // Determine which module pre-build jobs must be re-run
     let modulesRequiringReBuild =
-      try moduleDependencyGraph.computeInvalidatedModuleDependencies(fileSystem: fileSystem, reporter: reporter)
+      try moduleDependencyGraph.computeInvalidatedModuleDependencies(fileSystem: fileSystem, 
+                                                                     reporter: reporter)
 
     // Filter the `.generatePCM` and `.compileModuleFromInterface` jobs for 
     // modules which do *not* need re-building.

Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift

@@ -120,109 +120,14 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
     }
 
     // Verify that each dependnecy is up-to-date with respect to its inputs
-    guard try verifyInterModuleDependenciesUpToDate(in: priorInterModuleDependencyGraph,
-                                                    buildRecordInfo: buildRecordInfo,
-                                                    reporter: reporter) else {
+    guard try priorInterModuleDependencyGraph.computeInvalidatedModuleDependencies(fileSystem: buildRecordInfo.fileSystem).isEmpty else {
       reporter?.reportExplicitBuildMustReScan("Not all dependencies are up-to-date.")
       return nil
     }
 
     reporter?.report("Confirmed prior inter-module dependency graph is up-to-date at: \(buildRecordInfo.interModuleDependencyGraphPath)")
     return priorInterModuleDependencyGraph
   }
-
-  static func verifyModuleDependencyUpToDate(moduleID: ModuleDependencyId, moduleInfo: ModuleInfo,
-                                             fileSystem: FileSystem,
-                                             reporter: IncrementalCompilationState.Reporter?) throws -> Bool {
-    // Verify that the specified input exists and is older than the specified output
-    let verifyInputOlderThanOutputModTime: (String, VirtualPath, VirtualPath, TimePoint) -> Bool =
-    { moduleName, inputPath, outputPath, outputModTime in
-      guard let inputModTime =
-              try? fileSystem.lastModificationTime(for: inputPath) else {
-        reporter?.report("Unable to 'stat' \(inputPath.description)")
-        return false
-      }
-      if inputModTime > outputModTime {
-        reporter?.reportExplicitDependencyOutOfDate(moduleName,
-                                                    inputPath: inputPath.description)
-        return false
-      }
-      return true
-    }
-
-    switch moduleInfo.details {
-    case .swift(let swiftDetails):
-      guard let outputModTime = try? fileSystem.lastModificationTime(for: VirtualPath.lookup(moduleInfo.modulePath.path)) else {
-        reporter?.report("Module output not found: '\(moduleID.moduleNameForDiagnostic)'")
-        return false
-      }
-      if let moduleInterfacePath = swiftDetails.moduleInterfacePath {
-        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
-                                              VirtualPath.lookup(moduleInterfacePath.path),
-                                              VirtualPath.lookup(moduleInfo.modulePath.path),
-                                              outputModTime) {
-          return false
-        }
-      }
-      if let bridgingHeaderPath = swiftDetails.bridgingHeaderPath {
-        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
-                                              VirtualPath.lookup(bridgingHeaderPath.path),
-                                              VirtualPath.lookup(moduleInfo.modulePath.path),
-                                              outputModTime) {
-          return false
-        }
-      }
-      for bridgingSourceFile in swiftDetails.bridgingSourceFiles ?? [] {
-        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
-                                              VirtualPath.lookup(bridgingSourceFile.path),
-                                              VirtualPath.lookup(moduleInfo.modulePath.path),
-                                              outputModTime) {
-          return false
-        }
-      }
-    case .clang(_):
-      guard let outputModTime = try? fileSystem.lastModificationTime(for: VirtualPath.lookup(moduleInfo.modulePath.path)) else {
-        reporter?.report("Module output not found: '\(moduleID.moduleNameForDiagnostic)'")
-        return false
-      }
-      for inputSourceFile in moduleInfo.sourceFiles ?? [] {
-        if !verifyInputOlderThanOutputModTime(moduleID.moduleName,
-                                              try VirtualPath(path: inputSourceFile),
-                                              VirtualPath.lookup(moduleInfo.modulePath.path),
-                                              outputModTime) {
-          return false
-        }
-      }
-    case .swiftPrebuiltExternal(_):
-      // TODO: We have to give-up here until we have a way to verify the timestamp of the binary module.
-      // We can do better here by knowing if this module hasn't change - which would allows us to not
-      // invalidate any of the dependencies that depend on it.
-      reporter?.report("Unable to verify binary module dependency up-to-date: \(moduleID.moduleNameForDiagnostic)")
-      return false;
-    case .swiftPlaceholder(_):
-      // TODO: This should never ever happen. Hard error?
-      return false;
-    }
-    return true
-  }
-
-  /// For each direct and transitive module dependency, check if any of the inputs are newer than the output
-  static func verifyInterModuleDependenciesUpToDate(in graph: InterModuleDependencyGraph,
-                                                    buildRecordInfo: BuildRecordInfo,
-                                                    reporter: IncrementalCompilationState.Reporter?) throws -> Bool {
-    for module in graph.modules {
-      if module.key == .swift(graph.mainModuleName) {
-        continue
-      }
-      if try !verifyModuleDependencyUpToDate(moduleID: module.key, 
-                                             moduleInfo: module.value,
-                                             fileSystem: buildRecordInfo.fileSystem,
-                                             reporter: reporter) {
-        return false
-      }
-    }
-    return true
-  }
 }
 
 /// Builds the `InitialState`

Tests/SwiftDriverTests/IncrementalCompilationTests.swift

@@ -452,9 +452,9 @@ extension IncrementalCompilationTests {
     let nameOfGModule = try XCTUnwrap(modCacheEntries.first { $0.hasPrefix("G") && $0.hasSuffix(".swiftmodule")})
     let pathToGModule = explicitModuleCacheDir.appending(component: nameOfGModule)
     // Just update the time-stamp of one of the module dependencies' outputs.
-    // Also add a dependency to cause a re-scan.
     touch(pathToGModule)
-    replace(contentsOf: "other", with: "import J;import R")
+    // Touch one of the inputs to actually trigger the incremental build
+    touch(inputPath(basename: "other"))
 
     // Changing a dependency will mean that we both re-run the dependency scan,
     // and also ensure that all source-files are re-built with a non-cascading build
@@ -468,19 +468,16 @@ extension IncrementalCompilationTests {
       readGraph
       enablingCrossModule
       readInterModuleGraph
-      explicitMustReScanDueToChangedImports
+      explicitMustReScanDueToChangedDependencyInput
       maySkip("main")
       schedulingChangedInitialQueuing("other")
       skipping("main")
+      explicitDependencyModuleOlderThanInput("J")
+      moduleWillBeRebuiltOutOfDate("J")
+      explicitModulesWillBeRebuilt(["J"])
+      compilingExplicitSwiftDependency("J")
       findingBatchingCompiling("other")
       reading(deps: "other")
-      fingerprintsChanged("other")
-      moduleOutputNotFound("R")
-      moduleWillBeRebuiltOutOfDate("R")
-      compilingExplicitSwiftDependency("R")
-      explicitModulesWillBeRebuilt(["J", "R"])
-      explicitDependencyNewerModuleInputs("J")
-      compilingExplicitSwiftDependency("J")
       skipped("main")
       schedulingPostCompileJobs
       linking
@@ -1700,9 +1697,6 @@ extension DiagVerifiable {
   @DiagsBuilder func explicitDependencyInvalidatedDownstream(_ moduleName: String) -> [Diagnostic.Message] {
     "Incremental compilation: Dependency module '\(moduleName)' will be re-built: Invalidated by downstream dependency"
   }
-  @DiagsBuilder func explicitDependencyNewerModuleInputs(_ moduleName: String) -> [Diagnostic.Message] {
-    "Incremental compilation: Dependency module '\(moduleName)' will be re-built: Has newer module dependency inputs"
-  }
 
   // MARK: - misc
   @DiagsBuilder var enablingCrossModule: [Diagnostic.Message] {