Stop throwing an exception if a Package was successfully created but contains errors. Instead, require callers to process the package and throw if they need to.

This allows us to avoid embedding a Package in an exception, which is icky. This also allows us to remove Package#containsTemporaryErrors.

Most callers' changes are fairly straightforward. The exception is EnvironmentBackedRecursivePackageProvider, which cannot throw an exception of its own in case of a package with errors (because it doesn't do that in keep_going mode), but whose request for a package with errors *should* shut down the build in case of nokeep_going mode. To do this in Skyframe, we have a new PackageErrorFunction which is to be called only in this situation, and will unconditionally throw. EnvironmentBackedRecursivePackageProvider can then catch this exception and continue on as usual, except that the exception will shut down the thread pool in a nokeep_going build.

--
MOS_MIGRATED_REVID=103247761

29 files changed, 298 insertions, 235 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java
index 1fe07a96d6..ebecdba6b1 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java
@@ -261,7 +261,14 @@ public abstract class RepositoryFunction implements SkyFunction {
     if (packageValue == null) {
       return null;
     }
-    return (ExternalPackage) packageValue.getPackage();
+    ExternalPackage externalPackage = (ExternalPackage) packageValue.getPackage();
+    if (externalPackage.containsErrors()) {
+      throw new RepositoryFunctionException(
+          new BuildFileContainsErrorsException(
+              ExternalPackage.PACKAGE_IDENTIFIER, "Could not load //external package"),
+          Transience.PERSISTENT);
+    }
+    return externalPackage;
   }
 
   @Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java b/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java
