Support declared providers for aspects Fixes #2016 -- PiperOrigin-RevId: 149102037 MOS_MIGRATED_REVID=149102037
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java index 8578e76..48fdc0c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.packages.AspectParameters; import com.google.devtools.build.lib.packages.ClassObjectConstructor; import com.google.devtools.build.lib.packages.SkylarkClassObject; +import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.util.Preconditions; import java.util.Arrays; @@ -110,6 +111,8 @@ private final Map<String, NestedSetBuilder<Artifact>> outputGroupBuilders = new TreeMap<>(); private final ImmutableMap.Builder<String, Object> skylarkProviderBuilder = ImmutableMap.builder(); + private final ImmutableMap.Builder<SkylarkClassObjectConstructor.Key, SkylarkClassObject> + skylarkDeclaredProvidersBuilder = ImmutableMap.builder(); private final RuleContext ruleContext; private final AspectDescriptor descriptor; @@ -195,6 +198,19 @@ return this; } + public Builder addSkylarkDeclaredProvider(SkylarkClassObject declaredProvider, Location loc) + throws EvalException { + ClassObjectConstructor constructor = declaredProvider.getConstructor(); + SkylarkProviderValidationUtil.validateAndThrowEvalException( + constructor.getPrintableName(), declaredProvider, loc); + if (!constructor.isExported()) { + throw new EvalException( + constructor.getLocation(), "All providers must be top level values"); + } + skylarkDeclaredProvidersBuilder.put(constructor.getKey(), declaredProvider); + return this; + } + public ConfiguredAspect build() { if (!outputGroupBuilders.isEmpty()) { ImmutableMap.Builder<String, NestedSet<Artifact>> outputGroups = ImmutableMap.builder(); @@ -210,10 +226,10 @@ } ImmutableMap<String, Object> skylarkProvidersMap = skylarkProviderBuilder.build(); - if (!skylarkProvidersMap.isEmpty()) { - providers.add( - new SkylarkProviders(skylarkProvidersMap, - ImmutableMap.<ClassObjectConstructor.Key, SkylarkClassObject>of())); + ImmutableMap<SkylarkClassObjectConstructor.Key, SkylarkClassObject> + skylarkDeclaredProvidersMap = skylarkDeclaredProvidersBuilder.build(); + if (!skylarkProvidersMap.isEmpty() || !skylarkDeclaredProvidersMap.isEmpty()) { + providers.add(new SkylarkProviders(skylarkProvidersMap, skylarkDeclaredProvidersMap)); } addProvider(
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 be75e14..a568329 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
@@ -60,6 +60,11 @@ return skylarkProviders.keySet(); } + /** Returns the keys for the declared providers. */ + public ImmutableCollection<ClassObjectConstructor.Key> getDeclaredProviderKeys() { + return declaredProviders.keySet(); + } + /** * Returns a Skylark provider; "key" must be one from {@link #getKeys()}. */
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 3bf2cdc..809265e 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
@@ -94,8 +94,7 @@ && !(target instanceof Iterable)) { ruleContext.ruleError( String.format( - "Rule should return a return a struct or a list, but got %s", - SkylarkType.typeOf(target))); + "Rule should return a struct or a list, but got %s", SkylarkType.typeOf(target))); return null; } else if (!expectFailure.isEmpty()) { ruleContext.ruleError("Expected failure not found: " + expectFailure); @@ -246,9 +245,13 @@ } else if (target instanceof Iterable) { loc = ruleContext.getRule().getRuleClassObject().getConfiguredTargetFunction().getLocation(); for (Object o : (Iterable) target) { - SkylarkClassObject declaredProvider = SkylarkType.cast(o, SkylarkClassObject.class, loc, - "A return value of rule implementation function should be " - + "a sequence of declared providers"); + SkylarkClassObject declaredProvider = + SkylarkType.cast( + o, + SkylarkClassObject.class, + loc, + "A return value of a rule implementation function should be " + + "a sequence of declared providers"); if (declaredProvider.getConstructor().getKey().equals( SkylarkRuleContext.getDefaultProvider().getKey())) { parseProviderKeys(declaredProvider, true, ruleContext, loc, executable,
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 e80a788..bd0900b 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
@@ -31,6 +31,7 @@ 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.EvalUtils; import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.SkylarkType; import java.util.Map; @@ -65,7 +66,7 @@ .setGlobals(skylarkAspect.getFuncallEnv().getGlobals()) .setEventHandler(ruleContext.getAnalysisEnvironment().getEventHandler()) .build(); // NB: loading phase functions are not available: this is analysis already, - // so we do *not* setLoadingPhase(). + // so we do *not* setLoadingPhase(). Object aspectSkylarkObject; try { aspectSkylarkObject = @@ -79,26 +80,15 @@ if (ruleContext.hasErrors()) { return null; - } else if (!(aspectSkylarkObject instanceof SkylarkClassObject)) { - ruleContext.ruleError("Aspect implementation doesn't return a struct"); + } else if (!(aspectSkylarkObject instanceof SkylarkClassObject) + && !(aspectSkylarkObject instanceof Iterable)) { + ruleContext.ruleError( + String.format( + "Aspect implementation should return a struct or a list, but got %s", + SkylarkType.typeOf(aspectSkylarkObject))); return null; } - - ConfiguredAspect.Builder builder = new ConfiguredAspect.Builder( - aspectDescriptor, ruleContext); - - SkylarkClassObject struct = (SkylarkClassObject) aspectSkylarkObject; - Location loc = struct.getCreationLoc(); - for (String key : struct.getKeys()) { - if (key.equals("output_groups")) { - addOutputGroups(struct.getValue(key), loc, builder); - } else { - builder.addSkylarkTransitiveInfo(key, struct.getValue(key), loc); - } - } - ConfiguredAspect configuredAspect = builder.build(); - SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext); - return configuredAspect; + return createAspect(aspectSkylarkObject, aspectDescriptor, ruleContext); } catch (EvalException e) { addAspectToStackTrace(base, e); ruleContext.ruleError("\n" + e.print()); @@ -107,6 +97,58 @@ } } + private ConfiguredAspect createAspect( + Object aspectSkylarkObject, AspectDescriptor aspectDescriptor, RuleContext ruleContext) + throws EvalException { + + ConfiguredAspect.Builder builder = new ConfiguredAspect.Builder(aspectDescriptor, ruleContext); + + if (aspectSkylarkObject instanceof Iterable) { + addDeclaredProviders(builder, (Iterable) aspectSkylarkObject); + } else { + SkylarkClassObject struct = (SkylarkClassObject) aspectSkylarkObject; + Location loc = struct.getCreationLoc(); + for (String key : struct.getKeys()) { + if (key.equals("output_groups")) { + addOutputGroups(struct.getValue(key), loc, builder); + } else if (key.equals("providers")) { + Object value = struct.getValue(key); + Iterable providers = + SkylarkType.cast( + value, + Iterable.class, + loc, + "The value for \"providers\" should be a list of declared providers, " + + "got %s instead", + EvalUtils.getDataTypeName(value, false)); + addDeclaredProviders(builder, providers); + } else { + builder.addSkylarkTransitiveInfo(key, struct.getValue(key), loc); + } + } + } + + ConfiguredAspect configuredAspect = builder.build(); + SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext); + return configuredAspect; + } + + private void addDeclaredProviders(ConfiguredAspect.Builder builder, Iterable aspectSkylarkObject) + throws EvalException { + for (Object o : aspectSkylarkObject) { + Location loc = skylarkAspect.getImplementation().getLocation(); + SkylarkClassObject declaredProvider = + SkylarkType.cast( + o, + SkylarkClassObject.class, + loc, + "A return value of an aspect implementation function should be " + + "a sequence of declared providers"); + Location creationLoc = declaredProvider.getCreationLocOrNull(); + builder.addSkylarkDeclaredProvider(declaredProvider, creationLoc != null ? creationLoc : loc); + } + } + private static void addOutputGroups(Object value, Location loc, ConfiguredAspect.Builder builder) throws EvalException {
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 9088a52..b5e951b 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
@@ -34,6 +34,8 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.packages.AspectDefinition; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; +import com.google.devtools.build.lib.packages.ClassObjectConstructor.Key; +import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor.SkylarkKey; import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.java.Jvm; import com.google.devtools.build.lib.skyframe.AspectValue; @@ -41,6 +43,7 @@ import com.google.devtools.build.lib.syntax.SkylarkNestedSet; import com.google.devtools.build.lib.vfs.FileSystemUtils; import java.util.Arrays; +import java.util.List; import javax.annotation.Nullable; import org.junit.Test; import org.junit.runner.RunWith; @@ -74,6 +77,54 @@ .containsExactly("//test:aspect.bzl%MyAspect(//test:xxx)"); } + @Test + public void aspectWithDeclaredProviders() throws Exception { + scratch.file( + "test/aspect.bzl", + "foo = provider()", + "bar = provider()", + "def _impl(target, ctx):", + " return [foo(), bar()]", + "MyAspect = aspect(implementation=_impl)"); + scratch.file("test/BUILD", "java_library(name = 'xxx',)"); + + AnalysisResult analysisResult = + update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx"); + assertThat(getAspectDescriptions(analysisResult)) + .containsExactly("//test:aspect.bzl%MyAspect(//test:xxx)"); + + List<Key> providers = getDeclaredProviderKeys(analysisResult); + assertThat((providers.get(0))) + .isEqualTo(new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "foo")); + assertThat((providers.get(1))) + .isEqualTo(new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "bar")); + } + + @Test + public void aspectWithDeclaredProvidersInAStruct() throws Exception { + scratch.file( + "test/aspect.bzl", + "foo = provider()", + "bar = provider()", + "def _impl(target, ctx):", + " return struct(foobar='foobar', providers=[foo(), bar()])", + "MyAspect = aspect(implementation=_impl)"); + scratch.file("test/BUILD", "java_library(name = 'xxx',)"); + + AnalysisResult analysisResult = + update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx"); + assertThat(getAspectDescriptions(analysisResult)) + .containsExactly("//test:aspect.bzl%MyAspect(//test:xxx)"); + + List<Key> providers = getDeclaredProviderKeys(analysisResult); + assertThat((providers.get(0))) + .isEqualTo(new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "foo")); + assertThat((providers.get(1))) + .isEqualTo(new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "bar")); + } + private Iterable<String> getAspectDescriptions(AnalysisResult analysisResult) { return transform( analysisResult.getAspects(), @@ -89,6 +140,25 @@ }); } + private List<Key> getDeclaredProviderKeys(AnalysisResult analysisResult) { + return transform( + analysisResult.getAspects(), + new Function<AspectValue, List<Key>>() { + @Nullable + @Override + public List<Key> apply(AspectValue aspectValue) { + return aspectValue + .getConfiguredAspect() + .getProviders() + .getProvider(SkylarkProviders.class) + .getDeclaredProviderKeys() + .asList(); + } + }) + .iterator() + .next(); // Assume there's only one aspect + } + @Test public void aspectCommandLineLabel() throws Exception { scratch.file( @@ -585,7 +655,7 @@ } catch (ViewCreationFailedException e) { // expect to fail. } - assertContainsEvent("Aspect implementation doesn't return a struct"); + assertContainsEvent("Aspect implementation should return a struct or a list, but got int"); } @Test