Skip to content

Commit

Permalink
Make experimental rules API usable through allowlists
Browse files Browse the repository at this point in the history 
The validation happens a bit later, when the target is created.

PiperOrigin-RevId: 633155677
Change-Id: I3c7530ca4f8b41a075ceba3934ef0e02d3e7d096
  • Loading branch information
@comius
comius authored and Copybara-Service committed May 13, 2024
1 parent 73b0faf commit f417aa9
Show file tree
Hide file tree
Showing 13 changed files with 188 additions and 54 deletions.
110 changes: 84 additions & 26 deletions ...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
View file
Expand Up @@ -120,7 +120,6 @@
import net.starlark.java.eval.StarlarkCallable;
import net.starlark.java.eval.StarlarkFunction;
import net.starlark.java.eval.StarlarkInt;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.eval.Tuple;
import net.starlark.java.syntax.Identifier;
Expand Down Expand Up @@ -384,15 +383,6 @@ public StarlarkRuleFunction rule(
// Ensure we're initializing a .bzl file, which also means we have a RuleDefinitionEnvironment.
BzlInitThreadContext bazelContext = BzlInitThreadContext.fromOrFail(thread, "rule()");

if (initializer != Starlark.NONE
|| parentUnchecked != Starlark.NONE
|| !subrules.isEmpty()
|| extendableUnchecked != Starlark.NONE) {
if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_RULE_EXTENSION_API)) {
BuiltinRestriction.failIfCalledOutsideAllowlist(thread, ALLOWLIST_RULE_EXTENSION_API);
}
}

final RuleClass parent;
final boolean executable;
final boolean test;
Expand Down Expand Up @@ -438,21 +428,15 @@ public StarlarkRuleFunction rule(
failIf(buildSetting != Starlark.NONE, "build_setting is not supported when extending rules.");
}

// Get the callstack, sans the last entry, which is the builtin 'rule' callable itself.
ImmutableList<StarlarkThread.CallStackEntry> callStack = thread.getCallStack();
callStack = callStack.subList(0, callStack.size() - 1);

LabelConverter labelConverter = LabelConverter.forBzlEvaluatingThread(thread);

return createRule(
// Contextual parameters.
bazelContext,
thread.getCallerLocation(),
callStack,
thread,
bazelContext.getBzlFile(),
bazelContext.getTransitiveDigest(),
labelConverter,
thread.getSemantics(),
// rule() parameters
parent,
extendableUnchecked,
Expand Down Expand Up @@ -489,12 +473,10 @@ public StarlarkRuleFunction rule(
public static StarlarkRuleFunction createRule(
// Contextual parameters.
RuleDefinitionEnvironment ruleDefinitionEnvironment,
Location loc,
ImmutableList<StarlarkThread.CallStackEntry> definitionCallstack,
StarlarkThread thread,
Label bzlFile,
byte[] transitiveDigest,
LabelConverter labelConverter,
StarlarkSemantics starlarkSemantics,
// Parameters that come from rule().
@Nullable RuleClass parent,
@Nullable Object extendableUnchecked,
Expand Down Expand Up @@ -577,11 +559,66 @@ public static StarlarkRuleFunction createRule(
}
}

if (parent != null
&& !thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_RULE_EXTENSION_API)
&& !bzlFile.getRepository().getName().equals("_builtins")) {
builder.addAllowlistChecker(EXTEND_RULE_API_ALLOWLIST_CHECKER);
if (!builder.contains("$allowlist_extend_rule")) {
Attribute.Builder<Label> allowlistAttr =
attr("$allowlist_extend_rule_api", LABEL)
.cfg(ExecutionTransitionFactory.createFactory())
.mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class))
.value(
ruleDefinitionEnvironment.getToolsLabel(
"//tools/allowlists/extend_rule_allowlist:extend_rule_api_allowlist"));
builder.add(allowlistAttr);
}
}

if (initializer != null) {
if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_RULE_EXTENSION_API)
&& !bzlFile.getRepository().getName().equals("_builtins")) {
builder.addAllowlistChecker(INITIALIZER_ALLOWLIST_CHECKER);
if (!builder.contains("$allowlist_initializer")) {
// the allowlist already exist if this is an extended rule
Attribute.Builder<Label> allowlistAttr =
attr("$allowlist_initializer", LABEL)
.cfg(ExecutionTransitionFactory.createFactory())
.mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class))
.value(
ruleDefinitionEnvironment.getToolsLabel(
"//tools/allowlists/initializer_allowlist"));
builder.add(allowlistAttr);
}
}
}

