From 9d179e145652cad28fea100f4a01007047e30dc1 Mon Sep 17 00:00:00 2001 From: laurentlb Date: Thu, 27 Sep 2018 08:15:42 -0700 Subject: [PATCH] Introduce --incompatible_no_transitive_loads With flag set, loaded symbols are not automatically re-exported. #5636 RELNOTES: None. PiperOrigin-RevId: 214776940 --- site/docs/skylark/backward-compatibility.md | 22 +++++++++++ .../lib/packages/SkylarkSemanticsOptions.java | 15 +++++++ .../build/lib/syntax/Environment.java | 39 ++++++++++++++++++- .../devtools/build/lib/syntax/Eval.java | 2 +- .../devtools/build/lib/syntax/LValue.java | 2 +- .../build/lib/syntax/SkylarkSemantics.java | 5 +++ .../SkylarkSemanticsConsistencyTest.java | 2 + .../lib/skylark/SkylarkIntegrationTest.java | 29 +++++++++++++- 8 files changed, 111 insertions(+), 5 deletions(-) diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md index 6e0760decd2383..9467a094e59e28 100644 --- a/site/docs/skylark/backward-compatibility.md +++ b/site/docs/skylark/backward-compatibility.md @@ -443,6 +443,28 @@ The proposal is not fully implemented yet. * Flag: `--incompatible_static_name_resolution` * Default: `false` +### Disallow transitive loads + +When the flag is set, `load` can only import symbols that were explicitly +defined in the target file, using either `=` or `def`. + +When the flag is unset (legacy behavior), `load` may also import symbols that +come from other `load` statements. + +In other words, the `x` below is exported only if the flag is unset: + +```python +load(":file.bzl", "x") + +y = 1 +``` + +* Flag: `--incompatible_no_transitive_loads` +* Default: `false` +* Introduced in: `0.19.0` +* Tracking issue: https://github.com/bazelbuild/bazel/issues/5636 + + ### Disable InMemory Tools Defaults Package If false, Bazel constructs an in-memory `//tools/defaults` package based on the diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java index 4236b1208098e7..07ec878ebb3cd7 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java @@ -318,6 +318,20 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable + "passed to the action, it is an error for any tools to appear in the `inputs`.") public boolean incompatibleNoSupportToolsInActionInputs; + @Option( + name = "incompatible_no_transitive_loads", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If set to true, only symbols explicitly defined in the file can be loaded; " + + "symbols introduced by load are not implicitly re-exported.") + public boolean incompatibleNoTransitiveLoads; + @Option( name = "incompatible_package_name_is_a_function", defaultValue = "false", @@ -451,6 +465,7 @@ public SkylarkSemantics toSkylarkSemantics() { .incompatibleGenerateJavaCommonSourceJar(incompatibleGenerateJavaCommonSourceJar) .incompatibleNewActionsApi(incompatibleNewActionsApi) .incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs) + .incompatibleNoTransitiveLoads(incompatibleNoTransitiveLoads) .incompatiblePackageNameIsAFunction(incompatiblePackageNameIsAFunction) .incompatibleRangeType(incompatibleRangeType) .incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository) diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java index 3a4f0c9c958305..8e01e898c69209 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -271,12 +272,16 @@ public static final class GlobalFrame implements Frame { /** Bindings are maintained in order of creation. */ private final LinkedHashMap bindings; + /** Set of bindings that are exported (can be loaded from other modules). */ + private final HashSet exportedBindings; + /** Constructs an uninitialized instance; caller must call {@link #initialize} before use. */ public GlobalFrame() { this.mutability = null; this.universe = null; this.label = null; this.bindings = new LinkedHashMap<>(); + this.exportedBindings = new HashSet<>(); } public GlobalFrame( @@ -298,6 +303,7 @@ public GlobalFrame( if (bindings != null) { this.bindings.putAll(bindings); } + this.exportedBindings = new HashSet<>(); } public GlobalFrame(Mutability mutability) { @@ -399,6 +405,21 @@ public Map getBindings() { return Collections.unmodifiableMap(bindings); } + /** + * Returns a map of bindings that are exported (i.e. symbols declared using `=` and + * `def`, but not `load`). + */ + public Map getExportedBindings() { + checkInitialized(); + ImmutableMap.Builder result = new ImmutableMap.Builder<>(); + for (Map.Entry entry : bindings.entrySet()) { + if (exportedBindings.contains(entry.getKey())) { + result.put(entry); + } + } + return result.build(); + } + @Override public Map getTransitiveBindings() { checkInitialized(); @@ -523,7 +544,14 @@ public Extension(ImmutableMap bindings, String transitiveContent * and that {@code Environment}'s transitive hash code. */ public Extension(Environment env) { - this(ImmutableMap.copyOf(env.globalFrame.bindings), env.getTransitiveContentHashCode()); + // Legacy behavior: all symbols from the global Frame are exported (including symbols + // introduced by load). + this( + ImmutableMap.copyOf( + env.getSemantics().incompatibleNoTransitiveLoads() + ? env.globalFrame.getExportedBindings() + : env.globalFrame.getBindings()), + env.getTransitiveContentHashCode()); } public String getTransitiveContentHashCode() { @@ -981,6 +1009,15 @@ void removeLocalBinding(String varname) { } } + /** Modifies a binding in the current Frame. If it is the module Frame, also export it. */ + public Environment updateAndExport(String varname, Object value) throws EvalException { + update(varname, value); + if (isGlobal()) { + globalFrame.exportedBindings.add(varname); + } + return this; + } + /** * Modifies a binding in the current Frame of this Environment, as would an {@link * AssignmentStatement}. Does not try to modify an inherited binding. This will shadow any diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java index a91ffae4836520..d7e30dd4ad20a8 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java @@ -121,7 +121,7 @@ void execDef(FunctionDefStatement node) throws EvalException, InterruptedExcepti throw new EvalException(node.getLocation(), "Keyword-only argument is forbidden."); } - env.update( + env.updateAndExport( node.getIdentifier().getName(), new UserDefinedFunction( node.getIdentifier().getName(), diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java index 223e192eedf6e6..c37d0328d1e7f3 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java @@ -89,7 +89,7 @@ private static void assign(Expression expr, Object value, Environment env, Locat /** Binds a variable to the given value in the environment. */ private static void assignIdentifier(Identifier ident, Object value, Environment env) throws EvalException { - env.update(ident.getName(), value); + env.updateAndExport(ident.getName(), value); } /** diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java index 686c21eecb6ea1..2d606cc5413ed9 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java @@ -125,6 +125,8 @@ public boolean isFeatureEnabledBasedOnTogglingFlags( public abstract boolean incompatibleNoSupportToolsInActionInputs(); + public abstract boolean incompatibleNoTransitiveLoads(); + public abstract boolean incompatiblePackageNameIsAFunction(); public abstract boolean incompatibleRangeType(); @@ -175,6 +177,7 @@ public static Builder builderWithDefaults() { .incompatibleGenerateJavaCommonSourceJar(false) .incompatibleNewActionsApi(false) .incompatibleNoSupportToolsInActionInputs(false) + .incompatibleNoTransitiveLoads(false) .incompatiblePackageNameIsAFunction(false) .incompatibleRangeType(false) .incompatibleRemoveNativeGitRepository(false) @@ -228,6 +231,8 @@ public abstract static class Builder { public abstract Builder incompatibleNoSupportToolsInActionInputs(boolean value); + public abstract Builder incompatibleNoTransitiveLoads(boolean value); + public abstract Builder incompatiblePackageNameIsAFunction(boolean value); public abstract Builder incompatibleRangeType(boolean value); diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index b490e6e365f5e8..0d32a8917718ca 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -140,6 +140,7 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex "--incompatible_generate_javacommon_source_jar=" + rand.nextBoolean(), "--incompatible_new_actions_api=" + rand.nextBoolean(), "--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(), + "--incompatible_no_transitive_loads=" + rand.nextBoolean(), "--incompatible_package_name_is_a_function=" + rand.nextBoolean(), "--incompatible_range_type=" + rand.nextBoolean(), "--incompatible_remove_native_git_repository=" + rand.nextBoolean(), @@ -177,6 +178,7 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) { .incompatibleGenerateJavaCommonSourceJar(rand.nextBoolean()) .incompatibleNewActionsApi(rand.nextBoolean()) .incompatibleNoSupportToolsInActionInputs(rand.nextBoolean()) + .incompatibleNoTransitiveLoads(rand.nextBoolean()) .incompatiblePackageNameIsAFunction(rand.nextBoolean()) .incompatibleRangeType(rand.nextBoolean()) .incompatibleRemoveNativeGitRepository(rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 310066f2411e52..f72a68124d2761 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -1537,6 +1537,7 @@ public void testRecursiveImport2() throws Exception { @Test public void testSymbolPropagateThroughImports() throws Exception { + setSkylarkSemanticsOptions("--incompatible_no_transitive_loads=false"); scratch.file("test/skylark/implementation.bzl", "def custom_rule_impl(ctx):", " return None"); scratch.file( @@ -1547,15 +1548,39 @@ public void testSymbolPropagateThroughImports() throws Exception { "test/skylark/extension1.bzl", "load('//test/skylark:extension2.bzl', 'custom_rule_impl')", "", - "custom_rule = rule(implementation = custom_rule_impl,", - " attrs = {'dep': attr.label_list()})"); + "custom_rule = rule(implementation = custom_rule_impl)"); + + scratch.file( + "test/skylark/BUILD", + "load('//test/skylark:extension1.bzl', 'custom_rule')", + "custom_rule(name = 'cr')"); + + getConfiguredTarget("//test/skylark:cr"); + } + + @Test + public void testSymbolDoNotPropagateThroughImports() throws Exception { + setSkylarkSemanticsOptions("--incompatible_no_transitive_loads=true"); + scratch.file("test/skylark/implementation.bzl", "def custom_rule_impl(ctx):", " return None"); + + scratch.file( + "test/skylark/extension2.bzl", + "load('//test/skylark:implementation.bzl', 'custom_rule_impl')"); + + scratch.file( + "test/skylark/extension1.bzl", + "load('//test/skylark:extension2.bzl', 'custom_rule_impl')", + "", + "custom_rule = rule(implementation = custom_rule_impl)"); scratch.file( "test/skylark/BUILD", "load('//test/skylark:extension1.bzl', 'custom_rule')", "custom_rule(name = 'cr')"); + reporter.removeHandler(failFastHandler); getConfiguredTarget("//test/skylark:cr"); + assertContainsEvent("does not contain symbol 'custom_rule_impl'"); } @Test