Disallow certain APIs within symbolic macros
Symbolic macros are not permitted to call APIs that depend upon the order in which they are invoked relative to the rest of the package's evaluation. This rules out mutations to package-level state (other than declarations of targets), such as the `package()` callable, as well as accessors like `native.existing_rules()`. This restriction is to enable future lazy evaluation of symbolic macros, as well as general build health.
The disallowed APIs are:
- package(), licenses()
- environment_group()
- glob(), subpackages(),
- existing_rule(), existing_rules()
`glob()` and `subpackages()` aren't really side-effectful, but stylistically their result should be passed in as arguments, to reduce implicit entanglement between a macro definition and the package.
Repo machinery APIs are also disallowed since they make no sense in the context of symbolic macros. These include: `workspace()`, `register_toolchains()`, `register_execution_platforms()`, `bind()`, and repository rule instantiation.
Some of the disallowed APIs are not even currently reachable from symbolic macro implementations, because they are not available under `native` and symbolic macros can't take arbitrary arguments. We don't add tests for these situations but they are still disallowed nonetheless.
Note that this CL only bans `native.existing_rules()` within symbolic macros, not legacy macros. Targets declared in a symbolic macro are visible to legacy macros via `existing_rules()`, but we intend to either change those semantics or deprecate `existing_rules()` entirely in the future.
Code changes:
- PackageFactory#getContext is replaced by Package.Builder#fromOrFail and a new variant, Package.Builder#fromOrFailNoSymbolicMacros. The latter is used by the disallowed APIs to produce a clean error when called inside a symbolic macro.
- MacroClass#executeMacroImplementation now maintains a stack in the Package.Builder of currently executing symbolic macros. This is used by fromOrFailNoSymbolicMacros to determine whether we are inside a symbolic macro.
- Deleted some unused static helpers in TargetDefinitionContext. That class is kind of unnecessary at the moment but may be useful when we move to a lazy macro evaluation implementation.
- Checking for an expected error in SymbolicMacroTest is factored to a helper. Created a new helper specifically for checking errors when accessing a restricted API.
Work toward #19922
PiperOrigin-RevId: 630437890
Change-Id: I010dc5f1cc47866956acc11191a5d0c81aba8a7d
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
index e786f8f..4c43f17 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
@@ -36,7 +36,6 @@
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.BzlInitThreadContext;
import com.google.devtools.build.lib.packages.Package;
-import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
@@ -256,7 +255,8 @@
String ruleClassName = getRuleClassName();
try {
RuleClass ruleClass = builder.build(ruleClassName, ruleClassName);
- Package.Builder pkgBuilder = PackageFactory.getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "repository rules");
// TODO(adonovan): is this cast safe? Check.
String name = (String) kwargs.get("name");
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java
index d35734a..2ae17ab 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java
@@ -75,7 +75,8 @@
StarlarkThread thread)
throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "environment_group");
- Package.Builder pkgBuilder = PackageFactory.getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "environment_group()");
List<Label> environments =
BuildType.LABEL_LIST.convert(
environmentsList, "'environment_group argument'", pkgBuilder.getLabelConverter());
@@ -116,7 +117,8 @@
StarlarkThread thread)
throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "licenses");
- Package.Builder pkgBuilder = PackageFactory.getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "licenses()");
try {
License license = BuildType.LICENSE.convert(licensesList, "'licenses' operand");
pkgBuilder.mergePackageArgsFrom(PackageArgs.builder().setLicense(license));
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
index 6544edc..78d1742 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
@@ -251,6 +251,7 @@
// ConfiguredRuleClassProvider. For instance, we could put it in the builder.
try {
+ builder.pushMacro(macro);
Starlark.call(
thread,
macro.getMacroClass().getImplementation(),
@@ -263,6 +264,9 @@
Package.error(
/* location= */ null, ex.getMessageWithStack(), Code.STARLARK_EVAL_ERROR));
builder.setContainsErrors();
+ } finally {
+ MacroInstance top = builder.popMacro();
+ Preconditions.checkState(top == macro, "inconsistent macro stack state");
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 42a5b8f..b8de2b3 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -44,6 +44,7 @@
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings;
import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
@@ -958,6 +959,14 @@
// All instances of symbolic macros created during package construction.
private final Map<String, MacroInstance> macros = new LinkedHashMap<>();
+ /**
+ * A stack of currently executing symbolic macros, outermost first.
+ *
+ * <p>Certain APIs are only available when this stack is empty (i.e. not in any symbolic macro).
+ * See user documentation on {@code macro()} ({@link StarlarkRuleFunctionsApi#macro}).
+ */
+ private final List<MacroInstance> macroStack = new ArrayList<>();
+
private enum NameConflictCheckingPolicy {
UNKNOWN,
NOT_GUARANTEED,
@@ -1132,13 +1141,51 @@
public static Builder fromOrFail(StarlarkThread thread, String what) throws EvalException {
@Nullable BazelStarlarkContext ctx = thread.getThreadLocal(BazelStarlarkContext.class);
if (!(ctx instanceof Builder)) {
- // TODO: #19922 - Clarify in the message that we can't be in a symbolic ("first-class")
- // macro.
- throw Starlark.errorf("%s can only be used while evaluating a BUILD file", what);
+ // This error message might be a little misleading for APIs that can be called from either
+ // BUILD or WORKSPACE threads. In that case, we expect the calling API will do a separate
+ // check that we're in a WORKSPACE thread and emit an appropriate message before calling
+ // fromOrFail().
+ throw Starlark.errorf(
+ "%s can only be used while evaluating a BUILD file and its macros", what);
}
return (Builder) ctx;
}
+ /**
+ * Same as {@link #fromOrFail}, but also throws {@link EvalException} if we're currently
+ * executing a symbolic macro implementation.
+ *
+ * <p>Use this method when implementing APIs that should not be accessible from symbolic macros,
+ * such as {@code glob()} or {@code package()}.
+ *
+ * <p>This method succeeds when called from a legacy macro (that is not itself called from any
+ * symbolic macro).
+ */
+ @CanIgnoreReturnValue
+ public static Builder fromOrFailDisallowingSymbolicMacros(StarlarkThread thread, String what)
+ throws EvalException {
+ @Nullable BazelStarlarkContext ctx = thread.getThreadLocal(BazelStarlarkContext.class);
+ if (ctx instanceof Builder builder) {
+ if (builder.macroStack.isEmpty()) {
+ return builder;
+ }
+ }
+
+ boolean macrosEnabled =
+ thread
+ .getSemantics()
+ .getBool(BuildLanguageOptions.EXPERIMENTAL_ENABLE_FIRST_CLASS_MACROS);
+ // As in fromOrFail() above, some APIs can be used from either BUILD or WORKSPACE threads,
+ // so this error message might be misleading (e.g. if a symbolic macro attempts to call a
+ // feature available in WORKSPACE). But that type of misuse seems unlikely, and WORKSPACE is
+ // going away soon anyway, so we won't tweak the message for it.
+ throw Starlark.errorf(
+ macrosEnabled
+ ? "%s can only be used while evaluating a BUILD file or legacy macro"
+ : "%s can only be used while evaluating a BUILD file and its macros",
+ what);
+ }
+
PackageIdentifier getPackageIdentifier() {
return pkg.getPackageIdentifier();
}
@@ -1730,6 +1777,16 @@
macros.put(macro.getName(), macro);
}
+ /** Pushes a macro instance onto the stack of currently executing symbolic macros. */
+ public void pushMacro(MacroInstance macro) {
+ macroStack.add(macro);
+ }
+
+ /** Pops the stack of currently executing symbolic macros. */
+ public MacroInstance popMacro() {
+ return macroStack.remove(macroStack.size() - 1);
+ }
+
void addRegisteredExecutionPlatforms(List<TargetPattern> platforms) {
this.registeredExecutionPlatforms.addAll(platforms);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java b/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java
index 97c3c6b..d72b0c0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java
@@ -38,7 +38,8 @@
useStarlarkThread = true)
public Object packageCallable(Map<String, Object> kwargs, StarlarkThread thread)
throws EvalException {
- Package.Builder pkgBuilder = PackageFactory.getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "package()");
if (pkgBuilder.isPackageFunctionUsed()) {
throw new EvalException("'package' can only be used once per BUILD file");
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 622d963..3c78104 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -208,24 +208,6 @@
return ruleClassProvider;
}
- /**
- * Retrieves the {@link Package.Builder} from the given {@link StarlarkThread}, or throws {@link
- * EvalException} if unavailable.
- */
- // TODO(#19922): The name is a holdover from when we had PackageContext. Migrate this to a static
- // fromOrFail method on Package.Builder or a new parent interface of it.
- public static Package.Builder getContext(StarlarkThread thread) throws EvalException {
- Package.Builder value = Package.Builder.fromOrNull(thread);
- if (value == null) {
- // if PackageBuilder is missing, we're not called from a BUILD file. This happens if someone
- // uses native.some_func() in the wrong place.
- throw Starlark.errorf(
- "The native module can be accessed only from a BUILD thread. "
- + "Wrap the function in a macro and call it from a BUILD file");
- }
- return value;
- }
-
public Package.Builder newExternalPackageBuilder(
WorkspaceFileKey workspaceFileKey,
String workspaceName,
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index 4be90fa..9414b20 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -282,7 +282,7 @@
}
BazelStarlarkContext.checkLoadingOrWorkspacePhase(thread, ruleClass.getName());
try {
- Package.Builder pkgBuilder = PackageFactory.getContext(thread);
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "rules");
RuleFactory.createAndAddRule(
pkgBuilder,
ruleClass,
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
index a2c1c83..52b7cf2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
@@ -14,7 +14,6 @@
package com.google.devtools.build.lib.packages;
-import static com.google.devtools.build.lib.packages.PackageFactory.getContext;
import static java.util.Comparator.naturalOrder;
import com.google.common.base.Joiner;
@@ -29,7 +28,6 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.LabelValidator;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.io.FileSymlinkException;
import com.google.devtools.build.lib.packages.Globber.BadGlobException;
@@ -98,7 +96,8 @@
StarlarkThread thread)
throws EvalException, InterruptedException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.glob");
- Package.Builder pkgBuilder = getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "glob()");
List<String> includes = Types.STRING_LIST.convert(include, "'glob' argument");
List<String> excludes = Types.STRING_LIST.convert(exclude, "'glob' argument");
@@ -436,7 +435,8 @@
return Starlark.NONE;
}
BazelStarlarkContext.checkLoadingOrWorkspacePhase(thread, "native.existing_rule");
- Package.Builder pkgBuilder = getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "existing_rule()");
Target target = pkgBuilder.getTarget(name);
if (target instanceof Rule /* `instanceof` also verifies that target != null */) {
Rule rule = (Rule) target;
@@ -509,7 +509,8 @@
return Dict.empty();
}
BazelStarlarkContext.checkLoadingOrWorkspacePhase(thread, "native.existing_rules");
- Package.Builder pkgBuilder = getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "existing_rules()");
if (thread
.getSemantics()
.getBool(BuildLanguageOptions.INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW)) {
@@ -532,7 +533,7 @@
String name, Sequence<?> packagesO, Sequence<?> includesO, StarlarkThread thread)
throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.package_group");
- Package.Builder pkgBuilder = getContext(thread);
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "package_group()");
List<String> packages =
Types.STRING_LIST.convert(packagesO, "'package_group.packages argument'");
@@ -568,7 +569,7 @@
Sequence<?> srcs, Object visibilityO, Object licensesO, StarlarkThread thread)
throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.exports_files");
- Package.Builder pkgBuilder = getContext(thread);
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "exports_files()");
List<String> files = Types.STRING_LIST.convert(srcs, "'exports_files' operand");
RuleVisibility visibility =
@@ -609,8 +610,8 @@
@Override
public String packageName(StarlarkThread thread) throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.package_name");
- PackageIdentifier packageId = getContext(thread).getPackageIdentifier();
- return packageId.getPackageFragment().getPathString();
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "package_name()");
+ return pkgBuilder.getPackageIdentifier().getPackageFragment().getPathString();
}
@Override
@@ -623,18 +624,19 @@
@Override
public String repoName(StarlarkThread thread) throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.repo_name");
- return getContext(thread).getPackageIdentifier().getRepository().getName();
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "repo_name()");
+ return pkgBuilder.getPackageIdentifier().getRepository().getName();
}
@Override
public Label packageRelativeLabel(Object input, StarlarkThread thread) throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.package_relative_label");
- if (input instanceof Label) {
- return (Label) input;
+ if (input instanceof Label inputLabel) {
+ return inputLabel;
}
try {
- String s = (String) input;
- return getContext(thread).getLabelConverter().convert(s);
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "package_relative_label()");
+ return pkgBuilder.getLabelConverter().convert((String) input);
} catch (LabelSyntaxException e) {
throw Starlark.errorf("invalid label in native.package_relative_label: %s", e.getMessage());
}
@@ -644,14 +646,16 @@
@Nullable
public String moduleName(StarlarkThread thread) throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.module_name");
- return getContext(thread).getAssociatedModuleName().orElse(null);
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "module_name()");
+ return pkgBuilder.getAssociatedModuleName().orElse(null);
}
@Override
@Nullable
public String moduleVersion(StarlarkThread thread) throws EvalException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.module_version");
- return getContext(thread).getAssociatedModuleVersion().orElse(null);
+ Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "module_version()");
+ return pkgBuilder.getAssociatedModuleVersion().orElse(null);
}
private static Dict<String, Object> getRuleDict(Rule rule, Mutability mu) throws EvalException {
@@ -832,7 +836,8 @@
Sequence<?> include, Sequence<?> exclude, boolean allowEmpty, StarlarkThread thread)
throws EvalException, InterruptedException {
BazelStarlarkContext.checkLoadingPhase(thread, "native.subpackages");
- Package.Builder pkgBuilder = getContext(thread);
+ Package.Builder pkgBuilder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "subpackages()");
List<String> includes = Types.STRING_LIST.convert(include, "'subpackages' argument");
List<String> excludes = Types.STRING_LIST.convert(exclude, "'subpackages' argument");
diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java b/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java
index 7cb43a6..86283dd 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java
@@ -14,17 +14,16 @@
package com.google.devtools.build.lib.packages;
-import com.google.errorprone.annotations.CanIgnoreReturnValue;
-import javax.annotation.Nullable;
-import net.starlark.java.eval.EvalException;
-import net.starlark.java.eval.Starlark;
-import net.starlark.java.eval.StarlarkThread;
-
/**
* A context object, usually stored in a {@link StarlarkThread}, upon which rules and symbolic
* macros can be instantiated.
*/
-// TODO(#19922): Elevate some Package.Builder methods to this class.
+// TODO(#19922): This class isn't really needed until we implement lazy macro evaluation. At that
+// point, we'll need to split the concept of a Package.Builder into a separate PackagePiece.Builder
+// that represents the object produced by evaluating a macro implementation. Then we can factor the
+// accessors and mutations that are common to BUILD files / lazy macros and to symbolic macros into
+// this common parent class, while Package.Builder retains the stuff that's prohibited inside
+// symbolic macros.
public abstract class TargetDefinitionContext extends BazelStarlarkContext {
/**
@@ -44,25 +43,4 @@
protected TargetDefinitionContext(Phase phase) {
super(phase);
}
-
- /** Retrieves this object from a Starlark thread. Returns null if not present. */
- @Nullable
- public static TargetDefinitionContext fromOrNull(StarlarkThread thread) {
- BazelStarlarkContext ctx = thread.getThreadLocal(BazelStarlarkContext.class);
- return (ctx instanceof TargetDefinitionContext) ? (TargetDefinitionContext) ctx : null;
- }
-
- /**
- * Retrieves this object from a Starlark thread. If not present, throws {@code EvalException} with
- * an error message indicating that {@code what} can't be used in this Starlark environment.
- */
- @CanIgnoreReturnValue
- public static TargetDefinitionContext fromOrFail(StarlarkThread thread, String what)
- throws EvalException {
- @Nullable BazelStarlarkContext ctx = thread.getThreadLocal(BazelStarlarkContext.class);
- if (!(ctx instanceof TargetDefinitionContext)) {
- throw Starlark.errorf("%s can only be used while evaluating a BUILD file or macro", what);
- }
- return (TargetDefinitionContext) ctx;
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index 2a13fc0..ebe2c32 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -244,7 +244,8 @@
throw new EvalException("unexpected positional arguments");
}
try {
- Package.Builder builder = PackageFactory.getContext(thread);
+ Package.Builder builder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "repository rules");
// TODO(adonovan): this cast doesn't look safe!
String externalRepoName = (String) kwargs.get("name");
if (!allowOverride
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
index be7cf520..2632671 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
@@ -68,7 +68,7 @@
}
// Add entry in repository map from "@name" --> "@" to avoid issue where bazel
// treats references to @name as a separate external repo
- PackageFactory.getContext(thread)
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "workspace()")
.setWorkspaceName(name)
.addRepositoryMappingEntry(RepositoryName.MAIN, name, RepositoryName.MAIN);
}
@@ -104,7 +104,9 @@
public void registerExecutionPlatforms(Sequence<?> platformLabels, StarlarkThread thread)
throws EvalException {
// Add to the package definition for later.
- Package.Builder builder = PackageFactory.getContext(thread);
+ Package.Builder builder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(
+ thread, "register_execution_platforms()");
List<String> patterns = Sequence.cast(platformLabels, String.class, "platform_labels");
builder.addRegisteredExecutionPlatforms(parsePatterns(patterns, builder, thread));
}
@@ -113,7 +115,8 @@
public void registerToolchains(Sequence<?> toolchainLabels, StarlarkThread thread)
throws EvalException {
// Add to the package definition for later.
- Package.Builder builder = PackageFactory.getContext(thread);
+ Package.Builder builder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "register_toolchains()");
List<String> patterns = Sequence.cast(toolchainLabels, String.class, "toolchain_labels");
builder.addRegisteredToolchains(
parsePatterns(patterns, builder, thread),
@@ -130,7 +133,8 @@
throw Starlark.errorf("%s", e.getMessage());
}
try {
- Package.Builder builder = PackageFactory.getContext(thread);
+ Package.Builder builder =
+ Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "bind()");
RuleClass ruleClass = ruleClassMap.get("bind");
RepositoryName currentRepo = getCurrentRepoName(thread);
WorkspaceFactoryHelper.addBindRule(
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java
index fb671ab..24731a5 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java
@@ -225,9 +225,21 @@
site of the rule. Such attributes can be assigned a default value (as in
<code>attr.label(default="//pkg:foo")</code>) to create an implicit dependency on a label.
+<p>Certain APIs are not available within symbolic macros. These include:
+<ul>
+ <li>package(), licenses()
+ <li>environment_group()
+ <li>glob(), subpackages()
+ <li>existing_rules(), existing_rule(),
+ <li>(for WORKSPACE threads) workspace(), register_toolchains(),
+ register_execution_platforms(), bind(), repository rule instantiation
+</ul>
+
<p>To limit memory usage, there is a cap on the number of attributes that may be declared.
"""),
- // TODO(#19922): Make good on the above threat of enforcing a cap on the number of
+ // TODO: #19922 - Make a concepts page for symbolic macros, migrate some details like the
+ // list of disallowed APIs to there.
+ // TODO: #19922 - Make good on the above threat of enforcing a cap on the number of
// attributes.
@Param(
name = "doc",
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java
index d8a6621..9e956a1 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java
@@ -53,6 +53,22 @@
assertThat(pkg.containsErrors()).isFalse();
}
+ /**
+ * Convenience method for asserting that a package evaluates in error and produces an event
+ * containing the given substring.
+ *
+ * <p>Note that this is not suitable for errors that occur during top-level .bzl evaluation (i.e.,
+ * triggered by load() rather than during BUILD evaluation), since our test framework fails to
+ * produce a result in that case (b/26382502).
+ */
+ private void assertGetPackageFailsWithEvent(String pkgName, String msg) throws Exception {
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage(pkgName);
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent(msg);
+ }
+
@Test
public void implementationIsInvokedWithNameParam() throws Exception {
scratch.file(
@@ -90,11 +106,7 @@
my_macro(name="abc")
""");
- reporter.removeHandler(failFastHandler);
- Package pkg = getPackage("pkg");
- assertThat(pkg).isNotNull();
- assertThat(pkg.containsErrors()).isTrue();
- assertContainsEvent("_impl() got unexpected keyword argument: name");
+ assertGetPackageFailsWithEvent("pkg", "_impl() got unexpected keyword argument: name");
}
@Test
@@ -143,17 +155,20 @@
assertThat(pkg.getTargets()).containsKey("abc$inner$lib");
}
- // TODO: #19922 - Invert this, symbolic macros shouldn't be able to call glob().
- @Test
- public void macroCanCallGlob() throws Exception {
- scratch.file("pkg/foo.txt");
+ /**
+ * Implementation of a test that ensures a given API cannot be called from inside a symbolic
+ * macro.
+ */
+ private void doCannotCallApiTest(String apiName, String usageLine) throws Exception {
scratch.file(
"pkg/foo.bzl",
- """
- def _impl(name):
- print("Glob result: %s" % native.glob(["foo*"]))
- my_macro = macro(implementation=_impl)
- """);
+ String.format(
+ """
+ def _impl(name):
+ %s
+ my_macro = macro(implementation=_impl)
+ """,
+ usageLine));
scratch.file(
"pkg/BUILD",
"""
@@ -161,35 +176,50 @@
my_macro(name="abc")
""");
- Package pkg = getPackage("pkg");
- assertPackageNotInError(pkg);
- assertContainsEvent("Glob result: [\"foo.bzl\", \"foo.txt\"]");
+ assertGetPackageFailsWithEvent(
+ "pkg",
+ String.format(
+ "%s can only be used while evaluating a BUILD file or legacy macro", apiName));
}
- // TODO: #19922 - Invert this, symbolic macros shouldn't be able to call existing_rules().
@Test
- public void macroCanCallExistingRules() throws Exception {
- scratch.file(
- "pkg/foo.bzl",
- """
- def _impl(name):
- native.cc_binary(name = name + "$lib")
- print("existing_rules() keys: %s" % native.existing_rules().keys())
- my_macro = macro(implementation=_impl)
- """);
- scratch.file(
- "pkg/BUILD",
- """
- load(":foo.bzl", "my_macro")
- cc_library(name = "outer_target")
- my_macro(name="abc")
- """);
-
- Package pkg = getPackage("pkg");
- assertPackageNotInError(pkg);
- assertContainsEvent("existing_rules() keys: [\"outer_target\", \"abc$lib\"]");
+ public void macroCannotCallPackage() throws Exception {
+ doCannotCallApiTest(
+ "package()", "native.package(default_visibility = ['//visibility:public'])");
}
+ @Test
+ public void macroCannotCallGlob() throws Exception {
+ doCannotCallApiTest("glob()", "native.glob(['foo*'])");
+ }
+
+ @Test
+ public void macroCannotCallSubpackages() throws Exception {
+ doCannotCallApiTest("subpackages()", "native.subpackages(include = ['*'])");
+ }
+
+ @Test
+ public void macroCannotCallExistingRule() throws Exception {
+ doCannotCallApiTest("existing_rule()", "native.existing_rule('foo')");
+ }
+
+ @Test
+ public void macroCannotCallExistingRules() throws Exception {
+ doCannotCallApiTest("existing_rules()", "native.existing_rules()");
+ }
+
+ // There are other symbols that must not be called from within symbolic macros, but we don't test
+ // them because they can't be obtained from a symbolic macro implementation anyway, since they are
+ // not under `native` (at least, for BUILD-loaded .bzl files) and because symbolic macros can't
+ // take arbitrary parameter types from their caller. These untested symbols include:
+ //
+ // - For BUILD threads: licenses(), environment_group()
+ // - For WORKSPACE threads: workspace(), register_toolchains(), register_execution_platforms(),
+ // bind(), and repository rules.
+ //
+ // Starlark-defined repository rules might technically be callable but we skip over that edge
+ // case here.
+
// TODO: #19922 - This behavior is necessary to preserve compatibility with use cases for
// native.existing_rules(), but it's a blocker for making symbolic macro evaluation lazy.
@Test
@@ -353,11 +383,8 @@
)
""");
- reporter.removeHandler(failFastHandler);
- Package pkg = getPackage("pkg");
- assertThat(pkg).isNotNull();
- assertThat(pkg.containsErrors()).isTrue();
- assertContainsEvent("missing value for mandatory attribute 'xyz' in 'my_macro' macro");
+ assertGetPackageFailsWithEvent(
+ "pkg", "missing value for mandatory attribute 'xyz' in 'my_macro' macro");
}
@Test
@@ -386,11 +413,8 @@
)
""");
- reporter.removeHandler(failFastHandler);
- Package pkg = getPackage("pkg");
- assertThat(pkg).isNotNull();
- assertThat(pkg.containsErrors()).isTrue();
- assertContainsEvent("no such attribute 'xyz' in 'my_macro' macro (did you mean 'xzz'?)");
+ assertGetPackageFailsWithEvent(
+ "pkg", "no such attribute 'xyz' in 'my_macro' macro (did you mean 'xzz'?)");
}
@Test
@@ -450,11 +474,7 @@
)
""");
- reporter.removeHandler(failFastHandler);
- Package pkg = getPackage("pkg");
- assertThat(pkg).isNotNull();
- assertThat(pkg.containsErrors()).isTrue();
- assertContainsEvent("Error in append: trying to mutate a frozen list value");
+ assertGetPackageFailsWithEvent("pkg", "Error in append: trying to mutate a frozen list value");
}
@Test
@@ -483,9 +503,7 @@
assertThat(pkg.getMacros()).containsKey("abc");
}
- // TODO: #19922 - Add more test cases for interaction between macros and environment_group,
- // package_group, implicit/explicit input files, and the package() function. But all of these
- // behaviors are about to change (from allowed to prohibited).
+ // TODO: #19922 - Add more test cases for implicit/explicit input files
@Test
public void attrsAllowSelectsByDefault() throws Exception {
@@ -582,11 +600,7 @@
)
""");
- reporter.removeHandler(failFastHandler);
- Package pkg = getPackage("pkg");
- assertThat(pkg).isNotNull();
- assertThat(pkg.containsErrors()).isTrue();
- assertContainsEvent("attribute \"xyz\" is not configurable");
+ assertGetPackageFailsWithEvent("pkg", "attribute \"xyz\" is not configurable");
}
// TODO(b/331193690): Prevent selects from being evaluated as bools
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java
index 83d6492..89aa723 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java
@@ -2093,7 +2093,7 @@
""");
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:my_glob");
- assertContainsEvent("The native module can be accessed only from a BUILD thread.");
+ assertContainsEvent("glob() can only be used while evaluating a BUILD file and its macros");
}
@Test
