[Driver] Address Brian G's feedback on 848b3eb. (#4046)

No functionality change.

Author: jrose-apple

lib/Driver/Compilation.cpp

@@ -464,57 +464,72 @@ int Compilation::performJobsImpl() {
       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:
-          DepGraph.markTransitive(Dependents, FinishedCmd);
-          break;
-        }
+      if (DependenciesFile.empty()) {
+        // If this job doesn't track dependencies, it must always be run.
+        // Note: In theory CheckDependencies makes sense as well (for a leaf
+        // node in the dependency graph), and maybe even NewlyAdded (for very
+        // coarse dependencies that always affect downstream nodes), but we're
+        // not using either of those right now, and this logic should probably
+        // be revisited when we are.
+        assert(FinishedCmd->getCondition() == Job::Condition::Always);
       } 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 we have a dependency file /and/ the frontend task exited normally,
+        // we can be discerning about what downstream files to rebuild.
+        if (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:
+            DepGraph.markTransitive(Dependents, FinishedCmd);
+            break;
+          }
+        } else {
+          // If there's an abnormal exit (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:
+            // Any incremental task that shows up here has already been marked;
+            // we didn't need to wait for it to finish to start downstream
+            // tasks.
+            assert(DepGraph.isMarked(FinishedCmd));
+            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;
+          }
         }
       }
     }

lib/Driver/Driver.cpp

@@ -1612,20 +1612,13 @@ handleCompileJobCondition(Job *J, CompileJobAction::InputInfo inputInfo,
   }
 
   Job::Condition condition;
-  if (!hasValidModTime || J->getInputModTime() != inputInfo.previousModTime) {
-    if (alwaysRebuildDependents ||
-        inputInfo.status == CompileJobAction::InputInfo::NeedsCascadingBuild) {
-      condition = Job::Condition::Always;
-    } else {
-      condition = Job::Condition::RunWithoutCascading;
-    }
-  } else {
+  if (hasValidModTime && J->getInputModTime() == inputInfo.previousModTime) {
     switch (inputInfo.status) {
     case CompileJobAction::InputInfo::UpToDate:
-      if (!llvm::sys::fs::exists(J->getOutput().getPrimaryOutputFilename()))
-        condition = Job::Condition::RunWithoutCascading;
-      else
+      if (llvm::sys::fs::exists(J->getOutput().getPrimaryOutputFilename()))
         condition = Job::Condition::CheckDependencies;
+      else
+        condition = Job::Condition::RunWithoutCascading;
       break;
     case CompileJobAction::InputInfo::NeedsCascadingBuild:
       condition = Job::Condition::Always;
@@ -1636,6 +1629,13 @@ handleCompileJobCondition(Job *J, CompileJobAction::InputInfo inputInfo,
     case CompileJobAction::InputInfo::NewlyAdded:
       llvm_unreachable("handled above");
     }
+  } else {
+    if (alwaysRebuildDependents ||
+        inputInfo.status == CompileJobAction::InputInfo::NeedsCascadingBuild) {
+      condition = Job::Condition::Always;
+    } else {
+      condition = Job::Condition::RunWithoutCascading;
+    }
   }
 
   J->setCondition(condition);

test/Driver/Dependencies/Inputs/update-dependencies-bad.py

@@ -44,5 +44,5 @@
     else:
         sys.exit(129)
 
-dir = os.path.dirname(os.path.abspath(__file__))
-execfile(os.path.join(dir, "update-dependencies.py"))
+execDir = os.path.dirname(os.path.abspath(__file__))
+execfile(os.path.join(execDir, "update-dependencies.py"))