diff options
6 files changed, 237 insertions, 68 deletions
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 7574e1fe4e..43d27a2147 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 @@ -429,7 +429,7 @@ public abstract class BaseFunction { } catch (EvalExceptionWithStackTrace ex) { throw updateStackTrace(ex, loc); } catch (EvalException | RuntimeException | InterruptedException ex) { - throw updateStackTrace(new EvalExceptionWithStackTrace(ex, Location.BUILTIN), loc); + throw updateStackTrace(new EvalExceptionWithStackTrace(ex, loc), loc); } } @@ -438,8 +438,7 @@ public abstract class BaseFunction { */ private EvalExceptionWithStackTrace updateStackTrace( EvalExceptionWithStackTrace ex, Location location) { - ex.registerFunction(this); - ex.setLocation(location); + ex.registerFunction(this, location); return ex; } 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 50475215bc..d967962229 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 @@ -13,53 +13,55 @@ // limitations under the License. package com.google.devtools.build.lib.syntax; +import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.vfs.PathFragment; + +import java.util.Deque; +import java.util.LinkedList; /** * EvalException with a stack trace */ public class EvalExceptionWithStackTrace extends EvalException { - private final StringBuilder builder = new StringBuilder(); - private Location location; - private boolean printStackTrace; + + private StackTraceElement mostRecentElement; public EvalExceptionWithStackTrace(Exception original, Location callLocation) { super(callLocation, original.getMessage(), original.getCause()); - setLocation(callLocation); - builder.append(super.getMessage()); - printStackTrace = false; } /** - * Adds a line for the given function to the stack trace. Requires that #setLocation() was called - * previously. + * Adds an entry for the given statement to the stack trace. */ - public void registerFunction(BaseFunction function) { - addStackFrame(function.getFullName()); + 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()); } /** - * Adds a line for the given rule to the stack trace. + * Adds an entry for the given function to the stack trace. */ - public void registerRule(Rule rule) { - setLocation(rule.getLocation()); - addStackFrame(String.format("%s(name = '%s', ...)", rule.getRuleClass(), rule.getName())); + public void registerFunction(BaseFunction function, Location location) { + addStackFrame(function.getFullName(), location); } /** - * Adds a line for the given scope (function or rule). + * Adds an entry for the given rule to the stack trace. */ - private void addStackFrame(String scope) { - builder.append(String.format("\n\tin %s [%s]", scope, location)); - printStackTrace |= (location != Location.BUILTIN); + public void registerRule(Rule rule) { + addStackFrame( + String.format("%s(name = '%s')", rule.getRuleClass(), rule.getName()), rule.getLocation()); } /** - * Sets the location for the next function to be added via #registerFunction(). + * Adds a line for the given frame. */ - public void setLocation(Location callLocation) { - this.location = callLocation; + private void addStackFrame(String label, Location location) { + mostRecentElement = new StackTraceElement(label, location, mostRecentElement); } /** @@ -76,7 +78,141 @@ public class EvalExceptionWithStackTrace extends EvalException { @Override public String print() { - // Only print the stack trace when it contains more than one built-in function. - return printStackTrace ? builder.toString() : getOriginalMessage(); + return print(StackTracePrinter.INSTANCE); + } + + /** + * Prints the stack trace iff it contains more than just one built-in function. + */ + public String print(StackTracePrinter printer) { + return canPrintStackTrace() + ? printer.print(getOriginalMessage(), mostRecentElement) + : getOriginalMessage(); + } + + /** + * Returns true when there is at least one non-built-in element. + */ + protected boolean canPrintStackTrace() { + return mostRecentElement != null && mostRecentElement.getCause() != null; + } + + /** + * An element in the stack trace which contains the name of the offending function / rule / + * statement and its location. + */ + protected final class StackTraceElement { + private final String label; + private final Location location; + private final StackTraceElement cause; + + StackTraceElement(String label, Location location, StackTraceElement cause) { + this.label = label; + this.location = location; + this.cause = cause; + } + + String getLabel() { + return label; + } + + Location getLocation() { + return location; + } + + StackTraceElement getCause() { + return cause; + } + } + + /** + * Singleton class that prints stack traces similar to Python. + */ + public enum StackTracePrinter { + INSTANCE; + + /** + * Turns the given message and StackTraceElements into a string. + */ + public final String print(String message, StackTraceElement mostRecentElement) { + Deque<String> output = new LinkedList<>(); + + while (mostRecentElement != null) { + String entry = print(mostRecentElement); + if (entry != null && entry.length() > 0) { + addEntry(output, entry); + } + + mostRecentElement = mostRecentElement.getCause(); + } + + addMessage(output, message); + return Joiner.on("\n").join(output); + } + + /** + * 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) { + return (element == null) ? Location.BUILTIN : element.getLocation(); + } + + /** + * Returns whether the given element describes the rule definition in a BUILD file. + */ + protected boolean describesRule(StackTraceElement element) { + PathFragment pathFragment = element.getLocation().getPath(); + return pathFragment != null && pathFragment.getPathString().contains("BUILD"); + } + + /** + * Returns the string representation of the given element. + */ + protected String print(StackTraceElement element) { + // Similar to Python, the first (most-recent) entry in the stack frame is printed only once. + // Consequently, we skip it here. + if (element.getCause() == null) { + return ""; + } + + // Prints a two-line string, similar to Python. + Location location = getDisplayLocation(element); + return String.format( + "\tFile \"%s\", line %d, in %s%n\t\t%s", + printPath(location.getPath()), + location.getStartLineAndColumn().getLine(), + element.getLabel(), + element.getCause().getLabel()); + } + + private String printPath(PathFragment path) { + return (path == null) ? "<unknown>" : path.getPathString(); + } + + /** + * Adds the given string to the specified Deque. + */ + protected void addEntry(Deque<String> output, String toAdd) { + output.addLast(toAdd); + } + + /** + * Adds the given message to the given output dequeue after all stack trace elements have been + * added. + */ + protected void addMessage(Deque<String> output, String message) { + output.addFirst("Traceback (most recent call last):"); + output.addLast(message); + } } } 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 5ebaa0ee00..2e90afd3f9 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 @@ -481,15 +481,7 @@ public final class FuncallExpression extends Expression { @Override Object eval(Environment env) throws EvalException, InterruptedException { - try { - return (obj != null) ? invokeObjectMethod(env) : invokeGlobalFunction(env); - } catch (EvalException ex) { - // BaseFunction will not get the exact location of some errors (such as exceptions thrown in - // #invokeJavaMethod), so we create a new stack trace with the correct location here. - throw (ex instanceof EvalExceptionWithStackTrace) - ? ex - : new EvalExceptionWithStackTrace(ex, ex.getLocation()); - } + return (obj != null) ? invokeObjectMethod(env) : invokeGlobalFunction(env); } /** 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 334a5eb8c0..750429d65b 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 @@ -57,12 +57,23 @@ public class UserDefinedFunction extends BaseFunction { env.update(name, arguments[i++]); } + Statement lastStatement = null; try { for (Statement stmt : statements) { + lastStatement = stmt; stmt.exec(env); } } catch (ReturnStatement.ReturnException e) { return e.getValue(); + } 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; } return Environment.NONE; } 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 1a489d29e1..29a94593c6 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,18 +42,35 @@ public class MethodLibraryTest extends EvaluationTestCase { } @Test + public void testStackTraceSkipBuiltInOnly() throws Exception { + // The error message should not include the stack trace when there is + // only one built-in function. + new SkylarkTest() + .testIfExactError( + "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", + "'test'.index(1)"); + } + + @Test public void testStackTrace() throws Exception { - new SkylarkTest().testIfExactError( - "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\n" - + "\tin string.index [Built-In]\n" - + "\tin bar [4:4]\n" - + "\tin foo [2:4]", - "def foo():", - " bar(1)", - "def bar(x):", - " 'test'.index(x)", - "foo()"); + // Unlike SkylarintegrationTests#testStackTraceErrorInFunction(), this test + // has neither a BUILD nor a bzl file. + new SkylarkTest() + .testIfExactError( + "Traceback (most recent call last):\n" + + "\tFile \"<unknown>\", line 2, in foo\n" + + "\t\tbar\n" + + "\tFile \"<unknown>\", line 4, in bar\n" + + "\t\tstring.index\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)", + "def bar(x):", + " 'test'.index(x)", + "foo()"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index 22c715358e..b11866dc0b 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java @@ -283,8 +283,11 @@ public class SkylarkEvaluationTest extends EvaluationTest { public void testForNotIterable() throws Exception { new SkylarkTest() .update("mock", new Mock()) - .testIfExactError("type 'int' is not iterable", "def func():", - " for i in mock.value_of('1'): a = i", "func()\n"); + .testIfErrorContains( + "type 'int' is not iterable", + "def func():", + " for i in mock.value_of('1'): a = i", + "func()\n"); } @Test @@ -777,14 +780,16 @@ public class SkylarkEvaluationTest extends EvaluationTest { @Test public void testListIndexAsLValueAsLValue() throws Exception { - new SkylarkTest().testIfExactError("unsupported operand type(s) for +: 'list' and 'dict'", - "def id(l):", - " return l", - "def func():", - " l = id([1])", - " l[0] = 2", - " return l", - "l = func()"); + new SkylarkTest() + .testIfErrorContains( + "unsupported operand type(s) for +: 'list' and 'dict'", + "def id(l):", + " return l", + "def func():", + " l = id([1])", + " l[0] = 2", + " return l", + "l = func()"); } @Test @@ -987,23 +992,32 @@ public class SkylarkEvaluationTest extends EvaluationTest { @Override @Test public void testListComprehensionsMultipleVariablesFail() throws Exception { - new SkylarkTest().testIfExactError("lvalue has length 3, but rvalue has has length 2", - "def foo (): return [x + y for x, y, z in [(1, 2), (3, 4)]]", - "foo()"); + new SkylarkTest() + .testIfErrorContains( + "lvalue has length 3, but rvalue has has length 2", + "def foo (): return [x + y for x, y, z in [(1, 2), (3, 4)]]", + "foo()"); - new SkylarkTest().testIfExactError("type 'int' is not a collection", - "def bar (): return [x + y for x, y in (1, 2)]", - "bar()"); + new SkylarkTest() + .testIfErrorContains( + "type 'int' is not a collection", + "def bar (): return [x + y for x, y in (1, 2)]", + "bar()"); - new SkylarkTest().testIfExactError("lvalue has length 3, but rvalue has has length 2", - "[x + y for x, y, z in [(1, 2), (3, 4)]]"); + new SkylarkTest() + .testIfErrorContains( + "lvalue has length 3, but rvalue has has length 2", + "[x + y for x, y, z in [(1, 2), (3, 4)]]"); // can't reuse the same local variable twice(!) - new SkylarkTest().testIfExactError("ERROR 2:1: Variable x is read only", - "[x + y for x, y in (1, 2)]", "[x + y for x, y in (1, 2)]"); + new SkylarkTest() + .testIfErrorContains( + "ERROR 2:1: Variable x is read only", + "[x + y for x, y in (1, 2)]", + "[x + y for x, y in (1, 2)]"); - new SkylarkTest().testIfExactError("type 'int' is not a collection", - "[x2 + y2 for x2, y2 in (1, 2)]"); + new SkylarkTest() + .testIfErrorContains("type 'int' is not a collection", "[x2 + y2 for x2, y2 in (1, 2)]"); } @Override |