Merge pull request swiftlang#1846 from swiftlang/owenv/ebm-verify-incremental-fix

Ensure we don't skip precompile jobs if a planned interface verification depends on recompiling modules

Author: owenv

Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift

@@ -139,9 +139,11 @@ extension IncrementalCompilationState.FirstWaveComputer {
   private func nonVerifyAfterCompileJobsDependOnBeforeCompileJobs() -> Bool {
     let beforeCompileJobOutputs = jobsInPhases.beforeCompiles.reduce(into: Set<TypedVirtualPath>(),
                                                                      { (pathSet, job) in pathSet.formUnion(job.outputs) })
-    let afterCompilesDependnigJobs = jobsInPhases.afterCompiles.filter {postCompileJob in postCompileJob.inputs.contains(where: beforeCompileJobOutputs.contains)}
-    if afterCompilesDependnigJobs.isEmpty || afterCompilesDependnigJobs.allSatisfy({ $0.kind == .verifyModuleInterface }) {
+    let afterCompilesDependingJobs = jobsInPhases.afterCompiles.filter {postCompileJob in postCompileJob.inputs.contains(where: beforeCompileJobOutputs.contains)}
+    if afterCompilesDependingJobs.isEmpty {
       return false
+    } else if afterCompilesDependingJobs.allSatisfy({ $0.kind == .verifyModuleInterface }) {
+      return jobsInPhases.beforeCompiles.contains { $0.kind == .generatePCM || $0.kind == .compileModuleFromInterface }
     } else {
       return true
     }

Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift

@@ -199,10 +199,12 @@ extension Driver {
     try commandLine.appendLast(.AssertConfig, from: &parsedOptions)
     try commandLine.appendLast(.autolinkForceLoad, from: &parsedOptions)
 
-    if let colorOption = parsedOptions.last(for: .colorDiagnostics, .noColorDiagnostics) {
-      commandLine.appendFlag(colorOption.option)
-    } else if shouldColorDiagnostics() {
+    if parsedOptions.hasFlag(positive: .colorDiagnostics, negative: .noColorDiagnostics, default: shouldColorDiagnostics()) {
       commandLine.appendFlag(.colorDiagnostics)
+      appendXccFlag("-fcolor-diagnostics")
+    } else {
+      commandLine.appendFlag(.noColorDiagnostics)
+      appendXccFlag("-fno-color-diagnostics")
     }
     try commandLine.appendLast(.fixitAll, from: &parsedOptions)
     try commandLine.appendLast(.warnSwift3ObjcInferenceMinimal, .warnSwift3ObjcInferenceComplete, from: &parsedOptions)

Tests/SwiftDriverTests/ExplicitModuleBuildTests.swift

@@ -773,6 +773,79 @@ final class ExplicitModuleBuildTests: XCTestCase {
     }
   }
 
+  // This is a regression test for the following scenario:
+  // 1. The user does a clean build of their project with explicit modules
+  // 2. The user starts a subsequent build using changed flags/environment which do not affect incremental builds (e.g. color diagnostics)
+  // 3. That flag/environment change does affect PCM hashes returned by the clang dependency scanner.
+  // 4. The driver determines that modules need to rebuild, sources don't need to rebuild, and verification needs to re-run.
+  // 5. The driver decides it's safe to skip the pre-compile jobs (the module builds) because the only post-compile jobs are for verification.
+  // 6. Verification fails because the modules with the new PCM hashes are missing.
+  // Ideally, this should never happen. If a flag doesn't invalidate Swift compilation, it shouldn;t be impacting module hashes either. But I
+  // think we should be resilient to this by rebuilding the modules in this scenario.
+  //
+  // The below test is somewhat fragile in that it will be redundant if we canonicalize away -fcolor-diagnostics in clang, but it's useful to ensure
+  // end to end behavior does not regress again.
+  func testExplicitModuleBuildDoesNotSkipPrecompiledModulesWhenOnlyVerificationIsInvalidated() throws {
+    try withTemporaryDirectory { path in
+      try localFileSystem.changeCurrentWorkingDirectory(to: path)
+      let moduleCachePath = path.appending(component: "ModuleCache")
+      try localFileSystem.createDirectory(moduleCachePath)
+      let main = path.appending(component: "testExplicitModuleBuildEndToEnd.swift")
+      try localFileSystem.writeFileContents(main, bytes:
+        """
+        import C;
+        import E;
+        import G;
+
+        func foo() {
+          funcE()
+        }
+        """
+      )
+      let outputFileMap = path.appending(component: "output-file-map.json")
+      try localFileSystem.writeFileContents(outputFileMap, bytes: ByteString(encodingAsUTF8: """
+      {
+        "": {
+          "swift-dependencies": "\(path.appending(component: "main.swiftdeps").nativePathString(escaped: true))"
+        },
+        "\(path.appending(component: "testExplicitModuleBuildEndToEnd.swift").nativePathString(escaped: true))": {
+          "swift-dependencies": "\(path.appending(component: "testExplicitModuleBuildEndToEnd.swiftdeps").nativePathString(escaped: true))",
+          "object": "\(path.appending(component: "testExplicitModuleBuildEndToEnd.o").nativePathString(escaped: true))"
+        }
+      }
+      """))
+
+      let cHeadersPath: AbsolutePath =
+          try testInputsPath.appending(component: "ExplicitModuleBuilds")
+                            .appending(component: "CHeaders")
+      let swiftModuleInterfacesPath: AbsolutePath =
+          try testInputsPath.appending(component: "ExplicitModuleBuilds")
+                            .appending(component: "Swift")
+      let sdkArgumentsForTesting = (try? Driver.sdkArgumentsForTesting()) ?? []
+      let invocationArguments = ["swiftc",
+                                 "-incremental", "-c",
+                                 "-emit-module",
+                                 "-enable-library-evolution", "-emit-module-interface", "-driver-show-incremental", "-v",
+                                 "-Xcc", "-Xclang", "-Xcc", "-fbuiltin-headers-in-system-modules",
+                                 "-I", cHeadersPath.nativePathString(escaped: true),
+                                 "-I", swiftModuleInterfacesPath.nativePathString(escaped: true),
+                                 "-explicit-module-build",
+                                 "-output-file-map", outputFileMap.nativePathString(escaped: true),
+                                 "-module-cache-path", moduleCachePath.nativePathString(escaped: true),
+                                 "-working-directory", path.nativePathString(escaped: true),
+                                 main.nativePathString(escaped: true)] + sdkArgumentsForTesting
+      var driver = try Driver(args: invocationArguments)
+      let jobs = try driver.planBuild()
+      try driver.run(jobs: jobs)
+      XCTAssertFalse(driver.diagnosticEngine.hasErrors)
+
+      var incrementalDriver = try Driver(args: invocationArguments + ["-color-diagnostics"])
+      let incrementalJobs = try incrementalDriver.planBuild()
+      try incrementalDriver.run(jobs: incrementalJobs)
+      XCTAssertFalse(incrementalDriver.diagnosticEngine.hasErrors)
+    }
+  }
+
   /// Test generation of explicit module build jobs for dependency modules when the driver
   /// is invoked with -explicit-module-build, -verify-emitted-module-interface and -enable-library-evolution.
   func testExplicitModuleVerifyInterfaceJobs() throws {