bazel syntax: remove unsound x.f() optimization
Previously, the interpreter had two different control paths for "y=x.f; y()" and x.f().
This was ostensibly an optimization to avoid allocating a bound method closure for x.f
only for it to be called immediately after. However, this was unsound
because it deferred the x.f operation until after the evaluation of any arguments
to the call, which causes effects to occur in the wrong order; see
https://github.com/bazelbuild/bazel/issues/10339.
This change removes the "optimization". It's unclear to me that saving the cost
of allocating the bound method closure is even measurable given the profligacy
of the current calling convention. In any case, follow-up changes will optimize
the calling convention much more than any loss in this change.
(Laurent says that this is a more a vestige of Starlark method overloading
than an optimization; see b/21392896#comment4.)
BuiltinCallable now accepts the MethodDescriptor if this is known,
as it is during an x.f() call, to avoid the need for a second lookup.
Our usual benchmarks of loading+analysis show slight improvements
that I can't explain (2 batches of 3 runs each):
mean median stddev pval
cpu: 672.737s (-1.38%) 669.890s (+0.32%) 15.927 0.02379
cpu: 648.187s (-4.44%) 640.020s (-3.89%) 17.961 0.02379
Removing the alternate code path revealed a number of other minor ways in
which the semantics of x.f() differed from its two-step decomposition,
such as error messages and suggestions, resolution of field/method
ambiguity, ConfiguredTarget providers vs. methods, and (in commit 59245cac3d8853598666a6e1f93dd4efd1cc0ca1),
treatment of the struct.to_{json,proto} methods.
Also:
- reorder the cases in EvalUtils.getAttr so that @SkylarkCallable-annotated
methods (like annotated fields) are dispatched before ClassObject-defined fields.
The only code that might care about this is user-defined instances of
ToolchainInfo.
- Order instanceof checks by frequency in evalArguments.
- Add test of correct order of effects.
- Update numerous error messages.
This change will make it hard to roll back commit 59245cac3d8853598666a6e1f93dd4efd1cc0ca1,
(which makes getattr(struct, "to_json", None) return a function)
should that be necessary.
This is a breaking change for copybara's tests.
RELNOTES:
x.f() is now equivalent to y=x.f; y(). That is, x.f should return the same
attribute value regardless of whether it is accessed as a field or called
like a method. Any arguments to the call are evaluated after the x.f operation.
PiperOrigin-RevId: 284823002
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
index dc3ed9c..c909d9e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
@@ -74,7 +74,7 @@
this.key = new NativeKey(name, getClass());
this.valueClass = valueClass;
this.errorMessageFormatForUnknownField =
- String.format("'%s' object has no attribute '%%s'", name);
+ String.format("'%s' value has no field or method '%%s'", name);
}
public final SkylarkProviderIdentifier id() {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Provider.java b/src/main/java/com/google/devtools/build/lib/packages/Provider.java
index f162ecf..7b0f579 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Provider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Provider.java
@@ -54,7 +54,7 @@
* <p>The format string must contain one {@code '%s'} placeholder for the field name.
*/
default String getErrorMessageFormatForUnknownField() {
- return String.format("'%s' object has no attribute '%%s'", getPrintableName());
+ return String.format("'%s' value has no field or method '%%s'", getPrintableName());
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
index 99ed600..f8d6012 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
@@ -203,7 +203,7 @@
}
private static String makeErrorMessageFormatForUnknownField(String exportedName) {
- return String.format("'%s' object has no attribute '%%s'", exportedName);
+ return String.format("'%s' value has no field or method '%%s'", exportedName);
}
@Override
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 c4a2704..8faf014 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
@@ -32,12 +32,30 @@
private final Object obj;
private final String methodName;
+ @Nullable private final MethodDescriptor desc;
- // This function is only public for the to_{json,proto} hack.
- // TODO(adonovan): make it private.
- public BuiltinCallable(Object obj, String methodName) {
+ /**
+ * Constructs a BuiltinCallable for a StarlarkCallable-annotated method of the given name (as seen
+ * by Starlark, not Java).
+ */
+ BuiltinCallable(Object obj, String methodName) {
+ this(obj, methodName, /*desc=*/ null);
+ }
+
+ /**
+ * Constructs a BuiltinCallable for a StarlarkCallable-annotated method of the given name (as seen
+ * by Starlark, not Java).
+ *
+ * <p>This constructor should be used only for ephemeral BuiltinCallable values created
+ * transiently during a call such as {@code x.f()}, when the caller has already looked up the
+ * MethodDescriptor using the same semantics as the thread that will be used in the call. Use the
+ * other (slower) constructor if there is any possibility that the semantics of the {@code x.f}
+ * operation differ from those of the thread used in the call.
+ */
+ BuiltinCallable(Object obj, String methodName, MethodDescriptor desc) {
this.obj = obj;
this.methodName = methodName;
+ this.desc = desc;
}
@Override
@@ -47,9 +65,8 @@
FuncallExpression ast,
StarlarkThread thread)
throws EvalException, InterruptedException {
- // Even though all callers of 'new BuiltinCallable' have a MethodDescriptor,
- // we have to look it up again with the correct semantics from the thread.
- MethodDescriptor methodDescriptor = getMethodDescriptor(thread.getSemantics());
+ MethodDescriptor methodDescriptor =
+ desc != null ? desc : getMethodDescriptor(thread.getSemantics());
Class<?> clazz;
Object objValue;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java
index 539dfdd..7cb4ac5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java
@@ -61,6 +61,7 @@
}
// Information derived from a SkylarkCallable-annotated class and a StarlarkSemantics.
+ // methods is a superset of fields.
private static class CacheValue {
@Nullable MethodDescriptor selfCall;
ImmutableMap<String, MethodDescriptor> fields; // sorted by Java method name
@@ -178,7 +179,7 @@
return getCacheValue(x.getClass(), semantics).fields.keySet();
}
- /** Returns the list of Skylark callable Methods of objClass with the given name. */
+ /** Returns the SkylarkCallable-annotated method of objClass with the given name. */
static MethodDescriptor getMethod(
StarlarkSemantics semantics, Class<?> objClass, String methodName) {
return getCacheValue(objClass, semantics).methods.get(methodName);
@@ -188,7 +189,7 @@
* Returns a set of the Skylark name of all Skylark callable methods for object of type {@code
* objClass}.
*/
- static Set<String> getMethodNames(StarlarkSemantics semantics, Class<?> objClass) {
+ static ImmutableSet<String> getMethodNames(StarlarkSemantics semantics, Class<?> objClass) {
return getCacheValue(objClass, semantics).methods.keySet();
}
@@ -214,25 +215,6 @@
}
/**
- * Returns a {@link BuiltinCallable} representing a {@link SkylarkCallable}-annotated instance
- * method of a given object with the given Starlark field name (not necessarily the same as the
- * Java method name).
- */
- static BuiltinCallable getBuiltinCallable(
- StarlarkSemantics semantics, Object obj, String methodName) {
- // TODO(adonovan): implement by EvalUtils.getAttr, once the latter doesn't require
- // a Thread and Location.
- Class<?> objClass = obj.getClass();
- MethodDescriptor desc = getMethod(semantics, objClass, methodName);
- if (desc == null) {
- throw new IllegalStateException(String.format(
- "Expected a method named '%s' in %s, but found none",
- methodName, objClass));
- }
- return new BuiltinCallable(obj, methodName);
- }
-
- /**
* Converts Starlark-defined arguments to an array of argument {@link Object}s that may be passed
* to a given callable-from-Starlark Java method.
*
@@ -444,15 +426,6 @@
}
}
- private static EvalException missingMethodException(
- FuncallExpression call, Class<?> objClass, String methodName) {
- return new EvalException(
- call.getLocation(),
- String.format(
- "type '%s' has no method %s()",
- EvalUtils.getDataTypeNameFromClass(objClass), methodName));
- }
-
/**
* Returns the extra interpreter arguments for the given {@link SkylarkCallable}, to be added at
* the end of the argument list for the callable.
@@ -526,67 +499,6 @@
return methodDescriptor.getName() + "(" + Joiner.on(", ").join(argTokens.build()) + ")";
}
- /** Invoke object.method() and return the result. */
- static Object callMethod(
- StarlarkThread thread,
- FuncallExpression call,
- Object object,
- ArrayList<Object> posargs,
- Map<String, Object> kwargs,
- String methodName,
- Location dotLocation)
- throws EvalException, InterruptedException {
- // Case 1: Object is a String. String is an unusual special case.
- if (object instanceof String) {
- return callStringMethod(thread, call, (String) object, methodName, posargs, kwargs);
- }
-
- // Case 2: Object is a Java object with a matching @SkylarkCallable method.
- // This is an optimization. For 'foo.bar()' where 'foo' is a java object with a callable
- // java method 'bar()', this avoids evaluating 'foo.bar' in isolation (which would require
- // creating a throwaway function-like object).
- MethodDescriptor methodDescriptor =
- getMethod(thread.getSemantics(), object.getClass(), methodName);
- if (methodDescriptor != null && !methodDescriptor.isStructField()) {
- Object[] javaArguments =
- convertStarlarkArgumentsToJavaMethodArguments(
- thread, call, methodDescriptor, object.getClass(), posargs, kwargs);
- return methodDescriptor.call(object, javaArguments, call.getLocation(), thread);
- }
-
- // Case 3: All other cases. Evaluate "foo.bar" as a dot expression, then try to invoke it
- // as a callable.
- Object functionObject = EvalUtils.getAttr(thread, dotLocation, object, methodName);
- if (functionObject == null) {
- throw missingMethodException(call, object.getClass(), methodName);
- } else {
- return call(thread, call, functionObject, posargs, kwargs);
- }
- }
-
- private static Object callStringMethod(
- StarlarkThread thread,
- 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(thread.getSemantics(), StringModule.class, methodName);
- if (method == null) {
- throw missingMethodException(call, StringModule.class, methodName);
- }
-
- Object[] javaArguments =
- convertStarlarkArgumentsToJavaMethodArguments(
- thread, call, method, StringModule.class, posargs, kwargs);
- return method.call(StringModule.INSTANCE, javaArguments, call.getLocation(), thread);
- }
-
static Object call(
StarlarkThread thread,
FuncallExpression call,
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 8f850cb..4800180 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,6 +16,7 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.events.Location;
import java.util.ArrayList;
import java.util.LinkedHashMap;
@@ -469,7 +470,7 @@
String name = dot.getField().getName();
Object result = EvalUtils.getAttr(thread, dot.getLocation(), object, name);
if (result == null) {
- throw EvalUtils.getMissingFieldException(object, name, thread.getSemantics(), "field");
+ throw EvalUtils.getMissingAttrException(object, name, thread.getSemantics());
}
return result;
}
@@ -477,21 +478,17 @@
case FUNCALL:
{
FuncallExpression call = (FuncallExpression) expr;
-
+ Object fn = eval(thread, call.getFunction());
ArrayList<Object> posargs = new ArrayList<>();
Map<String, Object> kwargs = new LinkedHashMap<>();
-
- // Optimization: call x.f() without materializing
- // a closure for x.f if f is a Java method.
- if (call.getFunction() instanceof DotExpression) {
- DotExpression dot = (DotExpression) call.getFunction();
- Object object = eval(thread, dot.getObject());
- evalArguments(thread, call, posargs, kwargs);
- return CallUtils.callMethod(
- thread, call, object, posargs, kwargs, dot.getField().getName(), dot.getLocation());
- }
-
- Object fn = eval(thread, call.getFunction());
+ // TODO(adonovan): optimize the calling convention to pass a contiguous array of
+ // positional and named arguments with an array of Strings for the names (which is
+ // constant for all non-**kwargs call sites). The caller would remain responsible for
+ // flattening f(*args) and f(**kwargs), but the callee would become responsible for
+ // duplicate name checking.
+ // Callees already need to do this work anyway---see BaseFunction.processArguments and
+ // CallUtils.convertStarlarkArgumentsToJavaMethodArguments---and this avoids constructing
+ // another hash table in nearly every call
evalArguments(thread, call, posargs, kwargs);
return CallUtils.call(thread, call, fn, posargs, kwargs);
}
@@ -763,7 +760,6 @@
* here instead of an immutable map builder to deal with duplicates without memory overhead
* @param thread the Starlark thread for the call
*/
- @SuppressWarnings("unchecked")
private static void evalArguments(
StarlarkThread thread,
FuncallExpression call,
@@ -775,15 +771,25 @@
ImmutableList.Builder<String> duplicatesBuilder = null;
// Iterate over the arguments. We assume all positional arguments come before any keyword
// or star arguments, because the argument list was already validated by the Parser,
- // which should be the only place that build FuncallExpression-s.
+ // which should be the only place that build FuncallExpressions.
// 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 < call.getArguments().size(); i++) {
- Argument arg = call.getArguments().get(i);
+ List<Argument> args = call.getArguments();
+ for (int i = 0; i < args.size(); i++) {
+ Argument arg = args.get(i);
Object value = eval(thread, arg.getValue());
if (arg instanceof Argument.Positional) {
// f(expr)
posargs.add(value);
+ } else if (arg instanceof Argument.Keyword) {
+ // f(id=expr)
+ String name = arg.getName();
+ if (addKeywordArgAndCheckIfDuplicate(kwargs, name, value)) {
+ if (duplicatesBuilder == null) {
+ duplicatesBuilder = ImmutableList.builder();
+ }
+ duplicatesBuilder.add(name);
+ }
} else if (arg instanceof Argument.Star) {
// f(*args): expand args
if (!(value instanceof StarlarkIterable)) {
@@ -791,10 +797,8 @@
call.getLocation(),
"argument after * must be an iterable, not " + EvalUtils.getDataTypeName(value));
}
- for (Object starArgUnit : (Iterable<Object>) value) {
- posargs.add(starArgUnit);
- }
- } else if (arg instanceof Argument.StarStar) {
+ Iterables.addAll(posargs, ((Iterable<?>) value));
+ } else {
// f(**kwargs): expand kwargs
ImmutableList<String> duplicates =
addKeywordArgsAndReturnDuplicates(kwargs, value, call.getLocation());
@@ -804,15 +808,6 @@
}
duplicatesBuilder.addAll(duplicates);
}
- } else {
- // f(id=expr)
- String name = arg.getName();
- if (addKeywordArgAndCheckIfDuplicate(kwargs, name, value)) {
- if (duplicatesBuilder == null) {
- duplicatesBuilder = ImmutableList.builder();
- }
- duplicatesBuilder.add(name);
- }
}
}
if (duplicatesBuilder != null) {
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 307e0fd..5c905fe 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
@@ -450,14 +450,18 @@
/** Returns the named field or method of value {@code x}, or null if not found. */
static Object getAttr(StarlarkThread thread, Location loc, Object x, String name)
throws EvalException, InterruptedException {
- // @SkylarkCallable-annotated field?
+ // @SkylarkCallable-annotated field or method?
MethodDescriptor method = CallUtils.getMethod(thread.getSemantics(), x.getClass(), name);
- if (method != null && method.isStructField()) {
- return method.call(
- x,
- CallUtils.extraInterpreterArgs(method, /*ast=*/ null, loc, thread).toArray(),
- loc,
- thread);
+ if (method != null) {
+ if (method.isStructField()) {
+ return method.call(
+ x,
+ CallUtils.extraInterpreterArgs(method, /*ast=*/ null, loc, thread).toArray(),
+ loc,
+ thread);
+ } else {
+ return new BuiltinCallable(x, name, method);
+ }
}
// user-defined field?
@@ -475,16 +479,11 @@
}
}
- // @SkylarkCallable-annotated method?
- if (method != null) {
- return new BuiltinCallable(x, name);
- }
-
return null;
}
- static EvalException getMissingFieldException(
- Object object, String name, StarlarkSemantics semantics, String accessName) {
+ static EvalException getMissingAttrException(
+ Object object, String name, StarlarkSemantics semantics) {
String suffix = "";
if (object instanceof ClassObject) {
String customErrorMessage = ((ClassObject) object).getErrorMessageForUnknownField(name);
@@ -495,20 +494,10 @@
} else {
suffix = SpellChecker.didYouMean(name, CallUtils.getFieldNames(semantics, object));
}
- 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";
- }
return new EvalException(
null,
String.format(
- "object of type '%s' has no %s '%s'%s",
- getDataTypeName(object), accessName, name, suffix));
- }
-
- /** Returns whether the given object has a method with the given name. */
- static boolean hasMethod(StarlarkSemantics semantics, Object object, String name) {
- return CallUtils.getMethodNames(semantics, object.getClass()).contains(name);
+ "'%s' value has no field or method '%s'%s", getDataTypeName(object), name, suffix));
}
/** Evaluates an eager binary operation, {@code x op y}. (Excludes AND and OR.) */
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 f51bb48..3293e8f 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
@@ -716,12 +716,14 @@
legacyNamed = true)
},
useStarlarkThread = true)
- public Boolean hasAttr(Object obj, String name, StarlarkThread thread) throws EvalException {
+ public Boolean hasattr(Object obj, String name, StarlarkThread thread) throws EvalException {
+ // TODO(adonovan): factor the core logic of hasattr, getattr, and dir into three adjacent
+ // functions so that we can convince ourselves of their ongoing consistency.
+ // Are we certain that getValue doesn't sometimes return None to mean 'no field'?
if (obj instanceof ClassObject && ((ClassObject) obj).getValue(name) != null) {
return true;
}
- // shouldn't this filter things with struct_field = false?
- return EvalUtils.hasMethod(thread.getSemantics(), obj, name);
+ return CallUtils.getMethodNames(thread.getSemantics(), obj.getClass()).contains(name);
}
@SkylarkCallable(
@@ -756,7 +758,7 @@
},
useLocation = true,
useStarlarkThread = true)
- public Object getAttr(
+ public Object getattr(
Object obj, String name, Object defaultValue, Location loc, StarlarkThread thread)
throws EvalException, InterruptedException {
Object result = EvalUtils.getAttr(thread, loc, obj, name);
@@ -764,7 +766,7 @@
if (defaultValue != Starlark.UNBOUND) {
return defaultValue;
}
- throw EvalUtils.getMissingFieldException(obj, name, thread.getSemantics(), "attribute");
+ throw EvalUtils.getMissingAttrException(obj, name, thread.getSemantics());
}
return result;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java b/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java
index 2d6ab08..4abb74e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java
@@ -228,7 +228,11 @@
// TODO(adonovan): logically this should be a parameter.
StarlarkSemantics semantics = StarlarkSemantics.DEFAULT_SEMANTICS;
for (String name : CallUtils.getMethodNames(semantics, v.getClass())) {
- env.put(name, CallUtils.getBuiltinCallable(semantics, v, name));
+ // We pass desc=null instead of the descriptor that CallUtils.getMethod would
+ // return because DEFAULT_SEMANTICS is probably incorrect for the call.
+ // The effect is that the default semantics determine which methods appear in
+ // env, but the thread's semantics determine which method calls succeed.
+ env.put(name, new BuiltinCallable(v, name, /*desc=*/ null));
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
index b13f04e..90caa5a 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
@@ -57,7 +57,7 @@
assertThat(provider.getName()).isEqualTo("prov");
assertThat(provider.getPrintableName()).isEqualTo("prov");
assertThat(provider.getErrorMessageFormatForUnknownField())
- .isEqualTo("'prov' object has no attribute '%s'");
+ .isEqualTo("'prov' value has no field or method '%s'");
assertThat(provider.isImmutable()).isTrue();
assertThat(Starlark.repr(provider)).isEqualTo("<provider>");
assertThat(provider.getKey()).isEqualTo(key);
diff --git a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformConfigurationApiTest.java b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformConfigurationApiTest.java
index 0787fe6..d2feb5f 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformConfigurationApiTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformConfigurationApiTest.java
@@ -170,6 +170,6 @@
assertThrows(AssertionError.class, () -> getConfiguredTarget("//verify:verify"));
assertThat(error)
.hasMessageThat()
- .contains("object of type 'platform' has no field 'enabled_toolchain_types'");
+ .contains("'platform' value has no field or method 'enabled_toolchain_types'");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 91c8041..2d442bd 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -402,7 +402,9 @@
public void testStackTraceMissingMethod() throws Exception {
runStackTraceTest(
"None",
- "\t\tNone.index(1)" + System.lineSeparator() + "type 'NoneType' has no method index()");
+ "\t\tNone.index"
+ + System.lineSeparator()
+ + "'NoneType' value has no field or method 'index'");
}
protected void runStackTraceTest(String object, String errorMessage) throws Exception {
@@ -1988,7 +1990,7 @@
checkError(
"test/skylark",
"r",
- "type 'Target' has no method output_group()",
+ "<target //test/skylark:lib> (rule 'cc_binary') doesn't have provider 'output_group'",
"load('//test/skylark:extension.bzl', 'my_rule')",
"cc_binary(name = 'lib', data = ['a.txt'])",
"my_rule(name='r', dep = ':lib')");
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 82f353d..710d364 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -645,7 +645,7 @@
EvalException expected = assertThrows(EvalException.class, () -> eval("attr.license()"));
assertThat(expected)
.hasMessageThat()
- .contains("type 'attr (a language module)' has no method license()");
+ .contains("'attr (a language module)' value has no field or method 'license'");
}
@Test
@@ -1123,7 +1123,7 @@
@Test
public void testStructAccessingUnknownField() throws Exception {
checkEvalErrorContains(
- "'struct' object has no attribute 'c'\n" + "Available attributes: a, b",
+ "'struct' value has no field or method 'c'\n" + "Available attributes: a, b",
"x = struct(a = 1, b = 2)",
"y = x.c");
}
@@ -1131,7 +1131,7 @@
@Test
public void testStructAccessingUnknownFieldWithArgs() throws Exception {
checkEvalErrorContains(
- "type 'struct' has no method c()", "x = struct(a = 1, b = 2)", "y = x.c()");
+ "'struct' value has no field or method 'c'", "x = struct(a = 1, b = 2)", "y = x.c()");
}
@Test
@@ -1206,7 +1206,7 @@
@Test
public void testGetattrNoAttr() throws Exception {
checkEvalErrorContains(
- "'struct' object has no attribute 'b'\nAvailable attributes: a",
+ "'struct' value has no field or method 'b'\nAvailable attributes: a",
"s = struct(a='val')",
"getattr(s, 'b')");
}
@@ -1645,8 +1645,8 @@
"p1 = p(y = 2)",
"x = p1.x"
);
- MoreAsserts.assertContainsEvent(ev.getEventCollector(),
- " 'p' object has no attribute 'x'");
+ MoreAsserts.assertContainsEvent(
+ ev.getEventCollector(), " 'p' value has no field or method 'x'");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index 5f2bb99..0dadd37 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -2427,7 +2427,7 @@
ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions());
CommandLineExpansionException e =
assertThrows(CommandLineExpansionException.class, () -> action.getArguments());
- assertThat(e.getMessage()).contains("type 'string' has no method nosuchmethod()");
+ assertThat(e).hasMessageThat().contains("'string' value has no field or method 'nosuchmethod'");
}
@Test
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 0d55dc1..56762cc 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
@@ -169,8 +169,7 @@
public void testGetAttrMissingField() throws Exception {
new SkylarkTest()
.testIfExactError(
- "object of type 'string' has no attribute 'not_there'",
- "getattr('a string', 'not_there')")
+ "'string' value has no field or method 'not_there'", "getattr('a string', 'not_there')")
.testExpression("getattr('a string', 'not_there', 'use this')", "use this")
.testExpression("getattr('a string', 'not there', None)", Starlark.NONE);
}
@@ -203,13 +202,13 @@
new SkylarkTest()
.update("s", new AStruct())
.testIfExactError(
- "object of type 'AStruct' has no attribute 'feild' (did you mean 'field'?)",
+ "'AStruct' value has no field or method 'feild' (did you mean 'field'?)",
"getattr(s, 'feild')");
}
@Test
public void testGetAttrWithMethods() throws Exception {
- String msg = "object of type 'string' has no attribute 'cnt'";
+ String msg = "'string' value has no field or method 'cnt'";
new SkylarkTest()
.testIfExactError(msg, "getattr('a string', 'cnt')")
.testExpression("getattr('a string', 'cnt', 'default')", "default");
@@ -478,7 +477,7 @@
.testExpression("repr(range(1,2,3))", "range(1, 2, 3)")
.testExpression("type(a)", "range")
.testIfErrorContains("unsupported operand type(s) for +: 'range' and 'range'", "a + a")
- .testIfErrorContains("type 'range' has no method append()", "a.append(3)")
+ .testIfErrorContains("'range' value has no field or method 'append'", "a.append(3)")
.testExpression("str(list(range(5)))", "[0, 1, 2, 3, 4]")
.testExpression("str(list(range(0)))", "[]")
.testExpression("str(list(range(1)))", "[0]")
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 56b1ca8..dcb9f0f 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
@@ -103,8 +103,7 @@
@SkylarkCallable(name = "struct_field_callable", documented = false, structField = true)
public BuiltinCallable structFieldCallable() {
- return CallUtils.getBuiltinCallable(
- StarlarkSemantics.DEFAULT_SEMANTICS, SkylarkEvaluationTest.this, "foobar");
+ return new BuiltinCallable(SkylarkEvaluationTest.this, "foobar");
}
@SkylarkCallable(
@@ -167,8 +166,7 @@
@SkylarkCallable(name = "struct_field_callable", documented = false, structField = true)
public Object structFieldCallable() {
- return CallUtils.getBuiltinCallable(
- StarlarkSemantics.DEFAULT_SEMANTICS, SkylarkEvaluationTest.this, "foobar");
+ return new BuiltinCallable(SkylarkEvaluationTest.this, "foobar");
}
@SkylarkCallable(name = "interrupted_struct_field", documented = false, structField = true)
@@ -1059,7 +1057,7 @@
public void testJavaCallsNotSkylarkCallable() throws Exception {
new SkylarkTest()
.update("mock", new Mock())
- .testIfExactError("type 'Mock' has no method value()", "mock.value()");
+ .testIfExactError("'Mock' value has no field or method 'value'", "mock.value()");
}
@Test
@@ -1073,20 +1071,21 @@
public void testJavaCallsNoMethod() throws Exception {
new SkylarkTest()
.update("mock", new Mock())
- .testIfExactError("type 'Mock' has no method bad()", "mock.bad()");
+ .testIfExactError("'Mock' value has no field or method 'bad'", "mock.bad()");
}
@Test
public void testJavaCallsNoMethodErrorMsg() throws Exception {
new SkylarkTest()
- .testIfExactError("type 'int' has no method bad()", "s = 3.bad('a', 'b', 'c')");
+ .testIfExactError("'int' value has no field or method 'bad'", "s = 3.bad('a', 'b', 'c')");
}
@Test
public void testJavaCallWithKwargs() throws Exception {
new SkylarkTest()
.update("mock", new Mock())
- .testIfExactError("type 'Mock' has no method isEmpty()", "mock.isEmpty(str='abc')");
+ .testIfExactError(
+ "'Mock' value has no field or method 'isEmpty'", "mock.isEmpty(str='abc')");
}
@Test
@@ -1260,7 +1259,8 @@
@Test
public void testNoJavaCallsWithoutSkylark() throws Exception {
- new SkylarkTest().testIfExactError("type 'int' has no method to_string()", "s = 3.to_string()");
+ new SkylarkTest()
+ .testIfExactError("'int' value has no field or method 'to_string'", "s = 3.to_string()");
}
@Test
@@ -1313,10 +1313,7 @@
@Test
public void testCallingInterruptedFunction() throws Exception {
- update(
- "interrupted_function",
- CallUtils.getBuiltinCallable(
- StarlarkSemantics.DEFAULT_SEMANTICS, this, "interrupted_function"));
+ update("interrupted_function", new BuiltinCallable(this, "interrupted_function"));
assertThrows(InterruptedException.class, () -> eval("interrupted_function()"));
}
@@ -1462,7 +1459,7 @@
new SkylarkTest()
.update("mock", new MockClassObject())
.testIfExactError(
- "object of type 'MockClassObject' has no field 'fild' (did you mean 'field'?)",
+ "'MockClassObject' value has no field or method 'fild' (did you mean 'field'?)",
"mock.fild");
}
@@ -1471,7 +1468,7 @@
new SkylarkTest()
.update("mock", new Mock())
.testIfExactError(
- "object of type 'Mock' has no field 'sturct_field' (did you mean 'struct_field'?)",
+ "'Mock' value has no field or method 'sturct_field' (did you mean 'struct_field'?)",
"v = mock.sturct_field");
}
@@ -1645,7 +1642,7 @@
@Test
public void testDotExpressionOnNonStructObject() throws Exception {
new SkylarkTest()
- .testIfExactError("object of type 'string' has no field 'field'", "x = 'a'.field");
+ .testIfExactError("'string' value has no field or method 'field'", "x = 'a'.field");
}
@Test
@@ -2082,7 +2079,8 @@
}
// This class extends NativeInfo (which provides @SkylarkCallable-annotated fields)
- // with additional fields from a map.
+ // with additional fields from a map. The only production code that currently
+ // does that is ToolchainInfo and its subclasses.
@SkylarkModule(name = "SkylarkClassObjectWithSkylarkCallables", doc = "")
private static final class SkylarkClassObjectWithSkylarkCallables extends NativeInfo {
@@ -2190,8 +2188,11 @@
.testLookup("v", "fromSkylarkCallable");
}
+
@Test
public void testStructMethodDefinedInValuesAndSkylarkCallable() throws Exception {
+ // This test exercises the resolution of ambiguity between @SkylarkCallable-annotated
+ // fields and those reported by ClassObject.getValue.
new SkylarkTest()
.update("val", new SkylarkClassObjectWithSkylarkCallables())
.setUp("v = val.collision_method()")
@@ -2204,7 +2205,7 @@
.update("val", new SkylarkClassObjectWithSkylarkCallables())
.testIfExactError(
// TODO(bazel-team): This should probably list callable_only_method as well.
- "'struct_with_skylark_callables' object has no attribute 'nonexistent_field'\n"
+ "'struct_with_skylark_callables' value has no field or method 'nonexistent_field'\n"
+ "Available attributes: callable_only_field, collision_field, collision_method, "
+ "values_only_field, values_only_method",
"v = val.nonexistent_field");
@@ -2215,8 +2216,9 @@
new SkylarkTest()
.update("val", new SkylarkClassObjectWithSkylarkCallables())
.testIfExactError(
- // TODO(bazel-team): This should probably match the error above better.
- "type 'SkylarkClassObjectWithSkylarkCallables' has no method nonexistent_method()",
+ "'struct_with_skylark_callables' value has no field or method 'nonexistent_method'\n"
+ + "Available attributes: callable_only_field, collision_field, collision_method, "
+ + "values_only_field, values_only_method",
"v = val.nonexistent_method()");
}
@@ -2313,4 +2315,14 @@
"var = GlobalSymbol")
.testLookup("var", "other");
}
+
+ @Test
+ public void testFunctionEvaluatedBeforeArguments() throws Exception {
+ // ''.nonesuch must be evaluated (and fail) before f().
+ new SkylarkTest()
+ .testIfErrorContains(
+ "'string' value has no field or method 'nonesuch'",
+ "def f(): x = 1//0",
+ "''.nonesuch(f())");
+ }
}
diff --git a/src/test/shell/integration/analysis_test_test.sh b/src/test/shell/integration/analysis_test_test.sh
index 4545749..1ad2f63 100755
--- a/src/test/shell/integration/analysis_test_test.sh
+++ b/src/test/shell/integration/analysis_test_test.sh
@@ -164,7 +164,7 @@
cat "${PRODUCT_NAME}-testlogs/package/test_target/test.log" > "$TEST_log"
expect_log "target_under_test failed"
- expect_log "type 'string' has no method method_doesnt_exist()"
+ expect_log "'string' value has no field or method 'method_doesnt_exist'"
}
run_suite "analysis_test rule tests"
diff --git a/src/test/starlark/testdata/function.sky b/src/test/starlark/testdata/function.sky
index d9a9043..a8fe7bb 100644
--- a/src/test/starlark/testdata/function.sky
+++ b/src/test/starlark/testdata/function.sky
@@ -54,7 +54,7 @@
assert_eq(y, [])
---
-getattr("", "abc") ### object of type 'string' has no attribute 'abc'
+getattr("", "abc") ### 'string' value has no field or method 'abc'
---
x = getattr("", "pop", "clear")
x() ### 'string' object is not callable
diff --git a/src/test/starlark/testdata/list_mutation.sky b/src/test/starlark/testdata/list_mutation.sky
index e3321e0..d9e5865 100644
--- a/src/test/starlark/testdata/list_mutation.sky
+++ b/src/test/starlark/testdata/list_mutation.sky
@@ -26,7 +26,7 @@
assert_eq(b, [])
---
-(1, 2).insert(3) ### type 'tuple' has no method insert\(\)
+(1, 2).insert(3) ### 'tuple' value has no field or method 'insert'
---
# append
@@ -38,7 +38,7 @@
assert_eq(foo, ['a', 'b', 'c', 'd'])
---
-(1, 2).append(3) ### type 'tuple' has no method append\(\)
+(1, 2).append(3) ### 'tuple' value has no field or method 'append'
---
# extend
@@ -50,7 +50,7 @@
assert_eq(foo, ['a', 'b', 'c', 'd', 'e', 'f', 'g'])
---
-(1, 2).extend([3, 4]) ### type 'tuple' has no method extend()
+(1, 2).extend([3, 4]) ### 'tuple' value has no field or method 'extend'
---
[1, 2].extend(3) ### type 'int' is not iterable
@@ -71,7 +71,7 @@
assert_eq(foo, [])
---
-(1, 2).remove(3) ### type 'tuple' has no method remove\(\)
+(1, 2).remove(3) ### 'tuple' value has no field or method 'remove'
---
[1, 2].remove(3) ### item 3 not found in list
---
@@ -93,7 +93,7 @@
---
[1, 2].pop(3) ### index out of range (index is 3, but sequence has 2 elements)
---
-(1, 2).pop() ### type 'tuple' has no method pop()
+(1, 2).pop() ### 'tuple' value has no field or method 'pop'
---
# clear
