Skip to content

Commit

Permalink
Enable having aspect parameters of type integer
Browse files Browse the repository at this point in the history 
This CL enables using `attr.int` as a type of the aspect parameters which used to only accept `attr.string`.

PiperOrigin-RevId: 417405071
  • Loading branch information
@mai93 @copybara-github
mai93 authored and copybara-github committed Dec 20, 2021
1 parent 69f8b17 commit 14292d1
Show file tree
Hide file tree
Showing 6 changed files with 641 additions and 42 deletions.
23 changes: 14 additions & 9 deletions ...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
import com.google.devtools.build.lib.util.Pair;
import com.google.errorprone.annotations.FormatMethod;
import java.util.Map;
import java.util.Objects;
import javax.annotation.Nullable;
import net.starlark.java.eval.Debug;
import net.starlark.java.eval.Dict;
Expand Down Expand Up @@ -630,15 +631,19 @@ public StarlarkAspect aspect(
String nativeName = nameDescriptorPair.first;
boolean hasDefault = nameDescriptorPair.second.hasDefault();
Attribute attribute = nameDescriptorPair.second.build(nameDescriptorPair.first);
if (attribute.getType() == Type.STRING
&& ((String) attribute.getDefaultValue(null)).isEmpty()) {
hasDefault = false; // isValueSet() is always true for attr.string.
}

if (!Attribute.isImplicit(nativeName) && !Attribute.isLateBound(nativeName)) {
if (attribute.getType() != Type.STRING) {
if (attribute.getType() == Type.STRING) {
// isValueSet() is always true for attr.string as default value is "" by default.
hasDefault = !Objects.equals(attribute.getDefaultValue(null), "");
} else if (attribute.getType() == Type.INTEGER) {
// isValueSet() is always true for attr.int as default value is 0 by default.
hasDefault = !Objects.equals(attribute.getDefaultValue(null), StarlarkInt.of(0));
} else {
throw Starlark.errorf(
"Aspect parameter attribute '%s' must have type 'string'.", nativeName);
"Aspect parameter attribute '%s' must have type 'int' or 'string'.", nativeName);
}

if (hasDefault && attribute.checkAllowedValues()) {
PredicateWithMessage<Object> allowed = attribute.getAllowedValues();
Object defaultVal = attribute.getDefaultValue(null);
Expand Down Expand Up @@ -794,13 +799,13 @@ private static void validateRulePropagatedAspects(RuleClass ruleClass) throws Ev
String aspectAttrName = aspectAttribute.getPublicName();
Type<?> aspectAttrType = aspectAttribute.getType();

// When propagated from a rule, explicit aspect attributes must be of type string and
// they must have `values` restriction.
// When propagated from a rule, explicit aspect attributes must be of type int or string
// and they must have `values` restriction.
if (!aspectAttribute.isImplicit() && !aspectAttribute.isLateBound()) {
if ((aspectAttrType != Type.STRING && aspectAttrType != Type.INTEGER)
|| !aspectAttribute.checkAllowedValues()) {
throw Starlark.errorf(
"Aspect %s: Aspect parameter attribute '%s' must have type 'string'"
"Aspect %s: Aspect parameter attribute '%s' must have type 'int' or 'string'"
+ " and use the 'values' restriction.",
aspect.getName(), aspectAttrName);
}
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,8 @@ public Builder add(Attribute attribute) {
Preconditions.checkArgument(
attribute.isImplicit()
|| attribute.isLateBound()
|| (attribute.getType() == Type.STRING && attribute.checkAllowedValues()),
|| (attribute.getType() == Type.STRING && attribute.checkAllowedValues())
|| (attribute.getType() == Type.INTEGER && attribute.checkAllowedValues()),
"%s: Invalid attribute '%s' (%s)",
aspectClass.getName(),
attribute.getName(),
Expand Down
96 changes: 67 additions & 29 deletions src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkCallable;
import net.starlark.java.eval.StarlarkInt;

/** A Starlark value that is a result of an 'aspect(..)' function call. */
public final class StarlarkDefinedAspect implements StarlarkExportable, StarlarkAspect {
Expand All @@ -36,7 +37,10 @@ public final class StarlarkDefinedAspect implements StarlarkExportable, Starlark
private final ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredProviders;
private final ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredAspectProviders;
private final ImmutableSet<StarlarkProviderIdentifier> provides;

/** Aspect attributes that are required to be specified by rules propagating this aspect. */
private final ImmutableSet<String> paramAttributes;

private final ImmutableSet<StarlarkAspect> requiredAspects;
private final ImmutableSet<String> fragments;
private final ConfigurationTransition hostTransition;
Expand Down Expand Up @@ -139,30 +143,19 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) {
for (Attribute attribute : attributes) {
Attribute attr = attribute; // Might be reassigned.
if (!aspectParams.getAttribute(attr.getName()).isEmpty()) {
String value = aspectParams.getOnlyValueOfAttribute(attr.getName());
Preconditions.checkState(!Attribute.isImplicit(attr.getName()));
Preconditions.checkState(attr.getType() == Type.STRING);
Type<?> attrType = attr.getType();
String attrName = attr.getName();
String attrValue = aspectParams.getOnlyValueOfAttribute(attrName);
Preconditions.checkState(!Attribute.isImplicit(attrName));
Preconditions.checkState(attrType == Type.STRING || attrType == Type.INTEGER);
Preconditions.checkArgument(
aspectParams.getAttribute(attr.getName()).size() == 1,
aspectParams.getAttribute(attrName).size() == 1,
"Aspect %s parameter %s has %s values (must have exactly 1).",
getName(),
attr.getName(),
aspectParams.getAttribute(attr.getName()).size());

if (!attr.checkAllowedValues()) {
// The aspect attribute can have no allowed values constraint if the aspect is used from
// command-line. However, AspectDefinition.Builder$add requires the existance of allowed
// values in all aspects string attributes for both native and starlark aspects.
// Therefore, allowedValues list is added here with only the current value of the
// attribute.
attr =
attr.cloneBuilder(Type.STRING)
.value(value)
.allowedValues(new Attribute.AllowedValueSet(value))
.build(attr.getName());
} else {
attr = attr.cloneBuilder(Type.STRING).value(value).build(attr.getName());
}
attrName,
aspectParams.getAttribute(attrName).size());

attr = addAttrValue(attr, attrValue);
}
builder.add(attr);
}
Expand All @@ -187,6 +180,30 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) {
return builder.build();
}

private static Attribute addAttrValue(Attribute attr, String attrValue) {
Attribute.Builder<?> attrBuilder;
Object castedValue = attrValue;

if (attr.getType() == Type.INTEGER) {
castedValue = StarlarkInt.parse(attrValue, /*base=*/ 0);
attrBuilder = attr.cloneBuilder(Type.INTEGER).value((StarlarkInt) castedValue);
} else {
attrBuilder = attr.cloneBuilder(Type.STRING).value((String) castedValue);
}

if (!attr.checkAllowedValues()) {
// The aspect attribute can have no allowed values constraint if the aspect is used from
// command-line. However, AspectDefinition.Builder$add requires the existence of allowed
// values in all aspects string attributes for both native and starlark aspects.
// Therefore, allowedValues list is added here with only the current value of the attribute.
return attrBuilder
.allowedValues(new Attribute.AllowedValueSet(attr.getType().cast(castedValue)))
.build(attr.getName());
} else {
return attrBuilder.build(attr.getName());
}
}

@Override
public boolean isExported() {
return aspectClass != null;
Expand Down Expand Up @@ -215,8 +232,8 @@ public Function<Rule, AspectParameters> getDefaultParametersExtractor() {
rule.getTargetKind(),
param);
Preconditions.checkArgument(
ruleAttr.getType() == Type.STRING,
"Cannot apply aspect %s to %s with non-string attribute '%s'.",
ruleAttr.getType() == Type.STRING || ruleAttr.getType() == Type.INTEGER,
"Cannot apply aspect %s to %s since attribute '%s' is not string and not integer.",
getName(),
rule.getTargetKind(),
param);
Expand All @@ -226,7 +243,7 @@ public Function<Rule, AspectParameters> getDefaultParametersExtractor() {
// If the attribute has a select() (which aspect attributes don't yet support), the
// error gets reported in RuleClass.checkAspectAllowedValues.
if (!ruleAttrs.isConfigurable(param)) {
builder.addAttribute(param, (String) ruleAttrs.get(param, ruleAttr.getType()));
builder.addAttribute(param, ruleAttrs.get(param, ruleAttr.getType()).toString());
}
}
}
Expand All @@ -239,24 +256,45 @@ public AspectParameters extractTopLevelParameters(ImmutableMap<String, String> p
AspectParameters.Builder builder = new AspectParameters.Builder();
for (Attribute aspectParameter : attributes) {
String parameterName = aspectParameter.getName();
Type<?> parameterType = aspectParameter.getType();

if (Attribute.isImplicit(parameterName) || Attribute.isLateBound(parameterName)) {
// These attributes are the private matters of the aspect
continue;
}

Preconditions.checkArgument(
aspectParameter.getType() == Type.STRING,
"Cannot pass value of non-string attribute '%s' in aspect %s.",
parameterType == Type.STRING || parameterType == Type.INTEGER,
"Aspect %s: Cannot pass value of attribute '%s' of type %s, only 'int' and 'string'"
+ " attributes are allowed.",
getName(),
parameterName);
parameterName,
parameterType);

String parameterValue =
parametersValues.getOrDefault(
parameterName, (String) aspectParameter.getDefaultValue(null));
parameterName, parameterType.cast(aspectParameter.getDefaultValue(null)).toString());

// Validate integer values for integer attributes
Object castedParameterValue = parameterValue;
if (parameterType == Type.INTEGER) {
try {
castedParameterValue = StarlarkInt.parse(parameterValue, /*base=*/ 0);
} catch (NumberFormatException e) {
throw new EvalException(
String.format(
"%s: expected value of type 'int' for attribute '%s' but got '%s'",
getName(), parameterName, parameterValue),
e);
}
}

if (aspectParameter.checkAllowedValues()) {
PredicateWithMessage<Object> allowedValues = aspectParameter.getAllowedValues();
if (!allowedValues.apply(parameterValue)) {
if (parameterType == Type.INTEGER) {
castedParameterValue = StarlarkInt.parse(parameterValue, /*base=*/ 0);
}
if (!allowedValues.apply(castedParameterValue)) {
throw Starlark.errorf(
"%s: invalid value in '%s' attribute: %s",
getName(), parameterName, allowedValues.getErrorReason(parameterValue));
Expand Down
Loading

0 comments on commit 14292d1

Please sign in to comment.