Skip to content

Commit

Permalink
Add required_providers attribute to Starlark defined aspects.
Browse files Browse the repository at this point in the history
`required_providers` attribute allows the aspect to limit its propagation to only the targets whose rules advertise the required providers. It accepts a list of either providers or providers lists. To make some rule targets visible to an aspect, the rule must advertise all providers from at least one of the lists specified in the aspect `required_providers`.

This CL also adds incompatible flag `incompatible_top_level_aspects_require_providers` which when set allows the top level aspects to only run on top level targets that advertise its required providers. It is needed to avoid breaking existing usages on command line aspects on targets not advertising its required providers.

PiperOrigin-RevId: 373738497
  • Loading branch information
mai93 committed Jul 15, 2021
1 parent f013c84 commit f64f071
Show file tree
Hide file tree
Showing 11 changed files with 960 additions and 3 deletions.
Expand Up @@ -521,6 +521,7 @@ public StarlarkAspect aspect(
StarlarkFunction implementation,
Sequence<?> attributeAspects,
Object attrs,
Sequence<?> requiredProvidersArg,
Sequence<?> requiredAspectProvidersArg,
Sequence<?> providesArg,
Sequence<?> fragments,
Expand Down Expand Up @@ -610,6 +611,7 @@ public StarlarkAspect aspect(
implementation,
attrAspects.build(),
attributes.build(),
StarlarkAttrModule.buildProviderPredicate(requiredProvidersArg, "required_providers"),
StarlarkAttrModule.buildProviderPredicate(
requiredAspectProvidersArg, "required_aspect_providers"),
StarlarkAttrModule.getStarlarkProviderIdentifiers(providesArg),
Expand Down
Expand Up @@ -283,6 +283,20 @@ public Builder requireProviders(Class<? extends TransitiveInfoProvider>... provi
return this;
}

/**
* Asserts that this aspect can only be evaluated for rules that supply all of the providers
* from at least one set of required providers.
*/
public Builder requireStarlarkProviderSets(
Iterable<ImmutableSet<StarlarkProviderIdentifier>> providerSets) {
for (ImmutableSet<StarlarkProviderIdentifier> providerSet : providerSets) {
if (!providerSet.isEmpty()) {
requiredProviders.addStarlarkSet(providerSet);
}
}
return this;
}

/**
* Asserts that this aspect can only be evaluated for rules that supply all of the specified
* Starlark providers.
Expand Down
Expand Up @@ -36,6 +36,7 @@ public class StarlarkDefinedAspect implements StarlarkExportable, StarlarkAspect
private final StarlarkCallable implementation;
private final ImmutableList<String> attributeAspects;
private final ImmutableList<Attribute> attributes;
private final ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredProviders;
private final ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredAspectProviders;
private final ImmutableSet<StarlarkProviderIdentifier> provides;
private final ImmutableSet<String> paramAttributes;
Expand All @@ -52,6 +53,7 @@ public StarlarkDefinedAspect(
StarlarkCallable implementation,
ImmutableList<String> attributeAspects,
ImmutableList<Attribute> attributes,
ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredProviders,
ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredAspectProviders,
ImmutableSet<StarlarkProviderIdentifier> provides,
ImmutableSet<String> paramAttributes,
Expand All @@ -65,6 +67,7 @@ public StarlarkDefinedAspect(
this.implementation = implementation;
this.attributeAspects = attributeAspects;
this.attributes = attributes;
this.requiredProviders = requiredProviders;
this.requiredAspectProviders = requiredAspectProviders;
this.provides = provides;
this.paramAttributes = paramAttributes;
Expand All @@ -83,6 +86,7 @@ public StarlarkDefinedAspect(
StarlarkCallable implementation,
ImmutableList<String> attributeAspects,
ImmutableList<Attribute> attributes,
ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredProviders,
ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredAspectProviders,
ImmutableSet<StarlarkProviderIdentifier> provides,
ImmutableSet<String> paramAttributes,
Expand All @@ -98,6 +102,7 @@ public StarlarkDefinedAspect(
implementation,
attributeAspects,
attributes,
requiredProviders,
requiredAspectProviders,
provides,
paramAttributes,
Expand Down Expand Up @@ -165,7 +170,7 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) {
builder.propagateAlongAttribute(attributeAspect);
}
}

for (Attribute attribute : attributes) {
Attribute attr = attribute; // Might be reassigned.
if (!aspectParams.getAttribute(attr.getName()).isEmpty()) {
Expand All @@ -182,6 +187,7 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) {
}
builder.add(attr);
}
builder.requireStarlarkProviderSets(requiredProviders);
builder.requireAspectsWithProviders(requiredAspectProviders);
ImmutableList.Builder<StarlarkProviderIdentifier> advertisedStarlarkProviders =
ImmutableList.builder();
Expand Down Expand Up @@ -272,6 +278,7 @@ public boolean equals(Object o) {
return Objects.equals(implementation, that.implementation)
&& Objects.equals(attributeAspects, that.attributeAspects)
&& Objects.equals(attributes, that.attributes)
&& Objects.equals(requiredProviders, that.requiredProviders)
&& Objects.equals(requiredAspectProviders, that.requiredAspectProviders)
&& Objects.equals(provides, that.provides)
&& Objects.equals(paramAttributes, that.paramAttributes)
Expand All @@ -289,6 +296,7 @@ public int hashCode() {
implementation,
attributeAspects,
attributes,
requiredProviders,
requiredAspectProviders,
provides,
paramAttributes,
Expand Down
Expand Up @@ -586,6 +586,21 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {
+ " (zero means no limit).")
public long maxComputationSteps;

@Option(
name = "incompatible_top_level_aspects_require_providers",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
help =
"If set to true, the top level aspect will honor its required providers and only run on"
+ " top level targets whose rules' advertised providers satisfy the required"
+ " providers of the aspect.")
public boolean incompatibleTopLevelAspectsRequireProviders;

/**
* An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
* never accumulate a large number of these and being able to shortcut on object identity makes a
Expand Down Expand Up @@ -656,6 +671,9 @@ public StarlarkSemantics toStarlarkSemantics() {
INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO,
incompatibleObjcProviderRemoveCompileInfo)
.set(MAX_COMPUTATION_STEPS, maxComputationSteps)
.setBool(
INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS,
incompatibleTopLevelAspectsRequireProviders)
.build();
return INTERNER.intern(semantics);
}
Expand Down Expand Up @@ -729,6 +747,8 @@ public StarlarkSemantics toStarlarkSemantics() {
"-incompatible_visibility_private_attributes_at_definition";
public static final String RECORD_RULE_INSTANTIATION_CALLSTACK =
"+record_rule_instantiation_callstack";
public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS =
"-incompatible_top_level_aspects_require_providers";

// non-booleans
public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
Expand Down
Expand Up @@ -58,12 +58,14 @@
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.OutputFile;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.StarlarkAspect;
import com.google.devtools.build.lib.packages.StarlarkAspectClass;
import com.google.devtools.build.lib.packages.StarlarkDefinedAspect;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.Type.ConversionException;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.profiler.memory.CurrentRuleTracker;
import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
Expand All @@ -78,6 +80,7 @@
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.StarlarkSemantics;

/**
* The Skyframe function that generates aspects.
Expand Down Expand Up @@ -324,6 +327,34 @@ public SkyValue compute(SkyKey skyKey, Environment env)
baseConfiguredTargetValue.getConfiguredTarget());
}

// If the incompatible flag is set, the top-level aspect should not be applied on top-level
// targets whose rules do not advertise the aspect's required providers. The aspect should not
// also propagate to these targets dependencies.
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (starlarkSemantics == null) {
return null;
}
boolean checkRuleAdvertisedProviders =
starlarkSemantics.getBool(
BuildLanguageOptions.INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS);
if (checkRuleAdvertisedProviders) {
Target target = associatedConfiguredTargetAndData.getTarget();
if (target instanceof Rule) {
if (!aspect
.getDefinition()
.getRequiredProviders()
.isSatisfiedBy(((Rule) target).getRuleClassObject().getAdvertisedProviders())) {
return new AspectValue(
key,
aspect,
target.getLocation(),
ConfiguredAspect.forNonapplicableTarget(),
/*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder
.<Package>stableOrder()
.build());
}
}
}

ImmutableList.Builder<Aspect> aspectPathBuilder = ImmutableList.builder();

Expand Down
Expand Up @@ -416,6 +416,31 @@ StarlarkCallable rule(
+ "the <code>values</code> restriction. Explicit attributes restrict the "
+ "aspect to only be used with rules that have attributes of the same "
+ "name, type, and valid values according to the restriction."),
@Param(
name = "required_providers",
named = true,
defaultValue = "[]",
doc =
"This attribute allows the aspect to limit its propagation to only the targets "
+ "whose rules advertise its required providers. The value must be a "
+ "list containing either individual providers or lists of providers but not "
+ "both. For example, <code>[[FooInfo], [BarInfo], [BazInfo, QuxInfo]]</code> "
+ "is a valid value while <code>[FooInfo, BarInfo, [BazInfo, QuxInfo]]</code> "
+ "is not valid."
+ ""
+ "<p>An unnested list of providers will automatically be converted to a list "
+ "containing one list of providers. That is, <code>[FooInfo, BarInfo]</code> "
+ "will automatically be converted to <code>[[FooInfo, BarInfo]]</code>."
+ ""
+ "<p>To make some rule (e.g. <code>some_rule</code>) targets visible to an "
+ "aspect, <code>some_rule</code> must advertise all providers from at least "
+ "one of the required providers lists. For example, if the "
+ "<code>required_providers</code> of an aspect are "
+ "<code>[[FooInfo], [BarInfo], [BazInfo, QuxInfo]]</code>, this aspect can "
+ "only see <code>some_rule</code> targets if and only if "
+ "<code>some_rule</code> provides <code>FooInfo</code> *or* "
+ "<code>BarInfo</code> *or* both <code>BazInfo</code> *and* "
+ "<code>QuxInfo</code>."),
@Param(
name = "required_aspect_providers",
named = true,
Expand Down Expand Up @@ -500,6 +525,7 @@ StarlarkAspectApi aspect(
StarlarkFunction implementation,
Sequence<?> attributeAspects,
Object attrs,
Sequence<?> requiredProvidersArg,
Sequence<?> requiredAspectProvidersArg,
Sequence<?> providesArg,
Sequence<?> fragments,
Expand Down
Expand Up @@ -180,6 +180,7 @@ public StarlarkAspectApi aspect(
StarlarkFunction implementation,
Sequence<?> attributeAspects,
Object attrs,
Sequence<?> requiredProvidersArg,
Sequence<?> requiredAspectProvidersArg,
Sequence<?> providesArg,
Sequence<?> fragments,
Expand Down
Expand Up @@ -33,6 +33,8 @@
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.StarlarkProvider;
import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.util.FileTypeSet;
import net.starlark.java.annot.StarlarkBuiltin;
Expand All @@ -55,6 +57,20 @@ private static final class P3 implements TransitiveInfoProvider {}

private static final class P4 implements TransitiveInfoProvider {}

private static final Label FAKE_LABEL = Label.parseAbsoluteUnchecked("//fake/label.bzl");

private static final StarlarkProviderIdentifier STARLARK_P1 =
StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P1"));

private static final StarlarkProviderIdentifier STARLARK_P2 =
StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P2"));

private static final StarlarkProviderIdentifier STARLARK_P3 =
StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P3"));

private static final StarlarkProviderIdentifier STARLARK_P4 =
StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P4"));

/**
* A dummy aspect factory. Is there to demonstrate how to define aspects and so that we can test
* {@code attributeAspect}.
Expand Down Expand Up @@ -150,7 +166,7 @@ public void testAttributeAspect_allAttributes() throws Exception {
}

@Test
public void testRequireProvider_addsToSetOfRequiredProvidersAndNames() throws Exception {
public void testRequireBuiltinProviders_addsToSetOfRequiredProvidersAndNames() throws Exception {
AspectDefinition requiresProviders =
new AspectDefinition.Builder(TEST_ASPECT_CLASS)
.requireProviders(P1.class, P2.class)
Expand All @@ -176,7 +192,8 @@ public void testRequireProvider_addsToSetOfRequiredProvidersAndNames() throws Ex
}

@Test
public void testRequireProvider_addsTwoSetsOfRequiredProvidersAndNames() throws Exception {
public void testRequireBuiltinProviders_addsTwoSetsOfRequiredProvidersAndNames()
throws Exception {
AspectDefinition requiresProviders =
new AspectDefinition.Builder(TEST_ASPECT_CLASS)
.requireProviderSets(
Expand All @@ -202,6 +219,73 @@ public void testRequireProvider_addsTwoSetsOfRequiredProvidersAndNames() throws

}

@Test
public void testRequireStarlarkProviders_addsFlatSetOfRequiredProviders() throws Exception {
AspectDefinition requiresProviders =
new AspectDefinition.Builder(TEST_ASPECT_CLASS)
.requireStarlarkProviders(STARLARK_P1, STARLARK_P2)
.build();

AdvertisedProviderSet expectedOkSet =
AdvertisedProviderSet.builder()
.addStarlark(STARLARK_P1)
.addStarlark(STARLARK_P2)
.addStarlark(STARLARK_P3)
.build();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet)).isTrue();

AdvertisedProviderSet expectedFailSet =
AdvertisedProviderSet.builder().addStarlark(STARLARK_P1).build();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedFailSet)).isFalse();

assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY))
.isTrue();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY))
.isFalse();
}

@Test
public void testRequireStarlarkProviders_addsTwoSetsOfRequiredProviders() throws Exception {
AspectDefinition requiresProviders =
new AspectDefinition.Builder(TEST_ASPECT_CLASS)
.requireStarlarkProviderSets(
ImmutableList.of(
ImmutableSet.of(STARLARK_P1, STARLARK_P2), ImmutableSet.of(STARLARK_P3)))
.build();

AdvertisedProviderSet expectedOkSet1 =
AdvertisedProviderSet.builder().addStarlark(STARLARK_P1).addStarlark(STARLARK_P2).build();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet1)).isTrue();

AdvertisedProviderSet expectedOkSet2 =
AdvertisedProviderSet.builder().addStarlark(STARLARK_P3).build();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet2)).isTrue();

AdvertisedProviderSet expectedFailSet =
AdvertisedProviderSet.builder().addStarlark(STARLARK_P4).build();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedFailSet)).isFalse();

assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY))
.isTrue();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY))
.isFalse();
}

@Test
public void testRequireProviders_defaultAcceptsEverything() {
AspectDefinition noRequiredProviders = new AspectDefinition.Builder(TEST_ASPECT_CLASS).build();

AdvertisedProviderSet expectedOkSet =
AdvertisedProviderSet.builder().addBuiltin(P4.class).addStarlark(STARLARK_P4).build();
assertThat(noRequiredProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet)).isTrue();

assertThat(noRequiredProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY))
.isTrue();
assertThat(
noRequiredProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY))
.isTrue();
}

@Test
public void testRequireAspectClass_defaultAcceptsNothing() {
AspectDefinition noAspects = new AspectDefinition.Builder(TEST_ASPECT_CLASS)
Expand Down

0 comments on commit f64f071

Please sign in to comment.