From 4be755449fd21f820cc5327a0b28974d0a313ae9 Mon Sep 17 00:00:00 2001 From: brandjon Date: Sun, 30 Aug 2020 14:34:01 -0700 Subject: [PATCH] Restrict the bzl environment for builtins injection a bit more This is to help protect the authors of exports.bzl from shooting themselves in the foot. Main changes: - Add validation of the injected builtins to ensure we don't inadvertently create new symbols, or override important symbols like `rule()`. - Don't allow access to overridable symbols (e.g., `CcInfo`) from within @builtins bzls, since their meaning would be different than in regular bzl files. Also: - Spin off a new test suite for PackageFactory's management of the environment. - Add a test that BUILD and WORKSPACE bzl files have minimal differences in their environments. They should eventually converge (#11954). - Update comments/TODOs to reference #11954. - Eliminate unneeded method PackageFactory#getNativeRules. It was originally intended for when injection logic was going to live inside StarlarkBuiltinsFunction. - Add error handling and test for case where StarlarkBuiltinsFunction fails due to disallowed injections. Work toward #11437 and #11954. RELNOTES: None PiperOrigin-RevId: 329207541 --- .../build/lib/packages/PackageFactory.java | 80 +++++++--- .../lib/skyframe/ASTFileLookupFunction.java | 8 +- .../skyframe/StarlarkBuiltinsFunction.java | 19 +-- .../BazelStarlarkEnvironmentTest.java | 148 ++++++++++++++++++ .../lib/skyframe/BuiltinsInjectionTest.java | 16 +- .../StarlarkBuiltinsFunctionTest.java | 38 ++--- 6 files changed, 235 insertions(+), 74 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java 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 30be14f8588484..2c8dd67053362e 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 @@ -329,15 +329,6 @@ public ImmutableMap getBuiltinsBzlEnv() { return builtinsBzlEnv; } - /** - * Returns the subset of bindings of the "native" module (for BUILD-loaded .bzls) that are rules. - * - *

