Be explicit about semantics of Skylark environments

All callers that do not use Environment.Builder#setSemantics should call #useDefaultSemantics. A follow-up CL will enforce this requirement. Motivation: It's more important that we are strict about semantics than about the other builder args. It's too easy for a mistake in semantics to go unnoticed. RELNOTES: None PiperOrigin-RevId: 172912829
rabadin · Oct 23, 2017 · 10a6b77 · 10a6b77
1 parent bc616ec
commit 10a6b77
Show file tree

Hide file tree

Showing 10 changed files with 35 additions and 14 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkModules.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkModules.java
@@ -56,7 +56,10 @@ public static Environment.Frame getGlobals(List<Class<?>> modules) {
 
   private static Environment.Frame createGlobals(List<Class<?>> modules) {
     try (Mutability mutability = Mutability.create("SkylarkModules")) {
-      Environment env = Environment.builder(mutability).setGlobals(BazelLibrary.GLOBALS).build();
+      Environment env = Environment.builder(mutability)
+          .useDefaultSemantics()
+          .setGlobals(BazelLibrary.GLOBALS)
+          .build();
       for (Class<?> moduleClass : modules) {
         Runtime.registerModuleGlobals(env, moduleClass);
       }

diff --git a/...ava/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/...ava/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
@@ -58,6 +58,7 @@ public RepositoryDirectoryValue.Builder fetch(Rule rule, Path outputDirectory,
       // This Skylark environment ignores command line flags.
       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();

diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -182,20 +182,22 @@ public void execute(BuildFileAST ast, Map<String, Extension> importedExtensions)
   private void execute(BuildFileAST ast, @Nullable Map<String, Extension> importedExtensions,
       StoredEventHandler localReporter)
       throws InterruptedException {
-    // Note that this Skylark environment ignores command line flags.
-    Environment.Builder environmentBuilder =
-        Environment.builder(mutability)
-            .setGlobals(BazelLibrary.GLOBALS)
-            .setEventHandler(localReporter);
     if (importedExtensions != null) {
-      Map<String, Extension> map = new HashMap<String, Extension>(parentImportMap);
+      Map<String, Extension> map = new HashMap<>(parentImportMap);
       map.putAll(importedExtensions);
-      importMap = ImmutableMap.<String, Extension>copyOf(importedExtensions);
+      importMap = ImmutableMap.copyOf(importedExtensions);
     } else {
       importMap = parentImportMap;
     }
-    environmentBuilder.setImportedExtensions(importMap);
-    Environment workspaceEnv = environmentBuilder.setPhase(Phase.WORKSPACE).build();
+    Environment workspaceEnv =
+        Environment.builder(mutability)
+            // Note that this Skylark environment ignores command line flags.
+            .useDefaultSemantics()
+            .setGlobals(BazelLibrary.GLOBALS)
+            .setEventHandler(localReporter)
+            .setImportedExtensions(importMap)
+            .setPhase(Phase.WORKSPACE)
+            .build();
     addWorkspaceFunctions(workspaceEnv, localReporter);
     for (Map.Entry<String, Object> binding : parentVariableBindings.entrySet()) {
       try {

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java
@@ -258,7 +258,9 @@ private static Environment.Frame createGlobals() {
     List<BaseFunction> bazelGlobalFunctions = ImmutableList.of(select, depset, type);
 
     try (Mutability mutability = Mutability.create("BUILD")) {
-      Environment env = Environment.builder(mutability).build();
+      Environment env = Environment.builder(mutability)
+          .useDefaultSemantics()
+          .build();
       Runtime.setupConstants(env);
       Runtime.setupMethodEnvironment(env, MethodLibrary.defaultGlobalFunctions);
       Runtime.setupMethodEnvironment(env, bazelGlobalFunctions);

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
@@ -981,15 +981,19 @@ public String getTransitiveContentHashCode() {
 
   private static Environment.Frame createConstantsGlobals() {
     try (Mutability mutability = Mutability.create("CONSTANTS")) {
-      Environment env = Environment.builder(mutability).build();
+      Environment env = Environment.builder(mutability)
+          .useDefaultSemantics()
+          .build();
       Runtime.setupConstants(env);
       return env.getGlobals();
     }
   }
 
   private static Environment.Frame createDefaultGlobals() {
     try (Mutability mutability = Mutability.create("BUILD")) {
-      Environment env = Environment.builder(mutability).build();
+      Environment env = Environment.builder(mutability)
+          .useDefaultSemantics()
+          .build();
       Runtime.setupConstants(env);
       Runtime.setupMethodEnvironment(env, MethodLibrary.defaultGlobalFunctions);
       return env.getGlobals();

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
@@ -178,6 +178,7 @@ static Object getDefaultValue(Param param, Iterator<Object> iterator) {
         // Note that this Skylark environment ignores command line flags.
         Environment env =
             Environment.builder(mutability)
+                .useDefaultSemantics()
                 .setGlobals(Environment.CONSTANTS_ONLY)
                 .setEventHandler(Environment.FAIL_FAST_HANDLER)
                 .build()

diff --git a/src/main/java/com/google/devtools/skylark/Skylark.java b/src/main/java/com/google/devtools/skylark/Skylark.java
@@ -56,6 +56,7 @@ public void handle(Event event) {
   private final Mutability mutability = Mutability.create("interpreter");
   private final Environment env =
       Environment.builder(mutability)
+          .useDefaultSemantics()
           .setGlobals(Environment.DEFAULT_GLOBALS)
           .setEventHandler(PRINT_HANDLER)
           .build();

diff --git a/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java b/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java
@@ -61,6 +61,7 @@ public Environment newEnvironment() throws Exception {
                 .build();
         Environment env =
             Environment.builder(mutability)
+                .useDefaultSemantics()
                 .setEventHandler(getEventHandler())
                 .setGlobals(SkylarkModules.getGlobals(modules))
                 .setPhase(Phase.LOADING)

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
@@ -100,14 +100,17 @@ public void testGetVariableNames() throws Exception {
     try (Mutability mut = Mutability.create("outer")) {
       outerEnv =
           Environment.builder(mut)
+              .useDefaultSemantics()
               .setGlobals(Environment.DEFAULT_GLOBALS)
               .build()
               .update("foo", "bar")
               .update("wiz", 3);
     }
     try (Mutability mut = Mutability.create("inner")) {
       innerEnv = Environment.builder(mut)
-          .setGlobals(outerEnv.getGlobals()).build()
+          .useDefaultSemantics()
+          .setGlobals(outerEnv.getGlobals())
+          .build()
           .update("foo", "bat")
           .update("quux", 42);
     }
@@ -200,6 +203,7 @@ public void testFrozen() throws Exception {
     try (Mutability mutability = Mutability.create("testFrozen")) {
       env =
           Environment.builder(mutability)
+              .useDefaultSemantics()
               .setGlobals(Environment.DEFAULT_GLOBALS)
               .setEventHandler(Environment.FAIL_FAST_HANDLER)
               .build();

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -71,6 +71,7 @@ protected void beforeInitialization() throws Exception {
   public Environment newBuildEnvironment() {
     Environment env =
         Environment.builder(mutability)
+            .useDefaultSemantics()
             .setGlobals(BazelLibrary.GLOBALS)
             .setEventHandler(getEventHandler())
             .setPhase(Phase.LOADING)
@@ -85,6 +86,7 @@ public Environment newBuildEnvironment() {
    */
   public Environment newSkylarkEnvironment() {
     return Environment.builder(mutability)
+        .useDefaultSemantics()
         .setGlobals(BazelLibrary.GLOBALS)
         .setEventHandler(getEventHandler())
         .build();