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

@@ -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

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)

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<String, Object> bindings;
 
+    /** Set of bindings that are exported (can be loaded from other modules). */
+    private final HashSet<String> 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<String, Object> 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<String, Object> getExportedBindings() {
+      checkInitialized();
+      ImmutableMap.Builder<String, Object> result = new ImmutableMap.Builder<>();
+      for (Map.Entry<String, Object> entry : bindings.entrySet()) {
+        if (exportedBindings.contains(entry.getKey())) {
+          result.put(entry);
+        }
+      }
+      return result.build();
+    }
+
     @Override
     public Map<String, Object> getTransitiveBindings() {
       checkInitialized();
@@ -523,7 +544,14 @@ public Extension(ImmutableMap<String, Object> 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

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(),

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);
   }
 
   /**

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);

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())

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