Rollback of commit 5a94e59f02833f9142bad9203acd72626b089535.

*** Reason for rollback ***

Breaks serialization of SkyValues.

--
MOS_MIGRATED_REVID=102457225

1 files changed, 22 insertions, 28 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index b3eb957019..0bd16ae90a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -27,7 +27,7 @@ import com.google.devtools.build.lib.skyframe.ASTFileLookupValue.ASTLookupInputE
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.Label.SyntaxException;
-import com.google.devtools.build.lib.syntax.Mutability;
+import com.google.devtools.build.lib.syntax.SkylarkEnvironment;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -76,7 +76,7 @@ public class SkylarkImportLookupFunction implements SkyFunction {
       throw new SkylarkImportLookupFunctionException(SkylarkImportFailedException.noFile(file));
     }
 
-    Map<PathFragment, com.google.devtools.build.lib.syntax.Environment> importMap = new HashMap<>();
+    Map<PathFragment, SkylarkEnvironment> importMap = new HashMap<>();
     ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
     BuildFileAST ast = astLookupValue.getAST();
     // TODO(bazel-team): Refactor this code and PackageFunction to reduce code duplications.
@@ -114,11 +114,10 @@ public class SkylarkImportLookupFunction implements SkyFunction {
           file));
     }
 
-    // Skylark UserDefinedFunction-s in that file will share this function definition Environment,
-    // which will be frozen by the time it is returned by createEnv.
-    com.google.devtools.build.lib.syntax.Environment extensionEnv =
-        createEnv(ast, file, importMap, env);
-
+    SkylarkEnvironment extensionEnv = createEnv(ast, file, importMap, env);
+    // Skylark UserDefinedFunctions are sharing function definition Environments, so it's extremely
+    // important not to modify them from this point. Ideally they should be only used to import
+    // symbols and serve as global Environments of UserDefinedFunctions.
     return new SkylarkImportLookupValue(
         extensionEnv, new SkylarkFileDependency(label, fileDependencies.build()));
   }
@@ -165,34 +164,29 @@ public class SkylarkImportLookupFunction implements SkyFunction {
   }
 
   /**
-   * Creates the Environment to be imported.
-   * After it's returned, the Environment must not be modified.
+   * Creates the SkylarkEnvironment to be imported. After it's returned, the Environment
+   * must not be modified.
    */
-  private com.google.devtools.build.lib.syntax.Environment createEnv(
-      BuildFileAST ast,
-      PathFragment file,
-      Map<PathFragment, com.google.devtools.build.lib.syntax.Environment> importMap,
-      Environment env)
+  private SkylarkEnvironment createEnv(BuildFileAST ast, PathFragment file,
+      Map<PathFragment, SkylarkEnvironment> importMap, Environment env)
           throws InterruptedException, SkylarkImportLookupFunctionException {
     StoredEventHandler eventHandler = new StoredEventHandler();
     // TODO(bazel-team): this method overestimates the changes which can affect the
     // Skylark RuleClass. For example changes to comments or unused functions can modify the hash.
     // A more accurate - however much more complicated - way would be to calculate a hash based on
     // the transitive closure of the accessible AST nodes.
-    try (Mutability mutability = Mutability.create("importing %s", file)) {
-      com.google.devtools.build.lib.syntax.Environment extensionEnv =
-          ruleClassProvider.createSkylarkRuleClassEnvironment(
-              mutability, eventHandler, ast.getContentHashCode(), importMap)
-          .setupOverride("native", packageFactory.getNativeModule());
-      ast.exec(extensionEnv, eventHandler);
-      SkylarkRuleClassFunctions.exportRuleFunctions(extensionEnv, file);
-
-      Event.replayEventsOn(env.getListener(), eventHandler.getEvents());
-      if (eventHandler.hasErrors()) {
-        throw new SkylarkImportLookupFunctionException(SkylarkImportFailedException.errors(file));
-      }
-      return extensionEnv;
-    }
+    SkylarkEnvironment extensionEnv = ruleClassProvider
+        .createSkylarkRuleClassEnvironment(eventHandler, ast.getContentHashCode());
+    extensionEnv.update("native", packageFactory.getNativeModule());
+    extensionEnv.setImportedExtensions(importMap);
+    ast.exec(extensionEnv, eventHandler);
+    SkylarkRuleClassFunctions.exportRuleFunctions(extensionEnv, file);
+
+    Event.replayEventsOn(env.getListener(), eventHandler.getEvents());
+    if (eventHandler.hasErrors()) {
+      throw new SkylarkImportLookupFunctionException(SkylarkImportFailedException.errors(file));
+    }
+    return extensionEnv;
   }
 
   @Override