Make AspectFunction error handling match TopLevelAspectFunction.

Also match ConfiguredTargetFunction for target loading.

It isn't currently possible to trigger either of these code paths
- the loading phase ensures that we never attempt to analyze targets that fail
  to load
- the Skylark import or conversion cannot fail, because Skylark checks during
  .bzl execution that all referenced symbols are Skylark aspects

Therefore, the only way to trigger this would be if there was a native rule
requesting a non-existent or broken Skylark aspect for its dependencies, but
that is currently not possible - native rules can only request native aspects.

However, for interleaved loading and analysis, we need to limit the set of
exception classes that can be thrown from AspectFunction - we do that here
by changing the constructor of AspectFunctionException to only accept either
NoSuchThingException or AspectCreationException.

That in turn requires that we re-throw the Skylark import and conversion
exceptions as AspectCreationException, which is exactly what
TopLevelAspectFunction is already doing, and necessary for correct error
handling if we do ever support Skylark aspects in native rules.

Alternatively, I could change the code path to crash Bazel, but that seems
strictly worse. Even if we can't test this code, it's conceptually the right
way to handle these errors.

I'll move part of the error handling into loadSkylarkAspect in a subsequent
change.

--
MOS_MIGRATED_REVID=112938284

2 files changed, 12 insertions, 16 deletions

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 dca5c79101..8d14af31f1 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
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
@@ -111,9 +112,11 @@ public final class AspectFunction implements SkyFunction {
             loadSkylarkAspect(
                 env, skylarkAspectClass.getExtensionLabel(), skylarkAspectClass.getExportedName());
       } catch (SkylarkImportFailedException e) {
-        throw new AspectFunctionException(skyKey, e);
+        env.getListener().handle(Event.error(e.getMessage()));
+        throw new AspectFunctionException(new AspectCreationException(e.getMessage()));
       } catch (ConversionException e) {
-        throw new AspectFunctionException(skyKey, e);
+        env.getListener().handle(Event.error(e.getMessage()));
+        throw new AspectFunctionException(new AspectCreationException(e.getMessage()));
       }
       if (skylarkAspect == null) {
         return null;
@@ -124,23 +127,22 @@ public final class AspectFunction implements SkyFunction {
       throw new IllegalStateException();
     }
 
+    // Keep this in sync with the same code in ConfiguredTargetFunction.
     PackageValue packageValue =
         (PackageValue) env.getValue(PackageValue.key(key.getLabel().getPackageIdentifier()));
     if (packageValue == null) {
       return null;
     }
-
     Package pkg = packageValue.getPackage();
     if (pkg.containsErrors()) {
       throw new AspectFunctionException(
-          skyKey, new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier()));
+          new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier()));
     }
-
     Target target;
     try {
       target = pkg.getTarget(key.getLabel().getName());
     } catch (NoSuchTargetException e) {
-      throw new AspectFunctionException(skyKey, e);
+      throw new AspectFunctionException(e);
     }
 
     if (!(target instanceof Rule)) {
@@ -302,18 +304,12 @@ public final class AspectFunction implements SkyFunction {
    * Used to indicate errors during the computation of an {@link AspectValue}.
    */
   private static final class AspectFunctionException extends SkyFunctionException {
-    public AspectFunctionException(AspectCreationException e) {
+    public AspectFunctionException(NoSuchThingException e) {
       super(e, Transience.PERSISTENT);
     }
 
-    /** Used to rethrow a child error that we cannot handle. */
-    public AspectFunctionException(SkyKey childKey, Exception transitiveError) {
-      super(transitiveError, childKey);
-    }
-
-    /** Used to rethrow a child error that we cannot handle. */
-    public AspectFunctionException(SkyKey childKey, NoSuchThingException e) {
-      super(e, childKey);
+    public AspectFunctionException(AspectCreationException e) {
+      super(e, Transience.PERSISTENT);
     }
   }
 }
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 1d6329a6a1..8b55b1c59a 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
@@ -135,7 +135,6 @@ final class ConfiguredTargetFunction implements SkyFunction {
     if (packageValue == null) {
       return null;
     }
-
     Package pkg = packageValue.getPackage();
     if (pkg.containsErrors()) {
       throw new ConfiguredTargetFunctionException(
@@ -147,6 +146,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
     } catch (NoSuchTargetException e) {
       throw new ConfiguredTargetFunctionException(e);
     }
+
     transitivePackages.add(packageValue.getPackage());
     // TODO(bazel-team): This is problematic - we create the right key, but then end up with a value
     // that doesn't match; we can even have the same value multiple times. However, I think it's