Do not crash when aspects provide duplicate things.

--
MOS_MIGRATED_REVID=138860974

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 <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass)
   }
 
   /** 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;

src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java

@@ -19,6 +19,7 @@
 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 NestedSet<Artifact> getOutputGroup(String outputGroupName) {
    * @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 static OutputGroupProvider merge(List<OutputGroupProvider> providers) {
     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));

src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java

@@ -16,6 +16,7 @@
 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 Map<SkylarkClassObjectConstructor.Key, SkylarkClassObject> apply(
    *
    * @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,15 +126,16 @@ public static SkylarkProviders merge(List<SkylarkProviders> providers) {
   }
 
   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) {
       Map<K, V> map = mapGetter.apply(provider);
       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));

src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java

@@ -46,6 +46,7 @@
 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 SkyValue compute(SkyKey skyKey, Environment env)
         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;
       }

src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java

@@ -37,6 +37,7 @@
 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 @@ static OrderedSetMultimap<Attribute, ConfiguredTarget> computeDependencies(
       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 @@ static OrderedSetMultimap<Attribute, ConfiguredTarget> computeDependencies(
     }
 
     // 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 @@ private static void checkForMissingFragments(Environment env, TargetAndConfigura
   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()) {

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -56,6 +56,7 @@
 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 ImmutableMap<Dependency, ConfiguredTarget> getConfiguredTargetMap(
         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();

src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java

@@ -560,6 +560,78 @@ public void topLevelAspectIsNotAnAspect() throws Exception {
     assertContainsEvent("MyAspect from //test:aspect.bzl is not an aspect");
   }
 
+
+  @Test
+  public void duplicateOutputGroups() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "  f = ctx.new_file('f.txt')",
+        "  ctx.file_action(f, 'f')",
+        "  return struct(output_groups = { 'duplicate' : set([f]) })",
+        "",
+        "MyAspect = aspect(implementation=_impl)",
+        "def _rule_impl(ctx):",
+        "  g = ctx.new_file('g.txt')",
+        "  ctx.file_action(g, 'g')",
+        "  return struct(output_groups = { 'duplicate' : set([g]) })",
+        "my_rule = rule(_rule_impl)",
+        "def _noop(ctx):",
+        "  pass",
+        "rbase = rule(_noop, attrs = { 'dep' : attr.label(aspects = [MyAspect]) })"
+    );
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'my_rule', 'rbase')",
+        "my_rule(name = 'xxx')",
+        "rbase(name = 'yyy', dep = ':xxx')"
+    );
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update("//test:yyy");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (ViewCreationFailedException e) {
+      // expect to fail.
+    }
+    assertContainsEvent("ERROR /workspace/test/BUILD:3:1: Output group duplicate provided twice");
+  }
+
+  @Test
+  public void duplicateSkylarkProviders() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "  return struct(duplicate = 'x')",
+        "",
+        "MyAspect = aspect(implementation=_impl)",
+        "def _rule_impl(ctx):",
+        "  return struct(duplicate = 'y')",
+        "my_rule = rule(_rule_impl)",
+        "def _noop(ctx):",
+        "  pass",
+        "rbase = rule(_noop, attrs = { 'dep' : attr.label(aspects = [MyAspect]) })"
+    );
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'my_rule', 'rbase')",
+        "my_rule(name = 'xxx')",
+        "rbase(name = 'yyy', dep = ':xxx')"
+    );
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update("//test:yyy");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (ViewCreationFailedException e) {
+      // expect to fail.
+    }
+    assertContainsEvent("ERROR /workspace/test/BUILD:3:1: Provider duplicate provided twice");
+  }
+
+
   @Test
   public void topLevelAspectDoesNotExist() throws Exception {
     scratch.file("test/aspect.bzl", "");