Do not use the unchecked MissingDepException to signify missing hin…

…ted inclusions.

Instead, just return and have callers check `SkyFunction.Environment#valuesMissing`.

PiperOrigin-RevId: 354105371

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

@@ -159,7 +159,7 @@ public final ActionOwner getOwner() {
 
   @Override
   public final synchronized boolean inputsDiscovered() {
-    return discoversInputs() ? inputsDiscovered : true;
+    return !discoversInputs() || inputsDiscovered;
   }
 
   /**
@@ -176,19 +176,23 @@ public boolean discoversInputs() {
   }
 
   /**
-   * Run input discovery on the action.
+   * Runs input discovery on the action.
    *
    * <p>Called by Blaze if {@link #discoversInputs()} returns true. It must return the set of input
-   * artifacts that were not known at analysis time. May also call {@link
-   * #updateInputs(NestedSet<Artifact>)}; if it doesn't, the action itself must arrange for the
-   * newly discovered artifacts to be available during action execution, probably by keeping state
-   * in the action instance and using a custom action execution context and for {@code
-   * #updateInputs()} to be called during the execution of the action.
+   * artifacts that were not known at analysis time. May also call {@link #updateInputs}; if it
+   * doesn't, the action itself must arrange for the newly discovered artifacts to be available
+   * during action execution, probably by keeping state in the action instance and using a custom
+   * action execution context and for {@link #updateInputs} to be called during the execution of the
+   * action.
    *
-   * <p>Since keeping state within an action bad, don't do that unless there is a very good reason
-   * to do so.
+   * <p>Since keeping state within an action is bad, don't do that unless there is a very good
+   * reason to do so.
+   *
+   * <p>May return {@code null} if more dependencies were requested from skyframe but were
+   * unavailable, meaning a restart is necessary.
    */
   @Override
+  @Nullable
   public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     throw new IllegalStateException("discoverInputs cannot be called for " + this.prettyPrint()

src/main/java/com/google/devtools/build/lib/includescanning/CppIncludeScanningContextImpl.java

@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.includescanning;
 
 import com.google.common.base.Preconditions;
-import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -41,19 +40,19 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Supplier;
+import javax.annotation.Nullable;
 
-/**
- * Include scanning context implementation.
- */
-public class CppIncludeScanningContextImpl implements CppIncludeScanningContext {
-  private final Supplier<? extends IncludeScannerSupplier> includeScannerSupplier;
+/** Include scanning context implementation. */
+public final class CppIncludeScanningContextImpl implements CppIncludeScanningContext {
+  private final Supplier<IncludeScannerSupplier> includeScannerSupplier;
 
-  public CppIncludeScanningContextImpl(
-      Supplier<? extends IncludeScannerSupplier> includeScannerSupplier) {
+  public CppIncludeScanningContextImpl(Supplier<IncludeScannerSupplier> includeScannerSupplier) {
     this.includeScannerSupplier = includeScannerSupplier;
   }
 
   @Override
+  @Nullable
   public List<Artifact> findAdditionalInputs(
       CppCompileAction action,
       ActionExecutionContext actionExecutionContext,
@@ -62,8 +61,6 @@ public List<Artifact> findAdditionalInputs(
     Preconditions.checkNotNull(includeScannerSupplier, action);
 
     Set<Artifact> includes = Sets.newConcurrentHashSet();
-
-    final List<PathFragment> absoluteBuiltInIncludeDirs = new ArrayList<>();
     includes.addAll(action.getBuiltInIncludeFiles());
 
     // Deduplicate include directories. This can occur especially with "built-in" and "system"
@@ -77,6 +74,7 @@ public List<Artifact> findAdditionalInputs(
     includeDirs.addAll(includeScanningHeaderData.getSystemIncludeDirs());
 
     // Add the system include paths to the list of include paths.
+    List<PathFragment> absoluteBuiltInIncludeDirs = new ArrayList<>();
     for (PathFragment pathFragment : action.getBuiltInIncludeDirectories()) {
       if (pathFragment.isAbsolute()) {
         absoluteBuiltInIncludeDirs.add(pathFragment);
@@ -105,6 +103,9 @@ public List<Artifact> findAdditionalInputs(
           action,
           actionExecutionContext,
           action.getGrepIncludes());
+      if (actionExecutionContext.getEnvironmentForDiscoveringInputs().valuesMissing()) {
+        return null;
+      }
       return collect(actionExecutionContext, includes, absoluteBuiltInIncludeDirs);
     } catch (IOException e) {
       throw new EnvironmentalExecException(

src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java

@@ -18,6 +18,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
 import com.google.common.flogger.GoogleLogger;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.MoreExecutors;
@@ -193,18 +194,14 @@ public void extractSwigIncludes(
               env.getDirectories(),
               env.getSkyframeBuildView().getArtifactFactory(),
               env.getExecRoot());
-      ImmutableMap.Builder<PathFragment, Artifact> pathToLegalOutputArtifact =
-          ImmutableMap.builder();
-      for (Artifact path : legalOutputPaths) {
-        pathToLegalOutputArtifact.put(path.getExecPath(), path);
-      }
       try {
         scanner.processAsync(
             source,
             ImmutableList.of(source),
             // For Swig include scanning just point to the output file in the map.
             new IncludeScanningHeaderData.Builder(
-                    pathToLegalOutputArtifact.build(), /*modularHeaders=*/ ImmutableSet.of())
+                    Maps.uniqueIndex(legalOutputPaths, Artifact::getExecPath),
+                    /*modularHeaders=*/ ImmutableSet.of())
                 .build(),
             ImmutableList.of(),
             includes,

src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java

@@ -589,7 +589,7 @@ private boolean isFile(
   }
 
   @Override
-  public void processAsync(
+  public final void processAsync(
       Artifact mainSource,
       Collection<Artifact> sources,
       IncludeScanningHeaderData includeScanningHeaderData,
@@ -599,10 +599,20 @@ public void processAsync(
       ActionExecutionContext actionExecutionContext,
       Artifact grepIncludes)
       throws IOException, ExecException, InterruptedException {
-    ImmutableSet<Artifact> pathHints =
-        prepare(actionExecutionContext.getEnvironmentForDiscoveringInputs());
-    IncludeVisitor visitor;
-    visitor =
+    ImmutableSet<Artifact> pathHints;
+    if (parser.getHints() == null) {
+      pathHints = ImmutableSet.of();
+    } else {
+      SkyFunction.Environment env = actionExecutionContext.getEnvironmentForDiscoveringInputs();
+      Collection<Artifact> artifacts =
+          parser.getHints().getPathLevelHintedInclusions(quoteIncludePaths, env);
+      if (env.valuesMissing()) {
+        return;
+      }
+      pathHints = ImmutableSet.copyOf(artifacts);
+    }
+
+    IncludeVisitor visitor =
         new IncludeVisitor(
             actionExecutionMetadata,
             actionExecutionContext,
@@ -611,21 +621,6 @@ public void processAsync(
     visitor.processInternal(mainSource, sources, cmdlineIncludes, includes, pathHints);
   }
 
-  private ImmutableSet<Artifact> prepare(SkyFunction.Environment env) throws InterruptedException {
-    if (parser.getHints() != null) {
-      Collection<Artifact> artifacts =
-          parser.getHints().getPathLevelHintedInclusions(quoteIncludePaths, env);
-      if (env.valuesMissing()) {
-        throw new MissingDepException();
-      }
-      ImmutableSet.Builder<Artifact> pathHints;
-      pathHints = ImmutableSet.builderWithExpectedSize(quoteIncludePaths.size());
-      pathHints.addAll(Preconditions.checkNotNull(artifacts, quoteIncludePaths));
-      return pathHints.build();
-    }
-    return ImmutableSet.of();
-  }
-
   private static void checkForInterrupt(String operation, Object source)
       throws InterruptedException {
     // We require passing in the operation and the source Path / Artifact to avoid intermediate

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -395,6 +395,7 @@ public NestedSet<Artifact> getPossibleInputsForTesting() {
   }
 
   /** Returns the results of include scanning. */
+  @Nullable
   private NestedSet<Artifact> findUsedHeaders(
       ActionExecutionContext actionExecutionContext, IncludeScanningHeaderData headerData)
       throws ActionExecutionException, InterruptedException {
@@ -406,6 +407,9 @@ private NestedSet<Artifact> findUsedHeaders(
             actionExecutionContext
                 .getContext(CppIncludeScanningContext.class)
                 .findAdditionalInputs(this, actionExecutionContext, headerData);
+        if (includes == null) {
+          return null;
+        }
 
         return NestedSetBuilder.wrap(Order.STABLE_ORDER, includes);
       } catch (IORuntimeException e) {
@@ -481,6 +485,9 @@ public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecution
               .setIsValidUndeclaredHeader(getValidUndeclaredHeaderPredicate(actionExecutionContext))
               .build();
       additionalInputs = findUsedHeaders(actionExecutionContext, includeScanningHeaderData);
+      if (additionalInputs == null) {
+        return null;
+      }
 
       if (useHeaderModules) {
         usedModules = ccCompilationContext.computeUsedModules(usePic, additionalInputs.toSet());

src/main/java/com/google/devtools/build/lib/rules/cpp/CppIncludeScanningContext.java

@@ -31,7 +31,7 @@ public interface CppIncludeScanningContext extends ActionContext {
   /**
    * Does include scanning to find the list of files needed to execute the action.
    *
-   * <p>Returns null if additional inputs will only be found during action execution, not before.
+   * <p>Returns {@code null} if a skyframe restart is necessary.
    */
   @Nullable
   List<Artifact> findAdditionalInputs(

src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanner.java

@@ -58,6 +58,12 @@ public interface IncludeScanner {
    *
    * <p>{@code mainSource} is the source file relative to which the {@code cmdlineIncludes} are
    * interpreted.
+   *
+   * <p>Additional dependencies may be requested via {@link
+   * ActionExecutionContext#getEnvironmentForDiscoveringInputs}. If any dependency is not
+   * immediately available, processing will be short-circuited. The caller should check {@link
+   * com.google.devtools.build.skyframe.SkyFunction.Environment#valuesMissing} - if it returns
+   * {@code true}, then include scanning did not complete and a skyframe restart is necessary.
    */
   void processAsync(
       Artifact mainSource,
@@ -74,7 +80,7 @@ void processAsync(
    * Holds pre-aggregated information that the {@link IncludeScanner} needs from the compilation
    * action.
    */
-  class IncludeScanningHeaderData {
+  final class IncludeScanningHeaderData {
     /**
      * Lookup table to find the {@link Artifact}s of generated files based on their {@link
      * Artifact#getExecPath}.

src/main/java/com/google/devtools/build/lib/rules/cpp/SwigIncludeScanningContext.java

@@ -31,16 +31,11 @@
 @ActionContextMarker(name = "SwigIncludeScanning")
 public interface SwigIncludeScanningContext extends ActionContext {
   /**
-   * Scan includes in a .swig file.
+   * Scans includes in a .swig file ({@code source}), placing the results into {@code includes}.
    *
-   * @param includes the result where the included files are put
-   * @param actionExecutionMetadata the owning action
-   * @param actionExecContext the execution context
-   * @param source the file to be scanned
-   * @param legalOutputPaths the output files that are allowed to be included
-   * @param swigIncludePaths the include paths in effect
-   * @throws IOException
-   * @throws ExecException
+   * <p>Like {@link IncludeScanner#processAsync}, may short-circuit if a skyframe restart is
+   * necessary. Callers must check {@link
+   * com.google.devtools.build.skyframe.SkyFunction.Environment#valuesMissing}.
    */
   void extractSwigIncludes(
       Set<Artifact> includes,

src/main/java/com/google/devtools/build/lib/standalone/DummyCppIncludeScanningContext.java

@@ -19,16 +19,14 @@
 import com.google.devtools.build.lib.rules.cpp.CppIncludeScanningContext;
 import com.google.devtools.build.lib.rules.cpp.IncludeScanner.IncludeScanningHeaderData;
 import java.util.List;
-import javax.annotation.Nullable;
 
-/** A CppIncludeScanningContext that does nothing. */
-class DummyCppIncludeScanningContext implements CppIncludeScanningContext {
+/** A {@link CppIncludeScanningContext} that does not expect to be called. */
+final class DummyCppIncludeScanningContext implements CppIncludeScanningContext {
   @Override
-  @Nullable
   public List<Artifact> findAdditionalInputs(
       CppCompileAction action,
       ActionExecutionContext actionExecutionContext,
       IncludeScanningHeaderData includeScanningHeaderData) {
-    return null;
+    throw new UnsupportedOperationException("Include scanning unexpected for " + action.describe());
   }
 }