bazel syntax: move all tree-walking evaluation logic into Eval

All tree-walking evaluation logic belongs in Eval.
All basic operations on Starlark values belong in EvalUtils.
The syntax classes should know nothing of these concepts.

In the interests of sanity, this change only moves existing
logic with minimal unnecessary edits.
(One exception: BinaryOperatorExpression.evaluateWithShortCircuiting was inlined.)

FuncallExpression has not been moved yet because it is so large,
but as a first step, its relevant methods have all been made static,
with an additional parameter.

Every doEval method is now a case in Eval.doEval;
The virtual method dispatch is replaced by switch(kind()).
Other helpers have moved into Eval, such as:
  createInvalidIdentifierException
  maybeTransformException
  getSpecialException
  createInvalidIdentifierException

StringLiteral constructor is now private.

Subsequent changes will move FuncallExpression, simplify Eval,
and move value operations into EvalUtils.

PiperOrigin-RevId: 269661142
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index 21f5327..7acd551 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -135,41 +135,20 @@
   }
 
   /**
-   * Evaluates a short-circuiting binary operator, i.e. boolean {@code and} or {@code or}.
-   *
-   * <p>In contrast to {@link #evaluate}, this method takes unevaluated expressions. The left-hand
-   * side expression is evaluated exactly once, and the right-hand side expression is evaluated
-   * either once or not at all.
-   *
-   * @throws IllegalArgumentException if {@code op} is not {@link Operator#AND} or {@link
-   *     Operator#OR}.
-   */
-  public static Object evaluateWithShortCircuiting(
-      TokenKind op, Expression x, Expression y, Environment env, Location loc)
-      throws EvalException, InterruptedException {
-    Object xval = x.eval(env);
-    switch (op) {
-      case AND:
-        return EvalUtils.toBoolean(xval) ? y.eval(env) : xval;
-      case OR:
-        return EvalUtils.toBoolean(xval) ? xval : y.eval(env);
-      default:
-        throw new IllegalArgumentException("Not a short-circuiting operator: " + op);
-    }
-  }
-
-  /**
    * Evaluates {@code x @ y}, where {@code @} is the operator, and returns the result.
    *
    * <p>This method does not implement any short-circuiting logic for boolean operations, as the
    * parameters are already evaluated.
    */
-  public static Object evaluate(TokenKind op, Object x, Object y, Environment env, Location loc)
+  // TODO(adonovan): move to EvalUtils.binary.
+  static Object evaluate(TokenKind op, Object x, Object y, Environment env, Location loc)
       throws EvalException, InterruptedException {
     return evaluate(op, x, y, env, loc, /*isAugmented=*/ false);
   }
 
