Inline TargetMarker in TransitiveTraversalFunction

Simplifies the runtime graph when TransitiveTraversalFunction is used.

Also moves an error reporting method from the base function to the
TransitiveTargetFunction, which is the only one that uses it.

--
MOS_MIGRATED_REVID=106109745

4 files changed, 96 insertions, 74 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java
index 3e3fbd5fb5..74d78dcba8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java
@@ -27,6 +27,8 @@ import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import javax.annotation.Nullable;
+
 /**
  * A SkyFunction for {@link TargetMarkerValue}s. Returns a {@link
  * TargetMarkerValue#TARGET_MARKER_INSTANCE} if the {@link Label} in the {@link SkyKey}
@@ -35,11 +37,21 @@ import com.google.devtools.build.skyframe.SkyValue;
  */
 public final class TargetMarkerFunction implements SkyFunction {
 
-  public TargetMarkerFunction() {
-  }
-
   @Override
   public SkyValue compute(SkyKey key, Environment env) throws TargetMarkerFunctionException {
+    try {
+      return computeTargetMarkerValue(key, env);
+    } catch (NoSuchTargetException e) {
+      throw new TargetMarkerFunctionException(e);
+    } catch (NoSuchPackageException e) {
+      // Re-throw this exception with our key because root causes should be targets, not packages.
+      throw new TargetMarkerFunctionException(e);
+    }
+  }
+
+  @Nullable
+  static TargetMarkerValue computeTargetMarkerValue(SkyKey key, Environment env)
+      throws NoSuchTargetException, NoSuchPackageException {
     Label label = (Label) key.argument();
     PathFragment pkgForLabel = label.getPackageFragment();
 
@@ -47,67 +59,54 @@ public final class TargetMarkerFunction implements SkyFunction {
       // This target is in a subdirectory, therefore it could potentially be invalidated by
       // a new BUILD file appearing in the hierarchy.
       PathFragment containingDirectory = label.toPathFragment().getParentDirectory();
-      ContainingPackageLookupValue containingPackageLookupValue = null;
+      ContainingPackageLookupValue containingPackageLookupValue;
       try {
         PackageIdentifier newPkgId = PackageIdentifier.create(
             label.getPackageIdentifier().getRepository(), containingDirectory);
         containingPackageLookupValue = (ContainingPackageLookupValue) env.getValueOrThrow(
             ContainingPackageLookupValue.key(newPkgId),
             BuildFileNotFoundException.class, InconsistentFilesystemException.class);
-      } catch (BuildFileNotFoundException e) {
-        // Thrown when there are IO errors looking for BUILD files.
-        throw new TargetMarkerFunctionException(e);
       } catch (InconsistentFilesystemException e) {
-        throw new TargetMarkerFunctionException(new NoSuchTargetException(label,
-            e.getMessage()));
+        throw new NoSuchTargetException(label, e.getMessage());
       }
       if (containingPackageLookupValue == null) {
         return null;
       }
+
       if (!containingPackageLookupValue.hasContainingPackage()) {
         // This means the label's package doesn't exist. E.g. there is no package 'a' and we are
         // trying to build the target for label 'a:b/foo'.
-        throw new TargetMarkerFunctionException(new BuildFileNotFoundException(
-            label.getPackageIdentifier(), "BUILD file not found on package path for '"
-                + pkgForLabel.getPathString() + "'"));
+        throw new BuildFileNotFoundException(
+            label.getPackageIdentifier(),
+            "BUILD file not found on package path for '" + pkgForLabel.getPathString() + "'");
       }
       if (!containingPackageLookupValue.getContainingPackageName().equals(
               label.getPackageIdentifier())) {
-        throw new TargetMarkerFunctionException(new NoSuchTargetException(label,
-            String.format("Label '%s' crosses boundary of subpackage '%s'", label,
-                containingPackageLookupValue.getContainingPackageName())));
+        throw new NoSuchTargetException(
+            label,
+            String.format(
+                "Label '%s' crosses boundary of subpackage '%s'",
+                label,
+                containingPackageLookupValue.getContainingPackageName()));
       }
     }
 
     SkyKey pkgSkyKey = PackageValue.key(label.getPackageIdentifier());
-    Package pkg;
-    try {
-      PackageValue value = (PackageValue)
-          env.getValueOrThrow(pkgSkyKey, NoSuchPackageException.class);
-      if (value == null) {
-        return null;
-      }
-      pkg = value.getPackage();
-    } catch (NoSuchPackageException e) {
-      // Re-throw this exception with our key because root causes should be targets, not packages.
-      throw new TargetMarkerFunctionException(e);
-    }
-
-    Target target;
-    try {
-      target = pkg.getTarget(label.getName());
-    } catch (NoSuchTargetException e) {
-      throw new TargetMarkerFunctionException(e);
+    PackageValue value =
+        (PackageValue) env.getValueOrThrow(pkgSkyKey, NoSuchPackageException.class);
+    if (value == null) {
+      return null;
     }
 
+    Package pkg = value.getPackage();
+    Target target = pkg.getTarget(label.getName());
     if (pkg.containsErrors()) {
       // There is a target, but its package is in error. We rethrow so that the root cause is the
       // target, not the package. Note that targets are only in error when their package is
       // "in error" (because a package is in error if there was an error evaluating the package, or
       // if one of its targets was in error).
-      throw new TargetMarkerFunctionException(
-          new NoSuchTargetException(
-              target, new BuildFileContainsErrorsException(label.getPackageIdentifier())));
+      throw new NoSuchTargetException(
+          target, new BuildFileContainsErrorsException(label.getPackageIdentifier()));
     }
     return TargetMarkerValue.TARGET_MARKER_INSTANCE;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
index 2d7ccc78e9..8623b78351 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
@@ -17,20 +17,17 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
-import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -61,8 +58,7 @@ import javax.annotation.Nullable;
  * {@link #computeSkyValue} with the {#code ProcessedTargets} to get the {@link SkyValue} to
  * return.
  */
-abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
-    implements SkyFunction {
+abstract class TransitiveBaseTraversalFunction<TProcessedTargets> implements SkyFunction {
 
   /**
    * Returns a {@link SkyKey} corresponding to the traversal of a target specified by {@code label}
@@ -80,8 +76,7 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
    */
   abstract SkyKey getKey(Label label);
 
-  abstract TProcessedTargets processTarget(Label label,
-      TargetAndErrorIfAny targetAndErrorIfAny);
+  abstract TProcessedTargets processTarget(Label label, TargetAndErrorIfAny targetAndErrorIfAny);
 
   abstract void processDeps(TProcessedTargets processedTargets, EventHandler eventHandler,
       TargetAndErrorIfAny targetAndErrorIfAny,
@@ -95,6 +90,14 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
   abstract SkyValue computeSkyValue(TargetAndErrorIfAny targetAndErrorIfAny,
       TProcessedTargets processedTargets);
 
+  /**
+   * Returns a {@link TargetMarkerValue} corresponding to the {@param targetMarkerKey} or {@code
+   * null} if the value isn't ready.
+   */
+  @Nullable
+  abstract TargetMarkerValue getTargetMarkerValue(SkyKey targetMarkerKey, Environment env)
+      throws NoSuchTargetException, NoSuchPackageException;
+
   @Override
   public SkyValue compute(SkyKey key, Environment env)
       throws TransitiveBaseTraversalFunctionException {
@@ -150,8 +153,8 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
   /**
    * Return an Iterable of SkyKeys corresponding to the Aspect-related dependencies of target.
    *
-   *  <p>This method may return a precise set of aspect keys, but may need to request additional
-   *  dependencies from the env to do so.
+   * <p>This method may return a precise set of aspect keys, but may need to request additional
+   * dependencies from the env to do so.
    */
   private Iterable<SkyKey> getStrictLabelAspectKeys(Target target,
           Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap,
@@ -179,14 +182,13 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
     return depKeys;
   }
 
-  /**
-   * Get the Aspect-related Label deps for the given edge.
-   */
-  protected abstract Collection<Label> getAspectLabels(Target fromTarget, Attribute attr,
-          Label toLabel,
-          ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
-          Environment env);
-
+  /** Get the Aspect-related Label deps for the given edge. */
+  protected abstract Collection<Label> getAspectLabels(
+      Target fromTarget,
+      Attribute attr,
+      Label toLabel,
+      ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
+      Environment env);
 
   private Iterable<SkyKey> getLabelDepKeys(Target target) {
     List<SkyKey> depKeys = Lists.newArrayList();
@@ -226,23 +228,6 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
     labels.addAll(packageGroup.getIncludes());
   }
 
-  protected void maybeReportErrorAboutMissingEdge(Target target, Label depLabel,
-      NoSuchThingException e, EventHandler eventHandler) {
-    if (e instanceof NoSuchTargetException) {
-      NoSuchTargetException nste = (NoSuchTargetException) e;
-      if (depLabel.equals(nste.getLabel())) {
-        eventHandler.handle(Event.error(TargetUtils.getLocationMaybe(target),
-            TargetUtils.formatMissingEdge(target, depLabel, e)));
-      }
-    } else if (e instanceof NoSuchPackageException) {
-      NoSuchPackageException nspe = (NoSuchPackageException) e;
-      if (nspe.getPackageId().equals(depLabel.getPackageIdentifier())) {
-        eventHandler.handle(Event.error(TargetUtils.getLocationMaybe(target),
-            TargetUtils.formatMissingEdge(target, depLabel, e)));
-      }
-    }
-  }
-
   enum LoadTargetResultsType {
     VALUES_MISSING,
     TARGET_AND_ERROR_IF_ANY
@@ -308,7 +293,7 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
     }
   }
 
-  private static LoadTargetResults loadTarget(Environment env, Label label)
+  private LoadTargetResults loadTarget(Environment env, Label label)
       throws NoSuchTargetException, NoSuchPackageException {
     SkyKey packageKey = PackageValue.key(label.getPackageIdentifier());
     SkyKey targetKey = TargetMarkerValue.key(label);
@@ -317,9 +302,10 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
     Target target;
     NoSuchTargetException errorLoadingTarget = null;
     try {
-      TargetMarkerValue targetValue = (TargetMarkerValue) env.getValueOrThrow(targetKey,
-          NoSuchTargetException.class, NoSuchPackageException.class);
-      if (targetValue == null) {
+      TargetMarkerValue targetValue = getTargetMarkerValue(targetKey, env);
+      boolean targetValueMissing = targetValue == null;
+      Preconditions.checkState(targetValueMissing == env.valuesMissing(), targetKey);
+      if (targetValueMissing) {
         return ValuesMissing.INSTANCE;
       }
       PackageValue packageValue = (PackageValue) env.getValueOrThrow(packageKey,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
index 8053543fab..a5e6320970 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AspectDefinition;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -34,6 +35,7 @@ import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.skyframe.TransitiveTargetFunction.TransitiveTargetValueBuilder;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -180,6 +182,35 @@ public class TransitiveTargetFunction
     }
   }
 
+  @Override
+  TargetMarkerValue getTargetMarkerValue(SkyKey targetMarkerKey, Environment env)
+      throws NoSuchTargetException, NoSuchPackageException {
+    return (TargetMarkerValue)
+        env.getValueOrThrow(
+            targetMarkerKey, NoSuchTargetException.class, NoSuchPackageException.class);
+  }
+
+  private void maybeReportErrorAboutMissingEdge(
+      Target target, Label depLabel, NoSuchThingException e, EventHandler eventHandler) {
+    if (e instanceof NoSuchTargetException) {
+      NoSuchTargetException nste = (NoSuchTargetException) e;
+      if (depLabel.equals(nste.getLabel())) {
+        eventHandler.handle(
+            Event.error(
+                TargetUtils.getLocationMaybe(target),
+                TargetUtils.formatMissingEdge(target, depLabel, e)));
+      }
+    } else if (e instanceof NoSuchPackageException) {
+      NoSuchPackageException nspe = (NoSuchPackageException) e;
+      if (nspe.getPackageId().equals(depLabel.getPackageIdentifier())) {
+        eventHandler.handle(
+            Event.error(
+                TargetUtils.getLocationMaybe(target),
+                TargetUtils.formatMissingEdge(target, depLabel, e)));
+      }
+    }
+  }
+
   /**
    * Returns every configuration fragment known to the system.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
index 7084ecfd01..abd6b5c598 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
@@ -114,6 +114,12 @@ public class TransitiveTraversalFunction
         : TransitiveTraversalValue.unsuccessfulTransitiveTraversal(firstErrorMessage);
   }
 
+  @Override
+  TargetMarkerValue getTargetMarkerValue(SkyKey targetMarkerKey, Environment env)
+      throws NoSuchTargetException, NoSuchPackageException {
+    return TargetMarkerFunction.computeTargetMarkerValue(targetMarkerKey, env);
+  }
+
   /**
    * Keeps track of the first error message encountered while traversing itself and its
    * dependencies.