Skip to content

Commit

Permalink
[7.0.0] Experimental extend rule and subrule functionality (#20429)
Browse files Browse the repository at this point in the history 
Following commits were cleanly cherry-picked:

e4c7cb6 [email protected] Support incoming configuration transition in
extended rules
faa2f3d  [email protected]	 Support subrules in extended rules
d70d198 [email protected] Add an escape hatch for any type attributes
in the initializer
c00c272 [email protected] Cleanup incoming configuration transitions
on Starlark rules
82a469f [email protected] Copy and lift labels in arguments of
initializers
6733c18  [email protected]	 Remove unused OBJECT_LIST type
e7a3dab  [email protected]	 Remove unused FILESET_ENTRY label class
c959990 [email protected] Implement type checks and label conversion
for Starlark values
c4f4073 [email protected] Move convertFromBuildLangType into BuildType
class

Additional commit "Define function_transition_allowlist in
StarlarkRuleClassFunctionsTest" just fixes the mocking of the the tests.
  • Loading branch information
@comius
comius authored Dec 4, 2023
1 parent b1d9fa8 commit e35c29f
Show file tree
Hide file tree
Showing 19 changed files with 794 additions and 168 deletions.
4 changes: 0 additions & 4 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,6 @@ private NestedSet<Label> transitiveLabels() {
*
* <p>This is done within {@link RuleConfiguredTargetBuilder} so that every rule always and
* automatically propagates the validation action output group.
*
* <p>Note that in addition to {@link LabelClass#DEPENDENCY}, there is also {@link
* LabelClass#FILESET_ENTRY}, however the fileset implementation takes care of propagating the
* validation action output group itself.
*/
private void propagateTransitiveValidationOutputGroups() {
if (outputGroupBuilders.containsKey(OutputGroupInfo.VALIDATION_TRANSITIVE)) {
Expand Down
108 changes: 72 additions & 36 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 @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.analysis.starlark;

import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.devtools.build.lib.analysis.BaseRuleClasses.RUN_UNDER;
import static com.google.devtools.build.lib.analysis.BaseRuleClasses.TIMEOUT_DEFAULT;
import static com.google.devtools.build.lib.analysis.BaseRuleClasses.getTestRuntimeLabelList;
Expand Down Expand Up @@ -43,11 +44,10 @@
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.config.transitions.StarlarkExposedRuleTransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.TransitionType;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.starlark.StarlarkAttrModule.Descriptor;
import com.google.devtools.build.lib.analysis.test.TestConfiguration;
Expand Down Expand Up @@ -101,6 +101,7 @@
import com.google.devtools.build.lib.util.Pair;
import com.google.errorprone.annotations.FormatMethod;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
Expand Down Expand Up @@ -380,10 +381,6 @@ public StarlarkRuleFunction rule(
executableUnchecked != Starlark.UNBOUND,
"Omit executable parameter when extending rules.");
failIf(testUnchecked != Starlark.UNBOUND, "Omit test parameter when extending rules.");
// TODO b/300201845 - add cfg support
failIf(cfg != Starlark.NONE, "cfg is not supported in extended rules yet.");
// TODO b/300201845 - add subrules support
failIf(!subrules.isEmpty(), "subrules are not supported in extended rules yet.");
failIf(
implicitOutputs != Starlark.NONE,
"implicit_outputs is not supported when extending rules (deprecated).");
Expand Down Expand Up @@ -496,7 +493,7 @@ public static StarlarkRuleFunction createRule(
builder = new RuleClass.Builder("", type, true, baseParent);
}

builder.initializer(initializer);
builder.initializer(initializer, labelConverter);

builder.setDefaultExtendableAllowlist(
ruleDefinitionEnvironment.getToolsLabel("//tools/allowlists/extend_rule_allowlist"));
Expand Down Expand Up @@ -646,38 +643,38 @@ public static StarlarkRuleFunction createRule(
if (!buildSetting.equals(Starlark.NONE)) {
builder.setBuildSetting((BuildSetting) buildSetting);
}

TransitionFactory<RuleTransitionData> transitionFactory = null;
if (!cfg.equals(Starlark.NONE)) {
if (cfg instanceof StarlarkDefinedConfigTransition) {
// defined in Starlark via, cfg = transition
StarlarkDefinedConfigTransition starlarkDefinedConfigTransition =
(StarlarkDefinedConfigTransition) cfg;
builder.cfg(new StarlarkRuleTransitionProvider(starlarkDefinedConfigTransition));
transitionFactory = new StarlarkRuleTransitionProvider(starlarkDefinedConfigTransition);
hasStarlarkDefinedTransition = true;
} else if (cfg instanceof PatchTransition) {
builder.cfg((PatchTransition) cfg);
} else if (cfg instanceof StarlarkExposedRuleTransitionFactory) {
// only used for native Android transitions (platforms and feature flags)
StarlarkExposedRuleTransitionFactory transition =
(StarlarkExposedRuleTransitionFactory) cfg;
builder.cfg(transition);
transition.addToStarlarkRule(ruleDefinitionEnvironment, builder);
} else if (cfg instanceof TransitionFactory) {
// This may be redundant with StarlarkExposedRuleTransitionFactory infra
TransitionFactory<? extends TransitionFactory.Data> transitionFactory =
(TransitionFactory<? extends TransitionFactory.Data>) cfg;
if (transitionFactory.transitionType().isCompatibleWith(TransitionType.RULE)) {
@SuppressWarnings("unchecked") // Actually checked due to above isCompatibleWith call.
TransitionFactory<RuleTransitionData> ruleTransitionFactory =
(TransitionFactory<RuleTransitionData>) transitionFactory;
builder.cfg(ruleTransitionFactory);
} else {
throw Starlark.errorf(
"`cfg` must be set to a transition appropriate for a rule, not an attribute-specific"
+ " transition.");
}
transitionFactory = transition;
} else {
throw Starlark.errorf(
"`cfg` must be set to a transition object initialized by the transition() function.");
}
}
if (parent != null && parent.getTransitionFactory() != null) {
if (transitionFactory == null) {
transitionFactory = parent.getTransitionFactory();
} else {
transitionFactory =
ComposingTransitionFactory.of(transitionFactory, parent.getTransitionFactory());
}
hasStarlarkDefinedTransition = true;
}
if (transitionFactory != null) {
builder.cfg(transitionFactory);
}

boolean hasFunctionTransitionAllowlist = false;
// Check for existence of the function transition allowlist attribute.
Expand Down Expand Up @@ -964,6 +961,16 @@ public StarlarkAspect aspect(
ImmutableSet.copyOf(subrules));
}

private static ImmutableSet<String> getLegacyAnyTypeAttrs(RuleClass ruleClass) {
Attribute attr = ruleClass.getAttributeByNameMaybe("$legacy_any_type_attrs");
if (attr == null
|| attr.getType() != STRING_LIST
|| !(attr.getDefaultValueUnchecked() instanceof List<?>)) {
return ImmutableSet.of();
}
return ImmutableSet.copyOf(STRING_LIST.cast(attr.getDefaultValueUnchecked()));
}

/**
* The implementation for the magic function "rule" that creates Starlark rule classes.
*
Expand Down Expand Up @@ -1028,9 +1035,17 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
if (ruleClass == null) {
throw new EvalException("Invalid rule class hasn't been exported by a bzl file");
}
PackageContext pkgContext = thread.getThreadLocal(PackageContext.class);
if (pkgContext == null) {
throw new EvalException(
"Cannot instantiate a rule when loading a .bzl file. "
+ "Rules may be instantiated only in a BUILD thread.");
}

validateRulePropagatedAspects(ruleClass);

ImmutableSet<String> legacyAnyTypeAttrs = getLegacyAnyTypeAttrs(ruleClass);

// Remove {@link BazelStarlarkContext} to prevent calls to load and analysis time functions.
// Mutating values in initializers is mostly not a problem, because the attribute values are
// copied before calling the initializers (<-TODO) and before they are set on the target.
Expand All @@ -1050,8 +1065,6 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
continue;
}

// TODO: b/298561048 - lift parameters to more accurate type - for example strings to
// Labels
// You might feel tempted to inspect the signature of the initializer function. The
// temptation might come from handling default values, making them work for better for the
// users.
Expand All @@ -1060,7 +1073,19 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
for (var attr : currentRuleClass.getAttributes()) {
if (attr.isPublic() && attr.starlarkDefined()) {
if (kwargs.containsKey(attr.getName())) {
initializerKwargs.put(attr.getName(), kwargs.get(attr.getName()));
Object value = kwargs.get(attr.getName());
if (value == Starlark.NONE) {
continue;
}
Object reifiedValue =
legacyAnyTypeAttrs.contains(attr.getName())
? value
: BuildType.copyAndLiftStarlarkValue(
currentRuleClass.getName(),
attr,
value,
pkgContext.getBuilder().getLabelConverter());
initializerKwargs.put(attr.getName(), reifiedValue);
}
}
}
Expand Down Expand Up @@ -1091,7 +1116,19 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
throw Starlark.errorf(
"Initializer can only set Starlark defined attributes, not '%s'", arg);
}
kwargs.putEntry(nativeName, newKwargs.get(arg));
Object value = newKwargs.get(arg);
Object reifiedValue =
attr == null
|| value == Starlark.NONE
|| legacyAnyTypeAttrs.contains(attr.getName())
? value
: BuildType.copyAndLiftStarlarkValue(
currentRuleClass.getName(),
attr,
value,
// Reify to the location of the initializer definition
currentRuleClass.getLabelConverterForInitializer());
kwargs.putEntry(nativeName, reifiedValue);
}
}
} finally {
Expand All @@ -1101,12 +1138,6 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
BuildLangTypedAttributeValuesMap attributeValues =
new BuildLangTypedAttributeValuesMap(kwargs);
try {
PackageContext pkgContext = thread.getThreadLocal(PackageContext.class);
if (pkgContext == null) {
throw new EvalException(
"Cannot instantiate a rule when loading a .bzl file. "
+ "Rules may be instantiated only in a BUILD thread.");
}
RuleFactory.createAndAddRule(
pkgContext.getBuilder(),
ruleClass,
Expand Down Expand Up @@ -1146,7 +1177,12 @@ public void export(EventHandler handler, Label starlarkLabel, String ruleClassNa
// exploit dependency resolution for "free"
ImmutableList<Pair<String, Descriptor>> subruleAttributes;
try {
subruleAttributes = StarlarkSubrule.discoverAttributes(builder.getSubrules());
var parentSubrules = builder.getParentSubrules();
ImmutableList<StarlarkSubruleApi> subrulesNotInParents =
builder.getSubrules().stream()
.filter(subrule -> !parentSubrules.contains(subrule))
.collect(toImmutableList());
subruleAttributes = StarlarkSubrule.discoverAttributes(subrulesNotInParents);
} catch (EvalException e) {
errorf(handler, "%s", e.getMessage());
return;
Expand Down
6 changes: 5 additions & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,10 +326,14 @@ ImmutableSet<? extends StarlarkSubruleApi> getSubrules() {
if (isForAspect()) {
return getRuleContext().getMainAspect().getDefinition().getSubrules();
} else {
return getRuleContext().getRule().getRuleClassObject().getSubrules();
return getRuleClassUnderEvaluation().getSubrules();
}
}

public RuleClass getRuleClassUnderEvaluation() {
return ruleClassUnderEvaluation;
}

void setLockedForSubrule(@Nullable SubruleContext lockedBy) {
this.lockedForSubruleEvaluation = lockedBy;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubrule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ private EvalException getUndeclaredSubruleError(StarlarkRuleContext starlarkRule
} else {
return Starlark.errorf(
"rule '%s' must declare '%s' in 'subrules'",
starlarkRuleContext.getRuleContext().getRule().getRuleClass(), this.getName());
starlarkRuleContext.getRuleClassUnderEvaluation(), this.getName());
}
}

Expand Down
Loading

0 comments on commit e35c29f

Please sign in to comment.