[Driver] Make sure to rebuild dependents when a dirty file fails. (#3935)

Otherwise we get into a situation like this:

1. Change made to the interface of file A.swift that also causes an
   error in A.swift.
2. Fixing the error in A.swift does not affect A.swift's interface.
3. File B.swift that depends on A.swift is not rebuilt, since the most
   recent change to A.swift did not change its interface.

To fix this, mark downstream files as needing to be rebuilt even when
a compilation job fails with errors. Additionally, attempt to be extra
conservative when there's a crash.

rdar://problem/25405605

Author: jrose-apple

lib/Driver/Compilation.cpp

@@ -456,6 +456,70 @@ int Compilation::performJobsImpl() {
         llvm::errs() << Output;
     }
 
+    // In order to handle both old dependencies that have disappeared and new
+    // dependencies that have arisen, we need to reload the dependency file.
+    // Do this whether or not the build succeeded.
+    SmallVector<const Job *, 16> Dependents;
+    if (getIncrementalBuildEnabled()) {
+      const CommandOutput &Output = FinishedCmd->getOutput();
+      StringRef DependenciesFile =
+        Output.getAdditionalOutputForType(types::TY_SwiftDeps);
+      if (!DependenciesFile.empty() &&
+          (ReturnCode == EXIT_SUCCESS || ReturnCode == EXIT_FAILURE)) {
+        bool wasCascading = DepGraph.isMarked(FinishedCmd);
+
+        switch (DepGraph.loadFromPath(FinishedCmd, DependenciesFile)) {
+        case DependencyGraphImpl::LoadResult::HadError:
+          if (ReturnCode == EXIT_SUCCESS) {
+            disableIncrementalBuild();
+            for (const Job *Cmd : DeferredCommands)
+              scheduleCommandIfNecessaryAndPossible(Cmd);
+            DeferredCommands.clear();
+            Dependents.clear();
+          } // else, let the next build handle it.
+          break;
+        case DependencyGraphImpl::LoadResult::UpToDate:
+          if (!wasCascading)
+            break;
+          SWIFT_FALLTHROUGH;
+        case DependencyGraphImpl::LoadResult::AffectsDownstream:
+            llvm::errs() << "DOWNSTREAM " << ReturnCode << "\n";
+          DepGraph.markTransitive(Dependents, FinishedCmd);
+          break;
+        }
+      } else {
+        // If there's a crash, assume the worst.
+        switch (FinishedCmd->getCondition()) {
+        case Job::Condition::NewlyAdded:
+          // The job won't be treated as newly added next time. Conservatively
+          // mark it as affecting other jobs, because some of them may have
+          // completed already.
+          DepGraph.markTransitive(Dependents, FinishedCmd);
+          break;
+        case Job::Condition::Always:
+          // This applies to non-incremental tasks as well, but any incremental
+          // task that shows up here has already been marked.
+          break;
+        case Job::Condition::RunWithoutCascading:
+          // If this file changed, it might have been a non-cascading change and
+          // it might not. Unfortunately, the interface hash has been updated or
+          // compromised, so we don't actually know anymore; we have to
+          // conservatively assume the changes could affect other files.
+          DepGraph.markTransitive(Dependents, FinishedCmd);
+          break;
+        case Job::Condition::CheckDependencies:
+          // If the only reason we're running this is because something else
+          // changed, then we can trust the dependency graph as to whether it's
+          // a cascading or non-cascading change. That is, if whatever /caused/
+          // the error isn't supposed to affect other files, and whatever
+          // /fixes/ the error isn't supposed to affect other files, then
+          // there's no need to recompile any other inputs. If either of those
+          // are false, we /do/ need to recompile other inputs.
+          break;
+        }
+      }
+    }
+
     if (ReturnCode != EXIT_SUCCESS) {
       // The task failed, so return true without performing any further
       // dependency analysis.
@@ -481,39 +545,10 @@ int Compilation::performJobsImpl() {
     // might have been blocked.
     markFinished(FinishedCmd);
 
-    // In order to handle both old dependencies that have disappeared and new
-    // dependencies that have arisen, we need to reload the dependency file.
-    if (getIncrementalBuildEnabled()) {
-      const CommandOutput &Output = FinishedCmd->getOutput();
-      StringRef DependenciesFile =
-        Output.getAdditionalOutputForType(types::TY_SwiftDeps);
-      if (!DependenciesFile.empty()) {
-        SmallVector<const Job *, 16> Dependents;
-        bool wasCascading = DepGraph.isMarked(FinishedCmd);
-
-        switch (DepGraph.loadFromPath(FinishedCmd, DependenciesFile)) {
-        case DependencyGraphImpl::LoadResult::HadError:
-          disableIncrementalBuild();
-          for (const Job *Cmd : DeferredCommands)
-            scheduleCommandIfNecessaryAndPossible(Cmd);
-          DeferredCommands.clear();
-          Dependents.clear();
-          break;
-        case DependencyGraphImpl::LoadResult::UpToDate:
-          if (!wasCascading)
-            break;
-          SWIFT_FALLTHROUGH;
-        case DependencyGraphImpl::LoadResult::AffectsDownstream:
-          DepGraph.markTransitive(Dependents, FinishedCmd);
-          break;
-        }
-
-        for (const Job *Cmd : Dependents) {
-          DeferredCommands.erase(Cmd);
-          noteBuilding(Cmd, "because of dependencies discovered later");
-          scheduleCommandIfNecessaryAndPossible(Cmd);
-        }
-      }
+    for (const Job *Cmd : Dependents) {
+      DeferredCommands.erase(Cmd);
+      noteBuilding(Cmd, "because of dependencies discovered later");
+      scheduleCommandIfNecessaryAndPossible(Cmd);
     }
 
     return TaskFinishedResponse::ContinueExecution;

lib/Driver/Driver.cpp

@@ -1604,36 +1604,40 @@ handleCompileJobCondition(Job *J, CompileJobAction::InputInfo inputInfo,
     return;
   }
 
-  if (!alwaysRebuildDependents) {
-    // Default all non-newly added files to being rebuilt without cascading.
-    J->setCondition(Job::Condition::RunWithoutCascading);
-  }
-
+  bool hasValidModTime = false;
   llvm::sys::fs::file_status inputStatus;
-  if (llvm::sys::fs::status(input, inputStatus))
-    return;
-
-  J->setInputModTime(inputStatus.getLastModificationTime());
-  if (J->getInputModTime() != inputInfo.previousModTime)
-    return;
+  if (!llvm::sys::fs::status(input, inputStatus)) {
+    J->setInputModTime(inputStatus.getLastModificationTime());
+    hasValidModTime = true;
+  }
 
   Job::Condition condition;
-  switch (inputInfo.status) {
-  case CompileJobAction::InputInfo::UpToDate:
-    if (!llvm::sys::fs::exists(J->getOutput().getPrimaryOutputFilename()))
+  if (!hasValidModTime || J->getInputModTime() != inputInfo.previousModTime) {
+    if (alwaysRebuildDependents ||
+        inputInfo.status == CompileJobAction::InputInfo::NeedsCascadingBuild) {
+      condition = Job::Condition::Always;
+    } else {
       condition = Job::Condition::RunWithoutCascading;
-    else
-      condition = Job::Condition::CheckDependencies;
-    break;
-  case CompileJobAction::InputInfo::NeedsCascadingBuild:
-    condition = Job::Condition::Always;
-    break;
-  case CompileJobAction::InputInfo::NeedsNonCascadingBuild:
-    condition = Job::Condition::RunWithoutCascading;
-    break;
-  case CompileJobAction::InputInfo::NewlyAdded:
-    llvm_unreachable("handled above");
+    }
+  } else {
+    switch (inputInfo.status) {
+    case CompileJobAction::InputInfo::UpToDate:
+      if (!llvm::sys::fs::exists(J->getOutput().getPrimaryOutputFilename()))
+        condition = Job::Condition::RunWithoutCascading;
+      else
+        condition = Job::Condition::CheckDependencies;
+      break;
+    case CompileJobAction::InputInfo::NeedsCascadingBuild:
+      condition = Job::Condition::Always;
+      break;
+    case CompileJobAction::InputInfo::NeedsNonCascadingBuild:
+      condition = Job::Condition::RunWithoutCascading;
+      break;
+    case CompileJobAction::InputInfo::NewlyAdded:
+      llvm_unreachable("handled above");
+    }
   }
+
   J->setCondition(condition);
 }
 

test/Driver/Dependencies/Inputs/crash-simple/crash.swift

@@ -0,0 +1,2 @@
+# Dependencies after compilation:
+provides-top-level: [a]

test/Driver/Dependencies/Inputs/crash-simple/main.swift

@@ -0,0 +1,2 @@
+# Dependencies after compilation:
+depends-top-level: [a]

test/Driver/Dependencies/Inputs/crash-simple/other.swift

@@ -0,0 +1,2 @@
+# Dependencies after compilation:
+depends-top-level: [!private a]

test/Driver/Dependencies/Inputs/crash-simple/output.json

@@ -0,0 +1,17 @@
+{
+  "./main.swift": {
+    "object": "./main.o",
+    "swift-dependencies": "./main.swiftdeps"
+  },
+  "./crash.swift": {
+    "object": "./crash.o",
+    "swift-dependencies": "./crash.swiftdeps"
+  },
+  "./other.swift": {
+    "object": "./other.o",
+    "swift-dependencies": "./other.swiftdeps"
+  },
+  "": {
+    "swift-dependencies": "./main~buildrecord.swiftdeps"
+  }  
+}

test/Driver/Dependencies/Inputs/fail-with-bad-deps/bad.swift

@@ -0,0 +1,4 @@
+# Dependencies after compilation:
+provides-top-level: [bad]
+interface-hash: "after"
+garbage: ""

test/Driver/Dependencies/Inputs/fail-with-bad-deps/bad.swiftdeps

@@ -0,0 +1,3 @@
+# Dependencies before compilation:
+provides-top-level: [bad]
+interface-hash: "before"

test/Driver/Dependencies/Inputs/fail-with-bad-deps/depends-on-bad.swift

@@ -0,0 +1,3 @@
+# Dependencies after compilation:
+depends-top-level: [bad]
+interface-hash: "after"

test/Driver/Dependencies/Inputs/fail-with-bad-deps/depends-on-bad.swiftdeps

@@ -0,0 +1,3 @@
+# Dependencies before compilation:
+depends-top-level: [bad]
+interface-hash: "before"