Customize the builtins bzl environment
This makes @builtins bzls able to load a new "_internal" module that will hold features not accessible to ordinary bzls. In particular, this will include the "original" native module field values before they get optionally replaced by builtins injection, as well as a view of the current StarlarkSemantics flags.
This changes also makes @builtins bzls unable to access the top-level "native" object; we will soon also prohibit direct access to other top-level rule logic symbols like CcInfo. (Rationale: These are the very symbols that @builtins bzls are helping to define for ordinary users. It would be confusing if regular bzls and @builtins bzls attached different meanings to the same names.)
To enable this change, ASTFileLookupFunction now retrieves the predeclared environment from PackageFactory using similar methods as BzlLoadFunction, instead of blindly applying the same environment to all bzls. As it turns out, ASTFileLookupFunction doesn't need access to the injected environment since builtins injection only changes the definitions of symbols, not whether or not the symbol exists at all. Therefore ASTFileLookupFunction doesn't need to request StarlarkBuiltinsValue from Skyframe.
A new builtins ASTFileLookupValue key type is created, and it is wired into the corresponding key type for BzlLoadValue (getASTKey()).
BuiltinsInjectionTest:
- Renamed some test cases to eliminate boilerplate prefix
- Split the test that @builtins bzls can't access stuff into two cases: "native", and rule logic symbols like CcInfo. The latter isn't implemented yet, so its assertions will be flipped.
- Rename "builtins_dummy.bzl" in test case for consistency with other test cases; simplify exports.bzl to remove irrelevant setup in a test case
- Add tests that @builtins bzls can access _internal and ordinary bzls can't
Drive-bys: Rename a test case in BzlLoadFunctionTest for clarity. Add @Nullable to an override in Module.java where the parent method already has it.
Word toward #11437.
RELNOTES: None
PiperOrigin-RevId: 328146383
diff --git a/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java b/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java
new file mode 100644
index 0000000..817f5bb
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java
@@ -0,0 +1,54 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.packages;
+
+import com.google.devtools.build.lib.syntax.Printer;
+import com.google.devtools.build.lib.syntax.StarlarkValue;
+import net.starlark.java.annot.StarlarkBuiltin;
+import net.starlark.java.annot.StarlarkDocumentationCategory;
+
+// TODO(#11437): Factor an API out into skylarkbuildapi, for stardoc's benefit. Otherwise, stardoc
+// can't run on @builtins bzls.
+/** The {@code _internal} Starlark object, visible only to {@code @builtins} .bzls. */
+@StarlarkBuiltin(
+ name = "_internal",
+ category = StarlarkDocumentationCategory.BUILTIN,
+ documented = false,
+ doc =
+ "A module accessible only to @builtins .bzls, that permits access to the original "
+ + "(uninjected) native builtins, as well as internal-only symbols not accessible to "
+ + "users.")
+public class InternalModule implements StarlarkValue {
+
+ // TODO(#11437): Can't use a singleton once we're re-exporting fields of the native object.
+ public static final InternalModule INSTANCE = new InternalModule();
+
+ private InternalModule() {}
+
+ @Override
+ public void repr(Printer printer) {
+ printer.append("<_internal module>");
+ }
+
+ @Override
+ public boolean isImmutable() {
+ return true;
+ }
+
+ @Override
+ public boolean isHashable() {
+ return true;
+ }
+}
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 6d28ac3..6bf161a 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
@@ -741,9 +741,9 @@
// Preconditions.checkState(!predeclared.containsKey("native"));
// Determine the "native" module.
- // TODO(bazel-team): Use the same "native" object for both BUILD- and WORKSPACE-loaded .bzls,
- // and just have it be a dynamic error to call the wrong thing at the wrong time. This is a
- // breaking change.
+ // TODO(#11954): Use the same "native" object for both BUILD- and WORKSPACE-loaded .bzls, and
+ // just have it be a dynamic error to call the wrong thing at the wrong time. This is a breaking
+ // change.
env.put("native", createNativeModule(uninjectedBuildBzlNativeBindings));
return ImmutableMap.copyOf(env);
@@ -763,8 +763,23 @@
private static ImmutableMap<String, Object> createBuiltinsBzlEnv(
RuleClassProvider ruleClassProvider) {
- // TODO(#11437): create the _internal name, prohibit other rule logic names, including "native".
- return ruleClassProvider.getEnvironment();
+ Map<String, Object> env = new HashMap<>();
+ env.putAll(ruleClassProvider.getEnvironment());
+
+ // TODO($11437): We shouldn't have to delete the (not fully formed) "native" object here; see
+ // above TODO in createUninjectedBuildBzlEnv().
+ env.remove("native");
+ // TODO(#11437): Prohibit other rule logic names, e.g. "CcInfo".
+
+ // TODO(#11437): To support inspection of StarlarkSemantics via _internal, we'll have to let
+ // this method be parameterized by the StarlarkSemantics, which means it'll need to be computed
+ // on the fly and not initialized on PackageFactory construction. To avoid computing it
+ // redundantly for each builtins bzl evaluation, we can either 1) create a second
+ // StarlarkBuiltinsValue-like object (which sounds like a lot of work), or 2) create a cache
+ // from StarlarkSemantics to builtins predeclared envs (sounds preferable to me).
+ env.put("_internal", InternalModule.INSTANCE);
+
+ return ImmutableMap.copyOf(env);
}
/** Constructs a "native" module object with the given contents. */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index 1d0793b..799b892 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -35,6 +35,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
+import java.util.Map;
import javax.annotation.Nullable;
/**
@@ -143,6 +144,18 @@
return null;
}
+ Map<String, Object> predeclared;
+ if (key.kind == ASTFileLookupValue.Kind.BUILTINS) {
+ predeclared = packageFactory.getBuiltinsBzlEnv();
+ } else {
+ // Use the predeclared environment for BUILD-loaded bzl files, ignoring injection. It is not
+ // the right env for the actual evaluation of either BUILD-loaded bzl files or
+ // WORKSPACE-loaded bzl files. But the names of the symbols are the same, and the names are
+ // all we need to do symbol resolution (modulo FlagGuardedValues -- see TODO in
+ // PackageFactory.createBuildBzlEnvUsingInjection()).
+ predeclared = packageFactory.getUninjectedBuildBzlEnv();
+ }
+
// We have all deps. Parse, resolve, and return.
ParserInput input = ParserInput.fromLatin1(bytes, inputName);
FileOptions options =
@@ -152,8 +165,7 @@
.restrictStringEscapes(semantics.incompatibleRestrictStringEscapes())
.build();
StarlarkFile file = StarlarkFile.parse(input, options);
- Module module =
- Module.withPredeclared(semantics, packageFactory.getRuleClassProvider().getEnvironment());
+ Module module = Module.withPredeclared(semantics, predeclared);
Resolver.resolveFile(file, module);
Event.replayEventsOn(env.getListener(), file.errors()); // TODO(adonovan): fail if !ok()?
return ASTFileLookupValue.withFile(file, digest);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
index 2dc5889..246107c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
@@ -138,10 +138,18 @@
/** Types of bzl files we may encounter. */
enum Kind {
- /** A regularly loaded .bzl file. */
+ /** A regular .bzl file loaded on behalf of a BUILD or WORKSPACE file. */
+ // The reason we can share a single key type for these environments is that they have the same
+ // symbol names, even though their symbol definitions (particularly for the "native" object)
+ // differ. (See also #11954, which aims to make even the symbol definitions the same.)
NORMAL,
+
+ /** A .bzl file loaded during evaluation of the {@code @builtins} pseudo-repository. */
+ BUILTINS,
+
/** The prelude file, whose declarations are implicitly loaded by all BUILD files. */
PRELUDE,
+
/**
* A virtual empty file that does not correspond to a lookup in the filesystem. This is used for
* the default prelude contents, when the real prelude's contents should be ignored (in
@@ -208,6 +216,13 @@
return keyInterner.intern(new Key(root, label, Kind.NORMAL));
}
+ /** Constructs a key for loading a builtins .bzl. */
+ // TODO(#11437): Retrieve the builtins bzl from the root given by
+ // --experimental_builtins_bzl_path, instead of making the caller specify it here.
+ public static Key keyForBuiltins(Root root, Label label) {
+ return keyInterner.intern(new Key(root, label, Kind.BUILTINS));
+ }
+
/** Constructs a key for loading the prelude .bzl. */
static Key keyForPrelude(Root root, Label label) {
return keyInterner.intern(new Key(root, label, Kind.PRELUDE));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
index 859349d..37b30c8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
@@ -274,7 +274,7 @@
@Override
ASTFileLookupValue.Key getASTKey(Root root) {
- return ASTFileLookupValue.key(root, label);
+ return ASTFileLookupValue.keyForBuiltins(root, label);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Module.java b/src/main/java/com/google/devtools/build/lib/syntax/Module.java
index fb88c90..1aec2f7 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Module.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Module.java
@@ -200,6 +200,7 @@
}
@Override
+ @Nullable
public String getUndeclaredNameError(String name) {
Object v = getPredeclared(name);
return v instanceof FlagGuardedValue
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
index 782fc54..a351695 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
@@ -117,7 +117,7 @@
}
@Test
- public void evalWithInjection_errorInEvaluatingBuiltins() throws Exception {
+ public void errorInEvaluatingBuiltins() throws Exception {
// Test case with a Starlark error in the @builtins pseudo-repo itself.
// TODO(#11437): Use @builtins//:... syntax for load, once supported.
scratch.file(
@@ -144,7 +144,7 @@
}
@Test
- public void evalWithInjection_errorInProcessingExports() throws Exception {
+ public void errorInProcessingExports() throws Exception {
// Test case with an error in the symbols exported by exports.bzl, but no actual Starlark errors
// in the @builtins files themselves.
writeExportsBzl(
@@ -194,8 +194,12 @@
assertContainsEvent("overridable_rule :: <built-in rule overridable_rule>");
}
+ // TODO(#11954): Once WORKSPACE- and BUILD-loaded bzls use the exact same environments, we'll want
+ // to apply injection to both. This is for uniformity, not because we actually care about builtins
+ // injection for WORKSPACE bzls. In the meantime, assert the status quo: WORKSPACE bzls do not use
+ // injection.
@Test
- public void evalWorkspaceBzl_doesNotUseInjection() throws Exception {
+ public void workspaceBzlDoesNotUseInjection() throws Exception {
writeExportsBzl(
"exported_toplevels = {'overridable_symbol': 'new_value'}",
"exported_rules = {'overridable_rule': 'new_rule'}",
@@ -219,41 +223,95 @@
// contain our original mock rule. We can test this for WORKSPACE files at the top-level though.
}
- // TODO(#11437): Broaden to also assert that it can't access rule-specific symbols (like our
- // override symbols) but can still access basic symbols (print(), rule()).
@Test
- public void evalBuiltinsBzl_cannotAccessNative() throws Exception {
+ public void builtinsBzlCannotAccessNative() throws Exception {
+ // TODO(#11437): Use @builtins//:... syntax for load, once supported.
+ scratch.file(
+ "tools/builtins_staging/helper.bzl", //
+ "builtins_dummy = None",
+ "print('made it to evaluation')",
+ "print('overridable_rule :: ' + str(native.overridable_rule))");
writeExportsBzl(
- // The load string here references a regular label, but it gets loaded using a
- // BzlLoadValue.KeyForBuiltins because we're in exports.bzl.
- // TODO(#11437): Rewrite this load syntax to @builtins//...
- "load('//pkg:builtins_dummy.bzl', 'builtins_dummy')",
- "exported_toplevels = {'overridable_symbol': 'new_value'}",
- "exported_rules = {'overridable_rule': 'new_rule'}",
+ "load('//tools/builtins_staging:helper.bzl', 'builtins_dummy')",
+ "exported_toplevels = {}",
+ "exported_rules = {}",
"exported_to_java = {}");
writePkgBzl();
- scratch.file(
- "pkg/builtins_dummy.bzl", //
- "builtins_dummy = None",
- "print('overridable_symbol :: ' + str(overridable_symbol))",
- "print('overridable_rule :: ' + str(native.overridable_rule))");
reporter.removeHandler(failFastHandler);
buildDummyWithoutAssertingSuccess();
- // Currently overridable_symbol, a piece of (mock) rule logic, is accessible, but a future
- // change will prevent that.
+ // The print() statement is never reached because we fail statically during name resolution in
+ // ASTFileLookupFunction.
+ assertDoesNotContainEvent("made it to evaluation");
+ assertContainsEvent("name 'native' is not defined");
+ }
+
+ // TODO(#11437): Invert this behavior and test (and test name) -- builtins bzls should not be
+ // able to see rule logic symbols like CcInfo (and in our test, overridable_symbol). But they
+ // should still be able to see print(), rule(), etc.
+ @Test
+ public void builtinsBzlCanAccessRuleLogicSymbols() throws Exception {
+ // TODO(#11437): Use @builtins//:... syntax for load, once supported.
+ scratch.file(
+ "tools/builtins_staging/helper.bzl", //
+ "builtins_dummy = None",
+ "print('made it to evaluation')",
+ "print('overridable_symbol :: ' + str(overridable_symbol))");
+ writeExportsBzl(
+ "load('//tools/builtins_staging:helper.bzl', 'builtins_dummy')",
+ "exported_toplevels = {}",
+ "exported_rules = {}",
+ "exported_to_java = {}");
+ writePkgBzl();
+ reporter.removeHandler(failFastHandler);
+
+ buildDummyWithoutAssertingSuccess();
+ // TODO(#11437): Uncomment this comment (ha!) and replace the below assertion with the commented
+ // ones when we flip this test case.
+ // // The print() statement is never reached because we fail statically during name resolution
+ // // in ASTFileLookupFunction.
+ // assertDoesNotContainEvent("made it to evaluation");
+ // assertContainsEvent("name 'overridable_symbol' is not defined");
assertContainsEvent("overridable_symbol :: original_value");
- // Currently, the "native" object actually exists, but isn't fully formed -- it contains generic
- // methods but not specific rules. We will change this to be completely absent from @builtins.
- assertContainsEvent("'native' value has no field or method 'overridable_rule'");
+ }
+
+ @Test
+ public void regularBzlCannotAccessInternal() throws Exception {
+ writeExportsBzl(
+ "exported_toplevels = {}", //
+ "exported_rules = {}",
+ "exported_to_java = {}");
+ writePkgBzl("print('_internal :: ' + str(_internal))");
+ reporter.removeHandler(failFastHandler);
+
+ buildDummyWithoutAssertingSuccess();
+ assertContainsEvent("name '_internal' is not defined");
+ }
+
+ @Test
+ public void builtinsBzlCanAccessInternal() throws Exception {
+ // TODO(#11437): Use @builtins//:... syntax for load, once supported.
+ scratch.file(
+ "tools/builtins_staging/helper.bzl", //
+ "builtins_dummy = None",
+ "print('_internal in helper.bzl :: ' + str(_internal))");
+ writeExportsBzl(
+ "load('//tools/builtins_staging:helper.bzl', 'builtins_dummy')",
+ "exported_toplevels = {}",
+ "exported_rules = {}",
+ "exported_to_java = {}",
+ "print('_internal in exports.bzl :: ' + str(_internal))");
+ writePkgBzl();
+
+ buildDummyAndAssertSuccess();
+ assertContainsEvent("_internal in helper.bzl :: <_internal module>");
+ assertContainsEvent("_internal in exports.bzl :: <_internal module>");
}
// TODO(#11437): Reject overriding symbols that don't already exist. Reject overriding "native".
// Reject overriding non-rule-logic symbols such as package(), select(), environment_extension,
// varref(), etc.
- // TODO(#11437): Add tests that the _internal symbol is accessible nowhere but builtins bzls.
-
// TODO(#11437): Add tests of the _internal symbol's usage within builtins bzls.
// TODO(#11437): Add test cases for BUILD file injection, and WORKSPACE file non-injection, when
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
index 999365e..e2e395d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
@@ -405,7 +405,7 @@
}
@Test
- public void testErrorReadingBzlFileInlineIsTransient() throws Exception {
+ public void testErrorReadingBzlFileIsTransientWhenUsingASTInlining() throws Exception {
CustomInMemoryFs fs = (CustomInMemoryFs) fileSystem;
scratch.file("a/BUILD");
fs.badPathForRead = scratch.file("a/a1.bzl", "doesntmatter");
