General cleanup for the configured target / aspect creation code.

- update some comments
- add some comments to make it easier to follow
- delete some dead code, in particular the SkyframeDependencyResolver can
  never be null; remove an non-applicable @Nullable annotation

I'm trying to figure out how the error handling code works, in order to add
support for interleaved loading+analysis, which requires handling loading
errors in this code path.

--
MOS_MIGRATED_REVID=112456674

8 files changed, 24 insertions, 32 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index 992e7a203b..3565324489 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -243,6 +243,7 @@ public final class ConfiguredTargetFactory {
         ruleContext.ruleError(missingFragmentError(ruleContext, configurationFragmentPolicy));
         return null;
       }
+      // Otherwise missingFragmentPolicy == MissingFragmentPolicy.CREATE_FAIL_ACTIONS:
       return createFailConfiguredTarget(ruleContext);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index d5a515abbc..92f7fec4b4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -485,7 +485,7 @@ public abstract class DependencyResolver {
                     "Late bound attribute '%s' is not a label or a label list",
                     attribute.getName()));
           }
-        } catch (ClassCastException e) {
+        } catch (ClassCastException e) { // From either of the cast calls above.
           throw new EvalException(
               rule.getLocation(),
               String.format(
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 fad923c237..aafa94364d 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
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NativeAspectClass;
 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;
@@ -159,13 +160,9 @@ public final class AspectFunction implements SkyFunction {
     }
 
     SkyframeDependencyResolver resolver = view.createDependencyResolver(env);
-    if (resolver == null) {
-      return null;
-    }
 
     TargetAndConfiguration ctgValue =
         new TargetAndConfiguration(target, key.getConfiguration());
-
     try {
       // Get the configuration targets that trigger this rule's configurable attributes.
       Set<ConfigMatchingProvider> configConditions = ConfiguredTargetFunction.getConfigConditions(
@@ -276,7 +273,7 @@ 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(Exception e) {
+    public AspectFunctionException(AspectCreationException e) {
       super(e, Transience.PERSISTENT);
     }
 
@@ -284,6 +281,10 @@ public final class AspectFunction implements SkyFunction {
     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);
+    }
+  }
 }
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 70d72331e5..0532d72990 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
@@ -65,7 +65,6 @@ import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.util.Preconditions;
 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;
@@ -151,8 +150,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
     Package pkg = packageValue.getPackage();
     if (pkg.containsErrors()) {
       throw new ConfiguredTargetFunctionException(
-          new BuildFileContainsErrorsException(lc.getLabel().getPackageIdentifier()),
-          Transience.PERSISTENT);
+          new BuildFileContainsErrorsException(lc.getLabel().getPackageIdentifier()));
     }
     Target target;
     try {
@@ -169,14 +167,11 @@ final class ConfiguredTargetFunction implements SkyFunction {
     if (!target.isConfigurable()) {
       configuration = null;
     }
-    TargetAndConfiguration ctgValue =
-        new TargetAndConfiguration(target, configuration);
 
     SkyframeDependencyResolver resolver = view.createDependencyResolver(env);
-    if (resolver == null) {
-      return null;
-    }
 
+    TargetAndConfiguration ctgValue =
+        new TargetAndConfiguration(target, configuration);
     try {
       // Get the configuration targets that trigger this rule's configurable attributes.
       Set<ConfigMatchingProvider> configConditions =
@@ -754,7 +749,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
     if (events.hasErrors()) {
       analysisEnvironment.disable(target);
       throw new ConfiguredTargetFunctionException(new ConfiguredValueCreationException(
-              "Analysis of target '" + target.getLabel() + "' failed; build aborted"));
+          "Analysis of target '" + target.getLabel() + "' failed; build aborted"));
     }
     Preconditions.checkState(!analysisEnvironment.hasErrors(),
         "Analysis environment hasError() but no errors reported");
@@ -765,13 +760,16 @@ final class ConfiguredTargetFunction implements SkyFunction {
     analysisEnvironment.disable(target);
     Preconditions.checkNotNull(configuredTarget, target);
 
+    Map<Artifact, Action> generatingActions;
+    // Check for conflicting actions within this configured target (that indicates a bug in the
+    // rule implementation).
     try {
-      return new ConfiguredTargetValue(configuredTarget,
-          filterSharedActionsAndThrowIfConflict(analysisEnvironment.getRegisteredActions()),
-          transitivePackages.build());
+      generatingActions = filterSharedActionsAndThrowIfConflict(analysisEnvironment.getRegisteredActions());
     } catch (ActionConflictException e) {
       throw new ConfiguredTargetFunctionException(e);
     }
+    return new ConfiguredTargetValue(
+        configuredTarget, generatingActions, transitivePackages.build());
   }
 
   static Map<Artifact, Action> filterSharedActionsAndThrowIfConflict(Iterable<Action> actions)
@@ -794,7 +792,6 @@ final class ConfiguredTargetFunction implements SkyFunction {
    * a ConfiguredTargetValue.
    */
   public static final class ConfiguredValueCreationException extends Exception {
-
     public ConfiguredValueCreationException(String message) {
       super(message);
     }
@@ -805,15 +802,10 @@ final class ConfiguredTargetFunction implements SkyFunction {
    * {@link ConfiguredTargetFunction#compute}.
    */
   public static final class ConfiguredTargetFunctionException extends SkyFunctionException {
-    public ConfiguredTargetFunctionException(NoSuchTargetException e) {
+    public ConfiguredTargetFunctionException(NoSuchThingException e) {
       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/PostConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
index a109dab801..c5a54dcdd5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
@@ -75,8 +75,6 @@ public class PostConfiguredTargetFunction implements SkyFunction {
     ImmutableMap<Action, ConflictException> badActions = PrecomputedValue.BAD_ACTIONS.get(env);
     ConfiguredTargetValue ctValue = (ConfiguredTargetValue)
         env.getValue(ConfiguredTargetValue.key((ConfiguredTargetKey) skyKey.argument()));
-    SkyframeDependencyResolver resolver =
-        buildViewProvider.getSkyframeBuildView().createDependencyResolver(env);
     if (env.valuesMissing()) {
       return null;
     }
@@ -101,6 +99,8 @@ public class PostConfiguredTargetFunction implements SkyFunction {
     try {
       BuildConfiguration hostConfiguration =
           buildViewProvider.getSkyframeBuildView().getHostConfiguration(ct.getConfiguration());
+      SkyframeDependencyResolver resolver =
+          buildViewProvider.getSkyframeBuildView().createDependencyResolver(env);
       deps =
           resolver.dependentNodeMap(
               ctgValue, hostConfiguration, /*aspect=*/ null, configConditions);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index f60d8804c9..be7ff9bba3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -534,7 +534,6 @@ public final class SkyframeBuildView {
     return trimmedConfig;
   }
 
-  @Nullable
   SkyframeDependencyResolver createDependencyResolver(Environment env) {
     return new SkyframeDependencyResolver(env);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
index 5cf7d97bf5..cac4c02216 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
@@ -89,7 +89,6 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction {
    * Exceptions thrown from ToplevelSkylarkAspectFunction.
    */
   public class LoadSkylarkAspectFunctionException extends SkyFunctionException {
-
     public LoadSkylarkAspectFunctionException(Exception cause, SkyKey childKey) {
       super(cause, childKey);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Type.java b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
index e543315251..49d0b74c3b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
@@ -143,9 +143,9 @@ public abstract class Type<T> {
 
   /**
    * Implementation of concatenation for this type (e.g. "val1 + val2"). Returns null to
-   * designate concatenation isn't supported.
+   * indicate concatenation isn't supported.
    */
-  public T concat(Iterable<T> elements) {
+  public T concat(@SuppressWarnings("unused") Iterable<T> elements) {
     return null;
   }