Allow declared providers in attribute and aspect defintions. -- PiperOrigin-RevId: 149169656 MOS_MIGRATED_REVID=149169656
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java index 8cc374f..8e2ae52 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -300,9 +300,7 @@ */ public Builder advertiseProvider(ImmutableList<SkylarkProviderIdentifier> providers) { for (SkylarkProviderIdentifier provider : providers) { - // todo(dslomov,vladmos): support declared providers - Preconditions.checkState(provider.isLegacy()); - advertisedProviders.addSkylark(provider.getLegacyId()); + advertisedProviders.addSkylark(provider); } return this; }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java index 21deb7b..58a5801 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java
@@ -43,7 +43,7 @@ private final ImmutableList<String> attributeAspects; private final ImmutableList<Attribute> attributes; private final ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> requiredAspectProviders; - private final ImmutableList<String> provides; + private final ImmutableSet<SkylarkProviderIdentifier> provides; private final ImmutableSet<String> paramAttributes; private final ImmutableSet<String> fragments; private final ImmutableSet<String> hostFragments; @@ -55,7 +55,7 @@ ImmutableList<String> attributeAspects, ImmutableList<Attribute> attributes, ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> requiredAspectProviders, - ImmutableList<String> provides, + ImmutableSet<SkylarkProviderIdentifier> provides, ImmutableSet<String> paramAttributes, ImmutableSet<String> fragments, ImmutableSet<String> hostFragments, @@ -147,8 +147,8 @@ builder.requireAspectsWithProviders(requiredAspectProviders); ImmutableList.Builder<SkylarkProviderIdentifier> advertisedSkylarkProviders = ImmutableList.builder(); - for (String provider : provides) { - advertisedSkylarkProviders.add(SkylarkProviderIdentifier.forLegacy(provider)); + for (SkylarkProviderIdentifier provider : provides) { + advertisedSkylarkProviders.add(provider); } builder.advertiseProvider(advertisedSkylarkProviders.build()); builder.requiresConfigurationFragmentsBySkylarkModuleName(fragments);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java index 4c18384..3f2b066 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; @@ -26,6 +27,7 @@ import com.google.devtools.build.lib.packages.Attribute.SplitTransition; import com.google.devtools.build.lib.packages.AttributeValueSource; import com.google.devtools.build.lib.packages.BuildType; +import com.google.devtools.build.lib.packages.ClassObjectConstructor; import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier; import com.google.devtools.build.lib.skylarkinterface.Param; @@ -256,7 +258,7 @@ Object obj = arguments.get(PROVIDERS_ARG); SkylarkType.checkType(obj, SkylarkList.class, PROVIDERS_ARG); ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> providersList = buildProviderPredicate( - (SkylarkList<?>) obj); + (SkylarkList<?>) obj, PROVIDERS_ARG, ast.getLocation()); builder.mandatoryProvidersList(providersList); } @@ -288,56 +290,84 @@ return builder; } - public static ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> buildProviderPredicate( - SkylarkList<?> obj) throws EvalException { + /** + * Builds a list of sets of accepted providers from Skylark list {@code obj}. + * The list can either be a list of providers (in that case the result is a list with one + * set) or a list of lists of providers (then the result is the list of sets). + * @param argumentName used in error messages. + * @param location location for error messages. + */ + static ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> buildProviderPredicate( + SkylarkList<?> obj, String argumentName, Location location) throws EvalException { if (obj.isEmpty()) { return ImmutableList.of(); } - boolean isSingleListOfStr = true; - for (Object o : (SkylarkList) obj) { - isSingleListOfStr = o instanceof String; - if (!isSingleListOfStr) { + boolean isListOfProviders = true; + for (Object o : obj) { + if (!isProvider(o)) { + isListOfProviders = false; break; } } - if (isSingleListOfStr) { - return ImmutableList.of(getSkylarkProviderIdentifiers(obj)); + if (isListOfProviders) { + return ImmutableList.of(getSkylarkProviderIdentifiers(obj, location)); } else { - return getProvidersList((SkylarkList) obj); + return getProvidersList(obj, argumentName, location); } } - private static ImmutableSet<SkylarkProviderIdentifier> getSkylarkProviderIdentifiers( - SkylarkList<?> obj) throws EvalException { + /** + * Returns true if {@code o} is a Skylark provider (either a declared provider or + * a legacy provider name. + */ + static boolean isProvider(Object o) { + return o instanceof String || o instanceof ClassObjectConstructor; + } + + /** + * Converts Skylark identifiers of providers (either a string or a provider value) + * to their internal representations. + */ + static ImmutableSet<SkylarkProviderIdentifier> getSkylarkProviderIdentifiers( + SkylarkList<?> list, Location location) throws EvalException { ImmutableList.Builder<SkylarkProviderIdentifier> result = ImmutableList.builder(); - List<String> contents = obj.getContents(String.class, PROVIDERS_ARG); - for (String legacyId : contents) { - result.add(SkylarkProviderIdentifier.forLegacy(legacyId)); + for (Object obj : list) { + if (obj instanceof String) { + result.add(SkylarkProviderIdentifier.forLegacy((String) obj)); + } else if (obj instanceof ClassObjectConstructor) { + ClassObjectConstructor constructor = (ClassObjectConstructor) obj; + if (!constructor.isExported()) { + throw new EvalException(location, + "Providers should be assigned to top-level values in modules"); + } + result.add(SkylarkProviderIdentifier.forKey(constructor.getKey())); + } } return ImmutableSet.copyOf(result.build()); } private static ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> getProvidersList( - SkylarkList<?> skylarkList) throws EvalException { + SkylarkList<?> skylarkList, String argumentName, Location location) throws EvalException { ImmutableList.Builder<ImmutableSet<SkylarkProviderIdentifier>> providersList = ImmutableList.builder(); String errorMsg = "Illegal argument: element in '%s' is of unexpected type. " - + "Should be list of string, but got %s. " - + "Notice: one single list of string as 'providers' is still supported."; + + "Either all elements should be providers, " + + "or all elements should be lists of providers, but got %s."; + for (Object o : skylarkList) { if (!(o instanceof SkylarkList)) { - throw new EvalException(null, String.format(errorMsg, PROVIDERS_ARG, - EvalUtils.getDataTypeName(o, true))); + throw new EvalException(location, String.format(errorMsg, PROVIDERS_ARG, + "an element of type " + EvalUtils.getDataTypeName(o, true))); } for (Object value : (SkylarkList) o) { - if (!(value instanceof String)) { - throw new EvalException(null, String.format(errorMsg, PROVIDERS_ARG, + if (!isProvider(value)) { + throw new EvalException(location, String.format(errorMsg, argumentName, "list with an element of type " + EvalUtils.getDataTypeNameFromClass(value.getClass()))); } } - providersList.add(getSkylarkProviderIdentifiers((SkylarkList<?>) o)); + providersList.add(getSkylarkProviderIdentifiers((SkylarkList<?>) o, location)); } return providersList.build(); }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java index 1f71424..b42ff3d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -590,12 +590,23 @@ attributes.add(attribute); } + for (Object o : providesArg) { + if (!SkylarkAttr.isProvider(o)) { + throw new EvalException(ast.getLocation(), + String.format("Illegal argument: element in 'provides' is of unexpected type. " + + "Should be list of providers, but got %s. ", + EvalUtils.getDataTypeName(o, true))); + } + } + return new SkylarkAspect( implementation, attrAspects.build(), attributes.build(), - SkylarkAttr.buildProviderPredicate(requiredAspectProvidersArg), - ImmutableList.copyOf(STRING_LIST.convert(providesArg, "provides")), + SkylarkAttr.buildProviderPredicate(requiredAspectProvidersArg, + "required_aspect_providers", ast.getLocation() + ), + SkylarkAttr.getSkylarkProviderIdentifiers(providesArg, ast.getLocation()), requiredParams.build(), ImmutableSet.copyOf(fragments.getContents(String.class, "fragments")), ImmutableSet.copyOf(hostFragments.getContents(String.class, "host_fragments")),
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java index f8b3d16..8852be8 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -43,6 +43,7 @@ import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor; import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier; import com.google.devtools.build.lib.rules.SkylarkAttr; +import com.google.devtools.build.lib.rules.SkylarkAttr.Descriptor; import com.google.devtools.build.lib.rules.SkylarkFileType; import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.RuleFunction; import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction; @@ -56,6 +57,7 @@ import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import com.google.devtools.build.lib.syntax.SkylarkNestedSet; import com.google.devtools.build.lib.syntax.Type; +import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.util.FileTypeSet; import java.util.Collection; import org.junit.Assert; @@ -148,12 +150,16 @@ @Test public void testAttrWithOnlyType() throws Exception { - Attribute attr = buildAttribute("a1", "attr.string_list()", ""); + Attribute attr = buildAttribute("a1", "attr.string_list()"); assertEquals(Type.STRING_LIST, attr.getType()); } private Attribute buildAttribute(String name, String... lines) throws Exception { - return ((SkylarkAttr.Descriptor) evalRuleClassCode(lines)).build(name); + String[] strings = lines.clone(); + strings[strings.length - 1] = String.format("%s = %s", name, strings[strings.length - 1]); + evalAndExport(strings); + Descriptor lookup = (Descriptor) ev.lookup(name); + return lookup != null ? lookup.build(name) : null; } @Test @@ -229,39 +235,64 @@ assertFalse(attr.getAllowedFileTypesPredicate().apply("a.txt")); } + private static SkylarkProviderIdentifier legacy(String legacyId) { + return SkylarkProviderIdentifier.forLegacy(legacyId); + } + + private static SkylarkProviderIdentifier declared(String exportedName) { + return SkylarkProviderIdentifier.forKey( + new SkylarkClassObjectConstructor.SkylarkKey(FAKE_LABEL, exportedName)); + } + @Test public void testAttrWithProviders() throws Exception { Attribute attr = - buildAttribute("a1", "attr.label_list(allow_files = True, providers = ['a', 'b'])"); + buildAttribute("a1", + "b = provider()", + "attr.label_list(allow_files = True, providers = ['a', b])"); assertThat(attr.getMandatoryProvidersList()) - .containsExactly(ImmutableSet.of(legacy("a"), legacy("b"))); - } - - private static SkylarkProviderIdentifier legacy(String legacyId) { - return SkylarkProviderIdentifier.forLegacy(legacyId); + .containsExactly(ImmutableSet.of(legacy("a"), declared("b"))); } @Test public void testAttrWithProvidersList() throws Exception { Attribute attr = - buildAttribute("a1", "attr.label_list(allow_files = True," - + " providers = [['a', 'b'], ['c']])"); + buildAttribute("a1", + "b = provider()", + "attr.label_list(allow_files = True, providers = [['a', b], ['c']])"); assertThat(attr.getMandatoryProvidersList()).containsExactly( - ImmutableSet.of(legacy("a"), legacy("b")), + ImmutableSet.of(legacy("a"), declared("b")), ImmutableSet.of(legacy("c"))); } + private void checkAttributeError(String expectedMessage, String... lines) throws Exception { + ev.setFailFast(false); + buildAttribute("fakeAttribute", lines); + MoreAsserts.assertContainsEvent(ev.getEventCollector(), expectedMessage); + } + @Test public void testAttrWithWrongProvidersList() throws Exception { - checkErrorContains( - "element in 'providers' is of unexpected type." - + " Should be list of string, but got list with an element of type int.", + checkAttributeError( + "element in 'providers' is of unexpected type. Either all elements should be providers," + + " or all elements should be lists of providers," + + " but got list with an element of type int.", "attr.label_list(allow_files = True, providers = [['a', 1], ['c']])"); - checkErrorContains( - "element in 'providers' is of unexpected type." - + " Should be list of string, but got string.", - "attr.label_list(allow_files = True, providers = [['a', 'b'], 'c'])"); + checkAttributeError( + "element in 'providers' is of unexpected type. Either all elements should be providers," + + " or all elements should be lists of providers," + + " but got an element of type string.", + "b = provider()", + "attr.label_list(allow_files = True, providers = [['a', b], 'c'])"); + + checkAttributeError( + "element in 'providers' is of unexpected type. Either all elements should be providers," + + " or all elements should be lists of providers," + + " but got an element of type string.", + "c = provider()", + "attr.label_list(allow_files = True, providers = [['a', b], c])"); + } @Test @@ -1163,7 +1194,8 @@ evalAndExport( "def _impl(target, ctx):", " pass", - "my_aspect = aspect(_impl, required_aspect_providers=['java', 'cc'])" + "cc = provider()", + "my_aspect = aspect(_impl, required_aspect_providers=['java', cc])" ); SkylarkAspect myAspect = (SkylarkAspect) lookup("my_aspect"); RequiredProviders requiredProviders = myAspect.getDefinition(AspectParameters.EMPTY) @@ -1172,7 +1204,7 @@ assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.EMPTY)).isFalse(); assertThat(requiredProviders.isSatisfiedBy( AdvertisedProviderSet.builder() - .addSkylark("cc") + .addSkylark(declared("cc")) .addSkylark("java") .build())) .isTrue(); @@ -1188,7 +1220,8 @@ evalAndExport( "def _impl(target, ctx):", " pass", - "my_aspect = aspect(_impl, required_aspect_providers=[['java'], ['cc']])" + "cc = provider()", + "my_aspect = aspect(_impl, required_aspect_providers=[['java'], [cc]])" ); SkylarkAspect myAspect = (SkylarkAspect) lookup("my_aspect"); RequiredProviders requiredProviders = myAspect.getDefinition(AspectParameters.EMPTY) @@ -1202,7 +1235,7 @@ .isTrue(); assertThat(requiredProviders.isSatisfiedBy( AdvertisedProviderSet.builder() - .addSkylark("cc") + .addSkylark(declared("cc")) .build())) .isTrue(); assertThat(requiredProviders.isSatisfiedBy( @@ -1241,6 +1274,38 @@ } @Test + public void aspectProvides() throws Exception { + evalAndExport( + "def _impl(target, ctx):", + " pass", + "y = provider()", + "my_aspect = aspect(_impl, provides = ['x', y])" + ); + SkylarkAspect myAspect = (SkylarkAspect) lookup("my_aspect"); + AdvertisedProviderSet advertisedProviders = myAspect.getDefinition(AspectParameters.EMPTY) + .getAdvertisedProviders(); + assertThat(advertisedProviders.canHaveAnyProvider()).isFalse(); + assertThat(advertisedProviders.getSkylarkProviders()) + .containsExactly(legacy("x"), declared("y")); + } + + @Test + public void aspectProvidesError() throws Exception { + ev.setFailFast(false); + evalAndExport( + "def _impl(target, ctx):", + " pass", + "y = provider()", + "my_aspect = aspect(_impl, provides = ['x', 1])" + ); + MoreAsserts.assertContainsEvent(ev.getEventCollector(), + " Illegal argument: element in 'provides' is of unexpected type." + + " Should be list of providers, but got int. "); + } + + + + @Test public void fancyExports() throws Exception { evalAndExport( "def _impla(target, ctx): pass",