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 creates it,
so no Environment is left open for modification after being created and exported;
exceptions for tests, the shell and initialization contexts.

Unify Environment, SkylarkEnvironment and EvaluationContext into Environment.
Have a notion of Frame for the bindings + parent + mutability.
Replace the updateAndPropagate mechanism by a dynamicFrame.
Simplify ValidationEnvironment, that is now always deduced from the Environment.

--
MOS_MIGRATED_REVID=102363438

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..2f298518e3 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;
 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, Environment>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 e0c074a126..5137b45c65 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 16bcb8010f..a473f9525b 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
@@ -526,7 +526,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);
+}