Skylark error messages now include a stack trace. This means that some tests had to be changed from using exact equality of error messages to working with contains() / startsWith(). -- MOS_MIGRATED_REVID=100923593
diff --git a/src/main/java/com/google/devtools/build/lib/events/Location.java b/src/main/java/com/google/devtools/build/lib/events/Location.java index 58301b7..608d307 100644 --- a/src/main/java/com/google/devtools/build/lib/events/Location.java +++ b/src/main/java/com/google/devtools/build/lib/events/Location.java
@@ -275,4 +275,20 @@ return line * 41 + column; } } + + /** + * Dummy location for built-in functions which ensures that stack traces contain "nice" location + * strings. + */ + public static final Location BUILTIN = new Location(0, 0) { + @Override + public String toString() { + return "Built-In"; + } + + @Override + public PathFragment getPath() { + return null; + } + }; }
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 683771e..274d527 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
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.analysis.RunfilesSupport; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.syntax.BaseFunction; @@ -35,6 +36,7 @@ import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject; import com.google.devtools.build.lib.syntax.Environment; import com.google.devtools.build.lib.syntax.EvalException; +import com.google.devtools.build.lib.syntax.EvalExceptionWithStackTrace; import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.SkylarkEnvironment; import com.google.devtools.build.lib.syntax.SkylarkNestedSet; @@ -75,13 +77,13 @@ ConfiguredTarget configuredTarget = createTarget(ruleContext, target); checkOrphanArtifacts(ruleContext); return configuredTarget; - } catch (InterruptedException e) { ruleContext.ruleError(e.getMessage()); return null; } catch (EvalException e) { + addRuleToStackTrace(e, ruleContext.getRule()); // If the error was expected, return an empty target. - if (!expectFailure.isEmpty() && e.getMessage().matches(expectFailure)) { + if (!expectFailure.isEmpty() && getMessageWithoutStackTrace(e).matches(expectFailure)) { return new com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder(ruleContext) .add(RunfilesProvider.class, RunfilesProvider.EMPTY) .build(); @@ -91,6 +93,25 @@ } } + /** + * Adds an entry for the given rule to the stack trace of the exception (if there is one). + */ + private static void addRuleToStackTrace(EvalException ex, Rule rule) { + if (ex instanceof EvalExceptionWithStackTrace) { + ((EvalExceptionWithStackTrace) ex).registerRule(rule); + } + } + + /** + * Returns the message of the given exception after removing the stack trace, if present. + */ + private static String getMessageWithoutStackTrace(EvalException ex) { + if (ex instanceof EvalExceptionWithStackTrace) { + return ((EvalExceptionWithStackTrace) ex).getOriginalMessage(); + } + return ex.getMessage(); + } + private static void checkOrphanArtifacts(RuleContext ruleContext) throws EvalException { ImmutableSet<Artifact> orphanArtifacts = ruleContext.getAnalysisEnvironment().getOrphanArtifacts();
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 5145f4b..0309af2 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
@@ -396,21 +396,31 @@ } /** + * Returns the environment for the scope of this function. + * + * <p>Since this is a BaseFunction, we don't create a new environment. + */ + @SuppressWarnings("unused") // For the exception + protected Environment getOrCreateChildEnvironment(Environment parent) throws EvalException { + return parent; + } + + /** * The outer calling convention to a BaseFunction. * * @param args a list of all positional arguments (as in *starArg) * @param kwargs a map for key arguments (as in **kwArgs) * @param ast the expression for this function's definition - * @param env the lexical Environment for the function call + * @param parentEnv the lexical Environment for the function call * @return the value resulting from evaluating the function with the given arguments * @throws construction of EvalException-s containing source information. */ public Object call(@Nullable List<Object> args, @Nullable Map<String, Object> kwargs, @Nullable FuncallExpression ast, - @Nullable Environment env) + @Nullable Environment parentEnv) throws EvalException, InterruptedException { - + Environment env = getOrCreateChildEnvironment(parentEnv); Preconditions.checkState(isConfigured(), "Function %s was not configured", getName()); // ast is null when called from Java (as there's no Skylark call site). @@ -421,24 +431,37 @@ try { return call(arguments, ast, env); - } catch (ConversionException e) { - throw new EvalException(loc, e.getMessage()); + } catch (EvalExceptionWithStackTrace ex) { + throw updateStackTrace(ex, loc); + } catch (EvalException | RuntimeException | InterruptedException ex) { + throw updateStackTrace(new EvalExceptionWithStackTrace(ex, Location.BUILTIN), 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); + ex.setLocation(location); + return ex; + } + + /** * Inner call to a BaseFunction * subclasses need to @Override this method. * * @param args an array of argument values sorted as per the signature. * @param ast the source code for the function if user-defined - * @param ast the lexical environment of the function call + * @param env the lexical environment of the function call */ // Don't make it abstract, so that subclasses may be defined that @Override the outer call() only. - public Object call(Object[] args, + protected Object call(Object[] args, @Nullable FuncallExpression ast, @Nullable Environment env) throws EvalException, ConversionException, InterruptedException { - throw new EvalException(ast.getLocation(), + throw new EvalException( + (ast == null) ? Location.BUILTIN : ast.getLocation(), String.format("function %s not implemented", getName())); } @@ -498,14 +521,22 @@ } /** - * Returns the signature as "[className.]methodName(paramType1, paramType2, ...)" + * Returns [class.]function (depending on whether func belongs to a class). */ - public String getShortSignature() { + public String getFullName() { + return String.format("%s%s", getObjectTypeString(), getName()); + } + + /** + * Returns the signature as "[className.]methodName(name1: paramType1, name2: paramType2, ...)" + * or "[className.]methodName(paramType1, paramType2, ...)", depending on the value of showNames. + */ + public String getShortSignature(boolean showNames) { StringBuilder builder = new StringBuilder(); boolean hasSelf = hasSelfArgument(); - builder.append(getObjectTypeString()).append(getName()).append("("); - signature.toStringBuilder(builder, true, false, false, hasSelf); + builder.append(getFullName()).append("("); + signature.toStringBuilder(builder, showNames, false, false, hasSelf); builder.append(")"); return builder.toString();
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 e8f901b..e2a333a 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
@@ -190,12 +190,9 @@ // BUILD file (as it is the most probable cause for the error). Location exnLoc = e.getLocation(); Location nodeLoc = stmt.getLocation(); - if (exnLoc == null || !nodeLoc.getPath().equals(exnLoc.getPath())) { - eventHandler.handle(Event.error(nodeLoc, - e.getMessage() + " (raised from " + exnLoc + ")")); - } else { - eventHandler.handle(Event.error(exnLoc, e.getMessage())); - } + eventHandler.handle(Event.error( + (exnLoc == null || !nodeLoc.getPath().equals(exnLoc.getPath())) ? nodeLoc : exnLoc, + e.getMessage())); } } return ok;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java index 7fc85ba..70173ce 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
@@ -177,7 +177,7 @@ loc, String.format( "Method %s is not applicable for arguments %s: '%s' is %s, but should be %s", - getShortSignature(), printTypeString(args, args.length - extraArgsCount), + getShortSignature(true), printTypeString(args, args.length - extraArgsCount), paramName, EvalUtils.getDataTypeName(args[i]), EvalUtils.getDataTypeNameFromClass(types[i]))); }
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 1b98a27..e6ae8a0 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
@@ -70,8 +70,12 @@ */ public static Object eval(Object objValue, String name, Location loc) throws EvalException { if (objValue instanceof ClassObject) { - Object result = ((ClassObject) objValue).getValue(name); - + 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) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java index 361ce9f..32f289b 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
@@ -70,7 +70,7 @@ if (message == null) { message = ""; } - if (cause != null) { + if (cause != null && !message.contains(cause.getMessage())) { message = message + (message.isEmpty() ? "" : ": ") + cause.getMessage(); } if (message.isEmpty()) { @@ -92,7 +92,16 @@ return (getLocation() == null ? "" : getLocation()) + ": " + (message == null ? "" : message + "\n") + (dueToIncompleteAST ? "due to incomplete AST\n" : "") - + (getCause() != null && getCause().getMessage() != null ? getCause().getMessage() : ""); + + getCauseMessage(); + } + + private String getCauseMessage() { + Throwable cause = getCause(); + if (cause == null) { + return ""; + } + String causeMessage = cause.getMessage(); + return (causeMessage == null || message.contains(causeMessage)) ? "" : causeMessage; } /**
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 new file mode 100644 index 0000000..5047521 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java
@@ -0,0 +1,82 @@ +// Copyright 2015 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// 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.packages.Rule; + +/** + * EvalException with a stack trace + */ +public class EvalExceptionWithStackTrace extends EvalException { + private final StringBuilder builder = new StringBuilder(); + private Location location; + private boolean printStackTrace; + + 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. + */ + public void registerFunction(BaseFunction function) { + addStackFrame(function.getFullName()); + } + + /** + * Adds a line for the given rule to the stack trace. + */ + public void registerRule(Rule rule) { + setLocation(rule.getLocation()); + addStackFrame(String.format("%s(name = '%s', ...)", rule.getRuleClass(), rule.getName())); + } + + /** + * Adds a line for the given scope (function or rule). + */ + private void addStackFrame(String scope) { + builder.append(String.format("\n\tin %s [%s]", scope, location)); + printStackTrace |= (location != Location.BUILTIN); + } + + /** + * Sets the location for the next function to be added via #registerFunction(). + */ + public void setLocation(Location callLocation) { + this.location = callLocation; + } + + /** + * Returns the exception message without the stack trace. + */ + public String getOriginalMessage() { + return super.getMessage(); + } + + @Override + public String getMessage() { + return print(); + } + + @Override + public String print() { + // Only print the stack trace when it contains more than one built-in function. + return printStackTrace ? builder.toString() : getOriginalMessage(); + } +}
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 a448325..32fda83 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,64 +481,91 @@ @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()); + } + } + + /** + * Invokes obj.func() and returns the result. + */ + private Object invokeObjectMethod(Environment env) throws EvalException, InterruptedException { + Object objValue = obj.eval(env); ImmutableList.Builder<Object> posargs = new ImmutableList.Builder<>(); // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or // we'd still have to have a HashMap on the side for the sake of properly handling duplicates. Map<String, Object> kwargs = new HashMap<>(); - Object returnValue; - BaseFunction function; - if (obj != null) { // obj.func(...) - Object objValue = obj.eval(env); - // Strings, lists and dictionaries (maps) have functions that we want to use in MethodLibrary. - // For other classes, we can call the Java methods. - function = - env.getFunction(EvalUtils.getSkylarkType(objValue.getClass()), func.getName()); - if (function != null) { - if (!isNamespace(objValue.getClass())) { - // Add self as an implicit parameter in front. - posargs.add(objValue); - } - evalArguments(posargs, kwargs, env, function); - returnValue = function.call( - posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env); - } else if (env.isSkylarkEnabled()) { - // Only allow native Java calls when using Skylark - // When calling a Java method, the name is not in the Environment, - // so evaluating 'func' would fail. - evalArguments(posargs, kwargs, env, null); - if (!kwargs.isEmpty()) { - throw new EvalException(func.getLocation(), - String.format("Keyword arguments are not allowed when calling a java method" - + "\nwhile calling method '%s' on object %s of type %s", - func.getName(), objValue, EvalUtils.getDataTypeName(objValue))); - } - if (objValue instanceof Class<?>) { - // Static Java method call. We can return the value from here directly because - // invokeJavaMethod() has special checks. - return invokeJavaMethod(null, (Class<?>) objValue, func.getName(), posargs.build()); - } else { - return invokeJavaMethod(objValue, objValue.getClass(), func.getName(), posargs.build()); - } - } else { - throw new EvalException(getLocation(), String.format( - "%s is not defined on object of type '%s'", - functionName(), EvalUtils.getDataTypeName(objValue))); + // Strings, lists and dictionaries (maps) have functions that we want to use in + // MethodLibrary. + // For other classes, we can call the Java methods. + BaseFunction function = + env.getFunction(EvalUtils.getSkylarkType(objValue.getClass()), func.getName()); + if (function != null) { + if (!isNamespace(objValue.getClass())) { + // Add self as an implicit parameter in front. + posargs.add(objValue); } - } else { // func(...) - Object funcValue = func.eval(env); - if ((funcValue instanceof BaseFunction)) { - function = (BaseFunction) funcValue; - evalArguments(posargs, kwargs, env, function); - returnValue = function.call( - posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env); - } else { - throw new EvalException(getLocation(), - "'" + EvalUtils.getDataTypeName(funcValue) - + "' object is not callable"); + evalArguments(posargs, kwargs, env, function); + return convertFromSkylark( + function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env), + env); + } else if (env.isSkylarkEnabled()) { + // Only allow native Java calls when using Skylark + // When calling a Java method, the name is not in the Environment, + // so evaluating 'func' would fail. + evalArguments(posargs, kwargs, env, null); + if (!kwargs.isEmpty()) { + throw new EvalException( + func.getLocation(), + String.format( + "Keyword arguments are not allowed when calling a java method" + + "\nwhile calling method '%s' on object %s of type %s", + func.getName(), objValue, EvalUtils.getDataTypeName(objValue))); } + if (objValue instanceof Class<?>) { + // Static Java method call. We can return the value from here directly because + // invokeJavaMethod() has special checks. + return invokeJavaMethod(null, (Class<?>) objValue, func.getName(), posargs.build()); + } else { + return invokeJavaMethod(objValue, objValue.getClass(), func.getName(), posargs.build()); + } + } else { + throw new EvalException( + getLocation(), + String.format("%s is not defined on object of type '%s'", functionName(), + EvalUtils.getDataTypeName(objValue))); } + } + /** + * Invokes func() and returns the result. + */ + private Object invokeGlobalFunction(Environment env) throws EvalException, InterruptedException { + Object funcValue = func.eval(env); + ImmutableList.Builder<Object> posargs = new ImmutableList.Builder<>(); + // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or + // we'd still have to have a HashMap on the side for the sake of properly handling duplicates. + Map<String, Object> kwargs = new HashMap<>(); + if ((funcValue instanceof BaseFunction)) { + BaseFunction function = (BaseFunction) funcValue; + evalArguments(posargs, kwargs, env, function); + return convertFromSkylark( + function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env), + env); + } else { + throw new EvalException( + getLocation(), "'" + EvalUtils.getDataTypeName(funcValue) + "' object is not callable"); + } + } + + protected Object convertFromSkylark(Object returnValue, Environment env) throws EvalException { EvalUtils.checkNotNull(this, returnValue); if (!env.isSkylarkEnabled()) { // The call happens in the BUILD language. Note that accessing "BUILD language" functions in
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 5831a28..334a5eb 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
@@ -46,35 +46,33 @@ return location; } - /** - * Since the types of parameters of user defined functions are unknown, we just return - * "name(parameterCount)" - */ - @Override - public String getShortSignature() { - return String.format("%s(%d)", getName(), getArgArraySize()); - } - @Override public Object call(Object[] arguments, FuncallExpression ast, Environment env) throws EvalException, InterruptedException { - SkylarkEnvironment functionEnv = SkylarkEnvironment.createEnvironmentForFunctionCalling( - env, definitionEnv, this); ImmutableList<String> names = signature.getSignature().getNames(); // Registering the functions's arguments as variables in the local Environment int i = 0; for (String name : names) { - functionEnv.update(name, arguments[i++]); + env.update(name, arguments[i++]); } try { for (Statement stmt : statements) { - stmt.exec(functionEnv); + stmt.exec(env); } } catch (ReturnStatement.ReturnException e) { return e.getValue(); } return Environment.NONE; } + + /** + * Creates a new environment for the execution of this function. + */ + @Override + protected Environment getOrCreateChildEnvironment(Environment parent) throws EvalException { + return SkylarkEnvironment.createEnvironmentForFunctionCalling( + parent, definitionEnv, this); + } }