Don't set globals in environments that aren't read

There are two Environments involved in running an implementation callback for rules/aspects/repository-rules: (1) The static environment of the function's definition, and (2) the caller's dynamic environment. The function will only consult (1), so trying to set anything on (2) is misleading.

RELNOTES: None
PiperOrigin-RevId: 172929301

4 files changed, 9 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index d993620ff8..a04501889e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -84,20 +84,14 @@ public final class SkylarkRuleConfiguredTargetUtil {
       Environment env =
           Environment.builder(mutability)
               .setCallerLabel(ruleContext.getLabel())
-              .setGlobals(
-                  ruleContext
-                      .getRule()
-                      .getRuleClassObject()
-                      .getRuleDefinitionEnvironment()
-                      .getGlobals())
               .setSemantics(skylarkSemantics)
               .setEventHandler(ruleContext.getAnalysisEnvironment().getEventHandler())
               .build(); // NB: loading phase functions are not available: this is analysis already,
       // so we do *not* setLoadingPhase().
       Object target =
           ruleImplementation.call(
-              ImmutableList.<Object>of(skylarkRuleContext),
-              ImmutableMap.<String, Object>of(),
+              /*args=*/ ImmutableList.of(skylarkRuleContext),
+              /*kwargs*/ ImmutableMap.of(),
               /*ast=*/ null,
               env);
 
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
index 18ac7b4060..6f708863fd 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
@@ -59,7 +59,6 @@ public class SkylarkRepositoryFunction extends RepositoryFunction {
       com.google.devtools.build.lib.syntax.Environment buildEnv =
           com.google.devtools.build.lib.syntax.Environment.builder(mutability)
               .useDefaultSemantics()
-              .setGlobals(rule.getRuleClassObject().getRuleDefinitionEnvironment().getGlobals())
               .setEventHandler(env.getListener())
               .build();
       SkylarkRepositoryContext skylarkRepositoryContext =
@@ -72,8 +71,8 @@ public class SkylarkRepositoryFunction extends RepositoryFunction {
       // structure as it is.
       Object retValue =
           function.call(
-              ImmutableList.<Object>of(skylarkRepositoryContext),
-              ImmutableMap.<String, Object>of(),
+              /*args=*/ ImmutableList.of(skylarkRepositoryContext),
+              /*kwargs=*/ ImmutableMap.of(),
               null,
               buildEnv);
       if (retValue != Runtime.NONE) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index 39238de5f2..8aeac24dac 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -66,19 +66,19 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory {
       }
       Environment env =
           Environment.builder(mutability)
-              .setGlobals(skylarkAspect.getFuncallEnv().getGlobals())
               .setSemantics(analysisEnv.getSkylarkSemantics())
               .setEventHandler(analysisEnv.getEventHandler())
-              .build(); // NB: loading phase functions are not available: this is analysis already,
-      // so we do *not* setLoadingPhase().
+              // NB: loading phase functions are not available: this is analysis already, so we do
+              // *not* setLoadingPhase().
+              .build();
       Object aspectSkylarkObject;
       try {
         aspectSkylarkObject =
             skylarkAspect
                 .getImplementation()
                 .call(
-                    ImmutableList.<Object>of(base, skylarkRuleContext),
-                    ImmutableMap.<String, Object>of(),
+                    /*args=*/ ImmutableList.of(base, skylarkRuleContext),
+                    /*kwarg=*/ ImmutableMap.of(),
                     /*ast=*/ null,
                     env);
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java
index 1786b9bc28..ed79584a05 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java
@@ -42,7 +42,6 @@ public class SkylarkCallbackFunction {
       Environment env = Environment.builder(mutability)
           .setSemantics(funcallEnv.getSemantics())
           .setEventHandler(funcallEnv.getEventHandler())
-          .setGlobals(funcallEnv.getGlobals())
           .build();
       return callback.call(buildArgumentList(ctx, arguments), null, ast, env);
     } catch (ClassCastException | IllegalArgumentException e) {