commit 4e2b4958d508ef2ed65db7b681c3be5939e7f147
author adonovan <adonovan@google.com> Tue Dec 10 12:19:20 2019 -0800
committer Copybara-Service <copybara-worker@google.com> Tue Dec 10 12:20:15 2019 -0800
tree 1401bd17b2db67b371823ca54cfb5a26d4dbb5de
parent 3c115a0944a81cec632c5493a3d84ff8ecb50f80

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