Fixed Skylark stack trace:

- Moved registration mechanism from BaseFunction into ASTNode / Statement / Expression
- Added more details about statements/expressions to the output trace (including if's)
- Fixed wrong locations

--
MOS_MIGRATED_REVID=102841164

28 files changed, 269 insertions, 109 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
index 405abe435c..3b1a24c41e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
@@ -85,7 +85,7 @@ public final class SkylarkRuleConfiguredTargetBuilder {
       ruleContext.ruleError(e.getMessage());
       return null;
     } catch (EvalException e) {
-      addRuleToStackTrace(e, ruleContext.getRule());
+      addRuleToStackTrace(e, ruleContext.getRule(), ruleImplementation);
       // If the error was expected, return an empty target.
       if (!expectFailure.isEmpty() && getMessageWithoutStackTrace(e).matches(expectFailure)) {
         return new com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder(ruleContext)
@@ -98,11 +98,11 @@ public final class SkylarkRuleConfiguredTargetBuilder {
   }
 
   /**
-   * Adds an entry for the given rule to the stack trace of the exception (if there is one).
+   * Adds the given rule to the stack trace of the exception (if there is one).
    */
-  private static void addRuleToStackTrace(EvalException ex, Rule rule) {
+  private static void addRuleToStackTrace(EvalException ex, Rule rule, BaseFunction ruleImpl) {
     if (ex instanceof EvalExceptionWithStackTrace) {
-      ((EvalExceptionWithStackTrace) ex).registerRule(rule);
+      ((EvalExceptionWithStackTrace) ex).registerRule(rule, ruleImpl);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java b/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
index 2fa492ee99..754035ef74 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
@@ -28,6 +28,35 @@ public abstract class ASTNode implements Serializable {
 
   protected ASTNode() {}
 
+  /**
+   * Returns whether this node represents a new scope, e.g. a function call.
+   */
+  protected boolean isNewScope()  {
+    return false;
+  }
+
+  /**
+   * Returns an exception which should be thrown instead of the original one.
+   */
+  protected final EvalException handleException(Exception 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 && isNewScope()) {
+      EvalExceptionWithStackTrace real = (EvalExceptionWithStackTrace) original;
+      real.registerNode(this);
+      return real;
+    }
+
+    // If the exception is an instance of a subclass of EvalException (such as
+    // ReturnStatement.ReturnException and FlowStatement.FlowException), we just return it
+    // unchanged.
+    if (original instanceof EvalException && !original.getClass().equals(EvalException.class)) {
+      return (EvalException) original;
+    }
+
+    return new EvalExceptionWithStackTrace(original, this);
+  }
+
   @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/AssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
index 6994231ea1..3074643553 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
@@ -51,7 +51,7 @@ public final class AssignmentStatement extends Statement {
   }
 
   @Override
-  void exec(Environment env) throws EvalException, InterruptedException {
+  void doExec(Environment env) throws EvalException, InterruptedException {
     Object rvalue = expression.eval(env);
     lvalue.assign(env, getLocation(), rvalue);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index f3f7819292..450791d691 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -425,22 +425,7 @@ public abstract class BaseFunction implements Serializable {
     Object[] arguments = processArguments(args, kwargs, loc);
     canonicalizeArguments(arguments, loc);
 
-    try {
-      return call(arguments, ast, env);
-    } catch (EvalExceptionWithStackTrace ex) {
-      throw updateStackTrace(ex, loc);
-    } catch (EvalException | RuntimeException | InterruptedException ex) {
-      throw updateStackTrace(new EvalExceptionWithStackTrace(ex, loc), loc);
-    }
-  }
-
-  /**
-   * Adds an entry for the current function to the stack trace of the exception.
-   */
-  private EvalExceptionWithStackTrace updateStackTrace(
-      EvalExceptionWithStackTrace ex, Location location) {
-    ex.registerFunction(this, location);
-    return ex;
+    return call(arguments, ast, env);
   }
 
   /**
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 632f8c6007..7619d67d43 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
@@ -98,7 +98,7 @@ public final class BinaryOperatorExpression extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     Object lval = lhs.eval(env);
 
     // Short-circuit operators
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 1c43bae7a7..a4c20633e0 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,7 +48,7 @@ public final class ConditionalExpression extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     if (EvalUtils.toBoolean(condition.eval(env))) {
       return thenCase.eval(env);
     } else {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
index 3687be7dcf..6803800c9c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
@@ -53,7 +53,7 @@ public class DictComprehension extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     // We want to keep the iteration order
     LinkedHashMap<Object, Object> map = new LinkedHashMap<>();
     Iterable<?> elements = EvalUtils.toIterable(listExpression.eval(env), getLocation());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
index 5d12140c2a..832f038401 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
@@ -70,7 +70,7 @@ public class DictionaryLiteral extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     // We need LinkedHashMap to maintain the order during iteration (e.g. for loops)
     Map<Object, Object> map = new LinkedHashMap<>();
     for (DictionaryEntryLiteral entry : entries) {
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 e6ae8a01c7..f930cb3c47 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
@@ -48,7 +48,7 @@ public final class DotExpression extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     Object objValue = obj.eval(env);
     String name = field.getName();
     Object result = eval(objValue, name, getLocation());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java
index e3a1978a2b..2ab20d02bf 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.base.Joiner;
-import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Rule;
@@ -22,6 +21,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.util.Deque;
 import java.util.LinkedList;
+import java.util.Objects;
 
 /**
  * EvalException with a stack trace.
@@ -30,20 +30,22 @@ public class EvalExceptionWithStackTrace extends EvalException {
 
   private StackTraceElement mostRecentElement;
 
-  public EvalExceptionWithStackTrace(Exception original, Location callLocation) {
-    super(
-        originalLocation(original, callLocation), getNonEmptyMessage(original), getCause(original));
+  public EvalExceptionWithStackTrace(Exception original, ASTNode culprit) {
+    super(extractLocation(original, culprit), getNonEmptyMessage(original), getCause(original));
+    registerNode(culprit);
   }
 
   /**
-   * Returns the location of the {@code original} exception, or {@code callLocation}
-   * if there's none.
+   * Returns the appropriate location for this exception.
+   *
+   * <p>If the {@code ASTNode} has a valid location, this one is used. Otherwise, we try to get the
+   * location of the exception.
    */
-  private static Location originalLocation(Exception original, Location callLocation) {
-    if (!(original instanceof EvalException)) {
-      return callLocation;
+  private static Location extractLocation(Exception original, ASTNode culprit) {
+    if (culprit != null && culprit.getLocation() != null) {
+      return culprit.getLocation();
     }
-    return MoreObjects.firstNonNull(((EvalException) original).getLocation(), callLocation);
+    return (original instanceof EvalException) ? ((EvalException) original).getLocation() : null;
   }
 
   /**
@@ -57,34 +59,78 @@ public class EvalExceptionWithStackTrace extends EvalException {
   }
 
   /**
-   * Adds an entry for the given statement to the stack trace.
+   * Adds an entry for the given {@code ASTNode} to the stack trace.
    */
-  public void registerStatement(Statement statement) {
-    Preconditions.checkState(
-        mostRecentElement == null, "Cannot add a statement to a non-empty stack trace.");
-    addStackFrame(statement.toString().trim(), statement.getLocation());
+  public void registerNode(ASTNode node) {
+    addStackFrame(node.toString().trim(), node.getLocation());
   }
 
   /**
-   * Adds an entry for the given function to the stack trace.
+   * Adds the given {@code Rule} to the stack trace.
    */
-  public void registerFunction(BaseFunction function, Location location) {
-    addStackFrame(function.getFullName(), location);
+  public void registerRule(Rule rule, BaseFunction ruleImpl) {
+    /* We have to model the transition from BUILD file to bzl file manually since the stack trace
+     * mechanism cannot do that by itself (because, for example, the rule implementation does not
+     * have a corresponding FuncallExpression).
+     *
+     * Consequently, we add two new frames to the stack:
+     * 1. Rule definition
+     * 2. Rule implementation
+     *
+     * Similar to Python, all functions that were entered (except for the top-level ones) appear
+     * twice in the stack trace output. This would lead to the following trace:
+     *
+     * File BUILD, line X, in <module>
+     *     rule_definition()
+     * File BUILD, line X, in rule_definition
+     *     rule_implementation()
+     * File bzl, line Y, in rule_implementation
+     *     ...
+     *
+     * Please note that lines 3 and 4 are quite confusing since a) the transition from
+     * rule_definition to rule_implementation happens internally and b) the locations do not make
+     * any sense.
+     * Consequently, we decided to omit lines 3 and 4 from the output via canPrint = false:
+     *
+     * File BUILD, line X, in <module>
+     *     rule_definition()
+     * File bzl, line Y, in rule_implementation
+     *     ...
+     *
+     * */
+    addStackFrame(ruleImpl.getName(), ruleImpl.getLocation());
+    addStackFrame(String.format("%s(name = '%s')", rule.getRuleClass(), rule.getName()),
+        rule.getLocation(), false);
   }
 
   /**
-   * Adds an entry for the given rule to the stack trace.
+   * Adds a line for the given frame.
    */
-  public void registerRule(Rule rule) {
-    addStackFrame(
-        String.format("%s(name = '%s')", rule.getRuleClass(), rule.getName()), rule.getLocation());
+  private void addStackFrame(String label, Location location, boolean canPrint) {
+    // We have to watch out for duplicate since ExpressionStatements add themselves twice:
+    // Statement#exec() calls Expression#eval(), both of which call this method.
+    if (mostRecentElement != null && isSameLocation(location, mostRecentElement.getLocation())) {
+      return;
+    }
+    mostRecentElement = new StackTraceElement(label, location, mostRecentElement, canPrint);
   }
 
   /**
-   * Adds a line for the given frame.
+   * Checks two locations for equality in paths and start offsets.
+   *
+   * <p> LexerLocation#equals cannot be used since it cares about different end offsets.
    */
-  private void addStackFrame(String label, Location location) {
-    mostRecentElement = new StackTraceElement(label, location, mostRecentElement);
+  private boolean isSameLocation(Location first, Location second) {
+    try {
+      return Objects.equals(first.getPath(), second.getPath())
+          && Objects.equals(first.getStartOffset(), second.getStartOffset());
+    } catch (NullPointerException ex) {
+      return first == second;
+    }
+  }
+
+  private void addStackFrame(String label, Location location)   {
+    addStackFrame(label, location, true);
   }
 
   /**
@@ -101,6 +147,7 @@ public class EvalExceptionWithStackTrace extends EvalException {
 
   @Override
   public String print() {
+    // Currently, we do not limit the text length per line.
     return print(StackTracePrinter.INSTANCE);
   }
 
@@ -124,15 +171,17 @@ public class EvalExceptionWithStackTrace extends EvalException {
    * An element in the stack trace which contains the name of the offending function / rule /
    * statement and its location.
    */
-  protected final class StackTraceElement {
+  protected static final class StackTraceElement {
     private final String label;
     private final Location location;
     private final StackTraceElement cause;
+    private final boolean canPrint;
 
-    StackTraceElement(String label, Location location, StackTraceElement cause) {
+    StackTraceElement(String label, Location location, StackTraceElement cause, boolean canPrint) {
       this.label = label;
       this.location = location;
       this.cause = cause;
+      this.canPrint = canPrint;
     }
 
     String getLabel() {
@@ -146,6 +195,16 @@ public class EvalExceptionWithStackTrace extends EvalException {
     StackTraceElement getCause() {
       return cause;
     }
+
+    boolean canPrint() {
+      return canPrint;
+    }
+
+    @Override
+    public String toString() {
+      return String.format(
+          "%s @ %s -> %s", label, location, (cause == null) ? "null" : cause.toString());
+    }
   }
 
   /**
@@ -160,10 +219,16 @@ public class EvalExceptionWithStackTrace extends EvalException {
     public final String print(String message, StackTraceElement mostRecentElement) {
       Deque<String> output = new LinkedList<>();
 
+      // Adds dummy element for the rule call that uses the location of the top-most function.
+      mostRecentElement = new StackTraceElement("", mostRecentElement.getLocation(),
+          (mostRecentElement.getCause() == null) ? null : mostRecentElement, true);
+
       while (mostRecentElement != null) {
-        String entry = print(mostRecentElement);
-        if (entry != null && entry.length() > 0) {
-          addEntry(output, entry);
+        if (mostRecentElement.canPrint()) {
+          String entry = print(mostRecentElement);
+          if (entry != null && entry.length() > 0) {
+            addEntry(output, entry);
+          }
         }
 
         mostRecentElement = mostRecentElement.getCause();
@@ -174,16 +239,6 @@ public class EvalExceptionWithStackTrace extends EvalException {
     }
 
     /**
-     * Returns the location which should be shown on the same line as the label of the given
-     * element.
-     */
-    protected Location getDisplayLocation(StackTraceElement element) {
-      // If there is a rule definition in this element, it should print its own location in
-      // the BUILD file instead of using a location in a bzl file.
-      return describesRule(element) ? element.getLocation() : getLocation(element.getCause());
-    }
-
-    /**
      * Returns the location of the given element or Location.BUILTIN if the element is null.
      */
     private Location getLocation(StackTraceElement element) {
@@ -209,17 +264,31 @@ public class EvalExceptionWithStackTrace extends EvalException {
       }
 
       // Prints a two-line string, similar to Python.
-      Location location = getDisplayLocation(element);
+      Location location = getLocation(element.getCause());
       return String.format(
-          "\tFile \"%s\", line %d, in %s%n\t\t%s",
-          printPath(location.getPath()),
-          location.getStartLine(),
-          element.getLabel(),
+          "\tFile \"%s\", line %d%s%n\t\t%s",
+          printPath(location),
+          getLine(location),
+          printFunction(element.getLabel()),
           element.getCause().getLabel());
     }
 
-    private String printPath(PathFragment path) {
-      return (path == null) ? "<unknown>" : path.getPathString();
+    private String printFunction(String func) {
+      if (func.isEmpty()) {
+        return "";
+      }
+
+      int pos = func.indexOf('(');
+      return String.format(", in %s", (pos < 0) ? func : func.substring(0, pos));
+    }
+
+    private String printPath(Location loc) {
+      return (loc == null || loc.getPath() == null) ? "<unknown>" : loc.getPath().getPathString();
+    }
+
+    private int getLine(Location loc) {
+      return (loc == null || loc.getStartLineAndColumn() == null)
+          ? 0 : loc.getStartLineAndColumn().getLine();
     }
 
     /**
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 1886843e86..f23746ad7e 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
@@ -36,8 +36,26 @@ public abstract class Expression extends ASTNode {
    * @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.
    */
-  abstract Object eval(Environment env) throws EvalException, InterruptedException;
+  final Object eval(Environment env) throws EvalException, InterruptedException {
+    try {
+      return doEval(env);
+    } catch (EvalException | RuntimeException ex) {
+      throw handleException(ex);
+    }
+  }
+
+  /**
+   * 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;
 
   /**
    * Returns the inferred type of the result of the Expression.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
index f742d40895..1ccca814b8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
@@ -35,7 +35,7 @@ public final class ExpressionStatement extends Statement {
   }
 
   @Override
-  void exec(Environment env) throws EvalException, InterruptedException {
+  void doExec(Environment env) throws EvalException, InterruptedException {
     expr.eval(env);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
index 127b366dea..1af7bfad02 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
@@ -36,7 +36,7 @@ public final class FlowStatement extends Statement {
   }
 
   @Override
-  void exec(Environment env) throws EvalException {
+  void doExec(Environment env) throws EvalException {
     throw ex;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index 1660ce1e15..c5333ce1f6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -57,7 +57,7 @@ public final class ForStatement extends Statement {
   }
 
   @Override
-  void exec(Environment env) throws EvalException, InterruptedException {
+  void doExec(Environment env) throws EvalException, InterruptedException {
     Object o = collection.eval(env);
     Iterable<?> col = EvalUtils.toIterable(o, getLocation());
 
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 1e4497b85b..f47e939181 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
@@ -272,7 +272,16 @@ public final class FuncallExpression extends Expression {
     if (obj != null) {
       sb.append(obj).append(".");
     }
-    Printer.printList(sb.append(func), args, "(", ", ", ")", null);
+    sb.append(func);
+    String backup = sb.toString();
+    try {
+      Printer.printList(sb, args, "(", ", ", ")", /* singletonTerminator */ null);
+    } catch (OutOfMemoryError ex) {
+      // export_files might lead to an OOM error (e.g. in
+      // PackageSerializationTest#testMassivePackageDeserializesFine).
+      // TODO(b/23967033): make the Printer limit its own output.
+      return backup + "(<too long>)";
+    }
     return sb.toString();
   }
 
@@ -487,7 +496,7 @@ public final class FuncallExpression extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     return (obj != null) ? invokeObjectMethod(env) : invokeGlobalFunction(env);
   }
 
@@ -645,4 +654,9 @@ public final class FuncallExpression extends Expression {
           String.format("function '%s' does not exist", func.getName()));
     }
   }
+
+  @Override
+  protected boolean isNewScope() {
+    return true;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
index acc09c9859..ba9f6f4ab5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
@@ -39,7 +39,7 @@ public class FunctionDefStatement extends Statement {
   }
 
   @Override
-  void exec(Environment env) throws EvalException, InterruptedException {
+  void doExec(Environment env) throws EvalException, InterruptedException {
     List<Expression> defaultExpressions = signature.getDefaultValues();
     ArrayList<Object> defaultValues = null;
     ArrayList<SkylarkType> types = null;
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 ce68d66d60..611204cf96 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
@@ -65,7 +65,7 @@ public final class Identifier extends Expression {
   }
   
   @Override
-  Object eval(Environment env) throws EvalException {
+  Object doEval(Environment env) throws EvalException {
     try {
       return env.lookup(name);
     } catch (Environment.NoSuchVariableException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
index 3ac98171dd..8506b242df 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
@@ -39,7 +39,7 @@ public final class IfStatement extends Statement {
     }
 
     @Override
-    void exec(Environment env) throws EvalException, InterruptedException {
+    void doExec(Environment env) throws EvalException, InterruptedException {
       for (Statement stmt : stmts) {
         stmt.exec(env);
       }
@@ -97,11 +97,12 @@ public final class IfStatement extends Statement {
   public String toString() {
     // TODO(bazel-team): if we want to print the complete statement, the function
     // needs an extra argument to specify indentation level.
-    return "if : ...\n";
+    // As guaranteed by the constructor, there must be at least one element in thenBlocks.
+    return String.format("if %s:\n", thenBlocks.get(0).getCondition());
   }
 
   @Override
-  void exec(Environment env) throws EvalException, InterruptedException {
+  void doExec(Environment env) throws EvalException, InterruptedException {
     for (ConditionalStatements stmt : thenBlocks) {
       if (EvalUtils.toBoolean(stmt.getCondition().eval(env))) {
         stmt.exec(env);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
index 5dc7a1cbb0..144d00273e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
@@ -216,7 +216,7 @@ public final class ListComprehension extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     List<Object> result = new ArrayList<>();
     evalStep(env, result, 0);
     return env.isSkylark() ? SkylarkList.list(result, getLocation()) : result;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
index da6e10c00f..5e332042dc 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
@@ -98,7 +98,7 @@ public final class ListLiteral extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     List<Object> result = new ArrayList<>();
     for (Expression expr : exprs) {
       // Convert NPEs to EvalExceptions.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Literal.java b/src/main/java/com/google/devtools/build/lib/syntax/Literal.java
index 92890815cd..cdc7a99946 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Literal.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Literal.java
@@ -38,7 +38,7 @@ public abstract class Literal<T> extends Expression {
   }
 
   @Override
-  Object eval(Environment env) {
+  Object doEval(Environment env) {
     return value;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
index b5e6650b75..fc6fa77045 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
@@ -61,7 +61,7 @@ public final class LoadStatement extends Statement {
   }
 
   @Override
-  void exec(Environment env) throws EvalException, InterruptedException {
+  void doExec(Environment env) throws EvalException, InterruptedException {
     for (Map.Entry<Identifier, String> entry : symbols.entrySet()) {
       try {
         Identifier current = entry.getKey();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/NotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/NotExpression.java
index 2fc3fe0cae..fc00abb85c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/NotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/NotExpression.java
@@ -29,7 +29,7 @@ public class NotExpression extends Expression {
   }
 
   @Override
-  Object eval(Environment env) throws EvalException, InterruptedException {
+  Object doEval(Environment env) throws EvalException, InterruptedException {
     return !EvalUtils.toBoolean(expression.eval(env));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index 6956c97437..0e377eb91f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -43,7 +43,7 @@ public class ReturnStatement extends Statement {
   }
 
   @Override
-  void exec(Environment env) throws EvalException, InterruptedException {
+  void doExec(Environment env) throws EvalException, InterruptedException {
     throw new ReturnException(returnExpression.getLocation(), returnExpression.eval(env));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
index 2c99209b3e..23ad3ae909 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
@@ -23,8 +23,26 @@ public abstract class Statement extends ASTNode {
    * modified.
    *
    * @throws EvalException if execution of the statement could not be completed.
+   * @throws InterruptedException may be thrown in a sub class.
    */
-  abstract void exec(Environment env) throws EvalException, InterruptedException;
+  final void exec(Environment env) throws EvalException, InterruptedException   {
+    try {
+      doExec(env);
+    } catch (EvalException | RuntimeException ex)  {
+      throw handleException(ex);
+    }
+  }
+
+  /**
+   * Executes the statement.
+   *
+   * <p>This method is only invoked by the super class {@link Statement} when calling {@link
+   * #exec(Environment)}.
+   *
+   * @throws EvalException if execution of the statement could not be completed.
+   * @throws InterruptedException may be thrown in a sub class.
+   */
+  abstract void doExec(Environment env) throws EvalException, InterruptedException;
 
   /**
    * Checks the semantics of the Statement using the Environment according to
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
index 074f79fb48..c12aaddff5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
@@ -61,7 +61,6 @@ public class UserDefinedFunction extends BaseFunction {
 
     Profiler.instance().startTask(ProfilerTask.SKYLARK_USER_FN,
         getLocationPathAndLine() + "#" + getName());
-    Statement lastStatement = null;
     try {
       env.enterScope(this, ast, definitionGlobals);
       ImmutableList<String> names = signature.getSignature().getNames();
@@ -74,22 +73,12 @@ public class UserDefinedFunction extends BaseFunction {
 
       try {
         for (Statement stmt : statements) {
-          lastStatement = stmt;
           stmt.exec(env);
         }
       } catch (ReturnStatement.ReturnException e) {
         return e.getValue();
       }
       return Runtime.NONE;
-    } catch (EvalExceptionWithStackTrace ex) {
-      // We need this block since the next "catch" must only catch EvalExceptions that don't have a
-      // stack trace yet.
-      throw ex;
-    } catch (EvalException ex) {
-      EvalExceptionWithStackTrace real =
-          new EvalExceptionWithStackTrace(ex, lastStatement.getLocation());
-      real.registerStatement(lastStatement);
-      throw real;
     } finally {
       Profiler.instance().completeTask(ProfilerTask.SKYLARK_USER_FN);
       env.exitScope();
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
index dd2e83156a..4e71d61ce4 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
@@ -26,11 +26,12 @@ import org.junit.runners.JUnit4;
  */
 @RunWith(JUnit4.class)
 public class ExceptionTest {
+  private static final ASTNode DUMMY_NODE = new Identifier("DUMMY");
 
   @Test
   public void testEmptyMessage() throws Exception {
     EvalExceptionWithStackTrace ex =
-        new EvalExceptionWithStackTrace(new NullPointerException(), Location.BUILTIN);
+        new EvalExceptionWithStackTrace(new NullPointerException(), DUMMY_NODE);
     assertThat(ex.getMessage())
         .contains("Null Pointer: ExceptionTest.testEmptyMessage() in ExceptionTest.java:");
   }
@@ -45,7 +46,7 @@ public class ExceptionTest {
   }
 
   private void runExceptionTest(Exception toThrow, Exception expectedCause) {
-    EvalExceptionWithStackTrace ex = new EvalExceptionWithStackTrace(toThrow, Location.BUILTIN);
+    EvalExceptionWithStackTrace ex = new EvalExceptionWithStackTrace(toThrow, DUMMY_NODE);
     assertThat(ex.getCause()).isEqualTo(expectedCause);
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index 60536e36f5..c7ab9da7cc 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -42,6 +42,39 @@ public class MethodLibraryTest extends EvaluationTestCase {
   }
 
   @Test
+  public void testStackTraceLocation() throws Exception {
+    new SkylarkTest().testIfErrorContains(
+        "Traceback (most recent call last):\n\t"
+        + "File \"<unknown>\", line 8\n\t\t"
+        + "foo()\n\t"
+        + "File \"<unknown>\", line 2, in foo\n\t\t"
+        + "bar(1)\n\t"
+        + "File \"<unknown>\", line 7, in bar\n\t\t"
+        + "'test'.index(x)",
+        "def foo():",
+        "  bar(1)",
+        "def bar(x):",
+        "  if x == 1:",
+        "    a = x",
+        "    b = 2",
+        "    'test'.index(x)",
+        "foo()");
+  }
+
+  @Test
+  public void testStackTraceWithIf() throws Exception {
+    new SkylarkTest().testIfErrorContains(
+        "File \"<unknown>\", line 5\n\t\t"
+        + "foo()\n\t"
+        + "File \"<unknown>\", line 3, in foo\n\t\ts[0]",
+        "def foo():",
+        "  s = set()",
+        "  if s[0] == 1:",
+        "    x = 1",
+        "foo()");
+  }
+
+  @Test
   public void testStackTraceSkipBuiltInOnly() throws Exception {
     // The error message should not include the stack trace when there is
     // only one built-in function.
@@ -59,17 +92,20 @@ public class MethodLibraryTest extends EvaluationTestCase {
     new SkylarkTest()
         .testIfExactError(
             "Traceback (most recent call last):\n"
+                + "\tFile \"<unknown>\", line 6\n"
+                + "\t\tfoo()\n"
                 + "\tFile \"<unknown>\", line 2, in foo\n"
-                + "\t\tbar\n"
-                + "\tFile \"<unknown>\", line 4, in bar\n"
-                + "\t\tstring.index\n"
+                + "\t\tbar(1)\n"
+                + "\tFile \"<unknown>\", line 5, in bar\n"
+                + "\t\t'test'.index(x)\n"
                 + "Method string.index(sub: string, start: int, end: int or NoneType) "
                 + "is not applicable "
                 + "for arguments (int, int, NoneType): 'sub' is int, but should be string",
             "def foo():",
-            "   bar(1)",
+            "  bar(1)",
             "def bar(x):",
-            "   'test'.index(x)",
+            "  if 1 == 1:",
+            "    'test'.index(x)",
             "foo()");
   }