if (!subrulesUnchecked.isEmpty()) {
if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_RULE_EXTENSION_API)
&& !bzlFile.getRepository().getName().equals("_builtins")) {
builder.addAllowlistChecker(SUBRULES_ALLOWLIST_CHECKER);
if (!builder.contains("$allowlist_subrules")) {
// the allowlist already exist if this is an extended rule
Attribute.Builder<Label> allowlistAttr =
attr("$allowlist_subrules", LABEL)
.cfg(ExecutionTransitionFactory.createFactory())
.mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class))
.value(
ruleDefinitionEnvironment.getToolsLabel(
"//tools/allowlists/subrules_allowlist"));
builder.add(allowlistAttr);
}
}
}

if (executable || test) {
builder.setExecutableStarlark();
}

builder.setCallStack(definitionCallstack);
// Get the callstack, sans the last entry, which is the builtin 'rule' callable itself.
ImmutableList<StarlarkThread.CallStackEntry> callStack = thread.getCallStack();
callStack = callStack.subList(0, callStack.size() - 1);
builder.setCallStack(callStack);

ImmutableList<Pair<String, StarlarkAttrModule.Descriptor>> attributes =
attrObjectToAttributesList(attrs);
Expand Down Expand Up @@ -655,7 +692,7 @@ public static StarlarkRuleFunction createRule(
if (implicitOutputs != Starlark.NONE) {
if (implicitOutputs instanceof StarlarkFunction) {
StarlarkCallbackHelper callback =
new StarlarkCallbackHelper((StarlarkFunction) implicitOutputs, starlarkSemantics);
new StarlarkCallbackHelper((StarlarkFunction) implicitOutputs, thread.getSemantics());
builder.setImplicitOutputsFunction(
new StarlarkImplicitOutputsFunctionWithCallback(callback));
} else {
Expand Down Expand Up @@ -796,7 +833,7 @@ public static StarlarkRuleFunction createRule(

return new StarlarkRuleFunction(
builder,
loc,
thread.getCallerLocation(),
Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString));
}

Expand Down Expand Up @@ -1503,6 +1540,30 @@ public boolean isImmutable() {
.setLocationCheck(AllowlistChecker.LocationCheck.DEFINITION)
.build();

@SerializationConstant
static final AllowlistChecker EXTEND_RULE_API_ALLOWLIST_CHECKER =
AllowlistChecker.builder()
.setAllowlistAttr("extend_rule_api")
.setErrorMessage("Non-allowlisted attempt to use extend rule APIs.")
.setLocationCheck(AllowlistChecker.LocationCheck.DEFINITION)
.build();

@SerializationConstant
static final AllowlistChecker INITIALIZER_ALLOWLIST_CHECKER =
AllowlistChecker.builder()
.setAllowlistAttr("initializer")
.setErrorMessage("Non-allowlisted attempt to use initializer.")
.setLocationCheck(AllowlistChecker.LocationCheck.DEFINITION)
.build();

@SerializationConstant
static final AllowlistChecker SUBRULES_ALLOWLIST_CHECKER =
AllowlistChecker.builder()
.setAllowlistAttr("subrules")
.setErrorMessage("Non-allowlisted attempt to use subrules.")
.setLocationCheck(AllowlistChecker.LocationCheck.DEFINITION)
.build();

@SerializationConstant
static final AllowlistChecker SKIP_VALIDATIONS_ALLOWLIST_CHECKER =
AllowlistChecker.builder()
Expand Down Expand Up @@ -1557,9 +1618,6 @@ public StarlarkSubruleApi subrule(
Sequence<?> subrulesUnchecked,
StarlarkThread thread)
throws EvalException {
if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_RULE_EXTENSION_API)) {
BuiltinRestriction.failIfCalledOutsideAllowlist(thread, ALLOWLIST_RULE_EXTENSION_API);
}
ImmutableMap<String, Descriptor> attrs =
ImmutableMap.copyOf(Dict.cast(attrsUnchecked, String.class, Descriptor.class, "attrs"));
ImmutableList<String> fragments =
Expand Down
4 changes: 0 additions & 4 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java
View file
Expand Up @@ -17,7 +17,6 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY;
import static com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.ALLOWLIST_RULE_EXTENSION_API;
import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;

import com.google.common.base.Optional;
Expand Down Expand Up @@ -596,9 +595,6 @@ public StarlarkActionFactory actions() {

@Override
public Object callParent(StarlarkThread thread) throws EvalException, InterruptedException {
if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_RULE_EXTENSION_API)) {
BuiltinRestriction.failIfCalledOutsideAllowlist(thread, ALLOWLIST_RULE_EXTENSION_API);
}
checkMutable("super()");
if (isForAspect()) {
throw Starlark.errorf("Can't use 'super' call in an aspect.");
Expand Down
9 changes: 1 addition & 8 deletions src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java
View file
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.rules.test;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.RunEnvironmentInfo;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions;
Expand Down Expand Up @@ -85,10 +84,6 @@ public void analysisTest(
throw Starlark.errorf("'name' cannot be set or overridden in 'attr_values'");
}

// Get the callstack, sans the last entry, which is the builtin 'analysis_test' callable itself.
ImmutableList<StarlarkThread.CallStackEntry> callStack = thread.getCallStack();
callStack = callStack.subList(0, callStack.size() - 1);

LabelConverter labelConverter = LabelConverter.forBzlEvaluatingThread(thread);

// Each call to analysis_test defines a rule class (the code right below this comment here) and
Expand Down Expand Up @@ -121,12 +116,10 @@ public void analysisTest(
StarlarkRuleClassFunctions.createRule(
// Contextual parameters.
ruleDefinitionEnvironment,
thread.getCallerLocation(),
callStack,
thread,
dummyBzlFile,
transitiveDigestToUse,
labelConverter,
thread.getSemantics(),
// rule() parameters.
/* parent= */ null,
/* extendableUnchecked= */ false,
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
View file
Expand Up @@ -512,13 +512,33 @@ def _impl(ctx):
packages = ["public"],
)
""");
config.create(
"embedded_tools/tools/allowlists/initializer_allowlist/BUILD",
"""
package_group(
name = "initializer_allowlist",
packages = [],
)
""");
config.create(
"embedded_tools/tools/allowlists/extend_rule_allowlist/BUILD",
"""
package_group(
name = "extend_rule_allowlist",
packages = ["public"],
)
package_group(
name = "extend_rule_api_allowlist",
packages = [],
)
""");
config.create(
"embedded_tools/tools/allowlists/subrules_allowlist/BUILD",
"""
package_group(
name = "subrules_allowlist",
packages = [],
)
""");

config.create(
Expand Down
37 changes: 28 additions & 9 deletions src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
View file
Expand Up @@ -48,6 +48,7 @@
import com.google.devtools.build.lib.testutil.TestConstants;
import net.starlark.java.eval.BuiltinFunction;
import net.starlark.java.eval.Sequence;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -59,6 +60,11 @@ public class StarlarkSubruleTest extends BuildViewTestCase {
private final BazelEvaluationTestCase evOutsideAllowlist =
new BazelEvaluationTestCase("//foo:bar");

@Before
public void allowExperimentalApi() throws Exception {
setBuildLanguageOptions("--experimental_rule_extension_api");
}

@Test
public void testSubruleFunctionSymbol_notVisibleInBUILD() throws Exception {
scratch.file("foo/BUILD", "subrule");
Expand Down Expand Up @@ -1771,17 +1777,30 @@ def _rule_impl(ctx):
assertThat(result).isEqualTo("from B: foo");
}

@Test
public void testSubruleInstantiation_outsideAllowlist_failsWithPrivateAPIError()
throws Exception {
evOutsideAllowlist.checkEvalErrorContains(
"'//foo:bar' cannot use private API", "subrule(implementation = lambda: 0 )");
}

@Test
public void testSubrulesParamForRule_isPrivateAPI() throws Exception {
evOutsideAllowlist.checkEvalErrorContains(
"'//foo:bar' cannot use private API", "rule(implementation = lambda: 0, subrules = [1])");
setBuildLanguageOptions("--noexperimental_rule_extension_api");
scratch.file(
"foo/myrule.bzl",
"""
def _impl(ctx):
pass
my_subrule = subrule(implementation = _impl)
my_rule = rule(_impl, subrules = [my_subrule])
""");
scratch.file(
"foo/BUILD",
"""
load("myrule.bzl", "my_rule")
my_rule(name = "foo")
""");

reporter.removeHandler(failFastHandler);
reporter.addHandler(ev.getEventCollector());
getConfiguredTarget("//foo");

ev.assertContainsError("Non-allowlisted attempt to use subrules.");
}

@Test
Expand Down

0 comments on commit f417aa9

Please sign in to comment.