output_group is not a real Skylark provider for aspects, as well as for rules.

--
MOS_MIGRATED_REVID=138863855

3 files changed, 123 insertions, 40 deletions

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));
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 0188b1e4c7..cebf63dccc 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -258,7 +258,6 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
         "",
         "MyAspect = aspect(",
         "   implementation=_impl,",
-        "   attr_aspects=['deps'],",
         ")");
     scratch.file(
         "test/BUILD",
@@ -293,6 +292,50 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
+  public void testAspectWithOutputGroupsAsList() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   g = target.output_group('_hidden_top_level" + INTERNAL_SUFFIX + "')",
+        "   return struct(output_groups = { 'my_result' : [ f for f in g] })",
+        "",
+        "MyAspect = aspect(",
+        "   implementation=_impl,",
+        ")");
+    scratch.file(
+        "test/BUILD",
+        "java_library(",
+        "     name = 'xxx',",
+        "     srcs = ['A.java'],",
+        ")");
+
+    AnalysisResult analysisResult =
+        update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+    assertThat(
+        transform(
+            analysisResult.getTargetsToBuild(),
+            new Function<ConfiguredTarget, String>() {
+              @Nullable
+              @Override
+              public String apply(ConfiguredTarget configuredTarget) {
+                return configuredTarget.getLabel().toString();
+              }
+            }))
+        .containsExactly("//test:xxx");
+    AspectValue aspectValue = analysisResult.getAspects().iterator().next();
+    OutputGroupProvider outputGroupProvider =
+        aspectValue.getConfiguredAspect().getProvider(OutputGroupProvider.class);
+    assertThat(outputGroupProvider).isNotNull();
+    NestedSet<Artifact> names = outputGroupProvider.getOutputGroup("my_result");
+    assertThat(names).isNotEmpty();
+    NestedSet<Artifact> expectedSet = getConfiguredTarget("//test:xxx")
+        .getProvider(OutputGroupProvider.class)
+        .getOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL);
+    assertThat(names).containsExactlyElementsIn(expectedSet);
+  }
+
+
+  @Test
   public void testAspectsFromSkylarkRules() throws Exception {
     scratch.file(
         "test/aspect.bzl",
@@ -599,6 +642,40 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
+  public void duplicateGroupsFormAspects() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _a1_impl(target, ctx):",
+        "  f = ctx.new_file('f.txt')",
+        "  ctx.file_action(f, 'f')",
+        "  return struct(output_groups = { 'a1_group' : set([f]) })",
+        "",
+        "a1 = aspect(implementation=_a1_impl, attr_aspects = ['dep'])",
+        "def _rule_impl(ctx):",
+        "  pass",
+        "my_rule1 = rule(_rule_impl, attrs = { 'dep' : attr.label(aspects = [a1]) })",
+        "def _a2_impl(target, ctx):",
+        "  f = ctx.new_file('f.txt')",
+        "  ctx.file_action(f, 'f')",
+        "  return struct(output_groups = { 'a2_group' : set([f]) })",
+        "",
+        "a2 = aspect(implementation=_a2_impl, attr_aspects = ['dep'])",
+        "my_rule2 = rule(_rule_impl, attrs = { 'dep' : attr.label(aspects = [a2]) })"
+        );
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'my_rule1', 'my_rule2')",
+        "my_rule1(name = 'base')",
+        "my_rule1(name = 'xxx', dep = ':base')",
+        "my_rule2(name = 'yyy', dep = ':xxx')"
+    );
+
+    // no error.
+    update("//test:yyy");
+  }
+
+
+  @Test
   public void duplicateSkylarkProviders() throws Exception {
     scratch.file(
         "test/aspect.bzl",