-  private static Object evaluate(
+  // TODO(adonovan): eliminate isAugmented parameter; make the caller handle the list+=iterable
+  // special case and desugar the rest to y=y+x.
+  static Object evaluate(
       TokenKind op, Object x, Object y, Environment env, Location location, boolean isAugmented)
       throws EvalException, InterruptedException {
     try {
@@ -257,22 +236,12 @@
    * <p>Whether or not {@code x} is mutated depends on its type. If it is mutated, then it is also
    * the return value.
    */
-  public static Object evaluateAugmented(
-      TokenKind op, Object x, Object y, Environment env, Location loc)
+  static Object evaluateAugmented(TokenKind op, Object x, Object y, Environment env, Location loc)
       throws EvalException, InterruptedException {
     return evaluate(op, x, y, env, loc, /*isAugmented=*/ true);
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    if (op == TokenKind.AND || op == TokenKind.OR) {
-      return evaluateWithShortCircuiting(op, x, y, env, getLocation());
-    } else {
-      return evaluate(op, x.eval(env), y.eval(env), env, getLocation());
-    }
-  }
-
-  @Override
   public void accept(NodeVisitor visitor) {
     visitor.visit(this);
   }
@@ -283,8 +252,8 @@
   }
 
   /** Implements 'x + y'. */
-  private static Object plus(
-      Object x, Object y, Environment env, Location location, boolean isAugmented)
+  // TODO(adonovan): move to EvalUtils.plus.
+  static Object plus(Object x, Object y, Environment env, Location location, boolean isAugmented)
       throws EvalException {
     // int + int
     if (x instanceof Integer && y instanceof Integer) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
index bacb239..6db31b2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
@@ -413,7 +413,7 @@
       expr = ((ExpressionStatement) statements.get(n - 1)).getExpression();
     }
     Eval.execStatements(env, stmts);
-    return expr == null ? null : expr.eval(env);
+    return expr == null ? null : Eval.eval(env, expr);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
index bb30057..a99161d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
@@ -61,8 +61,8 @@
     try (SilentCloseable c =
         Profiler.instance().profile(ProfilerTask.STARLARK_BUILTIN_FN, methodName)) {
       Object[] javaArguments =
-          ast.convertStarlarkArgumentsToJavaMethodArguments(
-              methodDescriptor, clazz, args, kwargs, env);
+          FuncallExpression.convertStarlarkArgumentsToJavaMethodArguments(
+              env, ast, methodDescriptor, clazz, args, kwargs);
       return methodDescriptor.call(objValue, javaArguments, ast.getLocation(), env);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java
index 522621a..e07906c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java
@@ -15,9 +15,7 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.events.Location;
 import java.io.IOException;
-import java.util.ArrayList;
 
 /**
  * Syntax node for list and dict comprehensions.
@@ -131,72 +129,4 @@
     return Kind.COMPREHENSION;
   }
 
-  @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    final SkylarkDict<Object, Object> dict = isDict ? SkylarkDict.of(env) : null;
-    final ArrayList<Object> list = isDict ? null : new ArrayList<>();
-
-    // The Lambda class serves as a recursive lambda closure.
-    class Lambda {
-      // execClauses(index) recursively executes the clauses starting at index,
-      // and finally evaluates the body and adds its value to the result.
-      void execClauses(int index) throws EvalException, InterruptedException {
-        // recursive case: one or more clauses
-        if (index < clauses.size()) {
-          Clause clause = clauses.get(index);
-          if (clause instanceof For) {
-            For forClause = (For) clause;
-
-            Object iterable = forClause.getIterable().eval(env);
-            Location loc = getLocation();
-            Iterable<?> listValue = EvalUtils.toIterable(iterable, loc, env);
-            EvalUtils.lock(iterable, loc);
-            try {
-              for (Object elem : listValue) {
-                Eval.assign(forClause.getVars(), elem, env, loc);
-                execClauses(index + 1);
-              }
-            } finally {
-              EvalUtils.unlock(iterable, loc);
-            }
-
-          } else {
-            If ifClause = (If) clause;
-            if (EvalUtils.toBoolean(ifClause.condition.eval(env))) {
-              execClauses(index + 1);
-            }
-          }
-          return;
-        }
-
-        // base case: evaluate body and add to result.
-        if (dict != null) {
-          DictExpression.Entry body = (DictExpression.Entry) Comprehension.this.body;
-          Object k = body.getKey().eval(env);
-          EvalUtils.checkValidDictKey(k, env);
-          Object v = body.getValue().eval(env);
-          dict.put(k, v, getLocation(), env);
-        } else {
-          list.add(((Expression) body).eval(env));
-        }
-      }
-    }
-    new Lambda().execClauses(0);
-
-    // Undefine loop variables (remove them from the environment).
-    // This code is useful for the transition, to make sure no one relies on the old behavior
-    // (where loop variables were leaking).
-    // TODO(laurentlb): Instead of removing variables, we should create them in a nested scope.
-    for (Clause clause : clauses) {
-      // Check if a loop variable conflicts with another local variable.
-      if (clause instanceof For) {
-        for (Identifier ident : Identifier.boundIdentifiers(((For) clause).getVars())) {
-          env.removeLocalBinding(ident.getName());
-        }
-      }
-    }
-
-    return isDict ? dict : SkylarkList.MutableList.copyOf(env, list);
-  }
-
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
index 79ff20f..0ebb4f3 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
@@ -48,15 +48,6 @@
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    if (EvalUtils.toBoolean(condition.eval(env))) {
-      return thenCase.eval(env);
-    } else {
-      return elseCase.eval(env);
-    }
-  }
-
-  @Override
   public void accept(NodeVisitor visitor) {
     visitor.visit(this);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java
index f8b48af..a6312e0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.events.Location;
 import java.io.IOException;
 import java.util.List;
 
@@ -60,22 +59,6 @@
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    SkylarkDict<Object, Object> dict = SkylarkDict.of(env);
-    Location loc = getLocation();
-    for (Entry entry : entries) {
-      Object key = entry.key.eval(env);
-      Object val = entry.value.eval(env);
-      if (dict.containsKey(key)) {
-        throw new EvalException(
-            loc, "Duplicated key " + Printer.repr(key) + " when creating dictionary");
-      }
-      dict.put(key, val, loc, env);
-    }
-    return dict;
-  }
-
-  @Override
   public void prettyPrint(Appendable buffer) throws IOException {
     buffer.append("{");
     String sep = "";
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index 33c6eb6..7e18593 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -13,8 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.syntax;
 
-import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.util.SpellChecker;
 import java.io.IOException;
 
 /** Syntax node for a dot expression. e.g. obj.field, but not obj.method() */
@@ -45,122 +43,6 @@
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    Object objValue = object.eval(env);
-    String name = field.getName();
-    Object result = eval(objValue, name, getLocation(), env);
-    return checkResult(objValue, result, name, getLocation(), env.getSemantics());
-  }
-
-  /** Throws the correct error message if the result is null depending on the objValue. */
-  public static Object checkResult(
-      Object objValue, Object result, String name, Location loc, StarlarkSemantics semantics)
-      throws EvalException {
-    if (result != null) {
-      return result;
-    }
-    throw getMissingFieldException(objValue, name, loc, semantics, "field");
-  }
-
-  static EvalException getMissingFieldException(
-      Object objValue, String name, Location loc, StarlarkSemantics semantics, String accessName) {
-    String suffix = "";
-    EvalException toSuppress = null;
-    if (objValue instanceof ClassObject) {
-      String customErrorMessage = ((ClassObject) objValue).getErrorMessageForUnknownField(name);
-      if (customErrorMessage != null) {
-        return new EvalException(loc, customErrorMessage);
-      }
-      try {
-        suffix = SpellChecker.didYouMean(name, ((ClassObject) objValue).getFieldNames());
-      } catch (EvalException ee) {
-        toSuppress = ee;
-      }
-    } else {
-      suffix =
-          SpellChecker.didYouMean(
-              name,
-              FuncallExpression.getStructFieldNames(
-                  semantics,
-                  objValue instanceof Class ? (Class<?>) objValue : objValue.getClass()));
-    }
-    if (suffix.isEmpty() && hasMethod(semantics, objValue, name)) {
-      // If looking up the field failed, then we know that this method must have struct_field=false
-      suffix = ", however, a method of that name exists";
-    }
-    EvalException ee =
-        new EvalException(
-            loc,
-            String.format(
-                "object of type '%s' has no %s '%s'%s",
-                EvalUtils.getDataTypeName(objValue), accessName, name, suffix));
-    if (toSuppress != null) {
-      ee.addSuppressed(toSuppress);
-    }
-    return ee;
-  }
-
-  /** Returns whether the given object has a method with the given name. */
-  static boolean hasMethod(StarlarkSemantics semantics, Object obj, String name) {
-    Class<?> cls = obj instanceof Class ? (Class<?>) obj : obj.getClass();
-    if (Runtime.getBuiltinRegistry().getFunctionNames(cls).contains(name)) {
-      return true;
-    }
-
-    return FuncallExpression.getMethodNames(semantics, cls).contains(name);
-  }
-
-  /**
-   * Returns the field of the given name of the struct objValue, or null if no such field exists.
-   */
-  public static Object eval(Object objValue, String name,
-      Location loc, Environment env) throws EvalException, InterruptedException {
-
-    MethodDescriptor method =
-        objValue instanceof Class<?>
-            ? FuncallExpression.getMethod(env.getSemantics(), (Class<?>) objValue, name)
-            : FuncallExpression.getMethod(env.getSemantics(), objValue.getClass(), name);
-
-    if (method != null && method.isStructField()) {
-      return method
-          .call(
-              objValue,
-              FuncallExpression.extraInterpreterArgs(method, /* ast = */ null, loc, env)
-                  .toArray(),
-              loc,
-              env);
-    }
-
-    if (objValue instanceof SkylarkClassObject) {
-      try {
-        return ((SkylarkClassObject) objValue).getValue(loc, env.getSemantics(), name);
-      } catch (IllegalArgumentException ex) {
-        throw new EvalException(loc, ex);
-      }
-    } else if (objValue instanceof ClassObject) {
-      Object result = null;
-      try {
-        result = ((ClassObject) objValue).getValue(name);
-      } catch (IllegalArgumentException ex) {
-        throw new EvalException(loc, ex);
-      }
-      // ClassObjects may have fields that are annotated with @SkylarkCallable.
-      // Since getValue() does not know about those, we cannot expect that result is a valid object.
-      if (result != null) {
-        result = SkylarkType.convertToSkylark(result, env);
-        // If we access NestedSets using ClassObject.getValue() we won't know the generic type,
-        // so we have to disable it. This should not happen.
-        SkylarkType.checkTypeAllowedInSkylark(result, loc);
-        return result;
-      }
-    }
-    if (method != null) {
-      return new BuiltinCallable(objValue, name);
-    }
-    return null;
-  }
-
-  @Override
   public void accept(NodeVisitor visitor) {
     visitor.visit(this);
   }
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
index f2cff04..1d587cb 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -1241,7 +1241,7 @@
       Event ev = handler.messages.get(0);
       throw new EvalException(ev.getLocation(), ev.getMessage());
     }
-    return expr.eval(this);
+    return Eval.eval(this, expr);
   }
 
   /** Executes a Skylark file (sequence of statements) in this environment. (Debugger API) */
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
index c68e2a8..6ec86cf 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -16,19 +16,18 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.util.SpellChecker;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 
-/**
- * Evaluation code for the Skylark AST. At the moment, it can execute only statements (and defers to
- * Expression.eval for evaluating expressions).
- */
+/** A syntax-tree-walking evaluator. */
 // TODO(adonovan): make this class the sole locus of tree-based evaluation logic.
 // Make all its methods static, and thread Environment (soon: StarlarkThread) explicitly.
 // The only actual state is the return value, which can be saved in the Environment's call frame.
-// Then move all Expression.eval logic in here too, and simplify.
+// Move remaining Expression.eval logic here, and simplify.
 final class Eval {
 
   private static final AtomicReference<Debugger> debugger = new AtomicReference<>();
@@ -66,7 +65,7 @@
   }
 
   private void execAssignment(AssignmentStatement node) throws EvalException, InterruptedException {
-    Object rvalue = node.getRHS().eval(env);
+    Object rvalue = eval(env, node.getRHS());
     assign(node.getLHS(), rvalue, env, node.getLocation());
   }
 
