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/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())");
+ }
}
