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/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
