Refactor Skylark Environment-s

Make Environment-s freezable: Introduce a class Mutability
as a revokable capability to mutate objects in an Environment.
For now, only Environment-s carry this capability.
Make sure that every Mutability is revoked in the same function that create...

This reinstates a change that previously rolled-back because it broke the
serializability of SkylarkLookupValue. Bad news: serializing it succeeds for the
wrong reason, because a SkylarkEnvironment was stored as a result (now an
Environment.Extension) that was Serializable but inherited its bindings from an Environment (now an Environment.BaseExtension) which wasn't Serializable.
Apparently, Java doesn't try to serialize the bindings then (or at least doesn't
error out when it fails), because these bindings map variable names to pretty
arbitrary objects, and a lot of those we find in practice aren't Serializable.
Thus the current code passes the same tests as the previous code, but obviously
the serialization is just as ineffective as it used to be.

--
MOS_MIGRATED_REVID=102776694

12 files changed, 250 insertions, 167 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java b/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java
index aa8d8f83a6..c7f85d6fcb 100644
--- a/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java
@@ -18,7 +18,7 @@ import com.google.devtools.build.lib.events.EventCollector;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.PrintingEventHandler;
 import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.syntax.EvaluationContext;
+import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.util.io.OutErr;
 
@@ -67,9 +67,9 @@ public class EventCollectionApparatus {
    */
   public void setFailFast(boolean failFast) {
     if (failFast) {
-      reporter.addHandler(EvaluationContext.FAIL_FAST_HANDLER);
+      reporter.addHandler(Environment.FAIL_FAST_HANDLER);
     } else {
-      reporter.removeHandler(EvaluationContext.FAIL_FAST_HANDLER);
+      reporter.removeHandler(Environment.FAIL_FAST_HANDLER);
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
index 24ac455762..6306e468fb 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
@@ -30,9 +30,9 @@ import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.PackageFactory.LegacyGlobber;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
+import com.google.devtools.build.lib.syntax.Environment.Extension;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
-import com.google.devtools.build.lib.syntax.SkylarkEnvironment;
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.testutil.TestUtils;
@@ -134,7 +134,7 @@ public class PackageFactoryApparatus {
     LegacyBuilder resultBuilder = factory.evaluateBuildFile(
         externalPkg, packageId, buildFileAST, buildFile,
         globber, ImmutableList.<Event>of(), ConstantRuleVisibility.PUBLIC, false, false,
-        new MakeEnvironment.Builder(), ImmutableMap.<PathFragment, SkylarkEnvironment>of(),
+        new MakeEnvironment.Builder(), ImmutableMap.<PathFragment, Extension>of(),
         ImmutableList.<Label>of());
     Package result = resultBuilder.build();
     Event.replayEventsOn(events.reporter(), result.getEvents());
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
index eff5badfb6..00e9062d63 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
@@ -22,11 +22,7 @@ import static org.junit.Assert.assertTrue;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventCollector;
-import com.google.devtools.build.lib.events.EventKind;
-import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
 import com.google.devtools.build.lib.packages.CachingPackageLocator;
-import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.vfs.Path;
 
@@ -37,13 +33,14 @@ import org.junit.runners.JUnit4;
 import java.io.IOException;
 import java.util.Arrays;
 
+/**
+ * Unit tests for BuildFileAST.
+ */
 @RunWith(JUnit4.class)
-public class BuildFileASTTest {
+public class BuildFileASTTest extends EvaluationTestCase {
 
   private Scratch scratch = new Scratch();
 
-  private EventCollectionApparatus events = new EventCollectionApparatus(EventKind.ALL_EVENTS);
-
   private class ScratchPathPackageLocator implements CachingPackageLocator {
     @Override
     public Path getBuildFileForPackage(PackageIdentifier packageName) {
@@ -53,13 +50,18 @@ public class BuildFileASTTest {
 
   private CachingPackageLocator locator = new ScratchPathPackageLocator();
 
+  @Override
+  public Environment newEnvironment() throws Exception {
+    return newBuildEnvironment();
+  }
+
   /**
    * Parses the contents of the specified string (using DUMMY_PATH as the fake
    * filename) and returns the AST. Resets the error handler beforehand.
    */
   private BuildFileAST parseBuildFile(String... lines) throws IOException {
     Path file = scratch.file("/a/build/file/BUILD", lines);
-    return BuildFileAST.parseBuildFile(file, events.reporter(), locator, false);
+    return BuildFileAST.parseBuildFile(file, getEventHandler(), locator, false);
   }
 
   @Test
@@ -69,11 +71,9 @@ public class BuildFileASTTest {
         "",
         "x = [1,2,'foo',4] + [1,2, \"%s%d\" % ('foo', 1)]");
 
-    Environment env = new Environment();
-    Reporter reporter = new Reporter();
-    BuildFileAST buildfile = BuildFileAST.parseBuildFile(buildFile, reporter, null, false);
+    BuildFileAST buildfile = BuildFileAST.parseBuildFile(buildFile, getEventHandler(), null, false);
 
-    assertTrue(buildfile.exec(env, reporter));
+    assertTrue(buildfile.exec(env, getEventHandler()));
 
     // Test final environment is correctly modified:
     //
@@ -91,15 +91,11 @@ public class BuildFileASTTest {
         "",
         "z = x + y");
 
-    Environment env = new Environment();
-    Reporter reporter = new Reporter();
-    EventCollector collector = new EventCollector(EventKind.ALL_EVENTS);
-    reporter.addHandler(collector);
-    BuildFileAST buildfile = BuildFileAST.parseBuildFile(buildFile, reporter, null, false);
+    setFailFast(false);
+    BuildFileAST buildfile = BuildFileAST.parseBuildFile(buildFile, getEventHandler(), null, false);
 
-    assertFalse(buildfile.exec(env, reporter));
-    Event e = MoreAsserts.assertContainsEvent(collector,
-        "unsupported operand type(s) for +: 'int' and 'List'");
+    assertFalse(buildfile.exec(env, getEventHandler()));
+    Event e = assertContainsEvent("unsupported operand type(s) for +: 'int' and 'List'");
     assertEquals(4, e.getLocation().getStartLineAndColumn().getLine());
   }
 
@@ -114,13 +110,12 @@ public class BuildFileASTTest {
 
   @Test
   public void testFailsIfNewlinesAreMissing() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
 
     BuildFileAST buildFileAST =
       parseBuildFile("foo() bar() something = baz() bar()");
 
-    Event event = events.collector().iterator().next();
-    assertEquals("syntax error at \'bar\': expected newline", event.getMessage());
+    Event event = assertContainsEvent("syntax error at \'bar\': expected newline");
     assertEquals("/a/build/file/BUILD",
                  event.getLocation().getPath().toString());
     assertEquals(1, event.getLocation().getStartLineAndColumn().getLine());
@@ -129,11 +124,10 @@ public class BuildFileASTTest {
 
   @Test
   public void testImplicitStringConcatenationFails() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     BuildFileAST buildFileAST = parseBuildFile("a = 'foo' 'bar'");
-    Event event = events.collector().iterator().next();
-    assertEquals("Implicit string concatenation is forbidden, use the + operator",
-        event.getMessage());
+    Event event = assertContainsEvent(
+        "Implicit string concatenation is forbidden, use the + operator");
     assertEquals("/a/build/file/BUILD",
                  event.getLocation().getPath().toString());
     assertEquals(1, event.getLocation().getStartLineAndColumn().getLine());
@@ -143,11 +137,10 @@ public class BuildFileASTTest {
 
   @Test
   public void testImplicitStringConcatenationAcrossLinesIsIllegal() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     BuildFileAST buildFileAST = parseBuildFile("a = 'foo'\n  'bar'");
 
-    Event event = events.collector().iterator().next();
-    assertEquals("indentation error", event.getMessage());
+    Event event = assertContainsEvent("indentation error");
     assertEquals("/a/build/file/BUILD",
                  event.getLocation().getPath().toString());
     assertEquals(2, event.getLocation().getStartLineAndColumn().getLine());
@@ -172,7 +165,7 @@ public class BuildFileASTTest {
 
   @Test
   public void testWithSyntaxErrorsDoesNotPrintDollarError() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     BuildFileAST buildFile = parseBuildFile(
         "abi = cxx_abi + '-glibc-' + glibc_version + '-' + generic_cpu + '-' + sysname",
         "libs = [abi + opt_level + '/lib/libcc.a']",
@@ -182,14 +175,11 @@ public class BuildFileASTTest {
         "           srcs = libs,",
         "           includes = [ abi + opt_level + '/include' ])");
     assertTrue(buildFile.containsErrors());
-    Event event = events.collector().iterator().next();
-    assertEquals("syntax error at '+': expected expression", event.getMessage());
-    Environment env = new Environment();
-    assertFalse(buildFile.exec(env, events.reporter()));
-    assertNull(findEvent(events.collector(), "$error$"));
+    assertContainsEvent("syntax error at '+': expected expression");
+    assertFalse(buildFile.exec(env, getEventHandler()));
+    assertNull(findEvent(getEventCollector(), "$error$"));
     // This message should not be printed anymore.
-    Event event2 = findEvent(events.collector(), "contains syntax error(s)");
-    assertNull(event2);
+    assertNull(findEvent(getEventCollector(), "contains syntax error(s)"));
   }
 
   @Test
@@ -202,7 +192,7 @@ public class BuildFileASTTest {
         + "include(\"//foo/bar:BUILD\")\n"
         + "b = 4\n");
 
-    BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(buildFile, events.reporter(),
+    BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(buildFile, getEventHandler(),
                                                             locator, false);
 
     assertFalse(buildFileAST.containsErrors());
@@ -217,15 +207,14 @@ public class BuildFileASTTest {
         "include(\"//foo/bar:defs\")\n"
         + "b = a + 1\n");
 
-    BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(buildFile, events.reporter(),
+    BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(buildFile, getEventHandler(),
                                                             locator, false);
 
     assertFalse(buildFileAST.containsErrors());
     assertThat(buildFileAST.getStatements()).hasSize(3);
 
-    Environment env = new Environment();
-    Reporter reporter = new Reporter();
-    assertFalse(buildFileAST.exec(env, reporter));
+    setFailFast(false);
+    assertFalse(buildFileAST.exec(env, getEventHandler()));
     assertEquals(2, env.lookup("b"));
   }
 
@@ -247,63 +236,53 @@ public class BuildFileASTTest {
     assertFalse(buildFileAST.containsErrors());
     assertThat(buildFileAST.getStatements()).hasSize(8);
 
-    Environment env = new Environment();
-    Reporter reporter = new Reporter();
-    assertFalse(buildFileAST.exec(env, reporter));
+    setFailFast(false);
+    assertFalse(buildFileAST.exec(env, getEventHandler()));
     assertEquals(5, env.lookup("b"));
     assertEquals(7, env.lookup("c"));
   }
 
   @Test
   public void testFailInclude() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     BuildFileAST buildFileAST = parseBuildFile("include(\"//nonexistent\")");
     assertThat(buildFileAST.getStatements()).hasSize(1);
-    events.assertContainsEvent("Include of '//nonexistent' failed");
-  }
-
-
-  private class EmptyPackageLocator implements CachingPackageLocator {
-    @Override
-    public Path getBuildFileForPackage(PackageIdentifier packageName) {
-      return null;
-    }
+    assertContainsEvent("Include of '//nonexistent' failed");
   }
 
-  private CachingPackageLocator emptyLocator = new EmptyPackageLocator();
 
   @Test
   public void testFailInclude2() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     Path buildFile = scratch.file("/foo/bar/BUILD",
         "include(\"//nonexistent:foo\")\n");
-    BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(buildFile, events.reporter(),
-                                                            emptyLocator, false);
+    BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(
+        buildFile, getEventHandler(), Environment.EMPTY_PACKAGE_LOCATOR, false);
     assertThat(buildFileAST.getStatements()).hasSize(1);
-    events.assertContainsEvent("Package 'nonexistent' not found");
+    assertContainsEvent("Package 'nonexistent' not found");
   }
 
   @Test
   public void testInvalidInclude() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     BuildFileAST buildFileAST = parseBuildFile("include(2)");
     assertThat(buildFileAST.getStatements()).isEmpty();
-    events.assertContainsEvent("syntax error at '2'");
+    assertContainsEvent("syntax error at '2'");
   }
 
   @Test
   public void testRecursiveInclude() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     Path buildFile = scratch.file("/foo/bar/BUILD",
         "include(\"//foo/bar:BUILD\")\n");
 
-    BuildFileAST.parseBuildFile(buildFile, events.reporter(), locator, false);
-    events.assertContainsEvent("Recursive inclusion");
+    BuildFileAST.parseBuildFile(buildFile, getEventHandler(), locator, false);
+    assertContainsEvent("Recursive inclusion");
   }
 
   @Test
   public void testParseErrorInclude() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
 
     scratch.file("/foo/bar/file",
         "a = 2 + % 3\n"); // parse error
@@ -311,14 +290,13 @@ public class BuildFileASTTest {
     parseBuildFile("include(\"//foo/bar:file\")");
 
     // Check the location is properly reported
-    Event event = events.collector().iterator().next();
+    Event event = assertContainsEvent("syntax error at '%': expected expression");
     assertEquals("/foo/bar/file:1:9", event.getLocation().print());
-    assertEquals("syntax error at '%': expected expression", event.getMessage());
   }
 
   @Test
   public void testNonExistentIncludeReported() throws Exception {
-    events.setFailFast(false);
+    setFailFast(false);
     BuildFileAST buildFileAST = parseBuildFile("include('//foo:bar')");
     assertThat(buildFileAST.getStatements()).hasSize(1);
   }
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
index a324b13ff5..02021245c6 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
@@ -31,8 +31,8 @@ import org.junit.runners.JUnit4;
 public class EnvironmentTest extends EvaluationTestCase {
 
   @Override
-  public EvaluationContext newEvaluationContext() {
-    return EvaluationContext.newBuildContext(getEventHandler());
+  public Environment newEnvironment() {
+    return newBuildEnvironment();
   }
 
   // Test the API directly
@@ -104,25 +104,41 @@ public class EnvironmentTest extends EvaluationTestCase {
 
   @Test
   public void testGetVariableNames() throws Exception {
-    update("foo", "bar");
-    update("wiz", 3);
-
-    Environment nestedEnv = new Environment(getEnvironment());
-    nestedEnv.update("foo", "bat");
-    nestedEnv.update("quux", 42);
+    Environment outerEnv;
+    Environment innerEnv;
+    try (Mutability mut = Mutability.create("outer")) {
+      outerEnv = Environment.builder(mut)
+          .setGlobals(Environment.BUILD).build()
+          .update("foo", "bar")
+          .update("wiz", 3);
+    }
+    try (Mutability mut = Mutability.create("inner")) {
+      innerEnv = Environment.builder(mut)
+          .setGlobals(outerEnv.getGlobals()).build()
+          .update("foo", "bat")
+          .update("quux", 42);
+    }
 
-    assertEquals(Sets.newHashSet("True", "False", "None", "foo", "wiz"),
-        getEnvironment().getVariableNames());
-    assertEquals(Sets.newHashSet("True", "False", "None", "foo", "wiz", "quux"),
-        nestedEnv.getVariableNames());
+    assertEquals(Sets.newHashSet("foo", "wiz",
+            "False", "None", "True",
+            "-", "bool", "dict", "enumerate", "int", "len", "list",
+            "range", "repr", "select", "sorted", "str", "zip"),
+        outerEnv.getVariableNames());
+    assertEquals(Sets.newHashSet("foo", "wiz", "quux",
+            "False", "None", "True",
+            "-", "bool", "dict", "enumerate", "int", "len", "list",
+            "range", "repr", "select", "sorted", "str", "zip"),
+        innerEnv.getVariableNames());
   }
 
   @Test
   public void testToString() throws Exception {
     update("subject", new StringLiteral("Hello, 'world'.", '\''));
     update("from", new StringLiteral("Java", '"'));
-    assertEquals("Environment{False -> false, None -> None, True -> true, from -> \"Java\", "
-        + "subject -> 'Hello, \\'world\\'.', }", getEnvironment().toString());
+    assertThat(getEnvironment().toString())
+        .startsWith("Environment(lexicalFrame=null, "
+            + "globalFrame=Frame[test]{\"from\": \"Java\", \"subject\": 'Hello, \\'world\\'.'}=>"
+            + "(BUILD){");
   }
 
   @Test
@@ -134,4 +150,58 @@ public class EnvironmentTest extends EvaluationTestCase {
       assertThat(e).hasMessage("update(value == null)");
     }
   }
+
+  @Test
+  public void testFrozen() throws Exception {
+    Environment env;
+    try (Mutability mutability = Mutability.create("testFrozen")) {
+      env = Environment.builder(mutability)
+          .setGlobals(Environment.BUILD).setEventHandler(Environment.FAIL_FAST_HANDLER).build();
+      env.update("x", 1);
+      assertEquals(env.lookup("x"), 1);
+      env.update("y", 2);
+      assertEquals(env.lookup("y"), 2);
+      assertEquals(env.lookup("x"), 1);
+      env.update("x", 3);
+      assertEquals(env.lookup("x"), 3);
+    }
+    try {
+      // This update to an existing variable should fail because the environment was frozen.
+      env.update("x", 4);
+      throw new Exception("failed to fail"); // not an AssertionError like fail()
+    } catch (AssertionError e) {
+      assertThat(e).hasMessage("Can't update x to 4 in frozen environment");
+    }
+    try {
+      // This update to a new variable should also fail because the environment was frozen.
+      env.update("newvar", 5);
+      throw new Exception("failed to fail"); // not an AssertionError like fail()
+    } catch (AssertionError e) {
+      assertThat(e).hasMessage("Can't update newvar to 5 in frozen environment");
+    }
+  }
+
+  @Test
+  public void testReadOnly() throws Exception {
+    Environment env = newSkylarkEnvironment()
+        .setup("special_var", 42)
+        .update("global_var", 666);
+
+    // We don't even get a runtime exception trying to modify these,
+    // because we get compile-time exceptions even before we reach runtime!
+    try {
+      env.eval("special_var = 41");
+      throw new AssertionError("failed to fail");
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("ERROR 1:1: Variable special_var is read only");
+    }
+
+    try {
+      env.eval("def foo(x): x += global_var; global_var = 36; return x", "foo(1)");
+      throw new AssertionError("failed to fail");
+    } catch (EvalExceptionWithStackTrace e) {
+      assertThat(e.getMessage()).contains("Variable 'global_var' is referenced before assignment. "
+          + "The variable is defined in the global scope.");
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index cf23858207..83a55be2d8 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -482,9 +482,9 @@ public class EvaluationTest extends EvaluationTestCase {
   @Test
   public void testDictComprehensions_ToString() throws Exception {
     assertEquals("{x: x for x in [1, 2]}",
-        evaluationContext.parseExpression("{x : x for x in [1, 2]}").toString());
+        parseExpression("{x : x for x in [1, 2]}").toString());
     assertEquals("{x + 'a': x for x in [1, 2]}",
-        evaluationContext.parseExpression("{x + 'a' : x for x in [1, 2]}").toString());
+        parseExpression("{x + 'a' : x for x in [1, 2]}").toString());
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java
index b2d661b7d4..e26234af0c 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java
@@ -35,48 +35,76 @@ import java.util.List;
  * Base class for test cases that use parsing and evaluation services.
  */
 public class EvaluationTestCase {
-  
+
   private EventCollectionApparatus eventCollectionApparatus;
   private PackageFactory factory;
   private TestMode testMode = TestMode.SKYLARK;
-  
-  protected EvaluationContext evaluationContext;
+  protected Environment env;
+  protected Mutability mutability = Mutability.create("test");
 
   public EvaluationTestCase()   {
     createNewInfrastructure();
   }
-  
+
   @Before
   public void setUp() throws Exception {
     createNewInfrastructure();
-    evaluationContext = newEvaluationContext();
+    env = newEnvironment();
+  }
+
+  /**
+   * Creates a standard Environment for tests in the BUILD language.
+   * No PythonPreprocessing, mostly empty mutable Environment.
+   */
+  public Environment newBuildEnvironment() {
+    return Environment.builder(mutability)
+        .setGlobals(Environment.BUILD)
+        .setEventHandler(getEventHandler())
+        .setLoadingPhase()
+        .build();
   }
 
-  public EvaluationContext newEvaluationContext() throws Exception {
+  /**
+   * Creates an Environment for Skylark with a mostly empty initial environment.
+   * For internal initialization or tests.
+   */
+  public Environment newSkylarkEnvironment() {
+    return Environment.builder(mutability)
+        .setSkylark()
+        .setGlobals(Environment.SKYLARK)
+        .setEventHandler(getEventHandler())
+        .build();
+  }
+
+  /**
+   * Creates a new Environment suitable for the test case. Subclasses may override it
+   * to fit their purpose and e.g. call newBuildEnvironment or newSkylarkEnvironment;
+   * or they may play with the testMode to run tests in either or both kinds of Environment.
+   * Note that all Environment-s may share the same Mutability, so don't close it.
+   * @return a fresh Environment.
+   */
+  public Environment newEnvironment() throws Exception {
     if (testMode == null) {
       throw new IllegalArgumentException(
           "TestMode is null. Please set a Testmode via setMode() or set the "
-          + "evaluatenContext manually by overriding newEvaluationContext()");
+          + "Environment manually by overriding newEnvironment()");
     }
-
-    return testMode.createContext(getEventHandler(), factory.getEnvironment());
+    return testMode.createEnvironment(getEventHandler(), null);
   }
 
   protected void createNewInfrastructure()  {
     eventCollectionApparatus = new EventCollectionApparatus(EventKind.ALL_EVENTS);
     factory = new PackageFactory(TestRuleClassProvider.getRuleClassProvider());
   }
-  
+
   /**
-   * Sets the specified {@code TestMode} and tries to create the appropriate {@code
-   * EvaluationContext}
-   * 
+   * Sets the specified {@code TestMode} and tries to create the appropriate {@code Environment}
    * @param testMode
    * @throws Exception
    */
   protected void setMode(TestMode testMode) throws Exception {
     this.testMode = testMode;
-    evaluationContext = newEvaluationContext();
+    env = newEnvironment();
   }
 
   protected void enableSkylarkMode() throws Exception {
@@ -96,32 +124,33 @@ public class EvaluationTestCase {
   }
 
   public Environment getEnvironment() {
-    return evaluationContext.getEnvironment();
+    return env;
   }
-  
+
   public boolean isSkylark() {
-    return evaluationContext.isSkylark();
+    return env.isSkylark();
   }
 
   protected List<Statement> parseFile(String... input) {
-    return evaluationContext.parseFile(input);
+    return env.parseFile(input);
   }
 
+  /** Parses an Expression from string without a supporting file */
   Expression parseExpression(String... input) {
-    return evaluationContext.parseExpression(input);
+    return Parser.parseExpression(env.createLexer(input), getEventHandler());
   }
 
   public EvaluationTestCase update(String varname, Object value) throws Exception {
-    evaluationContext.update(varname, value);
+    env.update(varname, value);
     return this;
   }
 
   public Object lookup(String varname) throws Exception {
-    return evaluationContext.lookup(varname);
+    return env.lookup(varname);
   }
 
   public Object eval(String... input) throws Exception {
-    return evaluationContext.eval(input);
+    return env.eval(input);
   }
 
   public void checkEvalError(String msg, String... input) throws Exception {
@@ -189,15 +218,14 @@ public class EvaluationTestCase {
 
   /**
    * Base class for test cases that run in specific modes (e.g. Build and/or Skylark)
-   *
-   */  
+   */
   protected abstract class ModalTestCase {
     private final SetupActions setup;
-    
-    protected ModalTestCase()   {
+
+    protected ModalTestCase() {
       setup = new SetupActions();
     }
-    
+
     /**
      * Allows the execution of several statements before each following test
      * @param statements The statement(s) to be executed
@@ -218,7 +246,7 @@ public class EvaluationTestCase {
       setup.registerUpdate(name, value);
       return this;
     }
-    
+
     /**
      * Evaluates two parameters and compares their results.
      * @param statement The statement to be evaluated
@@ -255,7 +283,7 @@ public class EvaluationTestCase {
       runTest(collectionTestable(statement, false, items));
       return this;
     }
-    
+
     /**
      * Evaluates the given statement and compares its result to the collection of expected objects
      * while considering their order
@@ -409,7 +437,7 @@ public class EvaluationTestCase {
   }
 
   /**
-   * A simple decorator that allows the execution of setup actions before running 
+   * A simple decorator that allows the execution of setup actions before running
    * a {@code Testable}
    */
   class TestableDecorator implements Testable {
@@ -480,7 +508,7 @@ public class EvaluationTestCase {
       }
     }
   }
-    
+
   /**
    * A class that executes each separate test in both modes (Build and Skylark)
    */
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index b5a8015716..75913b42d5 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -32,43 +32,45 @@ import org.junit.runners.JUnit4;
 import java.util.LinkedList;
 import java.util.List;
 
-
 /**
  *  Tests of parser behaviour.
  */
 @RunWith(JUnit4.class)
 public class ParserTest extends EvaluationTestCase {
 
-  EvaluationContext buildContext;
-  EvaluationContext buildContextWithPython;
+  Environment buildEnvironment;
 
   @Before
   @Override
   public void setUp() throws Exception {
     super.setUp();
-    buildContext = EvaluationContext.newBuildContext(getEventHandler());
-    buildContextWithPython = EvaluationContext.newBuildContext(
-        getEventHandler(), new Environment(), /*parsePython*/true);
+    buildEnvironment = newBuildEnvironment();
   }
 
   private Parser.ParseResult parseFileWithComments(String... input) {
-    return buildContext.parseFileWithComments(input);
+    return buildEnvironment.parseFileWithComments(input);
   }
+
+  /** Parses build code (not Skylark) */
   @Override
   protected List<Statement> parseFile(String... input) {
-    return buildContext.parseFile(input);
+    return buildEnvironment.parseFile(input);
   }
+
+  /** Parses a build code (not Skylark) with PythonProcessing enabled */
   private List<Statement> parseFileWithPython(String... input) {
-    return buildContextWithPython.parseFile(input);
+    return Parser.parseFile(
+        buildEnvironment.createLexer(input),
+        getEventHandler(),
+        Environment.EMPTY_PACKAGE_LOCATOR,
+        /*parsePython=*/true).statements;
   }
+
+  /** Parses Skylark code */
   private List<Statement> parseFileForSkylark(String... input) {
-    return evaluationContext.parseFile(input);
-  }
-  private Statement parseStatement(String... input) {
-    return buildContext.parseStatement(input);
+    return env.parseFile(input);
   }
 
-
   private static String getText(String text, ASTNode node) {
     return text.substring(node.getLocation().getStartOffset(),
                           node.getLocation().getEndOffset());
@@ -707,7 +709,7 @@ public class ParserTest extends EvaluationTestCase {
   @Test
   public void testParserContainsErrors() throws Exception {
     setFailFast(false);
-    parseStatement("+");
+    parseFile("+");
     assertContainsEvent("syntax error at '+'");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 88a2e31e4a..8668f50a2a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -533,7 +533,7 @@ public class SkylarkEvaluationTest extends EvaluationTest {
     new SkylarkTest()
         .update("mock", new Mock())
         .testIfExactError("Keyword arguments are not allowed when calling a java method"
-            + "\nwhile calling method 'string' on object of type Mock",
+            + "\nwhile calling method 'string' for type Mock",
             "mock.string(key=True)");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
index 86ad1fb557..7558b6c4f3 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
@@ -45,8 +45,8 @@ public class SkylarkListTest extends EvaluationTestCase {
       SkylarkList.lazyList(new CustomIterable(), Integer.class);
 
   @Override
-  public EvaluationContext newEvaluationContext() throws Exception {
-    return super.newEvaluationContext().update("lazy", list);
+  public Environment newEnvironment() throws Exception {
+    return newSkylarkEnvironment().update("lazy", list);
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkShell.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkShell.java
index c4bb7d84a9..b12f51b1d0 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkShell.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkShell.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.syntax;
 
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.rules.SkylarkModules;
 
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -42,8 +41,9 @@ class SkylarkShell {
 
   private final BufferedReader reader = new BufferedReader(
       new InputStreamReader(System.in, Charset.defaultCharset()));
-  private final EvaluationContext ev =
-      SkylarkModules.newEvaluationContext(PRINT_HANDLER);
+  private final Mutability mutability = Mutability.create("shell");
+  private final Environment env = Environment.builder(mutability)
+      .setSkylark().setGlobals(Environment.SKYLARK).setEventHandler(PRINT_HANDLER).build();
 
   public String read() {
     StringBuilder input = new StringBuilder();
@@ -70,7 +70,7 @@ class SkylarkShell {
     String input;
     while ((input = read()) != null) {
       try {
-        Object result = ev.eval(input);
+        Object result = env.eval(input);
         if (result != null) {
           System.out.println(Printer.repr(result));
         }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTests.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTests.java
index 57b341f3b9..3ebdf8dde4 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTests.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTests.java
@@ -95,7 +95,7 @@ public class ValidationTests extends EvaluationTestCase {
 
   @Test
   public void testBuiltinSymbolsAreReadOnly() throws Exception {
-    checkError("Variable rule is read only", "rule = 1");
+    checkError("Variable repr is read only", "repr = 1");
   }
 
   @Test
@@ -300,8 +300,8 @@ public class ValidationTests extends EvaluationTestCase {
   @Test
   public void testFunctionReturnsFunction() {
     parse(
-        "def impl(ctx):",
-        "  return None",
+        "def rule(*, implementation): return None",
+        "def impl(ctx): return None",
         "",
         "skylark_rule = rule(implementation = impl)",
         "",
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/TestMode.java b/src/test/java/com/google/devtools/build/lib/testutil/TestMode.java
index 5ec7ac27b2..ea887f9d68 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/TestMode.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/TestMode.java
@@ -14,31 +14,36 @@
 package com.google.devtools.build.lib.testutil;
 
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.rules.SkylarkModules;
 import com.google.devtools.build.lib.syntax.Environment;
-import com.google.devtools.build.lib.syntax.EvaluationContext;
+import com.google.devtools.build.lib.syntax.Mutability;
 
 /**
  * Describes a particular testing mode by determining how the
- * appropriate {@code EvaluationContext} has to be created
+ * appropriate {@code Environment} has to be created
  */
 public abstract class TestMode {
-  public static final TestMode BUILD = new TestMode() {
-    @Override
-    public EvaluationContext createContext(EventHandler eventHandler, Environment environment)
-        throws Exception {
-      return EvaluationContext.newBuildContext(eventHandler, environment);
-    }
-  };
+  public static final TestMode BUILD =
+      new TestMode() {
+        @Override
+        public Environment createEnvironment(EventHandler eventHandler, Environment environment) {
+          return Environment.builder(Mutability.create("build test"))
+              .setGlobals(environment == null ? Environment.BUILD : environment.getGlobals())
+              .setEventHandler(eventHandler)
+              .build();
+        }
+      };
 
-  public static final TestMode SKYLARK = new TestMode() {
-    @Override
-    public EvaluationContext createContext(EventHandler eventHandler, Environment environment)
-        throws Exception {
-      return SkylarkModules.newEvaluationContext(eventHandler);
-    }
-  };
+  public static final TestMode SKYLARK =
+      new TestMode() {
+        @Override
+        public Environment createEnvironment(EventHandler eventHandler, Environment environment) {
+          return Environment.builder(Mutability.create("skylark test"))
+              .setSkylark()
+              .setGlobals(environment == null ? Environment.SKYLARK : environment.getGlobals())
+              .setEventHandler(eventHandler)
+              .build();
+        }
+      };
 
-  public abstract EvaluationContext createContext(
-      EventHandler eventHandler, Environment environment) throws Exception;
-}
\ No newline at end of file
+  public abstract Environment createEnvironment(EventHandler eventHandler, Environment environment);
+}