Excludes non-rule functions such as {@code glob()}. - */ - public ImmutableMap getNativeRules() { - return ruleFunctions; - } - /** Creates the map of arguments for the 'package' function. */ private ImmutableMap> createPackageArguments() { ImmutableList.Builder> arguments = @@ -726,18 +717,12 @@ private static ImmutableMap createWorkspaceBzlNativeBindings( private static ImmutableMap createUninjectedBuildBzlEnv( RuleClassProvider ruleClassProvider, ImmutableMap uninjectedBuildBzlNativeBindings) { - // TODO(#11437): Add tests to assert that build and workspace bzl predeclareds have the same - // symbol names (though values may differ). Then ASTFileLookupFunction can request the - // appropriate predeclared map from PackageFactory, without using injection -- but add a comment - // explaining that that's safe. - Map env = new HashMap<>(); env.putAll(ruleClassProvider.getEnvironment()); // TODO(#11437): We *should* be able to uncomment the following line, but the native module is - // added prematurely (without its rule-logic fields) and overridden unconditionally. Fix this - // once ASTFileLookupFunction takes in the set of predeclared bindings (currently it directly - // checks getEnvironment()). + // added prematurely (without its rule-logic fields) and overridden unconditionally. There's no + // reason for this now that ASTFileLookupFunction observes the correct predeclared env. // Preconditions.checkState(!predeclared.containsKey("native")); // Determine the "native" module. @@ -766,10 +751,11 @@ private static ImmutableMap createBuiltinsBzlEnv( Map env = new HashMap<>(); env.putAll(ruleClassProvider.getEnvironment()); - // TODO($11437): We shouldn't have to delete the (not fully formed) "native" object here; see + // Clear out rule-specific symbols like CcInfo. + env.keySet().removeAll(ruleClassProvider.getNativeRuleSpecificBindings().keySet()); + // 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 @@ -787,8 +773,25 @@ private static Object createNativeModule(Map bindings) { return StructProvider.STRUCT.create(bindings, "no native function or rule '%s'"); } + /** Indicates a problem performing builtins injection. */ + public static final class InjectionException extends Exception { + public InjectionException(String message) { + super(message); + } + } + + /** + * Constructs an environment for a BUILD-loaded bzl file based on the default environment as well + * as the given injected top-level symbols and "native" bindings. + * + *

Injected symbols must override an existing symbol of that name. Furthermore, the overridden + * symbol must be a rule or a piece of a specific ruleset's logic (e.g., {@code CcInfo} or {@code + * cc_library}), not a generic built-in (e.g., {@code provider} or {@code glob}). Throws + * InjectionException if these conditions are not met. + */ public ImmutableMap createBuildBzlEnvUsingInjection( - ImmutableMap injectedToplevels, ImmutableMap injectedRules) { + ImmutableMap injectedToplevels, ImmutableMap injectedRules) + throws InjectionException { // TODO(#11437): Builtins injection should take into account StarlarkSemantics and // FlagGuardedValues. If a builtin is disabled by a flag, we can either: // @@ -799,18 +802,43 @@ public ImmutableMap createBuildBzlEnvUsingInjection( // 2) Allow it to be exported and automatically suppress/omit it from the final environment, // effectively rewrapping the injected builtin in the FlagGuardedValue. + // Determine top-level symbols. Map env = new HashMap<>(); - env.putAll(ruleClassProvider.getEnvironment()); - // TODO(#11437): Validate that all the injected symbols are overriding existing symbols. Throw - // an exception otherwise. - env.putAll(injectedToplevels); + env.putAll(uninjectedBuildBzlEnv); + for (Map.Entry symbol : injectedToplevels.entrySet()) { + String name = symbol.getKey(); + if (!env.containsKey(name) && !Starlark.UNIVERSE.containsKey(name)) { + throw new InjectionException( + String.format( + "Injected top-level symbol '%s' must override an existing symbol by that name", + name)); + } else if (!ruleClassProvider.getNativeRuleSpecificBindings().containsKey(name)) { + throw new InjectionException( + String.format("Cannot override top-level builtin '%s' with an injected value", name)); + } else { + env.put(name, symbol.getValue()); + } + } + // Determine "native" bindings. // See above comments for native in BUILD bzls. Map nativeBindings = new HashMap<>(); nativeBindings.putAll(uninjectedBuildBzlNativeBindings); - nativeBindings.putAll(injectedRules); - env.put("native", createNativeModule(nativeBindings)); + for (Map.Entry symbol : injectedRules.entrySet()) { + String name = symbol.getKey(); + Object preexisting = nativeBindings.put(name, symbol.getValue()); + if (preexisting == null) { + throw new InjectionException( + String.format( + "Injected native module field '%s' must override an existing symbol by that name", + name)); + } else if (!ruleFunctions.containsKey(name)) { + throw new InjectionException( + String.format("Cannot override native module field '%s' with an injected value", name)); + } + } + env.put("native", createNativeModule(nativeBindings)); return ImmutableMap.copyOf(env); } 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 101c0c5f3a446c..17f9ea6f2fda67 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 @@ -145,10 +145,14 @@ static ASTFileLookupValue computeInline( 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 + // the right env for the actual evaluation of BUILD-loaded bzl files because it doesn't + // map to the injected symbols. 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()). + // + // For WORKSPACE-loaded bzl files, the env isn't quite right not because of injection but + // because the "native" object is different. But A) that will be fixed with #11954, and B) we + // don't care for the same reason as above. predeclared = packageFactory.getUninjectedBuildBzlEnv(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java index 41f4bf9f321b3d..af12f3552ec9a2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.PackageFactory; +import com.google.devtools.build.lib.packages.PackageFactory.InjectionException; import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException; import com.google.devtools.build.lib.syntax.Dict; import com.google.devtools.build.lib.syntax.EvalException; @@ -47,16 +48,10 @@ // to migrate by adding a load()). Combine tombstones with reading the current incompatible flags // within @builtins for awesomeness. -// TODO(#11437): Currently, BUILD-loaded .bzls and WORKSPACE-loaded .bzls have the same initial -// static environment. Therefore, we should apply injection of top-level symbols to both -// environments, not just BUILD .bzls. Likewise for builtins that are shared by BUILD and WORKSPACE -// files, and for when the dynamic `native` values of the two .bzl dialects are unified. This can be -// facilitated by 1) making PackageFactory better understand the similarities between BUILD and -// WORKSPACE, e.g. by refactoring things like WorkspaceFactory#createWorkspaceFunctions into -// PackageFactory; and 2) making PackageFactory responsible for performing the actual injection -// (given the override mappings from exports.bzl) and returning the modified environments. Then any -// refactoring to unify BUILD with WORKPSACE and BUILD-bzl with WORKSPACE-bzl can proceed in -// PackageFactory without regard to this file. +// TODO(#11437, #11954, #11983): To the extent that BUILD-loaded .bzls and WORKSPACE-loaded .bzls +// have the same environment, builtins injection should apply to both of them, not just to +// BUILD-loaded .bzls. The same can be said for BUILD and WORKSPACE files themselves if we end up +// unifying those environments as well. /** * A Skyframe function that evaluates the {@code @builtins} pseudo-repository and reports the values @@ -182,7 +177,7 @@ private static StarlarkBuiltinsValue computeInternal( ImmutableMap predeclared = packageFactory.createBuildBzlEnvUsingInjection(exportedToplevels, exportedRules); return new StarlarkBuiltinsValue(predeclared, exportedToJava, transitiveDigest); - } catch (EvalException ex) { + } catch (EvalException | InjectionException ex) { throw BuiltinsFailedException.errorApplyingExports(ex); } } @@ -240,7 +235,7 @@ static BuiltinsFailedException errorEvaluatingBuiltinsBzls( transience); } - static BuiltinsFailedException errorApplyingExports(EvalException cause) { + static BuiltinsFailedException errorApplyingExports(Exception cause) { return new BuiltinsFailedException( String.format("Failed to apply declared builtins: %s", cause.getMessage()), cause, diff --git a/src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java new file mode 100644 index 00000000000000..db3814db0b5cf0 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java @@ -0,0 +1,148 @@ +// 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 static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.analysis.util.MockRule; +import com.google.devtools.build.lib.packages.PackageFactory.InjectionException; +import com.google.devtools.build.lib.syntax.ClassObject; +import com.google.devtools.build.lib.testutil.TestRuleClassProvider; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for PackageFactory's management of the predeclared Starlark symbols. */ +@RunWith(JUnit4.class) +public final class BazelStarlarkEnvironmentTest extends BuildViewTestCase { + + private static final MockRule OVERRIDABLE_RULE = () -> MockRule.define("overridable_rule"); + + @Override + protected ConfiguredRuleClassProvider createRuleClassProvider() { + // Add a fake rule and top-level symbol to override. + ConfiguredRuleClassProvider.Builder builder = + new ConfiguredRuleClassProvider.Builder() + // While reading, feel free to mentally substitute overridable_rule -> cc_library and + // overridable_symbol -> CcInfo. + .addRuleDefinition(OVERRIDABLE_RULE) + .addStarlarkAccessibleTopLevels("overridable_symbol", "original_value"); + TestRuleClassProvider.addStandardRules(builder); + return builder.build(); + } + + // TODO(#11954): We want BUILD- and WORKSPACE-loaded bzl files to have the exact same environment. + // In the meantime these two tests help avoid regressions. + + // This property is important for ASTFileLookupFunction, which relies on the symbol names in the + // env matching even if the symbols themselves differ. + @Test + public void buildAndWorkspaceBzlEnvsDeclareSameNames() throws Exception { + Set buildBzlNames = pkgFactory.getUninjectedBuildBzlEnv().keySet(); + Set workspaceBzlNames = pkgFactory.getWorkspaceBzlEnv().keySet(); + assertThat(buildBzlNames).isEqualTo(workspaceBzlNames); + } + + @Test + public void buildAndWorkspaceBzlEnvsAreSameExceptForNative() throws Exception { + Map buildBzlEnv = new HashMap<>(); + buildBzlEnv.putAll(pkgFactory.getUninjectedBuildBzlEnv()); + buildBzlEnv.remove("native"); + Map workspaceBzlEnv = new HashMap<>(); + workspaceBzlEnv.putAll(pkgFactory.getWorkspaceBzlEnv()); + workspaceBzlEnv.remove("native"); + assertThat(buildBzlEnv).isEqualTo(workspaceBzlEnv); + } + + @Test + public void injection() throws Exception { + Map env = + pkgFactory.createBuildBzlEnvUsingInjection( + ImmutableMap.of("overridable_symbol", "new_value"), + ImmutableMap.of("overridable_rule", "new_rule")); + assertThat(env).containsEntry("overridable_symbol", "new_value"); + assertThat(((ClassObject) env.get("native")).getValue("overridable_rule")) + .isEqualTo("new_rule"); + } + + /** Asserts that injection with the given maps fails with the given error substring. */ + private void assertInjectionFailure( + ImmutableMap injectedToplevels, + ImmutableMap injectedRules, + String message) { + InjectionException ex = + assertThrows( + InjectionException.class, + () -> pkgFactory.createBuildBzlEnvUsingInjection(injectedToplevels, injectedRules)); + assertThat(ex).hasMessageThat().contains(message); + } + + @Test + public void injectedNameMustOverrideExistingName_toplevelSymbol() throws Exception { + assertInjectionFailure( + ImmutableMap.of("brand_new_toplevel", "foo"), + ImmutableMap.of(), + "Injected top-level symbol 'brand_new_toplevel' must override an existing symbol by" + + " that name"); + } + + @Test + public void injectedNameMustOverrideExistingName_nativeField() throws Exception { + assertInjectionFailure( + ImmutableMap.of(), + ImmutableMap.of("brand_new_field", "foo"), + "Injected native module field 'brand_new_field' must override an existing symbol by " + + "that name"); + } + + @Test + public void cannotInjectGenericNonRuleSpecificSymbol_toplevelSymbol() { + assertInjectionFailure( + ImmutableMap.of("provider", "new_builtin"), + ImmutableMap.of(), + "Cannot override top-level builtin 'provider' with an injected value"); + } + + @Test + public void cannotInjectGenericNonRuleSpecificSymbol_nativeField() { + assertInjectionFailure( + ImmutableMap.of(), + ImmutableMap.of("glob", "new_builtin"), + "Cannot override native module field 'glob' with an injected value"); + } + + @Test + public void cannotInjectGenericNonRuleSpecificSymbol_nativeModuleItself() { + assertInjectionFailure( + ImmutableMap.of("native", "new_builtin"), + ImmutableMap.of(), + "Cannot override top-level builtin 'native' with an injected value"); + } + + @Test + public void cannotInjectGenericNonRuleSpecificSymbol_universeSymbol() { + assertInjectionFailure( + ImmutableMap.of("len", "new_builtin"), + ImmutableMap.of(), + "Cannot override top-level builtin 'len' with an injected value"); + } +} 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 a3516957905877..3cda7c50ef6d43 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 @@ -246,11 +246,8 @@ public void builtinsBzlCannotAccessNative() throws Exception { 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 { + public void builtinsBzlCannotAccessRuleSpecificSymbol() throws Exception { // TODO(#11437): Use @builtins//:... syntax for load, once supported. scratch.file( "tools/builtins_staging/helper.bzl", // @@ -266,13 +263,10 @@ public void builtinsBzlCanAccessRuleLogicSymbols() throws Exception { 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"); + // 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"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java index 50289b9a2a8f59..063276d5b7b24f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java @@ -48,29 +48,6 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() { return builder.build(); } - // The getNativeRuleLogicBindings_* cases properly belong in a test suite for PackageFactory, but - // that seems to require a lot of overhead dealing with PackageFactoryApparatus, etc. - // - // TODO(#11437): The methods getNativeRules() and getNativeRuleSpecificBindings() don't belong in - // PackageFactory's API now that builtins injection has been moved to be encapsulated by that - // class. Instead, test the injection itself -- verify that certain symbols can or cannot be - // injected. - - @Test - public void getNativeRuleLogicBindings_inPackageFactory() throws Exception { - assertThat(getPackageFactory().getNativeRules()).containsKey("cc_library"); - assertThat(getPackageFactory().getNativeRules()).doesNotContainKey("glob"); - assertThat(getPackageFactory().getNativeRules()).containsKey("overridable_rule"); - } - - @Test - public void getNativeRuleLogicBindings_inRuleClassProvider() throws Exception { - assertThat(getRuleClassProvider().getNativeRuleSpecificBindings()).containsKey("CcInfo"); - assertThat(getRuleClassProvider().getNativeRuleSpecificBindings()).doesNotContainKey("rule"); - assertThat(getRuleClassProvider().getNativeRuleSpecificBindings()) - .containsKey("overridable_symbol"); - } - // TODO(#11437): Add tests for predeclared env of BUILD (and WORKSPACE?) files, once // StarlarkBuiltinsFunction manages that functionality. @@ -191,6 +168,21 @@ public void evalExportsFails_badDictKey() throws Exception { + "want dict"); } + @Test + public void evalExportsFails_overrideNotAllowed() throws Exception { + Exception ex = + evalBuiltinsToException( + "exported_toplevels = {}", // + "exported_rules = {'glob': 'new_builtin'}", + "exported_to_java = {}"); + assertThat(ex).isInstanceOf(BuiltinsFailedException.class); + assertThat(ex) + .hasMessageThat() + .contains( + "Failed to apply declared builtins: Cannot override native module field 'glob' with an" + + " injected value"); + } + @Test public void evalExportsFails_parseError() throws Exception { reporter.removeHandler(failFastHandler);