Implement 'output_groups' provider.

This behavior - that 'output_groups' is a provider available
on targets and aspects - has been accidental, but people already depend
on it. This CL keeps that behavior, while fixing the bug that
two aspects could not both provide output groups.

--
MOS_MIGRATED_REVID=139578378

6 files changed, 115 insertions, 51 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 81f99e6b7c..c8462980f7 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
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor.Key;
@@ -85,8 +86,15 @@ public final class MergedConfiguredTarget extends AbstractConfiguredTarget {
         OutputGroupProvider.merge(getAllProviders(base, aspects, OutputGroupProvider.class));
 
     // Merge Skylark providers.
+    ImmutableMap<String, Object> premergedProviders =
+        mergedOutputGroupProvider == null
+        ? ImmutableMap.<String, Object>of()
+        : ImmutableMap.<String, Object>of(
+            OutputGroupProvider.SKYLARK_NAME, mergedOutputGroupProvider);
     SkylarkProviders mergedSkylarkProviders =
-        SkylarkProviders.merge(getAllProviders(base, aspects, SkylarkProviders.class));
+        SkylarkProviders.merge(
+            premergedProviders,
+            getAllProviders(base, aspects, SkylarkProviders.class));
 
     // Merge extra-actions provider.
     ExtraActionArtifactsProvider mergedExtraActionProviders = ExtraActionArtifactsProvider.merge(
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 d11e3091bc..51954c137c 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
@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.devtools.build.lib.syntax.EvalUtils.SKYLARK_COMPARATOR;
+
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
@@ -24,11 +26,15 @@ 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;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-
+import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
+import com.google.devtools.build.lib.syntax.SkylarkIndexable;
+import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
-
 import javax.annotation.Nullable;
 
 /**
@@ -45,7 +51,9 @@ import javax.annotation.Nullable;
  * not mentioned on the output.
  */
 @Immutable
-public final class OutputGroupProvider implements TransitiveInfoProvider {
+public final class OutputGroupProvider implements
+    TransitiveInfoProvider, SkylarkIndexable, Iterable<String> {
+  public static String SKYLARK_NAME = "output_groups";
 
   /**
    * Prefix for output groups that are not reported to the user on the terminal output of Blaze when
@@ -185,4 +193,33 @@ public final class OutputGroupProvider implements TransitiveInfoProvider {
 
     return ImmutableSortedSet.copyOf(current);
   }
+
+  @Override
+  public Object getIndex(Object key, Location loc) throws EvalException {
+    if (!(key instanceof String)) {
+      throw new EvalException(loc, String.format(
+          "Output grout names must be strings, got %s instead",
+          EvalUtils.getDataTypeName(key)));
+    }
+
+    NestedSet<Artifact> result = outputGroups.get(key);
+    if (result != null) {
+      return SkylarkNestedSet.of(Artifact.class, result);
+    } else {
+      throw new EvalException(loc, String.format(
+          "Output group %s not present", key
+      ));
+    }
+
+  }
+
+  @Override
+  public boolean containsKey(Object key, Location loc) throws EvalException {
+    return outputGroups.containsKey(key);
+  }
+
+  @Override
+  public Iterator<String> iterator() {
+    return SKYLARK_COMPARATOR.sortedCopy(outputGroups.keySet()).iterator();
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 51d2e6fe0a..71b8d545e0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -131,7 +131,9 @@ public final class RuleConfiguredTargetBuilder {
         outputGroups.put(entry.getKey(), entry.getValue().build());
       }
 
-      add(OutputGroupProvider.class, new OutputGroupProvider(outputGroups.build()));
+      OutputGroupProvider outputGroupProvider = new OutputGroupProvider(outputGroups.build());
+      addProvider(OutputGroupProvider.class, outputGroupProvider);
+      addSkylarkTransitiveInfo(OutputGroupProvider.SKYLARK_NAME, outputGroupProvider);
     }
 
     TransitiveInfoProviderMap providers = providersBuilder.build();
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 a449f5db4a..e8fe2d581c 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
@@ -106,33 +106,44 @@ public final class SkylarkProviders implements TransitiveInfoProvider {
   /**
    * Merges skylark providers. The set of providers must be disjoint.
    *
+   * @param premergedProviders providers that has already been merged. They will
+   *        be put into the result as-is and their presence will be ignored among {@code providers}.
    * @param providers providers to merge {@code this} with.
    */
-  public static SkylarkProviders merge(List<SkylarkProviders> providers)
+  public static SkylarkProviders merge(
+      Map<String, Object> premergedProviders,
+      List<SkylarkProviders> providers)
       throws DuplicateException {
-    if (providers.size() == 0) {
+    if (premergedProviders.size() == 0 && providers.size() == 0) {
       return null;
     }
-    if (providers.size() == 1) {
+    if (premergedProviders.size() == 0 && providers.size() == 1) {
       return providers.get(0);
     }
 
     ImmutableMap<String, Object> skylarkProviders = mergeMaps(providers,
-        SKYLARK_PROVIDERS_MAP_FUNCTION);
+        SKYLARK_PROVIDERS_MAP_FUNCTION,
+        premergedProviders);
+
     ImmutableMap<SkylarkClassObjectConstructor.Key, SkylarkClassObject> declaredProviders =
-        mergeMaps(providers, DECLARED_PROVIDERS_MAP_FUNCTION);
+        mergeMaps(providers, DECLARED_PROVIDERS_MAP_FUNCTION,
+            ImmutableMap.<SkylarkClassObjectConstructor.Key, SkylarkClassObject>of());
 
     return new SkylarkProviders(skylarkProviders, declaredProviders);
   }
 
   private static <K, V> ImmutableMap<K, V> mergeMaps(List<SkylarkProviders> providers,
-      Function<SkylarkProviders, Map<K, V>> mapGetter)
+      Function<SkylarkProviders, Map<K, V>> mapGetter, Map<K, V> premerged)
       throws DuplicateException {
-    ImmutableMap.Builder<K, V> resultBuilder = new ImmutableMap.Builder<>();
     Set<K> seenKeys = new HashSet<>();
+    ImmutableMap.Builder<K, V> resultBuilder = ImmutableMap.builder();
+    resultBuilder.putAll(premerged);
     for (SkylarkProviders provider : providers) {
       Map<K, V> map = mapGetter.apply(provider);
       for (K key : map.keySet()) {
+        if (premerged.containsKey(key)) {
+          continue;
+        }
         if (!seenKeys.add(key)) {
           // TODO(dslomov): add better diagnostics.
           throw new DuplicateException("Provider " + key + " provided twice");
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
index 94c507787d..84f6a5f359 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
@@ -186,41 +186,47 @@ public final class SkylarkRuleConfiguredTargetBuilder {
 
     for (String outputGroup : outputGroups.keySet()) {
       SkylarkValue objects = outputGroups.get(outputGroup);
-      NestedSet<Artifact> artifacts;
+      NestedSet<Artifact> artifacts = convertToOutputGroupValue(loc, outputGroup, objects);
+      builder.addOutputGroup(outputGroup, artifacts);
+    }
+  }
 
-      String typeErrorMessage =
-          "Output group '%s' is of unexpected type. "
-              + "Should be list or set of Files, but got '%s' instead.";
+  public static NestedSet<Artifact> convertToOutputGroupValue(Location loc, String outputGroup,
+      SkylarkValue objects) throws EvalException {
+    NestedSet<Artifact> artifacts;
 
-      if (objects instanceof SkylarkList) {
-        NestedSetBuilder<Artifact> nestedSetBuilder = NestedSetBuilder.stableOrder();
-        for (Object o : (SkylarkList) objects) {
-          if (o instanceof Artifact) {
-            nestedSetBuilder.add((Artifact) o);
-          } else {
-            throw new EvalException(
-                loc,
-                String.format(
-                    typeErrorMessage,
-                    outputGroup,
-                    "list with an element of " + EvalUtils.getDataTypeNameFromClass(o.getClass())));
-          }
+    String typeErrorMessage =
+        "Output group '%s' is of unexpected type. "
+            + "Should be list or set of Files, but got '%s' instead.";
+
+    if (objects instanceof SkylarkList) {
+      NestedSetBuilder<Artifact> nestedSetBuilder = NestedSetBuilder.stableOrder();
+      for (Object o : (SkylarkList) objects) {
+        if (o instanceof Artifact) {
+          nestedSetBuilder.add((Artifact) o);
+        } else {
+          throw new EvalException(
+              loc,
+              String.format(
+                  typeErrorMessage,
+                  outputGroup,
+                  "list with an element of " + EvalUtils.getDataTypeNameFromClass(o.getClass())));
         }
-        artifacts = nestedSetBuilder.build();
-      } else {
-        artifacts =
-            SkylarkType.cast(
-                    objects,
-                    SkylarkNestedSet.class,
-                    Artifact.class,
-                    loc,
-                    typeErrorMessage,
-                    outputGroup,
-                    EvalUtils.getDataTypeName(objects, true))
-                .getSet(Artifact.class);
       }
-      builder.addOutputGroup(outputGroup, artifacts);
+      artifacts = nestedSetBuilder.build();
+    } else {
+      artifacts =
+          SkylarkType.cast(
+                  objects,
+                  SkylarkNestedSet.class,
+                  Artifact.class,
+                  loc,
+                  typeErrorMessage,
+                  outputGroup,
+                  EvalUtils.getDataTypeName(objects, true))
+              .getSet(Artifact.class);
     }
+    return artifacts;
   }
 
   private static ConfiguredTarget addStructFieldsAndBuild(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index b5b8087726..eb3922ad1a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -25,12 +24,13 @@ import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.AspectParameters;
 import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.packages.SkylarkClassObject;
+import com.google.devtools.build.lib.rules.SkylarkRuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.rules.SkylarkRuleContext;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalExceptionWithStackTrace;
 import com.google.devtools.build.lib.syntax.Mutability;
-import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
 import com.google.devtools.build.lib.syntax.SkylarkType;
 import java.util.Map;
 
@@ -90,8 +90,9 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory {
         for (String key : struct.getKeys()) {
           if (key.equals("output_groups")) {
             addOutputGroups(struct.getValue(key), loc, builder);
+          } else {
+            builder.addSkylarkTransitiveInfo(key, struct.getValue(key), loc);
           }
-          builder.addSkylarkTransitiveInfo(key, struct.getValue(key), loc);
         }
         ConfiguredAspect configuredAspect = builder.build();
         SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext);
@@ -101,21 +102,20 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory {
         ruleContext.ruleError("\n" + e.print());
         return null;
       }
-
     }
   }
 
   private static void addOutputGroups(Object value, Location loc,
       ConfiguredAspect.Builder builder)
       throws EvalException {
-    Map<String, SkylarkNestedSet> outputGroups = SkylarkType
-        .castMap(value, String.class, SkylarkNestedSet.class, "output_groups");
+    Map<String, SkylarkValue> outputGroups =
+        SkylarkType.castMap(value, String.class, SkylarkValue.class, "output_groups");
 
     for (String outputGroup : outputGroups.keySet()) {
-      SkylarkNestedSet objects = outputGroups.get(outputGroup);
+      SkylarkValue objects = outputGroups.get(outputGroup);
+
       builder.addOutputGroup(outputGroup,
-          SkylarkType.cast(objects, SkylarkNestedSet.class, Artifact.class, loc,
-              "Output group '%s'", outputGroup).getSet(Artifact.class));
+          SkylarkRuleConfiguredTargetBuilder.convertToOutputGroupValue(loc, outputGroup, objects));
     }
   }