Skip to content

Commit 14292d1

Browse files
mai93copybara-github
mai93
authored and
copybara-github
committed
Enable having aspect parameters of type integer
This CL enables using `attr.int` as a type of the aspect parameters which used to only accept `attr.string`. PiperOrigin-RevId: 417405071
1 parent 69f8b17 commit 14292d1

6 files changed

Lines changed: 641 additions & 42 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
import com.google.devtools.build.lib.util.Pair;
102102
import com.google.errorprone.annotations.FormatMethod;
103103
import java.util.Map;
104+
import java.util.Objects;
104105
import javax.annotation.Nullable;
105106
import net.starlark.java.eval.Debug;
106107
import net.starlark.java.eval.Dict;
@@ -630,15 +631,19 @@ public StarlarkAspect aspect(
630631
String nativeName = nameDescriptorPair.first;
631632
boolean hasDefault = nameDescriptorPair.second.hasDefault();
632633
Attribute attribute = nameDescriptorPair.second.build(nameDescriptorPair.first);
633-
if (attribute.getType() == Type.STRING
634-
&& ((String) attribute.getDefaultValue(null)).isEmpty()) {
635-
hasDefault = false; // isValueSet() is always true for attr.string.
636-
}
634+
637635
if (!Attribute.isImplicit(nativeName) && !Attribute.isLateBound(nativeName)) {
638-
if (attribute.getType() != Type.STRING) {
636+
if (attribute.getType() == Type.STRING) {
637+
// isValueSet() is always true for attr.string as default value is "" by default.
638+
hasDefault = !Objects.equals(attribute.getDefaultValue(null), "");
639+
} else if (attribute.getType() == Type.INTEGER) {
640+
// isValueSet() is always true for attr.int as default value is 0 by default.
641+
hasDefault = !Objects.equals(attribute.getDefaultValue(null), StarlarkInt.of(0));
642+
} else {
639643
throw Starlark.errorf(
640-
"Aspect parameter attribute '%s' must have type 'string'.", nativeName);
644+
"Aspect parameter attribute '%s' must have type 'int' or 'string'.", nativeName);
641645
}
646+
642647
if (hasDefault && attribute.checkAllowedValues()) {
643648
PredicateWithMessage<Object> allowed = attribute.getAllowedValues();
644649
Object defaultVal = attribute.getDefaultValue(null);
@@ -794,13 +799,13 @@ private static void validateRulePropagatedAspects(RuleClass ruleClass) throws Ev
794799
String aspectAttrName = aspectAttribute.getPublicName();
795800
Type<?> aspectAttrType = aspectAttribute.getType();
796801

797-
// When propagated from a rule, explicit aspect attributes must be of type string and
798-
// they must have `values` restriction.
802+
// When propagated from a rule, explicit aspect attributes must be of type int or string
803+
// and they must have `values` restriction.
799804
if (!aspectAttribute.isImplicit() && !aspectAttribute.isLateBound()) {
800805
if ((aspectAttrType != Type.STRING && aspectAttrType != Type.INTEGER)
801806
|| !aspectAttribute.checkAllowedValues()) {
802807
throw Starlark.errorf(
803-
"Aspect %s: Aspect parameter attribute '%s' must have type 'string'"
808+
"Aspect %s: Aspect parameter attribute '%s' must have type 'int' or 'string'"
804809
+ " and use the 'values' restriction.",
805810
aspect.getName(), aspectAttrName);
806811
}

‎src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,8 @@ public Builder add(Attribute attribute) {
459459
Preconditions.checkArgument(
460460
attribute.isImplicit()
461461
|| attribute.isLateBound()
462-
|| (attribute.getType() == Type.STRING && attribute.checkAllowedValues()),
462+
|| (attribute.getType() == Type.STRING && attribute.checkAllowedValues())
463+
|| (attribute.getType() == Type.INTEGER && attribute.checkAllowedValues()),
463464
"%s: Invalid attribute '%s' (%s)",
464465
aspectClass.getName(),
465466
attribute.getName(),

‎src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java‎

Lines changed: 67 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import net.starlark.java.eval.Printer;
2828
import net.starlark.java.eval.Starlark;
2929
import net.starlark.java.eval.StarlarkCallable;
30+
import net.starlark.java.eval.StarlarkInt;
3031

3132
/** A Starlark value that is a result of an 'aspect(..)' function call. */
3233
public final class StarlarkDefinedAspect implements StarlarkExportable, StarlarkAspect {
@@ -36,7 +37,10 @@ public final class StarlarkDefinedAspect implements StarlarkExportable, Starlark
3637
private final ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredProviders;
3738
private final ImmutableList<ImmutableSet<StarlarkProviderIdentifier>> requiredAspectProviders;
3839
private final ImmutableSet<StarlarkProviderIdentifier> provides;
40+
41+
/** Aspect attributes that are required to be specified by rules propagating this aspect. */
3942
private final ImmutableSet<String> paramAttributes;
43+
4044
private final ImmutableSet<StarlarkAspect> requiredAspects;
4145
private final ImmutableSet<String> fragments;
4246
private final ConfigurationTransition hostTransition;
@@ -139,30 +143,19 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) {
139143
for (Attribute attribute : attributes) {
140144
Attribute attr = attribute; // Might be reassigned.
141145
if (!aspectParams.getAttribute(attr.getName()).isEmpty()) {
142-
String value = aspectParams.getOnlyValueOfAttribute(attr.getName());
143-
Preconditions.checkState(!Attribute.isImplicit(attr.getName()));
144-
Preconditions.checkState(attr.getType() == Type.STRING);
146+
Type<?> attrType = attr.getType();
147+
String attrName = attr.getName();
148+
String attrValue = aspectParams.getOnlyValueOfAttribute(attrName);
149+
Preconditions.checkState(!Attribute.isImplicit(attrName));
150+
Preconditions.checkState(attrType == Type.STRING || attrType == Type.INTEGER);
145151
Preconditions.checkArgument(
146-
aspectParams.getAttribute(attr.getName()).size() == 1,
152+
aspectParams.getAttribute(attrName).size() == 1,
147153
"Aspect %s parameter %s has %s values (must have exactly 1).",
148154
getName(),
149-
attr.getName(),
150-
aspectParams.getAttribute(attr.getName()).size());
151-
152-
if (!attr.checkAllowedValues()) {
153-
// The aspect attribute can have no allowed values constraint if the aspect is used from
154-
// command-line. However, AspectDefinition.Builder$add requires the existance of allowed
155-
// values in all aspects string attributes for both native and starlark aspects.
156-
// Therefore, allowedValues list is added here with only the current value of the
157-
// attribute.
158-
attr =
159-
attr.cloneBuilder(Type.STRING)
160-
.value(value)
161-
.allowedValues(new Attribute.AllowedValueSet(value))
162-
.build(attr.getName());
163-
} else {
164-
attr = attr.cloneBuilder(Type.STRING).value(value).build(attr.getName());
165-
}
155+
attrName,
156+
aspectParams.getAttribute(attrName).size());
157+
158+
attr = addAttrValue(attr, attrValue);
166159
}
167160
builder.add(attr);
168161
}
@@ -187,6 +180,30 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) {
187180
return builder.build();
188181
}
189182

183+
private static Attribute addAttrValue(Attribute attr, String attrValue) {
184+
Attribute.Builder<?> attrBuilder;
185+
Object castedValue = attrValue;
186+
187+
if (attr.getType() == Type.INTEGER) {
188+
castedValue = StarlarkInt.parse(attrValue, /*base=*/ 0);
189+
attrBuilder = attr.cloneBuilder(Type.INTEGER).value((StarlarkInt) castedValue);
190+
} else {
191+
attrBuilder = attr.cloneBuilder(Type.STRING).value((String) castedValue);
192+
}
193+
194+
if (!attr.checkAllowedValues()) {
195+
// The aspect attribute can have no allowed values constraint if the aspect is used from
196+
// command-line. However, AspectDefinition.Builder$add requires the existence of allowed
197+
// values in all aspects string attributes for both native and starlark aspects.
198+
// Therefore, allowedValues list is added here with only the current value of the attribute.
199+
return attrBuilder
200+
.allowedValues(new Attribute.AllowedValueSet(attr.getType().cast(castedValue)))
201+
.build(attr.getName());
202+
} else {
203+
return attrBuilder.build(attr.getName());
204+
}
205+
}
206+
190207
@Override
191208
public boolean isExported() {
192209
return aspectClass != null;
@@ -215,8 +232,8 @@ public Function<Rule, AspectParameters> getDefaultParametersExtractor() {
215232
rule.getTargetKind(),
216233
param);
217234
Preconditions.checkArgument(
218-
ruleAttr.getType() == Type.STRING,
219-
"Cannot apply aspect %s to %s with non-string attribute '%s'.",
235+
ruleAttr.getType() == Type.STRING || ruleAttr.getType() == Type.INTEGER,
236+
"Cannot apply aspect %s to %s since attribute '%s' is not string and not integer.",
220237
getName(),
221238
rule.getTargetKind(),
222239
param);
@@ -226,7 +243,7 @@ public Function<Rule, AspectParameters> getDefaultParametersExtractor() {
226243
// If the attribute has a select() (which aspect attributes don't yet support), the
227244
// error gets reported in RuleClass.checkAspectAllowedValues.
228245
if (!ruleAttrs.isConfigurable(param)) {
229-
builder.addAttribute(param, (String) ruleAttrs.get(param, ruleAttr.getType()));
246+
builder.addAttribute(param, ruleAttrs.get(param, ruleAttr.getType()).toString());
230247
}
231248
}
232249
}
@@ -239,24 +256,45 @@ public AspectParameters extractTopLevelParameters(ImmutableMap<String, String> p
239256
AspectParameters.Builder builder = new AspectParameters.Builder();
240257
for (Attribute aspectParameter : attributes) {
241258
String parameterName = aspectParameter.getName();
259+
Type<?> parameterType = aspectParameter.getType();
260+
242261
if (Attribute.isImplicit(parameterName) || Attribute.isLateBound(parameterName)) {
243262
// These attributes are the private matters of the aspect
244263
continue;
245264
}
246265

247266
Preconditions.checkArgument(
248-
aspectParameter.getType() == Type.STRING,
249-
"Cannot pass value of non-string attribute '%s' in aspect %s.",
267+
parameterType == Type.STRING || parameterType == Type.INTEGER,
268+
"Aspect %s: Cannot pass value of attribute '%s' of type %s, only 'int' and 'string'"
269+
+ " attributes are allowed.",
250270
getName(),
251-
parameterName);
271+
parameterName,
272+
parameterType);
252273

253274
String parameterValue =
254275
parametersValues.getOrDefault(
255-
parameterName, (String) aspectParameter.getDefaultValue(null));
276+
parameterName, parameterType.cast(aspectParameter.getDefaultValue(null)).toString());
277+
278+
// Validate integer values for integer attributes
279+
Object castedParameterValue = parameterValue;
280+
if (parameterType == Type.INTEGER) {
281+
try {
282+
castedParameterValue = StarlarkInt.parse(parameterValue, /*base=*/ 0);
283+
} catch (NumberFormatException e) {
284+
throw new EvalException(
285+
String.format(
286+
"%s: expected value of type 'int' for attribute '%s' but got '%s'",
287+
getName(), parameterName, parameterValue),
288+
e);
289+
}
290+
}
256291

257292
if (aspectParameter.checkAllowedValues()) {
258293
PredicateWithMessage<Object> allowedValues = aspectParameter.getAllowedValues();
259-
if (!allowedValues.apply(parameterValue)) {
294+
if (parameterType == Type.INTEGER) {
295+
castedParameterValue = StarlarkInt.parse(parameterValue, /*base=*/ 0);
296+
}
297+
if (!allowedValues.apply(castedParameterValue)) {
260298
throw Starlark.errorf(
261299
"%s: invalid value in '%s' attribute: %s",
262300
getName(), parameterName, allowedValues.getErrorReason(parameterValue));

0 commit comments

Comments
 (0)