Do not crash when aspects provide duplicate things.

--
MOS_MIGRATED_REVID=138860974

6 files changed, 62 insertions, 21 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
index 9a5317f5a0..81f99e6b7c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
@@ -31,6 +31,17 @@ public final class MergedConfiguredTarget extends AbstractConfiguredTarget {
   private final ConfiguredTarget base;
   private final TransitiveInfoProviderMap providers;
 
+  /**
+   * This exception is thrown when configured targets and aspects
+   * being merged provide duplicate things that they shouldn't
+   * (output groups or providers).
+   */
+  public static final class DuplicateException extends Exception {
+    public DuplicateException(String message) {
+      super(message);
+    }
+  }
+
   private MergedConfiguredTarget(ConfiguredTarget base, TransitiveInfoProviderMap providers) {
     super(base.getTarget(), base.getConfiguration());
     this.base = base;
@@ -62,7 +73,8 @@ public final class MergedConfiguredTarget extends AbstractConfiguredTarget {
   }
 
   /** Creates an instance based on a configured target and a set of aspects. */
-  public static ConfiguredTarget of(ConfiguredTarget base, Iterable<ConfiguredAspect> aspects) {
+  public static ConfiguredTarget of(ConfiguredTarget base, Iterable<ConfiguredAspect> aspects)
+      throws DuplicateException {
     if (Iterables.isEmpty(aspects)) {
       // If there are no aspects, don't bother with creating a proxy object
       return base;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java
index 113fa3f556..d11e3091bc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -124,7 +125,8 @@ public final class OutputGroupProvider implements TransitiveInfoProvider {
    * @param providers providers to merge {@code this} with.
    */
   @Nullable
-  public static OutputGroupProvider merge(List<OutputGroupProvider> providers) {
+  public static OutputGroupProvider merge(List<OutputGroupProvider> providers)
+      throws DuplicateException {
     if (providers.size() == 0) {
       return null;
     }
@@ -137,7 +139,8 @@ public final class OutputGroupProvider implements TransitiveInfoProvider {
     for (OutputGroupProvider provider : providers) {
       for (String outputGroup : provider.outputGroups.keySet()) {
         if (!seenGroups.add(outputGroup)) {
-          throw new IllegalStateException("Output group " + outputGroup + " provided twice");
+          throw new DuplicateException(
+              "Output group " + outputGroup + " provided twice");
         }
 
         resultBuilder.put(outputGroup, provider.getOutputGroup(outputGroup));
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
index 6ed41d9009..a449f5db4a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.analysis;
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor;
@@ -107,7 +108,8 @@ public final class SkylarkProviders implements TransitiveInfoProvider {
    *
    * @param providers providers to merge {@code this} with.
    */
-  public static SkylarkProviders merge(List<SkylarkProviders> providers) {
+  public static SkylarkProviders merge(List<SkylarkProviders> providers)
+      throws DuplicateException {
     if (providers.size() == 0) {
       return null;
     }
@@ -124,7 +126,8 @@ public final class SkylarkProviders implements TransitiveInfoProvider {
   }
 
   private static <K, V> ImmutableMap<K, V> mergeMaps(List<SkylarkProviders> providers,
-      Function<SkylarkProviders, Map<K, V>> mapGetter) {
+      Function<SkylarkProviders, Map<K, V>> mapGetter)
+      throws DuplicateException {
     ImmutableMap.Builder<K, V> resultBuilder = new ImmutableMap.Builder<>();
     Set<K> seenKeys = new HashSet<>();
     for (SkylarkProviders provider : providers) {
@@ -132,7 +135,7 @@ public final class SkylarkProviders implements TransitiveInfoProvider {
       for (K key : map.keySet()) {
         if (!seenKeys.add(key)) {
           // TODO(dslomov): add better diagnostics.
-          throw new IllegalStateException("Skylark provider " + key + " provided twice");
+          throw new DuplicateException("Provider " + key + " provided twice");
         }
 
         resultBuilder.put(key, map.get(key));
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 ba84c5b7fb..0b7c91160a 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
@@ -46,6 +46,7 @@ import com.google.devtools.build.lib.packages.SkylarkAspectClass;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.rules.AliasProvider;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredTargetFunctionException;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredValueCreationException;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.DependencyEvaluationException;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor.BuildViewProvider;
@@ -231,17 +232,21 @@ public final class AspectFunction implements SkyFunction {
         return null;
       }
 
-      OrderedSetMultimap<Attribute, ConfiguredTarget> depValueMap =
-          ConfiguredTargetFunction.computeDependencies(
-              env,
-              resolver,
-              originalTargetAndAspectConfiguration,
-              aspect,
-              configConditions,
-              ruleClassProvider,
-              view.getHostConfiguration(originalTargetAndAspectConfiguration.getConfiguration()),
-              transitivePackages,
-              transitiveRootCauses);
+      OrderedSetMultimap<Attribute, ConfiguredTarget> depValueMap;
+      try {
+        depValueMap = ConfiguredTargetFunction.computeDependencies(
+            env,
+            resolver,
+            originalTargetAndAspectConfiguration,
+            aspect,
+            configConditions,
+            ruleClassProvider,
+            view.getHostConfiguration(originalTargetAndAspectConfiguration.getConfiguration()),
+            transitivePackages,
+            transitiveRootCauses);
+      } catch (ConfiguredTargetFunctionException e) {
+        throw new AspectCreationException(e.getMessage());
+      }
       if (depValueMap == null) {
         return null;
       }
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 9c5c164384..c4bd5e5d78 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
@@ -37,6 +37,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.Dependency;
 import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
 import com.google.devtools.build.lib.analysis.MergedConfiguredTarget;
+import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -288,7 +289,8 @@ final class ConfiguredTargetFunction implements SkyFunction {
       BuildConfiguration hostConfiguration,
       NestedSetBuilder<Package> transitivePackages,
       NestedSetBuilder<Label> transitiveLoadingRootCauses)
-      throws DependencyEvaluationException, AspectCreationException, InterruptedException {
+      throws DependencyEvaluationException, ConfiguredTargetFunctionException,
+      AspectCreationException, InterruptedException {
     // Create the map from attributes to set of (target, configuration) pairs.
     OrderedSetMultimap<Attribute, Dependency> depValueNames;
     try {
@@ -328,7 +330,15 @@ final class ConfiguredTargetFunction implements SkyFunction {
     }
 
     // Merge the dependent configured targets and aspects into a single map.
-    return mergeAspects(depValueNames, depValues, depAspects);
+    try {
+      return mergeAspects(depValueNames, depValues, depAspects);
+    } catch (DuplicateException e) {
+      env.getListener().handle(
+          Event.error(ctgValue.getTarget().getLocation(), e.getMessage()));
+
+      throw new ConfiguredTargetFunctionException(
+          new ConfiguredValueCreationException(e.getMessage(), ctgValue.getLabel()));
+    }
   }
 
   /**
@@ -716,7 +726,8 @@ final class ConfiguredTargetFunction implements SkyFunction {
   private static OrderedSetMultimap<Attribute, ConfiguredTarget> mergeAspects(
       OrderedSetMultimap<Attribute, Dependency> depValueNames,
       Map<SkyKey, ConfiguredTarget> depConfiguredTargetMap,
-      OrderedSetMultimap<SkyKey, ConfiguredAspect> depAspectMap) {
+      OrderedSetMultimap<SkyKey, ConfiguredAspect> depAspectMap)
+      throws DuplicateException  {
     OrderedSetMultimap<Attribute, ConfiguredTarget> result = OrderedSetMultimap.create();
 
     for (Map.Entry<Attribute, Dependency> entry : depValueNames.entries()) {
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 ebb660ae96..6e7d591afd 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
@@ -56,6 +56,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.Dependency;
 import com.google.devtools.build.lib.analysis.MergedConfiguredTarget;
+import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
@@ -1243,7 +1244,13 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         configuredAspects.add(((AspectValue) result.get(aspectKey)).getConfiguredAspect());
       }
 
-      cts.put(key, MergedConfiguredTarget.of(configuredTarget, configuredAspects));
+      try {
+        cts.put(key, MergedConfiguredTarget.of(configuredTarget, configuredAspects));
+      } catch (DuplicateException e) {
+        throw new IllegalStateException(
+            String.format("Error creating %s", configuredTarget.getTarget().getLabel()),
+            e);
+      }
     }
 
     return cts.build();