@@ -81,7 +80,7 @@
   }
 
   private TokenKind execFor(ForStatement node) throws EvalException, InterruptedException {
-    Object o = node.getCollection().eval(env);
+    Object o = eval(env, node.getCollection());
     Iterable<?> col = EvalUtils.toIterable(o, node.getLocation(), env);
     EvalUtils.lock(o, node.getLocation());
     try {
@@ -116,7 +115,7 @@
     if (defaultExpressions != null) {
       defaultValues = new ArrayList<>(defaultExpressions.size());
       for (Expression expr : defaultExpressions) {
-        defaultValues.add(expr.eval(env));
+        defaultValues.add(eval(env, expr));
       }
     }
 
@@ -141,7 +140,7 @@
     // Avoid iterator overhead - most of the time there will be one or few "if"s.
     for (int i = 0; i < thenBlocks.size(); i++) {
       IfStatement.ConditionalStatements stmt = thenBlocks.get(i);
-      if (EvalUtils.toBoolean(stmt.getCondition().eval(env))) {
+      if (EvalUtils.toBoolean(eval(env, stmt.getCondition()))) {
         return exec(stmt);
       }
     }
@@ -171,7 +170,7 @@
   private TokenKind execReturn(ReturnStatement node) throws EvalException, InterruptedException {
     Expression ret = node.getReturnExpression();
     if (ret != null) {
-      this.result = ret.eval(env);
+      this.result = eval(env, ret);
     }
     return TokenKind.RETURN;
   }
@@ -184,7 +183,7 @@
     try {
       return execDispatch(st);
     } catch (EvalException ex) {
-      throw st.maybeTransformException(ex);
+      throw maybeTransformException(st, ex);
     }
   }
 
@@ -199,7 +198,7 @@
       case CONDITIONAL:
         return execIfBranch((IfStatement.ConditionalStatements) st);
       case EXPRESSION:
-        ((ExpressionStatement) st).getExpression().eval(env);
+        eval(env, ((ExpressionStatement) st).getExpression());
         return TokenKind.PASS;
       case FLOW:
         return ((FlowStatement) st).getKind();
@@ -235,15 +234,13 @@
    * Updates the environment bindings, and possibly mutates objects, so as to assign the given value
    * to the given expression. The expression must be valid for an {@code LValue}.
    */
-  // TODO(adonovan): make this a private instance method once all Expression.eval methods move here,
-  // in particular, AbstractComprehension.
-  static void assign(Expression expr, Object value, Environment env, Location loc)
+  private static void assign(Expression expr, Object value, Environment env, Location loc)
       throws EvalException, InterruptedException {
     if (expr instanceof Identifier) {
       assignIdentifier((Identifier) expr, value, env);
     } else if (expr instanceof IndexExpression) {
-      Object object = ((IndexExpression) expr).getObject().eval(env);
-      Object key = ((IndexExpression) expr).getKey().eval(env);
+      Object object = eval(env, ((IndexExpression) expr).getObject());
+      Object key = eval(env, ((IndexExpression) expr).getKey());
       assignItem(object, key, value, env, loc);
     } else if (expr instanceof ListExpression) {
       ListExpression list = (ListExpression) expr;
@@ -329,16 +326,16 @@
       throws EvalException, InterruptedException {
     if (expr instanceof Identifier) {
       Object result =
-          BinaryOperatorExpression.evaluateAugmented(op, expr.eval(env), rhs.eval(env), env, loc);
+          BinaryOperatorExpression.evaluateAugmented(op, eval(env, expr), eval(env, rhs), env, loc);
       assignIdentifier((Identifier) expr, result, env);
     } else if (expr instanceof IndexExpression) {
       IndexExpression indexExpression = (IndexExpression) expr;
       // The object and key should be evaluated only once, so we don't use expr.eval().
-      Object object = indexExpression.getObject().eval(env);
-      Object key = indexExpression.getKey().eval(env);
+      Object object = eval(env, indexExpression.getObject());
+      Object key = eval(env, indexExpression.getKey());
       Object oldValue = IndexExpression.evaluate(object, key, env, loc);
       // Evaluate rhs after lhs.
-      Object rhsValue = rhs.eval(env);
+      Object rhsValue = eval(env, rhs);
       Object result = BinaryOperatorExpression.evaluateAugmented(op, oldValue, rhsValue, env, loc);
       assignItem(object, key, result, env, loc);
     } else if (expr instanceof ListExpression) {
@@ -348,4 +345,376 @@
       throw new EvalException(loc, "cannot perform augmented assignment on '" + expr + "'");
     }
   }
+
+  // ---- expressions ----
+
+  /**
+   * Returns the result of evaluating this build-language expression in the specified environment.
+   * All BUILD language datatypes are mapped onto the corresponding Java types as follows:
+   *
+   * <pre>
+   *    int   -> Integer
+   *    float -> Double          (currently not generated by the grammar)
+   *    str   -> String
+   *    [...] -> List&lt;Object>    (mutable)
+   *    (...) -> List&lt;Object>    (immutable)
+   *    {...} -> Map&lt;Object, Object>
+   *    func  -> Function
+   * </pre>
+   *
+   * @return the result of evaluting the expression: a Java object corresponding to a datatype in
+   *     the BUILD language.
+   * @throws EvalException if the expression could not be evaluated.
+   * @throws InterruptedException may be thrown in a sub class.
+   */
+  static Object eval(Environment env, Expression expr) throws EvalException, InterruptedException {
+    // TODO(adonovan): don't push and pop all the time. We should only need the stack of function
+    // call frames, and we should recycle them.
+    // TODO(adonovan): put the Environment (Starlark thread) into the Java thread-local store
+    // once only, in enterScope, and undo this in exitScope.
+    try {
+      if (Callstack.enabled) {
+        Callstack.push(expr);
+      }
+      try {
+        return doEval(env, expr);
+      } catch (EvalException ex) {
+        throw maybeTransformException(expr, ex);
+      }
+    } finally {
+      if (Callstack.enabled) {
+        Callstack.pop();
+      }
+    }
+  }
+
+  private static Object doEval(Environment env, Expression expr)
+      throws EvalException, InterruptedException {
+    switch (expr.kind()) {
+      case BINARY_OPERATOR:
+        {
+          // AND and OR require short-circuit evaluation.
+          BinaryOperatorExpression binop = (BinaryOperatorExpression) expr;
+          switch (binop.getOperator()) {
+            case AND:
+              {
+                Object xval = eval(env, binop.getX());
+                return EvalUtils.toBoolean(xval) ? Eval.eval(env, binop.getY()) : xval;
+              }
+            case OR:
+              {
+                Object xval = eval(env, binop.getX());
+                return EvalUtils.toBoolean(xval) ? xval : Eval.eval(env, binop.getY());
+              }
+            default:
+              return BinaryOperatorExpression.evaluate(
+                  binop.getOperator(),
+                  eval(env, binop.getX()),
+                  eval(env, binop.getY()),
+                  env,
+                  binop.getLocation());
+          }
+        }
+
+      case COMPREHENSION:
+        {
+          return evalComprehension(env, (Comprehension) expr);
+        }
+
+      case CONDITIONAL:
+        {
+          ConditionalExpression cond = (ConditionalExpression) expr;
+          Object v = eval(env, cond.getCondition());
+          return eval(env, EvalUtils.toBoolean(v) ? cond.getThenCase() : cond.getElseCase());
+        }
+
+      case DICT_EXPR:
+        {
+          DictExpression dictexpr = (DictExpression) expr;
+          SkylarkDict<Object, Object> dict = SkylarkDict.of(env);
+          Location loc = dictexpr.getLocation();
+          for (DictExpression.Entry entry : dictexpr.getEntries()) {
+            Object k = eval(env, entry.getKey());
+            Object v = eval(env, entry.getValue());
+            int before = dict.size();
+            dict.put(k, v, loc, env);
+            if (dict.size() == before) {
+              throw new EvalException(
+                  loc, "Duplicated key " + Printer.repr(k) + " when creating dictionary");
+            }
+          }
+          return dict;
+        }
+
+      case DOT:
+        {
+          DotExpression dot = (DotExpression) expr;
+          Object object = eval(env, dot.getObject());
+          String name = dot.getField().getName();
+          Object result = EvalUtils.getAttr(env, dot.getLocation(), object, name);
+          return checkResult(object, result, name, dot.getLocation(), env.getSemantics());
+        }
+
+      case FUNCALL:
+        {
+          FuncallExpression call = (FuncallExpression) expr;
+          // TODO(adonovan): inline this one in a follow-up. It's a whopper.
+          return FuncallExpression.call(env, call);
+        }
+
+      case IDENTIFIER:
+        {
+          Identifier id = (Identifier) expr;
+          String name = id.getName();
+          if (id.getScope() == null) {
+            // Legacy behavior, to be removed.
+            Object result = env.lookup(name);
+            if (result == null) {
+              throw createInvalidIdentifierException(id, env.getVariableNames());
+            }
+            return result;
+          }
+
+          Object result;
+          switch (id.getScope()) {
+            case Local:
+              result = env.localLookup(name);
+              break;
+            case Module:
+              result = env.moduleLookup(name);
+              break;
+            case Universe:
+              result = env.universeLookup(name);
+              break;
+            default:
+              throw new IllegalStateException(id.getScope().toString());
+          }
+          if (result == null) {
+            // Since Scope was set, we know that the variable is defined in the scope.
+            // However, the assignment was not yet executed.
+            EvalException e = getSpecialException(id);
+            throw e != null
+                ? e
+                : new EvalException(
+                    id.getLocation(),
+                    id.getScope().getQualifier()
+                        + " variable '"
+                        + name
+                        + "' is referenced before assignment.");
+          }
+          return result;
+        }
+
+      case INDEX:
+        {
+          IndexExpression index = (IndexExpression) expr;
+          Object object = eval(env, index.getObject());
+          Object key = eval(env, index.getKey());
+          return IndexExpression.evaluate(object, key, env, index.getLocation());
+        }
+
+      case INTEGER_LITERAL:
+        return ((IntegerLiteral) expr).getValue();
+
+      case LIST_EXPR:
+        {
+          ListExpression list = (ListExpression) expr;
+          ArrayList<Object> result = new ArrayList<>(list.getElements().size());
+          for (Expression elem : list.getElements()) {
+            // Convert NPEs to EvalExceptions.
+            if (elem == null) {
+              throw new EvalException(list.getLocation(), "null expression in " + list);
+            }
+            result.add(eval(env, elem));
+          }
+          return list.isTuple()
+              ? SkylarkList.Tuple.copyOf(result) // TODO(adonovan): opt: avoid copy
+              : SkylarkList.MutableList.wrapUnsafe(env, result);
+        }
+
+      case SLICE:
+        {
+          SliceExpression slice = (SliceExpression) expr;
+          Object object = eval(env, slice.getObject());
+          Object start = slice.getStart() == null ? Runtime.NONE : eval(env, slice.getStart());
+          Object end = slice.getEnd() == null ? Runtime.NONE : eval(env, slice.getEnd());
+          Object step = slice.getStep() == null ? Runtime.NONE : eval(env, slice.getStep());
+          Location loc = slice.getLocation();
+
+          // TODO(adonovan): move the rest into a public EvalUtils.slice() operator.
+
+          if (object instanceof SkylarkList) {
+            return ((SkylarkList<?>) object).getSlice(start, end, step, loc, env.mutability());
+          }
+
+          if (object instanceof String) {
+            String string = (String) object;
+            List<Integer> indices =
+                EvalUtils.getSliceIndices(start, end, step, string.length(), loc);
+            // TODO(adonovan): opt: optimize for common case, step=1.
+            char[] result = new char[indices.size()];
+            char[] original = string.toCharArray();
+            int resultIndex = 0;
+            for (int originalIndex : indices) {
+              result[resultIndex] = original[originalIndex];
+              ++resultIndex;
+            }
+            return new String(result);
+          }
+
+          throw new EvalException(
+              loc,
+              String.format(
+                  "type '%s' has no operator [:](%s, %s, %s)",
+                  EvalUtils.getDataTypeName(object),
+                  EvalUtils.getDataTypeName(start),
+                  EvalUtils.getDataTypeName(end),
+                  EvalUtils.getDataTypeName(step)));
+        }
+
+      case STRING_LITERAL:
+        return ((StringLiteral) expr).getValue();
+
+      case UNARY_OPERATOR:
+        {
+          UnaryOperatorExpression unop = (UnaryOperatorExpression) expr;
+          Object x = eval(env, unop.getX());
+          return UnaryOperatorExpression.evaluate(unop.getOperator(), x, unop.getLocation());
+        }
+    }
+    throw new IllegalArgumentException("unexpected expression: " + expr.kind());
+  }
+
+  /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */
+  private static EvalException getSpecialException(Identifier id) {
+    if (id.getName().equals("PACKAGE_NAME")) {
+      return new EvalException(
+          id.getLocation(),
+          "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', "
+              + "please use the latter ("
+              + "https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name). ");
+    }
+    if (id.getName().equals("REPOSITORY_NAME")) {
+      return new EvalException(
+          id.getLocation(),
+          "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', please"
+              + " use the latter ("
+              + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name).");
+    }
+    return null;
+  }
+
+  static EvalException createInvalidIdentifierException(Identifier id, Set<String> symbols) {
+    if (id.getName().equals("$error$")) {
+      return new EvalException(id.getLocation(), "contains syntax error(s)", true);
+    }
+
+    EvalException e = getSpecialException(id);
+    if (e != null) {
+      return e;
+    }
+
+    String suggestion = SpellChecker.didYouMean(id.getName(), symbols);
+    return new EvalException(
+        id.getLocation(), "name '" + id.getName() + "' is not defined" + suggestion);
+  }
+
+  private static Object evalComprehension(Environment env, Comprehension comp)
+      throws EvalException, InterruptedException {
+    final SkylarkDict<Object, Object> dict = comp.isDict() ? SkylarkDict.of(env) : null;
+    final ArrayList<Object> list = comp.isDict() ? null : new ArrayList<>();
+
+    // The Lambda class serves as a recursive lambda closure.
+    class Lambda {
+      // execClauses(index) recursively executes the clauses starting at index,
+      // and finally evaluates the body and adds its value to the result.
+      void execClauses(int index) throws EvalException, InterruptedException {
+        // recursive case: one or more clauses
+        if (index < comp.getClauses().size()) {
+          Comprehension.Clause clause = comp.getClauses().get(index);
+          if (clause instanceof Comprehension.For) {
+            Comprehension.For forClause = (Comprehension.For) clause;
+
+            Object iterable = eval(env, forClause.getIterable());
+            Location loc = comp.getLocation();
+            Iterable<?> listValue = EvalUtils.toIterable(iterable, loc, env);
+            EvalUtils.lock(iterable, loc);
+            try {
+              for (Object elem : listValue) {
+                Eval.assign(forClause.getVars(), elem, env, loc);
+                execClauses(index + 1);
+              }
+            } finally {
+              EvalUtils.unlock(iterable, loc);
+            }
+
+          } else {
+            Comprehension.If ifClause = (Comprehension.If) clause;
+            if (EvalUtils.toBoolean(eval(env, ifClause.getCondition()))) {
+              execClauses(index + 1);
+            }
+          }
+          return;
+        }
+
+        // base case: evaluate body and add to result.
+        if (dict != null) {
+          DictExpression.Entry body = (DictExpression.Entry) comp.getBody();
+          Object k = eval(env, body.getKey());
+          EvalUtils.checkValidDictKey(k, env);
+          Object v = eval(env, body.getValue());
+          dict.put(k, v, comp.getLocation(), env);
+        } else {
+          list.add(eval(env, ((Expression) comp.getBody())));
+        }
+      }
+    }
+    new Lambda().execClauses(0);
+
+    // Undefine loop variables (remove them from the environment).
+    // This code is useful for the transition, to make sure no one relies on the old behavior
+    // (where loop variables were leaking).
+    // TODO(laurentlb): Instead of removing variables, we should create them in a nested scope.
+    for (Comprehension.Clause clause : comp.getClauses()) {
+      // Check if a loop variable conflicts with another local variable.
+      if (clause instanceof Comprehension.For) {
+        for (Identifier ident :
+            Identifier.boundIdentifiers(((Comprehension.For) clause).getVars())) {
+          env.removeLocalBinding(ident.getName());
+        }
+      }
+    }
+
+    return comp.isDict() ? dict : SkylarkList.MutableList.copyOf(env, list);
+  }
+
+  /** Returns an exception which should be thrown instead of the original one. */
+  private static EvalException maybeTransformException(Node node, EvalException original) {
+    // If there is already a non-empty stack trace, we only add this node iff it describes a
+    // new scope (e.g. FuncallExpression).
+    if (original instanceof EvalExceptionWithStackTrace) {
+      EvalExceptionWithStackTrace real = (EvalExceptionWithStackTrace) original;
+      if (node.isNewScope()) {
+        real.registerNode(node);
+      }
+      return real;
+    }
+
+    if (original.canBeAddedToStackTrace()) {
+      return new EvalExceptionWithStackTrace(original, node);
+    } else {
+      return original;
+    }
+  }
+
+  /** Throws the correct error message if the result is null depending on the objValue. */
+  // TODO(adonovan): inline sole call and simplify.
+  private static Object checkResult(
+      Object objValue, Object result, String name, Location loc, StarlarkSemantics semantics)
+      throws EvalException {
+    if (result != null) {
+      return result;
+    }
+    throw EvalUtils.getMissingFieldException(objValue, name, loc, semantics, "field");
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 5208d513..27e9a84 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -26,15 +26,17 @@
 import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
+import com.google.devtools.build.lib.util.SpellChecker;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 
-/**
- * Utilities used by the evaluator.
- */
+/** Utilities used by the evaluator. */
+// TODO(adonovan): rename this class to Starlark. Its API should contain all the fundamental values
+// and operators of the language: None, len, truth, str, iterate, equal, compare, getattr, index,
+// slice, parse, exec, eval, and so on.
 public final class EvalUtils {
 
   private EvalUtils() {}
@@ -608,4 +610,97 @@
   public static void setDebugger(Debugger dbg) {
     Eval.setDebugger(dbg);
   }
+
+  /** Returns the named field or method of the specified object. */
+  static Object getAttr(Environment env, Location loc, Object object, String name)
+      throws EvalException, InterruptedException {
+    MethodDescriptor method =
+        object instanceof Class<?>
+            ? FuncallExpression.getMethod(env.getSemantics(), (Class<?>) object, name)
+            : FuncallExpression.getMethod(env.getSemantics(), object.getClass(), name);
+    if (method != null && method.isStructField()) {
+      return method.call(
+          object,
+          FuncallExpression.extraInterpreterArgs(method, /*ast=*/ null, loc, env).toArray(),
+          loc,
+          env);
+    }
+
+    if (object instanceof SkylarkClassObject) {
+      try {
+        return ((SkylarkClassObject) object).getValue(loc, env.getSemantics(), name);
+      } catch (IllegalArgumentException ex) { // TODO(adonovan): why necessary?
+        throw new EvalException(loc, ex);
+      }
+    }
+
+    if (object instanceof ClassObject) {
+      Object result = null;
+      try {
+        result = ((ClassObject) object).getValue(name);
+      } catch (IllegalArgumentException ex) {
+        throw new EvalException(loc, ex);
+      }
+      // ClassObjects may have fields that are annotated with @SkylarkCallable.
+      // Since getValue() does not know about those, we cannot expect that result is a valid object.
+      if (result != null) {
+        result = SkylarkType.convertToSkylark(result, env);
+        // If we access NestedSets using ClassObject.getValue() we won't know the generic type,
+        // so we have to disable it. This should not happen.
+        SkylarkType.checkTypeAllowedInSkylark(result, loc);
+        return result;
+      }
+    }
+    if (method != null) {
+      return new BuiltinCallable(object, name);
+    }
+    return null;
+  }
+
+  static EvalException getMissingFieldException(
+      Object object, String name, Location loc, StarlarkSemantics semantics, String accessName) {
+    String suffix = "";
+    EvalException toSuppress = null;
+    if (object instanceof ClassObject) {
+      String customErrorMessage = ((ClassObject) object).getErrorMessageForUnknownField(name);
+      if (customErrorMessage != null) {
+        return new EvalException(loc, customErrorMessage);
+      }
+      try {
+        suffix = SpellChecker.didYouMean(name, ((ClassObject) object).getFieldNames());
+      } catch (EvalException ee) {
+        toSuppress = ee;
+      }
+    } else {
+      suffix =
+          SpellChecker.didYouMean(
+              name,
+              FuncallExpression.getStructFieldNames(
+                  semantics, object instanceof Class ? (Class<?>) object : object.getClass()));
+    }
+    if (suffix.isEmpty() && hasMethod(semantics, object, name)) {
+      // If looking up the field failed, then we know that this method must have struct_field=false
+      suffix = ", however, a method of that name exists";
+    }
+    EvalException ee =
+        new EvalException(
+            loc,
+            String.format(
+                "object of type '%s' has no %s '%s'%s",
+                EvalUtils.getDataTypeName(object), accessName, name, suffix));
+    if (toSuppress != null) {
+      ee.addSuppressed(toSuppress);
+    }
+    return ee;
+  }
+
+  /** Returns whether the given object has a method with the given name. */
+  static boolean hasMethod(StarlarkSemantics semantics, Object object, String name) {
+    Class<?> cls = object instanceof Class ? (Class<?>) object : object.getClass();
+    if (Runtime.getBuiltinRegistry().getFunctionNames(cls).contains(name)) {
+      return true;
+    }
+
+    return FuncallExpression.getMethodNames(semantics, cls).contains(name);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
index 807f8d1..5368e2e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
@@ -47,54 +47,6 @@
     UNARY_OPERATOR,
   }
 
-  /**
-   * Returns the result of evaluating this build-language expression in the
-   * specified environment. All BUILD language datatypes are mapped onto the
-   * corresponding Java types as follows:
-   *
-   * <pre>
-   *    int   -> Integer
-   *    float -> Double          (currently not generated by the grammar)
-   *    str   -> String
-   *    [...] -> List&lt;Object>    (mutable)
-   *    (...) -> List&lt;Object>    (immutable)
-   *    {...} -> Map&lt;Object, Object>
-   *    func  -> Function
-   * </pre>
-   *
-   * @return the result of evaluting the expression: a Java object corresponding
-   *         to a datatype in the BUILD language.
-   * @throws EvalException if the expression could not be evaluated.
-   * @throws InterruptedException may be thrown in a sub class.
-   */
-  public final Object eval(Environment env) throws EvalException, InterruptedException {
-    try {
-      if (Callstack.enabled) {
-        Callstack.push(this);
-      }
-      try {
-        return doEval(env);
-      } catch (EvalException ex) {
-        throw maybeTransformException(ex);
-      }
-    } finally {
-      if (Callstack.enabled) {
-        Callstack.pop();
-      }
-    }
-  }
-
-  /**
-   * Evaluates the expression and returns the result.
-   *
-   * <p>This method is only invoked by the super class {@link Expression} when calling {@link
-   * #eval(Environment)}.
-   *
-   * @throws EvalException if the expression could not be evaluated
-   * @throws InterruptedException may be thrown in a sub class.
-   */
-  abstract Object doEval(Environment env) throws EvalException, InterruptedException;
-
   @Override
   public final void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     prettyPrint(buffer);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index 3b45142..d4e0ed0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -451,8 +451,8 @@
   /**
    * Invokes the given structField=true method and returns the result.
    *
-   * <p>The given method must <b>not</b> require extra-interpreter parameters, such as
-   * {@link Environment}. This method throws {@link IllegalArgumentException} for violations.</p>
+   * <p>The given method must <b>not</b> require extra-interpreter parameters, such as {@link
+   * Environment}. This method throws {@link IllegalArgumentException} for violations.
    *
    * @param methodDescriptor the descriptor of the method to invoke
    * @param fieldName the name of the struct field
@@ -460,6 +460,7 @@
    * @return the method return value
    * @throws EvalException if there was an issue evaluating the method
    */
+  // TODO(adonovan): Where does this belong? Clearly not here, but EvalUtils? MethodDescriptor?
   public static Object invokeStructField(
       MethodDescriptor methodDescriptor, String fieldName, Object obj)
       throws EvalException, InterruptedException {
@@ -489,12 +490,13 @@
    *     example, if any arguments are of unexpected type, or not all mandatory parameters are
    *     specified by the user
    */
-  public Object[] convertStarlarkArgumentsToJavaMethodArguments(
+  static Object[] convertStarlarkArgumentsToJavaMethodArguments(
+      Environment environment,
+      FuncallExpression call,
       MethodDescriptor method,
       Class<?> objClass,
       List<Object> args,
-      Map<String, Object> kwargs,
-      Environment environment)
+      Map<String, Object> kwargs)
       throws EvalException {
     Preconditions.checkArgument(!method.isStructField(),
         "struct field methods should be handled by DotExpression separately");
@@ -531,6 +533,7 @@
         value = args.get(argIndex);
         if (!type.contains(value)) {
           throw argumentMismatchException(
+              call,
               String.format(
                   "expected value of type '%s' for parameter '%s'", type, param.getName()),
               method,
@@ -538,6 +541,7 @@
         }
         if (param.isNamed() && keys.contains(param.getName())) {
           throw argumentMismatchException(
+              call,
               String.format("got multiple values for keyword argument '%s'", param.getName()),
               method,
               objClass);
@@ -549,6 +553,7 @@
           value = kwargs.get(param.getName());
           if (!type.contains(value)) {
             throw argumentMismatchException(
+                call,
                 String.format(
                     "expected value of type '%s' for parameter '%s'", type, param.getName()),
                 method,
@@ -556,7 +561,7 @@
           }
         } else { // Param not specified by user. Use default value.
           if (param.getDefaultValue().isEmpty()) {
-            throw unspecifiedParameterException(param, method, objClass, kwargs);
+            throw unspecifiedParameterException(call, param, method, objClass, kwargs);
           }
           value =
               SkylarkSignatureProcessor.getDefaultValue(
@@ -565,7 +570,10 @@
       }
       if (!param.isNoneable() && value instanceof NoneType) {
         throw argumentMismatchException(
-            String.format("parameter '%s' cannot be None", param.getName()), method, objClass);
+            call,
+            String.format("parameter '%s' cannot be None", param.getName()),
+            method,
+            objClass);
       }
       builder.add(value);
     }
@@ -581,6 +589,7 @@
         extraArgs = extraArgsBuilder.build();
       } else {
         throw argumentMismatchException(
+            call,
             String.format(
                 "expected no more than %s positional arguments, but got %s", argIndex, args.size()),
             method,
@@ -597,7 +606,7 @@
         }
         extraKwargs = extraKwargsBuilder.build();
       } else {
-        throw unexpectedKeywordArgumentException(keys, method, objClass, environment);
+        throw unexpectedKeywordArgumentException(call, keys, method, objClass, environment);
       }
     }
 
@@ -608,28 +617,34 @@
     if (acceptsExtraKwargs) {
       builder.add(SkylarkDict.copyOf(environment, extraKwargs));
     }
-    appendExtraInterpreterArgs(builder, method, this, getLocation(), environment);
+    appendExtraInterpreterArgs(builder, method, call, call.getLocation(), environment);
 
     return builder.toArray();
   }
 
-  private EvalException unspecifiedParameterException(
+  private static EvalException unspecifiedParameterException(
+      FuncallExpression call,
       ParamDescriptor param,
       MethodDescriptor method,
       Class<?> objClass,
       Map<String, Object> kwargs) {
     if (kwargs.containsKey(param.getName())) {
       return argumentMismatchException(
+          call,
           String.format("parameter '%s' may not be specified by name", param.getName()),
           method,
           objClass);
     } else {
       return argumentMismatchException(
-          String.format("parameter '%s' has no default value", param.getName()), method, objClass);
+          call,
+          String.format("parameter '%s' has no default value", param.getName()),
+          method,
+          objClass);
     }
   }
 
-  private EvalException unexpectedKeywordArgumentException(
+  private static EvalException unexpectedKeywordArgumentException(
+      FuncallExpression call,
       Set<String> unexpectedKeywords,
       MethodDescriptor method,
       Class<?> objClass,
@@ -643,14 +658,14 @@
         // If the flag is True, it must be a deprecation flag. Otherwise it's an experimental flag.
         if (env.getSemantics().flagValue(flagIdentifier)) {
           return new EvalException(
-              getLocation(),
+              call.getLocation(),
               String.format(
                   "parameter '%s' is deprecated and will be removed soon. It may be "
                       + "temporarily re-enabled by setting --%s=false",
                   param.getName(), flagIdentifier.getFlagName()));
         } else {
           return new EvalException(
-              getLocation(),
+              call.getLocation(),
               String.format(
                   "parameter '%s' is experimental and thus unavailable with the current "
                       + "flags. It may be enabled by setting --%s",
@@ -660,6 +675,7 @@
     }
 
     return argumentMismatchException(
+        call,
         String.format(
             "unexpected keyword%s %s",
             unexpectedKeywords.size() > 1 ? "s" : "",
@@ -668,17 +684,20 @@
         objClass);
   }
 
-  private EvalException argumentMismatchException(
-      String errorDescription, MethodDescriptor methodDescriptor, Class<?> objClass) {
+  private static EvalException argumentMismatchException(
+      FuncallExpression call,
+      String errorDescription,
+      MethodDescriptor methodDescriptor,
+      Class<?> objClass) {
     if (methodDescriptor.isSelfCall() || SkylarkInterfaceUtils.hasSkylarkGlobalLibrary(objClass)) {
       return new EvalException(
-          getLocation(),
+          call.getLocation(),
           String.format(
               "%s, for call to function %s",
               errorDescription, formatMethod(objClass, methodDescriptor)));
     } else {
       return new EvalException(
-          getLocation(),
+          call.getLocation(),
           String.format(
               "%s, for call to method %s of '%s'",
               errorDescription,
@@ -687,9 +706,10 @@
     }
   }
 
-  private EvalException missingMethodException(Class<?> objClass, String methodName) {
+  private static EvalException missingMethodException(
+      FuncallExpression call, Class<?> objClass, String methodName) {
     return new EvalException(
-        getLocation(),
+        call.getLocation(),
         String.format(
             "type '%s' has no method %s()",
             EvalUtils.getDataTypeNameFromClass(objClass), methodName));
@@ -702,7 +722,7 @@
    * <p>This method accepts null {@code ast} only if {@code callable.useAst()} is false. It is up to
    * the caller to validate this invariant.
    */
-  public static List<Object> extraInterpreterArgs(
+  static List<Object> extraInterpreterArgs(
       MethodDescriptor method, @Nullable FuncallExpression ast, Location loc, Environment env) {
     List<Object> builder = new ArrayList<>();
     appendExtraInterpreterArgs(builder, method, ast, loc, env);
@@ -813,18 +833,17 @@
   }
 
   /**
-   * Evaluate this FuncallExpression's arguments, and put the resulting evaluated expressions
-   * into the given {@code posargs} and {@code kwargs} collections.
+   * Evaluate this FuncallExpression's arguments, and put the resulting evaluated expressions into
+   * the given {@code posargs} and {@code kwargs} collections.
    *
    * @param posargs a list to which all positional arguments will be added
-   * @param kwargs a mutable map to which all keyword arguments will be added. A mutable map
-   *     is used here instead of an immutable map builder to deal with duplicates
-   *     without memory overhead
+   * @param kwargs a mutable map to which all keyword arguments will be added. A mutable map is used
+   *     here instead of an immutable map builder to deal with duplicates without memory overhead
    * @param env the current environment
    */
   @SuppressWarnings("unchecked")
-  private void evalArguments(
-      List<Object> posargs, Map<String, Object> kwargs, Environment env)
+  private static void evalArguments(
+      Environment env, FuncallExpression call, List<Object> posargs, Map<String, Object> kwargs)
       throws EvalException, InterruptedException {
 
     // Optimize allocations for the common case where they are no duplicates.
@@ -835,15 +854,15 @@
     // which should be the only place that build FuncallExpression-s.
     // Argument lists are typically short and functions are frequently called, so go by index
     // (O(1) for ImmutableList) to avoid the iterator overhead.
-    for (int i = 0; i < arguments.size(); i++) {
-      Argument.Passed arg = arguments.get(i);
-      Object value = arg.getValue().eval(env);
+    for (int i = 0; i < call.arguments.size(); i++) {
+      Argument.Passed arg = call.arguments.get(i);
+      Object value = Eval.eval(env, arg.getValue());
       if (arg.isPositional()) {
         posargs.add(value);
       } else if (arg.isStar()) { // expand the starArg
         if (!(value instanceof Iterable)) {
           throw new EvalException(
-              getLocation(),
+              call.getLocation(),
               "argument after * must be an iterable, not " + EvalUtils.getDataTypeName(value));
         }
         for (Object starArgUnit : (Iterable<Object>) value) {
@@ -851,7 +870,7 @@
         }
       } else if (arg.isStarStar()) { // expand the kwargs
         ImmutableList<String> duplicates =
-            addKeywordArgsAndReturnDuplicates(kwargs, value, getLocation());
+            addKeywordArgsAndReturnDuplicates(kwargs, value, call.getLocation());
         if (duplicates != null) {
           if (duplicatesBuilder == null) {
             duplicatesBuilder = ImmutableList.builder();
@@ -870,24 +889,27 @@
     if (duplicatesBuilder != null) {
       ImmutableList<String> dups = duplicatesBuilder.build();
       throw new EvalException(
-          getLocation(),
+          call.getLocation(),
           "duplicate keyword"
               + (dups.size() > 1 ? "s" : "")
               + " '"
               + Joiner.on("', '").join(dups)
               + "' in call to "
-              + function);
+              + call.function);
     }
   }
 
+  // TODO(adonovan): move to EvalUtils.
   @VisibleForTesting
   public static boolean isNamespace(Class<?> classObject) {
     return classObject.isAnnotationPresent(SkylarkModule.class)
         && classObject.getAnnotation(SkylarkModule.class).namespace();
   }
 
-  @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
+  // TODO(adonovan): move the basic call operation to EvalUtils.call,
+  // once subexpression evaluation has been moved to Eval.
+  static Object call(Environment env, FuncallExpression call)
+      throws EvalException, InterruptedException {
     // This is a hack which provides some performance improvement over the alternative:
     // Consider 'foo.bar()'. Without this hack, the parser would evaluate the DotExpression
     // 'foo.bar' first, determine that 'foo.bar' is a callable object, and then invoke the
@@ -895,28 +917,29 @@
     // new function object to represent 'foo.bar' *just* so it could invoke method 'bar' of 'foo'.
     // Constructing throwaway function objects would be a performance hit, so instead this code
     // effectively 'looks ahead' to invoke an object method directly.
-    if (function instanceof DotExpression) {
-      return invokeObjectMethod(env, (DotExpression) function);
+    if (call.function instanceof DotExpression) {
+      return invokeObjectMethod(env, call, (DotExpression) call.function);
     }
-    Object funcValue = function.eval(env);
+    Object funcValue = Eval.eval(env, call.function);
     ArrayList<Object> posargs = new ArrayList<>();
     Map<String, Object> kwargs = new LinkedHashMap<>();
-    evalArguments(posargs, kwargs, env);
-    return callFunction(funcValue, posargs, kwargs, env);
+    evalArguments(env, call, posargs, kwargs);
+    return callFunction(env, call, funcValue, posargs, kwargs);
   }
 
   /** Invokes object.function() and returns the result. */
-  private Object invokeObjectMethod(Environment env, DotExpression dot)
+  private static Object invokeObjectMethod(
+      Environment env, FuncallExpression call, DotExpression dot)
       throws EvalException, InterruptedException {
-    Object objValue = dot.getObject().eval(env);
+    Object objValue = Eval.eval(env, dot.getObject());
     String methodName = dot.getField().getName();
     ArrayList<Object> posargs = new ArrayList<>();
     Map<String, Object> kwargs = new LinkedHashMap<>();
-    evalArguments(posargs, kwargs, env);
+    evalArguments(env, call, posargs, kwargs);
 
     // Case 1: Object is a String. String is an unusual special case.
     if (objValue instanceof String) {
-      return callStringMethod((String) objValue, methodName, posargs, kwargs, env);
+      return callStringMethod(env, call, (String) objValue, methodName, posargs, kwargs);
     }
 
     // Case 2: Object is a Java object with a matching @SkylarkCallable method.
@@ -926,9 +949,10 @@
     MethodDescriptor methodDescriptor =
         FuncallExpression.getMethod(env.getSemantics(), objValue.getClass(), methodName);
     if (methodDescriptor != null && !methodDescriptor.isStructField()) {
-      Object[] javaArguments = convertStarlarkArgumentsToJavaMethodArguments(
-          methodDescriptor, objValue.getClass(), posargs, kwargs, env);
-      return methodDescriptor.call(objValue, javaArguments, getLocation(), env);
+      Object[] javaArguments =
+          convertStarlarkArgumentsToJavaMethodArguments(
+              env, call, methodDescriptor, objValue.getClass(), posargs, kwargs);
+      return methodDescriptor.call(objValue, javaArguments, call.getLocation(), env);
     }
 
     // Case 3: Object is a function registered with the BuiltinRegistry.
@@ -938,69 +962,85 @@
         Runtime.getBuiltinRegistry().getFunction(objValue.getClass(), methodName);
     if (legacyRuntimeFunction != null) {
       return callLegacyBuiltinRegistryFunction(
-          legacyRuntimeFunction, objValue, posargs, kwargs, env);
+          call, legacyRuntimeFunction, objValue, posargs, kwargs, env);
     }
 
     // Case 4: All other cases. Evaluate "foo.bar" as a dot expression, then try to invoke it
     // as a callable.
-    Object functionObject = DotExpression.eval(objValue, methodName, dot.getLocation(), env);
+    Object functionObject = EvalUtils.getAttr(env, dot.getLocation(), objValue, methodName);
     if (functionObject == null) {
-      throw missingMethodException(objValue.getClass(), methodName);
+      throw missingMethodException(call, objValue.getClass(), methodName);
     } else {
-      return callFunction(functionObject, posargs, kwargs, env);
+      return callFunction(env, call, functionObject, posargs, kwargs);
     }
   }
 
-  private Object callLegacyBuiltinRegistryFunction(BaseFunction legacyRuntimeFunction,
-      Object objValue, ArrayList<Object> posargs, Map<String, Object> kwargs, Environment env)
+  private static Object callLegacyBuiltinRegistryFunction(
+      FuncallExpression call,
+      BaseFunction legacyRuntimeFunction,
+      Object objValue,
+      ArrayList<Object> posargs,
+      Map<String, Object> kwargs,
+      Environment env)
       throws EvalException, InterruptedException {
     if (!isNamespace(objValue.getClass())) {
       posargs.add(0, objValue);
     }
-    return legacyRuntimeFunction.call(posargs, kwargs, this, env);
+    return legacyRuntimeFunction.call(posargs, kwargs, call, env);
   }
 
-  private Object callStringMethod(String objValue, String methodName,
-      ArrayList<Object> posargs, Map<String, Object> kwargs, Environment env)
+  private static Object callStringMethod(
+      Environment env,
+      FuncallExpression call,
+      String objValue,
+      String methodName,
+      ArrayList<Object> posargs,
+      Map<String, Object> kwargs)
       throws InterruptedException, EvalException {
     // String is a special case, since it can't be subclassed. Methods on strings defer
     // to StringModule, and thus need to include the actual string as a 'self' parameter.
     posargs.add(0, objValue);
 
-    MethodDescriptor method = getMethod(env.getSemantics(), StringModule.class, methodName);
+    MethodDescriptor method =
+        FuncallExpression.getMethod(env.getSemantics(), StringModule.class, methodName);
     if (method == null) {
-      throw missingMethodException(StringModule.class, methodName);
+      throw missingMethodException(call, StringModule.class, methodName);
     }
 
-    Object[] javaArguments = convertStarlarkArgumentsToJavaMethodArguments(
-        method, StringModule.class, posargs, kwargs, env);
-    return method.call(
-        StringModule.INSTANCE, javaArguments, getLocation(), env);
+    Object[] javaArguments =
+        convertStarlarkArgumentsToJavaMethodArguments(
+            env, call, method, StringModule.class, posargs, kwargs);
+    return method.call(StringModule.INSTANCE, javaArguments, call.getLocation(), env);
   }
 
-  private Object callFunction(
-      Object fn, ArrayList<Object> posargs, Map<String, Object> kwargs, Environment env)
+  private static Object callFunction(
+      Environment env,
+      FuncallExpression call,
+      Object fn,
+      ArrayList<Object> posargs,
+      Map<String, Object> kwargs)
       throws EvalException, InterruptedException {
 
     if (fn instanceof StarlarkCallable) {
       StarlarkCallable callable = (StarlarkCallable) fn;
-      return callable.call(posargs, ImmutableMap.copyOf(kwargs), this, env);
+      return callable.call(posargs, ImmutableMap.copyOf(kwargs), call, env);
     } else if (hasSelfCallMethod(env.getSemantics(), fn.getClass())) {
       MethodDescriptor descriptor = getSelfCallMethodDescriptor(env.getSemantics(), fn);
       Object[] javaArguments =
           convertStarlarkArgumentsToJavaMethodArguments(
-              descriptor, fn.getClass(), posargs, kwargs, env);
-      return descriptor.call(fn, javaArguments, getLocation(), env);
+              env, call, descriptor, fn.getClass(), posargs, kwargs);
+      return descriptor.call(fn, javaArguments, call.getLocation(), env);
     } else {
       throw new EvalException(
-          getLocation(), "'" + EvalUtils.getDataTypeName(fn) + "' object is not callable");
+          call.getLocation(), "'" + EvalUtils.getDataTypeName(fn) + "' object is not callable");
     }
   }
 
   /**
-   * Returns the value of the argument 'name' (or null if there is none).
-   * This function is used to associate debugging information to rules created by skylark "macros".
+   * Returns the value of the argument 'name' (or null if there is none). This function is used to
+   * associate debugging information to rules created by skylark "macros".
    */
+  // TODO(adonovan): move this into sole caller.
   @Nullable
   public String getNameArg() {
     for (Argument.Passed arg : arguments) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index 457ad19..65c924c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -16,9 +16,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.util.SpellChecker;
 import java.io.IOException;
-import java.util.Set;
 import javax.annotation.Nullable;
 
 // TODO(bazel-team): For performance, avoid doing HashMap lookups at runtime, and compile local
@@ -50,6 +48,10 @@
     return name.startsWith("_");
   }
 
+  ValidationEnvironment.Scope getScope() {
+    return scope;
+  }
+
   @Override
   public void prettyPrint(Appendable buffer) throws IOException {
     buffer.append(name);
@@ -61,46 +63,6 @@
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException {
-    Object result;
-    if (scope == null) {
-      // Legacy behavior, to be removed.
-      result = env.lookup(name);
-      if (result == null) {
-        throw createInvalidIdentifierException(env.getVariableNames());
-      }
-      return result;
-    }
-
-    switch (scope) {
-      case Local:
-        result = env.localLookup(name);
-        break;
-      case Module:
-        result = env.moduleLookup(name);
-        break;
-      case Universe:
-        result = env.universeLookup(name);
-        break;
-      default:
-        throw new IllegalStateException(scope.toString());
-    }
-
-    if (result == null) {
-      // Since Scope was set, we know that the variable is defined in the scope.
-      // However, the assignment was not yet executed.
-      EvalException e = getSpecialException();
-      throw e != null
-          ? e
-          : new EvalException(
-              getLocation(),
-              scope.getQualifier() + " variable '" + name + "' is referenced before assignment.");
-    }
-
-    return result;
-  }
-
-  @Override
   public void accept(NodeVisitor visitor) {
     visitor.visit(this);
   }
@@ -110,41 +72,8 @@
     return Kind.IDENTIFIER;
   }
 
-  /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */
-  private EvalException getSpecialException() {
-    if (name.equals("PACKAGE_NAME")) {
-      return new EvalException(
-          getLocation(),
-          "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', "
-              + "please use the latter ("
-              + "https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name). ");
-    }
-    if (name.equals("REPOSITORY_NAME")) {
-      return new EvalException(
-          getLocation(),
-          "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', please"
-              + " use the latter ("
-              + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name).");
-    }
-    return null;
-  }
-
-  EvalException createInvalidIdentifierException(Set<String> symbols) {
-    if (name.equals("$error$")) {
-      return new EvalException(getLocation(), "contains syntax error(s)", true);
-    }
-
-    EvalException e = getSpecialException();
-    if (e != null) {
-      return e;
-    }
-
-    String suggestion = SpellChecker.didYouMean(name, symbols);
-    return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion);
-  }
-
   /** @return The {@link Identifier} of the provided name. */
-  public static Identifier of(String name) {
+  static Identifier of(String name) {
     return new Identifier(name);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
index 8044770..3564be6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
@@ -48,16 +48,12 @@
     buffer.append(']');
   }
 
-  @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    return evaluate(object.eval(env), key.eval(env), env, getLocation());
-  }
-
   /**
    * Retrieves the value associated with a key in the given object.
    *
    * @throws EvalException if {@code object} is not a list or dictionary
    */
+  // TODO(adonovan): move to EvalUtils.index.
   public static Object evaluate(Object object, Object key, Environment env, Location loc)
       throws EvalException, InterruptedException {
     if (object instanceof SkylarkIndexable) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
index 2cb85c1..b1bee68 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
@@ -28,11 +28,6 @@
   }
 
   @Override
-  Object doEval(Environment env) {
-    return value;
-  }
-
-  @Override
   public void prettyPrint(Appendable buffer) throws IOException {
     buffer.append(String.valueOf(value));
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java
index 371fafc..28b4bbe 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java
@@ -13,10 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.syntax;
 
-import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
-import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.List;
 
 /** Syntax node for list and tuple expressions. */
@@ -65,19 +62,6 @@
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    ArrayList<Object> result = new ArrayList<>(elements.size());
-    for (Expression element : elements) {
-      // Convert NPEs to EvalExceptions.
-      if (element == null) {
-        throw new EvalException(getLocation(), "null expression in " + this);
-      }
-      result.add(element.eval(env));
-    }
-    return isTuple() ? Tuple.copyOf(result) : MutableList.wrapUnsafe(env, result);
-  }
-
-  @Override
   public void accept(NodeVisitor visitor) {
     visitor.visit(this);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
index 21e1336..7d991b1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
@@ -752,7 +752,7 @@
       return true;
     }
     // shouldn't this filter things with struct_field = false?
-    return DotExpression.hasMethod(env.getSemantics(), obj, name);
+    return EvalUtils.hasMethod(env.getSemantics(), obj, name);
   }
 
   @SkylarkCallable(
@@ -789,12 +789,12 @@
       useEnvironment = true)
   public Object getAttr(Object obj, String name, Object defaultValue, Location loc, Environment env)
       throws EvalException, InterruptedException {
-    Object result = DotExpression.eval(obj, name, loc, env);
+    Object result = EvalUtils.getAttr(env, loc, obj, name);
     if (result == null) {
       if (defaultValue != Runtime.UNBOUND) {
         return defaultValue;
       }
-      throw DotExpression.getMissingFieldException(obj, name, loc, env.getSemantics(), "attribute");
+      throw EvalUtils.getMissingFieldException(obj, name, loc, env.getSemantics(), "attribute");
     }
     return result;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Node.java b/src/main/java/com/google/devtools/build/lib/syntax/Node.java
index f14adfe..76c17a1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Node.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Node.java
@@ -33,25 +33,6 @@
     return false;
   }
 
-  /** Returns an exception which should be thrown instead of the original one. */
-  protected final EvalException maybeTransformException(EvalException original) {
-    // If there is already a non-empty stack trace, we only add this node iff it describes a
-    // new scope (e.g. FuncallExpression).
-    if (original instanceof EvalExceptionWithStackTrace) {
-      EvalExceptionWithStackTrace real = (EvalExceptionWithStackTrace) original;
-      if (isNewScope()) {
-        real.registerNode(this);
-      }
-      return real;
-    }
-
-    if (original.canBeAddedToStackTrace()) {
-      return new EvalExceptionWithStackTrace(original, this);
-    } else {
-      return original;
-    }
-  }
-
   @VisibleForTesting  // productionVisibility = Visibility.PACKAGE_PRIVATE
   public void setLocation(Location location) {
     this.location = location;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
index f2f3da7..06184a0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
@@ -13,9 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.syntax;
 
-import com.google.devtools.build.lib.events.Location;
 import java.io.IOException;
-import java.util.List;
 import javax.annotation.Nullable;
 
 /** Syntax node for a slice expression, e.g. obj[:len(obj):2]. */
@@ -70,41 +68,6 @@
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    Object objValue = object.eval(env);
-    Object startValue = start == null ? Runtime.NONE : start.eval(env);
-    Object endValue = end == null ? Runtime.NONE : end.eval(env);
-    Object stepValue = step == null ? Runtime.NONE : step.eval(env);
-    Location loc = getLocation();
-
-    if (objValue instanceof SkylarkList) {
-      return ((SkylarkList<?>) objValue).getSlice(
-          startValue, endValue, stepValue, loc, env.mutability());
-    } else if (objValue instanceof String) {
-      String string = (String) objValue;
-      List<Integer> indices = EvalUtils.getSliceIndices(startValue, endValue, stepValue,
-          string.length(), loc);
-      char[] result = new char[indices.size()];
-      char[] original = ((String) objValue).toCharArray();
-      int resultIndex = 0;
-      for (int originalIndex : indices) {
-        result[resultIndex] = original[originalIndex];
-        ++resultIndex;
-      }
-      return new String(result);
-    }
-
-    throw new EvalException(
-        loc,
-        String.format(
-            "type '%s' has no operator [:](%s, %s, %s)",
-            EvalUtils.getDataTypeName(objValue),
-            EvalUtils.getDataTypeName(startValue),
-            EvalUtils.getDataTypeName(endValue),
-            EvalUtils.getDataTypeName(stepValue)));
-  }
-
-  @Override
   public void accept(NodeVisitor visitor) {
     visitor.visit(this);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
index e156d4f..7ac909a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
@@ -30,13 +30,7 @@
     return value;
   }
 
-  @Override
-  Object doEval(Environment env) {
-    return value;
-  }
-
-  // TODO(adonovan): lock down, after removing last use in skyframe serialization.
-  public StringLiteral(String value) {
+  StringLiteral(String value) {
     this.value = value;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
index 78389ed..3e1ced3 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
@@ -56,7 +56,8 @@
     return (op == TokenKind.NOT ? "not " : op.toString()) + x;
   }
 
-  private static Object evaluate(TokenKind op, Object value, Location loc)
+  // TODO(adonovan): move to EvalUtils.unary.
+  static Object evaluate(TokenKind op, Object value, Location loc)
       throws EvalException, InterruptedException {
     switch (op) {
       case NOT:
@@ -94,11 +95,6 @@
   }
 
   @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    return evaluate(op, x.eval(env), getLocation());
-  }
-
-  @Override
   public void accept(NodeVisitor visitor) {
     visitor.visit(this);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index 47ad90b..4d96cd6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -185,7 +185,7 @@
             result.getEvalExceptionFromAttemptingAccess(
                 node.getLocation(), env.getSemantics(), node.getName()));
       }
-      throw new ValidationException(node.createInvalidIdentifierException(getAllSymbols()));
+      throw new ValidationException(Eval.createInvalidIdentifierException(node, getAllSymbols()));
     }
     // TODO(laurentlb): In BUILD files, calling setScope will throw an exception. This happens
     // because some AST nodes are shared across multipe ASTs (due to the prelude file).