index a6e4c87de7..8b1ed43643 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java
@@ -16,15 +16,16 @@ package com.google.devtools.build.lib.packages;
 
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 
-import javax.annotation.Nullable;
-
 /**
  * Exception indicating a failed attempt to access a package that could not
  * be read or had syntax errors.
  */
 public class BuildFileContainsErrorsException extends NoSuchPackageException {
-
-  private Package pkg;
+  public BuildFileContainsErrorsException(PackageIdentifier packageIdentifier) {
+    super(
+        packageIdentifier,
+        "Package '" + packageIdentifier.getPackageFragment().getPathString() + "' contains errors");
+  }
 
   public BuildFileContainsErrorsException(PackageIdentifier packageIdentifier, String message) {
     super(packageIdentifier, "error loading package", message);
@@ -34,15 +35,4 @@ public class BuildFileContainsErrorsException extends NoSuchPackageException {
       Throwable cause) {
     super(packageIdentifier, "error loading package", message, cause);
   }
-
-  public BuildFileContainsErrorsException(Package pkg, String msg) {
-    this(pkg.getPackageIdentifier(), msg);
-    this.pkg = pkg;
-  }
-
-  @Override
-  @Nullable
-  public Package getPackage() {
-    return pkg;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java b/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java
index 9c2316ad19..00824dd509 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java
@@ -16,8 +16,6 @@ package com.google.devtools.build.lib.packages;
 
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 
-import javax.annotation.Nullable;
-
 /**
  * Exception indicating an attempt to access a package which is not found, does
  * not exist, or can't be parsed into a package.
@@ -50,12 +48,4 @@ public abstract class NoSuchPackageException extends NoSuchThingException {
   public PackageIdentifier getPackageId() {
     return packageId;
   }
-
-  /**
-   * Return the package if parsing completed enough to construct it. May return null.
-   */
-  @Nullable
-  public Package getPackage() {
-    return null;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 0fbc72681a..c78b6d022a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -169,12 +169,6 @@ public class Package implements Serializable {
   private boolean containsErrors;
 
   /**
-   * True iff this package contains errors that were caused by temporary conditions (e.g. an I/O
-   * error). If this is true, {@link #containsErrors} is also true.
-   */
-  private boolean containsTemporaryErrors;
-
-  /**
    * The set of labels subincluded by this package.
    */
   private Set<Label> subincludes;
@@ -354,7 +348,6 @@ public class Package implements Serializable {
     }
     this.buildFile = builder.buildFile;
     this.containsErrors = builder.containsErrors;
-    this.containsTemporaryErrors = builder.containsTemporaryErrors;
     this.subincludes = builder.subincludes.keySet();
     this.skylarkFileDependencies = builder.skylarkFileDependencies;
     this.defaultLicense = builder.defaultLicense;
@@ -473,14 +466,6 @@ public class Package implements Serializable {
     return containsErrors;
   }
 
-  /**
-   * True iff this package contains errors that were caused by temporary conditions (e.g. an I/O
-   * error). If this is true, {@link #containsErrors()} also returns true.
-   */
-  public boolean containsTemporaryErrors() {
-    return containsTemporaryErrors;
-  }
-
   public List<Event> getEvents() {
     return events;
   }
@@ -782,7 +767,6 @@ public class Package implements Serializable {
     private List<String> features = new ArrayList<>();
     private List<Event> events = Lists.newArrayList();
     private boolean containsErrors = false;
-    private boolean containsTemporaryErrors = false;
 
     private License defaultLicense = License.NO_LICENSE;
     private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
@@ -856,6 +840,10 @@ public class Package implements Serializable {
       return filename;
     }
 
+    public List<Event> getEvents() {
+      return events;
+    }
+
     /**
      * Sets this package's Make environment.
      */
@@ -955,12 +943,6 @@ public class Package implements Serializable {
       return containsErrors;
     }
 
-    Builder setContainsTemporaryErrors() {
-      setContainsErrors();
-      containsTemporaryErrors = true;
-      return this;
-    }
-
     public Builder addEvents(Iterable<Event> events) {
       for (Event event : events) {
         addEvent(event);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
index 9dbb7efdc4..867a83aa1b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
@@ -387,9 +387,6 @@ public class PackageDeserializer {
     if (packagePb.hasContainsErrors() && packagePb.getContainsErrors()) {
       builder.setContainsErrors();
     }
-    if (packagePb.hasContainsTemporaryErrors() && packagePb.getContainsTemporaryErrors()) {
-      builder.setContainsTemporaryErrors();
-    }
 
     deserializeTargets(in, context);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 82361f7bbb..2b09e7b803 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -538,7 +538,7 @@ public final class PackageFactory {
     } catch (IOException expected) {
       context.eventHandler.handle(Event.error(ast.getLocation(),
               "error globbing [" + Joiner.on(", ").join(includes) + "]: " + expected.getMessage()));
-      context.pkgBuilder.setContainsTemporaryErrors();
+      context.pkgBuilder.setContainsErrors();
       return GlobList.captureResults(includes, excludes, ImmutableList.<String>of());
     } catch (GlobCache.BadGlobException e) {
       throw new EvalException(ast.getLocation(), e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
index 73cafd2a86..9a336c1d80 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
@@ -166,7 +166,6 @@ public class PackageSerializer {
     }
 
     builder.setContainsErrors(pkg.containsErrors());
-    builder.setContainsTemporaryErrors(pkg.containsTemporaryErrors());
 
     builder.build().writeDelimitedTo(out);
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
index f3c792fbb9..ec206a794d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
@@ -114,8 +114,8 @@ public interface Preprocessor {
     public static Result noPreprocessing(ParserInputSource buildFileSource) {
       return new Result(
           buildFileSource,
-          /*preprocessed=*/false,
-          /*containsErrors=*/false,
+          /*preprocessed=*/ false,
+          /*containsErrors=*/ false,
           ImmutableList.<Event>of());
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 683c06d69d..4826baba6c 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.collect.CompactHashSet;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.graph.Digraph;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Package;
@@ -373,6 +374,10 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
     try {
       PackageValue packageValue = (PackageValue) graph.getValue(packageKey);
       if (packageValue != null) {
+        Package pkg = packageValue.getPackage();
+        if (pkg.containsErrors()) {
+          throw new BuildFileContainsErrorsException(label.getPackageIdentifier());
+        }
         return packageValue.getPackage().getTarget(label.getName());
       } else {
         throw (NoSuchThingException) Preconditions.checkNotNull(
@@ -601,7 +606,10 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
     Map<SkyKey, SkyValue> packageValues = graph.getSuccessfulValues(resultKeys);
     ImmutableSet.Builder<Target> result = ImmutableSet.builder();
     for (SkyValue value : packageValues.values()) {
-      result.add(((PackageValue) value).getPackage().getBuildFile());
+      Package pkg = ((PackageValue) value).getPackage();
+      if (!pkg.containsErrors()) {
+        result.add(pkg.getBuildFile());
+      }
     }
     return result.build();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
index dfe87157cf..c3e9589d69 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -228,7 +228,8 @@ public class GenQuery implements RuleConfiguredTargetFactory {
     ImmutableMap.Builder<PackageIdentifier, Package> packageMapBuilder = ImmutableMap.builder();
     for (PackageIdentifier pkgId : packageNames.build()) {
       PackageValue pkg = (PackageValue) env.getValue(PackageValue.key(pkgId));
-      Preconditions.checkState(pkg != null, "package %s not preloaded", pkgId);
+      Preconditions.checkNotNull(pkg, "package %s not preloaded", pkgId);
+      Preconditions.checkState(!pkg.getPackage().containsErrors(), pkgId);
       packageMapBuilder.put(pkg.getPackage().getPackageIdentifier(), pkg.getPackage());
     }
     return Pair.of(packageMapBuilder.build(), validTargets.build().toSet());
@@ -384,7 +385,7 @@ public class GenQuery implements RuleConfiguredTargetFactory {
         for (Map.Entry<SkyKey, ValueOrException<NoSuchPackageException>> entry :
           env.getValuesOrThrow(packageKeys, NoSuchPackageException.class).entrySet()) {
           PackageIdentifier pkgName = (PackageIdentifier) entry.getKey().argument();
-          Package pkg = null;
+          Package pkg;
           try {
             PackageValue packageValue = (PackageValue) entry.getValue().get();
             if (packageValue == null) {
@@ -393,11 +394,7 @@ public class GenQuery implements RuleConfiguredTargetFactory {
             }
             pkg = packageValue.getPackage();
           } catch (NoSuchPackageException nspe) {
-            if (nspe.getPackage() != null) {
-              pkg = nspe.getPackage();
-            } else {
-              continue;
-            }
+            continue;
           }
           Preconditions.checkNotNull(pkg, pkgName);
           packages.put(pkgName, pkg);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index ede34cfdc6..d4f071e10d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.AspectFactory;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
@@ -74,9 +75,15 @@ public final class AspectFunction implements SkyFunction {
       return null;
     }
 
+    Package pkg = packageValue.getPackage();
+    if (pkg.containsErrors()) {
+      throw new AspectFunctionException(
+          skyKey, new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier()));
+    }
+
     Target target;
     try {
-      target = packageValue.getPackage().getTarget(key.getLabel().getName());
+      target = pkg.getTarget(key.getLabel().getName());
     } catch (NoSuchTargetException e) {
       throw new AspectFunctionException(skyKey, e);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index ebc1fa72db..43f35213b7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.packages.AspectDefinition;
 import com.google.devtools.build.lib.packages.AspectFactory;
 import com.google.devtools.build.lib.packages.AspectParameters;
 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;
@@ -67,6 +68,7 @@ import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException;
@@ -150,6 +152,12 @@ final class ConfiguredTargetFunction implements SkyFunction {
       return null;
     }
 
+    Package pkg = packageValue.getPackage();
+    if (pkg.containsErrors()) {
+      throw new ConfiguredTargetFunctionException(
+          new BuildFileContainsErrorsException(lc.getLabel().getPackageIdentifier()),
+          Transience.PERSISTENT);
+    }
     Target target;
     try {
       target = packageValue.getPackage().getTarget(lc.getLabel().getName());
@@ -791,6 +799,11 @@ final class ConfiguredTargetFunction implements SkyFunction {
       super(e, Transience.PERSISTENT);
     }
 
+    public ConfiguredTargetFunctionException(
+        BuildFileContainsErrorsException e, Transience transience) {
+      super(e, transience);
+    }
+
     private ConfiguredTargetFunctionException(ConfiguredValueCreationException error) {
       super(error, Transience.PERSISTENT);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 63e8e1d32a..4d2413aa7b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -13,11 +13,13 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
@@ -46,21 +48,26 @@ public final class EnvironmentBackedRecursivePackageProvider implements Recursiv
   public Package getPackage(EventHandler eventHandler, PackageIdentifier packageName)
       throws NoSuchPackageException, MissingDepException {
     SkyKey pkgKey = PackageValue.key(packageName);
-    Package pkg;
-    try {
-      PackageValue pkgValue =
-          (PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
-      if (pkgValue == null) {
+    PackageValue pkgValue =
+        (PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
+    if (pkgValue == null) {
+      throw new MissingDepException();
+    }
+    Package pkg = pkgValue.getPackage();
+    if (pkg.containsErrors()) {
+      // If this is a nokeep_going build, we must shut the build down by throwing an exception. To
+      // do that, we request a node that will throw an exception, and then try to catch it and
+      // continue. This gives the framework notification to shut down the build if it should.
+      try {
+        env.getValueOrThrow(
+            PackageErrorFunction.key(packageName), BuildFileContainsErrorsException.class);
+        Preconditions.checkState(env.valuesMissing(), "Should have thrown for %s", packageName);
         throw new MissingDepException();
-      }
-      pkg = pkgValue.getPackage();
-    } catch (NoSuchPackageException e) {
-      pkg = e.getPackage();
-      if (pkg == null) {
-        throw e;
+      } catch (BuildFileContainsErrorsException e) {
+        // Expected.
       }
     }
-    return pkg;
+    return pkgValue.getPackage();
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
index ab25cd8145..6dd38da11d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
@@ -131,8 +131,11 @@ public class ExternalFilesHelper {
         throw new FileOutsidePackageRootsException(rootedPath);
       }
     } else if (getFileType(rootedPath) == FileType.EXTERNAL_IMMUTABLE_FILE) {
-      Preconditions.checkNotNull(
-          env.getValue(PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER)));
+      PackageValue pkgValue =
+          (PackageValue)
+              Preconditions.checkNotNull(
+                  env.getValue(PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER)));
+      Preconditions.checkState(!pkgValue.getPackage().containsErrors());
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index f657f89d4d..81cf6eca27 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -62,13 +62,8 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
     if (graph.exists(pkgKey)) {
       pkgValue = (PackageValue) graph.getValue(pkgKey);
       if (pkgValue == null) {
-        NoSuchPackageException noSuchPackageException =
-            (NoSuchPackageException) Preconditions.checkNotNull(graph.getException(pkgKey), pkgKey);
-        Package pkg = noSuchPackageException.getPackage();
-        if (pkg == null) {
-          throw noSuchPackageException;
-        }
-        return pkg;
+        throw (NoSuchPackageException)
+            Preconditions.checkNotNull(graph.getException(pkgKey), pkgKey);
       }
     } else {
       // If the package key does not exist in the graph, then it must not correspond to any package,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java
new file mode 100644
index 0000000000..c392c045d3
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java
@@ -0,0 +1,76 @@
+// Copyright 2015 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
+import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+
+import javax.annotation.Nullable;
+
+/**
+ * SkyFunction that throws a {@link BuildFileContainsErrorsException} for {@link Package} that
+ * loaded, but was in error. Must only be requested when a SkyFunction wishes to ignore the errors
+ * in a {@link Package} in keep_going mode, but to shut down the build in nokeep_going mode. Thus,
+ * this SkyFunction should only be requested when the corresponding {@link PackageFunction} has
+ * already been successfully called and the resulting Package contains an error.
+ *
+ * <p>This SkyFunction never returns a value, only throws a {@link BuildFileNotFoundException}, and
+ * should never return null, since all of its dependencies should already be present.
+ */
+public class PackageErrorFunction implements SkyFunction {
+  public static SkyKey key(PackageIdentifier packageIdentifier) {
+    return new SkyKey(SkyFunctions.PACKAGE_ERROR, packageIdentifier);
+  }
+
+  @Nullable
+  @Override
+  public SkyValue compute(SkyKey skyKey, Environment env) throws PackageErrorFunctionException {
+    PackageIdentifier packageIdentifier = (PackageIdentifier) skyKey.argument();
+    try {
+      SkyKey packageKey = PackageValue.key(packageIdentifier);
+      // Callers must have tried to load the package already and gotten the package successfully.
+      Package pkg =
+          ((PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class))
+              .getPackage();
+      Preconditions.checkState(pkg.containsErrors(), skyKey);
+      throw new PackageErrorFunctionException(
+          new BuildFileContainsErrorsException(packageIdentifier), Transience.PERSISTENT);
+    } catch (NoSuchPackageException e) {
+      throw new IllegalStateException(
+          "Function should not have been called on package with exception", e);
+    }
+  }
+
+  @Nullable
+  @Override
+  public String extractTag(SkyKey skyKey) {
+    return null;
+  }
+
+  private static class PackageErrorFunctionException extends SkyFunctionException {
+    public PackageErrorFunctionException(
+        BuildFileContainsErrorsException cause, Transience transience) {
+      super(cause, transience);
+    }
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 1f8d05750a..eb580be9ba 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -219,10 +219,13 @@ public class PackageFunction implements SkyFunction {
    * inconsistent).
    */
   private static boolean markDependenciesAndPropagateInconsistentFilesystemExceptions(
-      Package pkg, Environment env, Collection<Pair<String, Boolean>> globPatterns,
-      Map<Label, Path> subincludes) throws InternalInconsistentFilesystemException {
-    boolean packageWasOriginallyInError = pkg.containsErrors();
-    boolean packageShouldBeInError = packageWasOriginallyInError;
+      Environment env,
+      Collection<Pair<String, Boolean>> globPatterns,
+      Map<Label, Path> subincludes,
+      PackageIdentifier packageIdentifier,
+      boolean containsErrors)
+      throws InternalInconsistentFilesystemException {
+    boolean packageShouldBeInError = containsErrors;
 
     // TODO(bazel-team): This means that many packages will have to be preprocessed twice. Ouch!
     // We need a better continuation mechanism to avoid repeating work. [skyframe-loading]
@@ -232,14 +235,13 @@ public class PackageFunction implements SkyFunction {
     // [skyframe-loading]
 
     Set<SkyKey> subincludePackageLookupDepKeys = Sets.newHashSet();
-    for (Label label : pkg.getSubincludeLabels()) {
+    for (Label label : subincludes.keySet()) {
       // Declare a dependency on the package lookup for the package giving access to the label.
       subincludePackageLookupDepKeys.add(PackageLookupValue.key(label.getPackageIdentifier()));
     }
     Pair<? extends Map<PathFragment, PackageLookupValue>, Boolean> subincludePackageLookupResult =
         getPackageLookupDepsAndPropagateInconsistentFilesystemExceptions(
-            pkg.getPackageIdentifier(), subincludePackageLookupDepKeys, env,
-            packageWasOriginallyInError);
+            packageIdentifier, subincludePackageLookupDepKeys, env, containsErrors);
     Map<PathFragment, PackageLookupValue> subincludePackageLookupDeps =
         subincludePackageLookupResult.getFirst();
     packageShouldBeInError |= subincludePackageLookupResult.getSecond();
@@ -254,53 +256,64 @@ public class PackageFunction implements SkyFunction {
       if (subincludePackageLookupValue != null) {
         // Declare a dependency on the actual file that was subincluded.
         Path subincludeFilePath = subincludeEntry.getValue();
-        if (subincludeFilePath != null) {
-          if (!subincludePackageLookupValue.packageExists()) {
-            // Legacy blaze puts a non-null path when only when the package does indeed exist.
-            throw new InternalInconsistentFilesystemException(pkg.getPackageIdentifier(),
-                String.format("Unexpected package in %s. Was it modified during the build?",
-                    subincludeFilePath));
-          }
+        if (subincludeFilePath != null && !subincludePackageLookupValue.packageExists()) {
+          // Legacy blaze puts a non-null path when only when the package does indeed exist.
+          throw new InternalInconsistentFilesystemException(
+              packageIdentifier,
+              String.format(
+                  "Unexpected package in %s. Was it modified during the build?",
+                  subincludeFilePath));
+        }
+        if (subincludePackageLookupValue.packageExists()) {
           // Sanity check for consistency of Skyframe and legacy blaze.
           Path subincludeFilePathSkyframe =
               subincludePackageLookupValue.getRoot().getRelative(label.toPathFragment());
-          if (!subincludeFilePathSkyframe.equals(subincludeFilePath)) {
-            throw new InternalInconsistentFilesystemException(pkg.getPackageIdentifier(),
-                String.format("Inconsistent package location for %s: '%s' vs '%s'. "
-                + "Was the source tree modified during the build?",
-                label.getPackageFragment(), subincludeFilePathSkyframe, subincludeFilePath));
+          if (subincludeFilePath != null
+              && !subincludeFilePathSkyframe.equals(subincludeFilePath)) {
+            throw new InternalInconsistentFilesystemException(
+                packageIdentifier,
+                String.format(
+                    "Inconsistent package location for %s: '%s' vs '%s'. "
+                        + "Was the source tree modified during the build?",
+                    label.getPackageFragment(),
+                    subincludeFilePathSkyframe,
+                    subincludeFilePath));
           }
           // The actual file may be under a different package root than the package being
           // constructed.
           SkyKey subincludeSkyKey =
-              FileValue.key(RootedPath.toRootedPath(subincludePackageLookupValue.getRoot(),
-                  subincludeFilePath));
+              FileValue.key(
+                  RootedPath.toRootedPath(
+                      subincludePackageLookupValue.getRoot(),
+                      label.getPackageFragment().getRelative(label.getName())));
           subincludeFileDepKeys.add(subincludeSkyKey);
         }
       }
     }
-    packageShouldBeInError |= markFileDepsAndPropagateInconsistentFilesystemExceptions(
-        pkg.getPackageIdentifier(), subincludeFileDepKeys, env, packageWasOriginallyInError);
+    packageShouldBeInError |=
+        markFileDepsAndPropagateInconsistentFilesystemExceptions(
+            packageIdentifier, subincludeFileDepKeys, env, containsErrors);
 
     // TODO(bazel-team): In the long term, we want to actually resolve the glob patterns within
     // Skyframe. For now, just logging the glob requests provides correct incrementality and
     // adequate performance.
-    PackageIdentifier packageId = pkg.getPackageIdentifier();
     List<SkyKey> globDepKeys = Lists.newArrayList();
     for (Pair<String, Boolean> globPattern : globPatterns) {
       String pattern = globPattern.getFirst();
       boolean excludeDirs = globPattern.getSecond();
       SkyKey globSkyKey;
       try {
-        globSkyKey = GlobValue.key(packageId, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
+        globSkyKey =
+            GlobValue.key(packageIdentifier, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
       } catch (InvalidGlobPatternException e) {
         // Globs that make it to pkg.getGlobPatterns() should already be filtered for errors.
         throw new IllegalStateException(e);
       }
       globDepKeys.add(globSkyKey);
     }
-    packageShouldBeInError |= markGlobDepsAndPropagateInconsistentFilesystemExceptions(
-        pkg.getPackageIdentifier(), globDepKeys, env, packageWasOriginallyInError);
+    packageShouldBeInError |=
+        markGlobDepsAndPropagateInconsistentFilesystemExceptions(
+            packageIdentifier, globDepKeys, env, containsErrors);
     return packageShouldBeInError;
   }
 
@@ -330,11 +343,6 @@ public class PackageFunction implements SkyFunction {
 
     Package pkg = workspace.getPackage();
     Event.replayEventsOn(env.getListener(), pkg.getEvents());
-    if (pkg.containsErrors()) {
-      throw new PackageFunctionException(new BuildFileContainsErrorsException(
-          ExternalPackage.PACKAGE_IDENTIFIER, "Package 'external' contains errors"),
-          pkg.containsTemporaryErrors() ? Transience.TRANSIENT : Transience.PERSISTENT);
-    }
 
     return new PackageValue(pkg);
   }
@@ -382,12 +390,17 @@ public class PackageFunction implements SkyFunction {
     if (packageId.equals(ExternalPackage.PACKAGE_IDENTIFIER)) {
       return getExternalPackage(env, packageLookupValue.getRoot());
     }
-    PackageValue externalPackage = (PackageValue) env.getValue(
-        PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER));
+    SkyKey externalPackageKey = PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER);
+    PackageValue externalPackage = (PackageValue) env.getValue(externalPackageKey);
     if (externalPackage == null) {
       return null;
     }
     Package externalPkg = externalPackage.getPackage();
+    if (externalPkg.containsErrors()) {
+      throw new PackageFunctionException(
+          new BuildFileContainsErrorsException(ExternalPackage.PACKAGE_IDENTIFIER),
+          Transience.PERSISTENT);
+    }
 
     PathFragment buildFileFragment = packageNameFragment.getChild("BUILD");
     RootedPath buildFileRootedPath = RootedPath.toRootedPath(packageLookupValue.getRoot(),
@@ -485,13 +498,12 @@ public class PackageFunction implements SkyFunction {
     }
     Collection<Pair<String, Boolean>> globPatterns = legacyPkgBuilder.getGlobPatterns();
     Map<Label, Path> subincludes = legacyPkgBuilder.getSubincludes();
-    Package pkg = legacyPkgBuilder.finishBuild();
-    Event.replayEventsOn(env.getListener(), pkg.getEvents());
+    Event.replayEventsOn(env.getListener(), legacyPkgBuilder.getEvents());
     boolean packageShouldBeConsideredInError;
     try {
       packageShouldBeConsideredInError =
-          markDependenciesAndPropagateInconsistentFilesystemExceptions(pkg, env,
-              globPatterns, subincludes);
+          markDependenciesAndPropagateInconsistentFilesystemExceptions(
+              env, globPatterns, subincludes, packageId, legacyPkgBuilder.containsErrors());
     } catch (InternalInconsistentFilesystemException e) {
       packageFunctionCache.invalidate(packageId);
       throw new PackageFunctionException(e,
@@ -501,14 +513,15 @@ public class PackageFunction implements SkyFunction {
     if (env.valuesMissing()) {
       return null;
     }
-    // We know this SkyFunction will not be called again, so we can remove the cache entry.
-    packageFunctionCache.invalidate(packageId);
 
     if (packageShouldBeConsideredInError) {
-      throw new PackageFunctionException(new BuildFileContainsErrorsException(pkg,
-          "Package '" + packageName + "' contains errors"),
-          pkg.containsTemporaryErrors() ? Transience.TRANSIENT : Transience.PERSISTENT);
+      legacyPkgBuilder.setContainsErrors();
     }
+    Package pkg = legacyPkgBuilder.finishBuild();
+
+    // We know this SkyFunction will not be called again, so we can remove the cache entry.
+    packageFunctionCache.invalidate(packageId);
+
     return new PackageValue(pkg);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
index ade85a7727..0acf9d9c0f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
@@ -17,6 +17,7 @@ import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -35,6 +36,12 @@ public class PackageValue implements SkyValue {
     this.pkg = Preconditions.checkNotNull(pkg);
   }
 
+  /**
+   * Returns the package. This package may contain errors, in which case the caller should throw
+   * a {@link BuildFileContainsErrorsException} if an error-free package is needed. See also
+   * {@link PackageErrorFunction} for the case where encountering a package with errors should shut
+   * down the build but the caller can handle packages with errors.
+   */
   public Package getPackage() {
     return pkg;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
index 4f9fcab1c7..d0c76333f2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
@@ -163,15 +163,18 @@ abstract class RecursiveDirectoryTraversalFunction
             return null;
           }
           pkg = pkgValue.getPackage();
+          if (pkg.containsErrors()) {
+            env
+                .getListener()
+                .handle(
+                    Event.error("package contains errors: " + rootRelativePath.getPathString()));
+          }
         } catch (NoSuchPackageException e) {
           // The package had errors, but don't fail-fast as there might be subpackages below the
-          // current directory, and there might be targets in the package that were successfully
-          // loaded.
-          env.getListener().handle(Event.error(
-              "package contains errors: " + rootRelativePath.getPathString()));
-          if (e.getPackage() != null) {
-            pkg = e.getPackage();
-          }
+          // current directory.
+          env
+              .getListener()
+              .handle(Event.error("package contains errors: " + rootRelativePath.getPathString()));
         }
         if (pkg != null) {
           visitor.visitPackageValue(pkg, env);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index 051215d39c..fa14cd9c47 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -68,16 +68,7 @@ public class RecursivePackageProviderBackedTargetPatternResolver
    */
   private Package getPackage(PackageIdentifier pkgIdentifier)
       throws NoSuchPackageException, InterruptedException {
-    Package pkg;
-    try {
-      pkg = recursivePackageProvider.getPackage(eventHandler, pkgIdentifier);
-    } catch (NoSuchPackageException e) {
-      pkg = e.getPackage();
-      if (pkg == null) {
-        throw e;
-      }
-    }
-    return pkg;
+    return recursivePackageProvider.getPackage(eventHandler, pkgIdentifier);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
index aba4e49062..37d625c8bb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -40,6 +40,7 @@ public final class SkyFunctions {
       SkyFunctionName.create("SKYLARK_IMPORTS_LOOKUP");
   public static final SkyFunctionName GLOB = SkyFunctionName.create("GLOB");
   public static final SkyFunctionName PACKAGE = SkyFunctionName.create("PACKAGE");
+  public static final SkyFunctionName PACKAGE_ERROR = SkyFunctionName.create("PACKAGE_ERROR");
   public static final SkyFunctionName TARGET_MARKER = SkyFunctionName.create("TARGET_MARKER");
   public static final SkyFunctionName TARGET_PATTERN = SkyFunctionName.create("TARGET_PATTERN");
   public static final SkyFunctionName PREPARE_DEPS_OF_PATTERNS =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
index 1c7cbfa751..6644611493 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
@@ -22,7 +22,6 @@ import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyKey;
-import com.google.devtools.build.skyframe.SkyValue;
 
 import javax.annotation.Nullable;
 
@@ -58,11 +57,10 @@ public final class SkyframeDependencyResolver extends DependencyResolver {
       return null;
     }
     SkyKey key = PackageValue.key(label.getPackageIdentifier());
-    SkyValue value = env.getValue(key);
-    if (value == null) {
+    PackageValue packageValue = (PackageValue) env.getValue(key);
+    if (packageValue == null || packageValue.getPackage().containsErrors()) {
       return null;
     }
-    PackageValue packageValue = (PackageValue) value;
     return packageValue.getPackage().getTarget(label.getName());
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 9a4f082abd..252df16a3b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -326,6 +326,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     map.put(SkyFunctions.PACKAGE, new PackageFunction(
         reporter, pkgFactory, packageManager, showLoadingProgress, packageFunctionCache,
         preprocessCache, numPackagesLoaded));
+    map.put(SkyFunctions.PACKAGE_ERROR, new PackageErrorFunction());
     map.put(SkyFunctions.TARGET_MARKER, new TargetMarkerFunction());
     map.put(SkyFunctions.TRANSITIVE_TARGET, new TransitiveTargetFunction(ruleClassProvider));
     map.put(SkyFunctions.TRANSITIVE_TRAVERSAL, new TransitiveTraversalFunction());
@@ -1356,20 +1357,27 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       }
 
       try {
-        return callUninterruptibly(new Callable<Set<Package>>() {
-          @Override
-          public Set<Package> call() throws Exception {
-            EvaluationResult<PackageValue> result = buildDriver.evaluate(
-                valueNames, false, ResourceUsage.getAvailableProcessors(), errorEventListener);
-            Preconditions.checkState(!result.hasError(),
-                "unexpected errors: %s", result.errorMap());
-            Set<Package> packages = Sets.newHashSet();
-            for (PackageValue value : result.values()) {
-              packages.add(value.getPackage());
-            }
-            return packages;
-          }
-        });
+        return callUninterruptibly(
+            new Callable<Set<Package>>() {
+              @Override
+              public Set<Package> call() throws Exception {
+                EvaluationResult<PackageValue> result =
+                    buildDriver.evaluate(
+                        valueNames,
+                        false,
+                        ResourceUsage.getAvailableProcessors(),
+                        errorEventListener);
+                Preconditions.checkState(
+                    !result.hasError(), "unexpected errors: %s", result.errorMap());
+                Set<Package> packages = Sets.newHashSet();
+                for (PackageValue value : result.values()) {
+                  Package pkg = value.getPackage();
+                  Preconditions.checkState(!pkg.containsErrors(), pkg.getName());
+                  packages.add(pkg);
+                }
+                return packages;
+              }
+            });
       } catch (Exception e) {
         throw new IllegalStateException(e);
       }
@@ -1475,8 +1483,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         EvaluationResult<PackageValue> result =
             buildDriver.evaluate(ImmutableList.of(key), /*keepGoing=*/true,
                 DEFAULT_THREAD_COUNT, eventHandler);
-        if (result.hasError()) {
-          ErrorInfo error = result.getError();
+        ErrorInfo error = result.getError(key);
+        if (error != null) {
           if (!Iterables.isEmpty(error.getCycleInfo())) {
             reportCycles(result.getError().getCycleInfo(), key);
             // This can only happen if a package is freshly loaded outside of the target parsing
@@ -1486,7 +1494,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
           }
           Throwable e = error.getException();
           // PackageFunction should be catching, swallowing, and rethrowing all transitive
-          // errors as NoSuchPackageExceptions.
+          // errors as NoSuchPackageExceptions or constructing packages with errors, since we're in
+          // keep_going mode.
           Throwables.propagateIfInstanceOf(e, NoSuchPackageException.class);
           throw new IllegalStateException("Unexpected Exception type from PackageValue for '"
               + pkgName + "'' with root causes: " + Iterables.toString(error.getRootCauses()), e);
@@ -1500,16 +1509,14 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       // so this will never throw for packages that are not loaded. However, no code currently
       // relies on having the exception thrown.
       try {
-        return callUninterruptibly(new Callable<Package>() {
-          @Override
-          public Package call() throws Exception {
-            return getPackage(errorEventListener, pkgName);
-          }
-        });
+        return callUninterruptibly(
+            new Callable<Package>() {
+              @Override
+              public Package call() throws InterruptedException, NoSuchPackageException {
+                return getPackage(errorEventListener, pkgName);
+              }
+            });
       } catch (NoSuchPackageException e) {
-        if (e.getPackage() != null) {
-          return e.getPackage();
-        }
         throw e;
       } catch (Exception e) {
         throw new IllegalStateException(e);  // Should never happen.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
index 80ccb1380e..b7d94e7dd7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
@@ -46,7 +46,9 @@ class SkyframePackageLoaderWithValueEnvironment implements
     this.env = env;
   }
 
-  private Package getPackage(PackageIdentifier pkgIdentifier) throws NoSuchPackageException{
+  @Override
+  public Package getLoadedPackage(final PackageIdentifier pkgIdentifier)
+      throws NoSuchPackageException {
     SkyKey key = PackageValue.key(pkgIdentifier);
     PackageValue value = (PackageValue) env.getValueOrThrow(key, NoSuchPackageException.class);
     if (value != null) {
@@ -56,19 +58,6 @@ class SkyframePackageLoaderWithValueEnvironment implements
   }
 
   @Override
-  public Package getLoadedPackage(final PackageIdentifier pkgIdentifier)
-      throws NoSuchPackageException {
-    try {
-      return getPackage(pkgIdentifier);
-    } catch (NoSuchPackageException e) {
-      if (e.getPackage() != null) {
-        return e.getPackage();
-      }
-      throw e;
-    }
-  }
-
-  @Override
   public Target getLoadedTarget(Label label) throws NoSuchPackageException,
       NoSuchTargetException {
     Package pkg = getLoadedPackage(label.getPackageIdentifier());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index 161a218660..4742ffbcb8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -78,14 +78,7 @@ class SkyframePackageManager implements PackageManager {
   @Override
   public Package getPackage(EventHandler eventHandler, PackageIdentifier packageIdentifier)
       throws NoSuchPackageException, InterruptedException {
-    try {
-      return packageLoader.getPackage(eventHandler, packageIdentifier);
-    } catch (NoSuchPackageException e) {
-      if (e.getPackage() != null) {
-        return e.getPackage();
-      }
-      throw e;
-    }
+    return packageLoader.getPackage(eventHandler, packageIdentifier);
   }
 
   @Override
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 75d4ad80bd..23186442d9 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
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -79,7 +80,6 @@ public final class TargetMarkerFunction implements SkyFunction {
     }
 
     SkyKey pkgSkyKey = PackageValue.key(label.getPackageIdentifier());
-    NoSuchPackageException nspe = null;
     Package pkg;
     try {
       PackageValue value = (PackageValue)
@@ -89,14 +89,8 @@ public final class TargetMarkerFunction implements SkyFunction {
       }
       pkg = value.getPackage();
     } catch (NoSuchPackageException e) {
-      // For consistency with pre-Skyframe Blaze, we can return a valid Target from a Package
-      // containing errors.
-      pkg = e.getPackage();
-      if (pkg == null) {
-        // Re-throw this exception with our key because root causes should be targets, not packages.
-        throw new TargetMarkerFunctionException(e);
-      }
-      nspe = e;
+      // Re-throw this exception with our key because root causes should be targets, not packages.
+      throw new TargetMarkerFunctionException(e);
     }
 
     Target target;
@@ -106,12 +100,14 @@ public final class TargetMarkerFunction implements SkyFunction {
       throw new TargetMarkerFunctionException(e);
     }
 
-    if (nspe != null) {
+    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, nspe));
+      throw new TargetMarkerFunctionException(
+          new NoSuchTargetException(
+              target, new BuildFileContainsErrorsException(label.getPackageIdentifier())));
     }
     return TargetMarkerValue.TARGET_MARKER_INSTANCE;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
index d627a0c1f1..90fd670225 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
@@ -18,12 +18,10 @@ import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
-import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.syntax.Label;
-import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.WalkableGraph;
 
 import java.util.HashSet;
@@ -106,16 +104,9 @@ abstract class TargetPatternsResultBuilder {
 
   private Package findPackageInGraph(PackageIdentifier pkgIdentifier,
       WalkableGraph walkableGraph) {
-    SkyKey key = PackageValue.key(pkgIdentifier);
-    Package pkg = null;
-    NoSuchPackageException nspe = (NoSuchPackageException) walkableGraph.getException(key);
-    if (nspe != null) {
-      pkg = nspe.getPackage();
-    } else {
-      pkg = ((PackageValue) walkableGraph.getValue(key)).getPackage();
-    }
-    Preconditions.checkNotNull(pkg, pkgIdentifier);
-    return pkg;
+    return Preconditions.checkNotNull(
+            ((PackageValue) walkableGraph.getValue(PackageValue.key(pkgIdentifier))), pkgIdentifier)
+        .getPackage();
   }
 
   /**
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 5bb9c4f451..130f681d88 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
@@ -18,6 +18,7 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+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;
@@ -303,9 +304,13 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
         return ValuesMissing.INSTANCE;
       }
 
+      Package pkg = packageValue.getPackage();
+      if (pkg.containsErrors()) {
+        throw new BuildFileContainsErrorsException(label.getPackageIdentifier());
+      }
       packageLoadedSuccessfully = true;
       try {
-        target = packageValue.getPackage().getTarget(label.getName());
+        target = pkg.getTarget(label.getName());
       } catch (NoSuchTargetException unexpected) {
         // Not expected since the TargetMarkerFunction would have failed earlier if the Target
         // was not present.
@@ -318,18 +323,9 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
 
       // 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 ValuesMissing.INSTANCE;
-        }
-        throw new IllegalStateException(
-            "Expected bad package: " + label.getPackageIdentifier());
-      } catch (NoSuchPackageException nsp) {
-        pkg = Preconditions.checkNotNull(nsp.getPackage(), label.getPackageIdentifier());
-      }
+      PackageValue packageValue =
+          (PackageValue) Preconditions.checkNotNull(env.getValue(packageKey), label);
+      Package pkg = packageValue.getPackage();
       try {
         target = pkg.getTarget(label.getName());
       } catch (NoSuchTargetException nste) {
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 ab47e4f86c..6d6bce0a5a 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
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.packages.Attribute;
 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.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;
@@ -162,11 +163,17 @@ public class TransitiveTargetFunction
       for (Entry<Attribute, Label> entry : transitions.entries()) {
         SkyKey packageKey = PackageValue.key(entry.getValue().getPackageIdentifier());
         try {
-          PackageValue pkgValue = (PackageValue) env.getValueOrThrow(packageKey,
-              NoSuchThingException.class);
+          PackageValue pkgValue =
+              (PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class);
           if (pkgValue == null) {
             continue;
           }
+          Package pkg = pkgValue.getPackage();
+          if (pkg.containsErrors()) {
+            // Do nothing. This error was handled when we computed the corresponding
+            // TransitiveTargetValue.
+            continue;
+          }
           Collection<Label> labels = AspectDefinition.visitAspectsIfRequired(target, entry.getKey(),
               pkgValue.getPackage().getTarget(entry.getValue().getName())).values();
           for (Label label : labels) {