Skip to content

Commit 7e53880

Browse files
susinmotioncopybara-github
authored andcommitted
Use rule machinery for handling Symbolic Macro attrs in order to reduce the memory overhead of symbolic macros.
In particular, RuleOrMacroInstance holds shared logic between Rule and MacroInstance, and both RuleClass and MacroClass now hold an AttributeProvider, which gives access to their Attributes. Together, this gives us: - Use of the package's list interner for attributes - Map of Attributes is now a list (the key/name is already stored in the attribute) - Default values of Attributes are not stored - Compact attr representation once the macro is frozen - Attrs are stored as Native types instead of Starlark types As a side effect of doing this, Attribute.valueToStarlark() now works with Selects. PiperOrigin-RevId: 739568597 Change-Id: Ia6219f3105ede816c08105fec6c0df03d7b8a5e6
1 parent 4f40b7f commit 7e53880

File tree

48 files changed

+1619
-1249
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

48 files changed

+1619
-1249
lines changed

src/main/java/com/google/devtools/build/docgen/BuildDocCollector.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ private void processJavaSourceRuleAttributeDocs(
207207
if (ruleClass.isDocumented()) {
208208
Class<? extends RuleDefinition> ruleDefinition =
209209
ruleClassProvider.getRuleClassDefinition(ruleDoc.getRuleName()).getClass();
210-
for (Attribute attribute : ruleClass.getAttributes()) {
210+
for (Attribute attribute : ruleClass.getAttributeProvider().getAttributes()) {
211211
if (!attribute.isDocumented()) {
212212
continue;
213213
}

src/main/java/com/google/devtools/build/lib/analysis/DependencyResolutionHelpers.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,8 @@ private static void addExplicitDeps(
510510
&& !rule.isAttrDefined(attrName, BuildType.NODEP_LABEL_LIST)) {
511511
return;
512512
}
513-
Attribute attribute = rule.getRuleClassObject().getAttributeByName(attrName);
513+
Attribute attribute =
514+
rule.getRuleClassObject().getAttributeProvider().getAttributeByName(attrName);
514515
outgoingLabels.putAll(AttributeDependencyKind.forRule(attribute), labels);
515516
}
516517

@@ -528,7 +529,8 @@ private static ImmutableList<AttributeDependencyKind> getAttributes(
528529
// `ctx.rule.attr` with rule attributes taking precedence then aspects' attributes based on the
529530
// aspect order in the aspects path (lowest order to highest).
530531

531-
List<Attribute> ruleAttributes = rule.getRuleClassObject().getAttributes();
532+
List<Attribute> ruleAttributes =
533+
rule.getRuleClassObject().getAttributeProvider().getAttributes();
532534
for (Attribute attribute : ruleAttributes) {
533535
result.add(AttributeDependencyKind.forRule(attribute));
534536
ruleAndBaseAspectsProcessedAttributes.add(attribute.getName());

src/main/java/com/google/devtools/build/lib/analysis/RequiredFragmentsUtil.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ private static void addRequiredFragmentsFromRuleTransitions(
248248
.attributes(attributeMap)
249249
.analysisData(starlarkExecTransition)
250250
.build();
251-
for (Attribute attribute : target.getRuleClassObject().getAttributes()) {
251+
for (Attribute attribute : target.getRuleClassObject().getAttributeProvider().getAttributes()) {
252252
if (attribute.getTransitionFactory() != null) {
253253
attribute
254254
.getTransitionFactory()

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2169,7 +2169,7 @@ protected String getMacroMessageAppendix(String unusedAttrName) {
21692169
// but we no longer locations for individual attributes.
21702170
// We should record the instantiation call stack in each rule
21712171
// and report the position of its topmost frame here.
2172-
return rule.wasCreatedByMacro()
2172+
return rule.isRuleCreatedInLegacyMacro()
21732173
? String.format(
21742174
". Since this rule was created by the macro '%s', the error might have been "
21752175
+ "caused by the macro implementation",

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

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -466,12 +466,12 @@ private static ImmutableList<Attribute> getAttrsOf(Object inheritAttrsArg) throw
466466
return ImmutableList.of();
467467
} else if (inheritAttrsArg instanceof RuleFunction ruleFunction) {
468468
verifyInheritAttrsArgExportedIfExportable(ruleFunction);
469-
return ruleFunction.getRuleClass().getAttributes();
469+
return ruleFunction.getRuleClass().getAttributeProvider().getAttributes();
470470
} else if (inheritAttrsArg instanceof MacroFunction macroFunction) {
471471
verifyInheritAttrsArgExportedIfExportable(macroFunction);
472-
return macroFunction.getMacroClass().getAttributes().values().asList();
472+
return macroFunction.getMacroClass().getAttributeProvider().getAttributes();
473473
} else if (inheritAttrsArg.equals(COMMON_ATTRIBUTES_NAME)) {
474-
return baseRule.getAttributes();
474+
return baseRule.getAttributeProvider().getAttributes();
475475
}
476476
throw Starlark.errorf(
477477
"Invalid 'inherit_attrs' value %s; expected a rule, a macro, or \"common\"",
@@ -1390,7 +1390,8 @@ public StarlarkAspect aspect(
13901390
}
13911391

13921392
private static ImmutableSet<String> getLegacyAnyTypeAttrs(RuleClass ruleClass) {
1393-
Attribute attr = ruleClass.getAttributeByNameMaybe("$legacy_any_type_attrs");
1393+
Attribute attr =
1394+
ruleClass.getAttributeProvider().getAttributeByNameMaybe("$legacy_any_type_attrs");
13941395
if (attr == null
13951396
|| attr.getType() != STRING_LIST
13961397
|| !(attr.getDefaultValueUnchecked() instanceof List<?>)) {
@@ -1677,7 +1678,7 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
16771678
// users.
16781679
// The less magic the better. Do not give in those temptations!
16791680
Dict.Builder<String, Object> initializerKwargs = Dict.builder();
1680-
for (var attr : currentRuleClass.getAttributes()) {
1681+
for (var attr : currentRuleClass.getAttributeProvider().getAttributes()) {
16811682
if ((attr.isPublic() && attr.starlarkDefined()) || attr.getName().equals("name")) {
16821683
if (kwargs.containsKey(attr.getName())) {
16831684
Object value = kwargs.get(attr.getName());
@@ -1724,7 +1725,8 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
17241725
ALLOWLIST_RULE_EXTENSION_API_EXPERIMENTAL);
17251726
}
17261727
String nativeName = arg.startsWith("_") ? "$" + arg.substring(1) : arg;
1727-
Attribute attr = currentRuleClass.getAttributeByNameMaybe(nativeName);
1728+
Attribute attr =
1729+
currentRuleClass.getAttributeProvider().getAttributeByNameMaybe(nativeName);
17281730
if (attr != null && !attr.starlarkDefined()) {
17291731
throw Starlark.errorf(
17301732
"Initializer can only set Starlark defined attributes, not '%s'", arg);
@@ -1770,7 +1772,7 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
17701772
}
17711773

17721774
private static void validateRulePropagatedAspects(RuleClass ruleClass) throws EvalException {
1773-
for (Attribute attribute : ruleClass.getAttributes()) {
1775+
for (Attribute attribute : ruleClass.getAttributeProvider().getAttributes()) {
17741776
attribute.validateRulePropagatedAspectsParameters(ruleClass);
17751777
}
17761778
}

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,12 @@ private static Attribute getAttributeByName(StarlarkRuleContext ruleContext, Str
201201
if (ruleContext.isForAspect()) {
202202
return ruleContext.getRuleContext().getMainAspect().getDefinition().getAttributes().get(attr);
203203
} else {
204-
return ruleContext.getRuleContext().getRule().getRuleClassObject().getAttributeByName(attr);
204+
return ruleContext
205+
.getRuleContext()
206+
.getRule()
207+
.getRuleClassObject()
208+
.getAttributeProvider()
209+
.getAttributeByName(attr);
205210
}
206211
}
207212

src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,11 @@ private static Iterable<TransitiveInfoCollection> getPrerequisitesForAttributes(
456456
List<TransitiveInfoCollection> prerequisites = new ArrayList<>();
457457
for (String attributeName : attributeNames) {
458458
Attribute attribute =
459-
ruleContext.getRule().getRuleClassObject().getAttributeByNameMaybe(attributeName);
459+
ruleContext
460+
.getRule()
461+
.getRuleClassObject()
462+
.getAttributeProvider()
463+
.getAttributeByNameMaybe(attributeName);
460464
if (attribute != null) {
461465
prerequisites.addAll(attributeDependencyPrerequisites(attribute, ruleContext));
462466
}

src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,19 +31,19 @@
3131
* data before exposing it to consumers.
3232
*/
3333
public abstract class AbstractAttributeMapper implements AttributeMap {
34-
final RuleClass ruleClass;
35-
final Rule rule;
34+
final AttributeProvider ruleClass;
35+
final RuleOrMacroInstance rule;
3636
private final Label ruleLabel;
3737

38-
protected AbstractAttributeMapper(Rule rule) {
39-
this.ruleClass = rule.getRuleClassObject();
38+
protected AbstractAttributeMapper(RuleOrMacroInstance rule) {
39+
this.ruleClass = rule.getAttributeProvider();
4040
this.ruleLabel = rule.getLabel();
4141
this.rule = rule;
4242
}
4343

4444
@Override
4545
public String describeRule() {
46-
return String.format("%s %s", this.rule.getRuleClass(), getLabel());
46+
return String.format("%s %s", this.rule.getAttributeProvider(), getLabel());
4747
}
4848

4949
@Override
@@ -149,7 +149,7 @@ public boolean isAttributeValueExplicitlySpecified(String attributeName) {
149149

150150
@Override
151151
public PackageArgs getPackageArgs() {
152-
return rule.getPackageDeclarations().getPackageArgs();
152+
return rule.getPackageArgs();
153153
}
154154

155155
@Override
@@ -209,7 +209,7 @@ public final boolean isConfigurable(String attributeName) {
209209
* Check if an attribute is configurable (uses select) or, if it's a computed default, if any of
210210
* its inputs are configurable.
211211
*/
212-
public static boolean isConfigurable(Rule rule, String attributeName) {
212+
public static boolean isConfigurable(RuleOrMacroInstance rule, String attributeName) {
213213
Object attr = rule.getAttr(attributeName);
214214
if (attr instanceof Attribute.ComputedDefault) {
215215
for (String dep : ((Attribute.ComputedDefault) attr).dependencies()) {
@@ -219,7 +219,7 @@ public static boolean isConfigurable(Rule rule, String attributeName) {
219219
}
220220
return false;
221221
}
222-
Attribute attrDef = rule.getRuleClassObject().getAttributeByNameMaybe(attributeName);
222+
Attribute attrDef = rule.getAttributeProvider().getAttributeByNameMaybe(attributeName);
223223
return attrDef != null && rule.getSelectorList(attributeName, attrDef.getType()) != null;
224224
}
225225

src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,11 @@
4848
*/
4949
public class AggregatingAttributeMapper extends AbstractAttributeMapper {
5050

51-
private AggregatingAttributeMapper(Rule rule) {
51+
private AggregatingAttributeMapper(RuleOrMacroInstance rule) {
5252
super(rule);
5353
}
5454

55-
public static AggregatingAttributeMapper of(Rule rule) {
55+
public static AggregatingAttributeMapper of(RuleOrMacroInstance rule) {
5656
return new AggregatingAttributeMapper(rule);
5757
}
5858

@@ -61,7 +61,7 @@ public static AggregatingAttributeMapper of(Rule rule) {
6161
* available to computed defaults no matter what dependencies they've declared.
6262
*/
6363
private List<String> getNonConfigurableAttributes() {
64-
return rule.getRuleClassObject().getNonConfigurableAttributes();
64+
return rule.getAttributeProvider().getNonConfigurableAttributes();
6565
}
6666

6767
/**
@@ -184,7 +184,7 @@ public static <T> void visitLabelsInSelect(
184184
Attribute attribute,
185185
Type<T> type,
186186
Type.LabelVisitor visitor,
187-
@Nullable Rule rule,
187+
@Nullable RuleOrMacroInstance rule,
188188
boolean includeKeys,
189189
boolean includeValues) {
190190
var entryProcessor =

src/main/java/com/google/devtools/build/lib/packages/AspectsList.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ public void validateRulePropagatedAspectsParameters(RuleClass ruleClass) throws
104104
// Required aspect parameters must be specified by the rule propagating the aspect with
105105
// the same parameter type.
106106
if (requiredAspectParameters.contains(aspectAttrName)) {
107-
if (!ruleClass.hasAttr(aspectAttrName, aspectAttrType)) {
107+
if (!ruleClass.getAttributeProvider().hasAttr(aspectAttrName, aspectAttrType)) {
108108
throw Starlark.errorf(
109109
"Aspect %s requires rule %s to specify attribute '%s' with type %s.",
110110
aspect.getName(), ruleClass.getName(), aspectAttrName, aspectAttrType);

0 commit comments

Comments
 (0)