Remove direct store of Target from NoSuchTargetException. Instead, just note the fact that a valid Target exists, and instead request it directly from its Package.

--
MOS_MIGRATED_REVID=90107670

3 files changed, 41 insertions, 14 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/NoSuchTargetException.java b/src/main/java/com/google/devtools/build/lib/packages/NoSuchTargetException.java
index ab89d77224..6b311441c8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NoSuchTargetException.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NoSuchTargetException.java
@@ -25,9 +25,7 @@ import javax.annotation.Nullable;
 public class NoSuchTargetException extends NoSuchThingException {
 
   @Nullable private final Label label;
-  // TODO(bazel-team): rename/refactor this class and NoSuchPackageException since it's confusing
-  // that they embed Target/Package instances.
-  @Nullable private final Target target;
+  private final boolean hasTarget;
 
   public NoSuchTargetException(String message) {
     this(null, message);
@@ -46,7 +44,7 @@ public class NoSuchTargetException extends NoSuchThingException {
       @Nullable NoSuchPackageException nspe) {
     super(message, nspe);
     this.label = label;
-    this.target = target;
+    this.hasTarget = (target != null);
   }
 
   @Nullable
@@ -55,10 +53,9 @@ public class NoSuchTargetException extends NoSuchThingException {
   }
 
   /**
-   * Return the target (in error) if parsing completed enough to construct it. May return null.
+   * Return whether parsing completed enough to construct the target.
    */
-  @Nullable
-  public Target getTarget() {
-    return target;
+  public boolean hasTarget() {
+    return hasTarget;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
index 1abec5d635..a1c1968c35 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
@@ -121,6 +121,12 @@ final class SkyframeLabelVisitor implements TransitivePackageLoader {
       }
       warnAboutLoadingFailure(topLevelLabel, eventHandler);
       for (SkyKey badKey : errorInfo.getRootCauses()) {
+        if (badKey.functionName() == SkyFunctions.PACKAGE) {
+          // Transitive target function may ask for a Package, but don't include this in the root
+          // causes. We'll get more precise information from dependencies on transitive and direct
+          // target dependencies.
+          continue;
+        }
         Preconditions.checkState(badKey.argument() instanceof Label,
             "%s %s %s", key, errorInfo, badKey);
         rootCauses.put(topLevelLabel, (Label) badKey.argument());
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 8f1838a22d..1e02ef38c5 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
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -26,6 +27,7 @@ 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.PackageIdentifier;
 import com.google.devtools.build.lib.packages.Rule;
@@ -67,21 +69,43 @@ public class TransitiveTargetFunction implements SkyFunction {
         return null;
       }
       PackageValue packageValue = (PackageValue) env.getValueOrThrow(packageKey,
-          NoSuchThingException.class);
+          NoSuchPackageException.class);
       if (packageValue == null) {
         return null;
       }
 
       packageLoadedSuccessfully = true;
-      target = packageValue.getPackage().getTarget(label.getName());
+      try {
+        target = packageValue.getPackage().getTarget(label.getName());
+      } catch (NoSuchTargetException unexpected) {
+        // Not expected since the TargetMarkerFunction would have failed earlier if the Target
+        // was not present.
+        throw new IllegalStateException(unexpected);
+      }
     } catch (NoSuchTargetException e) {
-      target = e.getTarget();
-      if (target == null) {
+      if (!e.hasTarget()) {
         throw new TransitiveTargetFunctionException(e);
       }
 
-      // So we now have a Target here, but the only way for that to happen is if the package loaded
-      // at least partially, but had an error.
+      // We know that a Target may be extracted, but we need to get it out of the Package
+      // (which is known to be in error).
+      Package pkg;
+      try {
+        PackageValue packageValue = (PackageValue) env.getValueOrThrow(packageKey,
+            NoSuchPackageException.class);
+        if (packageValue == null) {
+          return null;
+        }
+        throw new IllegalStateException("Expected bad package: " + label.getPackageIdentifier());
+      } catch (NoSuchPackageException nsp) {
+        pkg = Preconditions.checkNotNull(nsp.getPackage(), label.getPackageIdentifier());
+      }
+      try {
+        target = pkg.getTarget(label.getName());
+      } catch (NoSuchTargetException nste) {
+        throw new IllegalStateException("Expected target to exist", nste);
+      }
+
       successfulTransitiveLoading = false;
       transitiveRootCauses.add(label);
       errorLoadingTarget = e;