Left-to-right evaluation order is unlikely to be enforced for instance method invocations

[eernstg]

[New status Apr 2022, eernstg: This issue has been lingering for years; it is now more likely than not that we will change the specification to make the existing behavior the specified behavior. I closed the subissues; we can easily create new ones if we change our minds.]

Cf. the discussion in #34497, the October 2017 language team decision described here which was integrated into the language specification as part of CL 51323 has not been implemented everywhere.

Said decision has the following contents: When an ordinary method invocation i of the form e.m(arguments) is such that m is a getter, the evaluation of i proceeds by evaluating e to an object o, then invoking the getter o.m to an object f, and then evaluating f(arguments).

In other words, the evaluation order is strictly left-to-right, including the getter invocation. The old rules specified that the argument list should be evaluated first, and then the getter should be invoked.

Surely, this change request could have been communicated better. However, at this point I believe the way ahead is to adjust the implementations such that they support the left-to-right evaluation order which was specified and intended.

Here is an example:

import "package:expect/expect.dart";

int i = 0;

void f() => i *= 2;

class A {
  void Function(void) get m {
    ++i;
    return (_) {};
  }
}

main() {
  A().m(f());
  Expect.equals(i, 2);
}

This currently fails on the following trybots (I just used the standard selection of trybots), illustrating that dart2js as well as the vm needs adjustments: dart2js-minified-strong-linux-x64-d8-try, dart2js-strong-hostasserts-linux-ia32-d8-try, dart2js-strong-linux-x64-chrome-try, vm-kernel-linux-product-x64-try, vm-kernel-linux-release-simdbc64-try, vm-kernel-linux-release-x64-try.

I've chosen 'type-enhancement' for this issue, because it is concerned with the implementation of a specification change.

Subtasks

• dart (VM: Implement left-to-right evaluation order for instance method invocations #36745)
• dart2js (dart2js: Implement left-to-right evaluation order for instance method invocations #36746)
• DDC (DDC: Implement left-to-right evaluation order for instance method invocations #36747)

[sigmundch]

/cc @leafpetersen @rakudrama

Are our tools currently consistent with one another? If so, it might be desirable to use a feature flag to implement this change or to time the work so that all tools make the change on the same release.

FWIW - We have concerns on the dart2js side that this will be a non-trivial effort to prevent a performance regression. In particular, the current implementation matches the calling convention in JS and allows us to generate compact code. We can add the new evaluation order, but we'll need to implement an analysis to determine when the evaluation order doesn't matter* and in those cases continue to use the old code-pattern so we can prevent a big code-size and performance regression on the generated code.

*e.g. some scenarios we can optimize away: statically known invocation sites, dynamic invocations on selectors that are never declared as getters, dynamic invocations on selectors that may be getters but that are always pure.

[leafpetersen]

@eernstg Your test should also test dynamic invocations, since at least one implementation (DDC) I believe uses a difference evaluation order for the two.

[leafpetersen]

@eernstg This is a breaking change, we will need to file a breaking change request.

[leafpetersen]

If all of the tools are already consistent on this (or mostly consistent), and if we have performance concerns about making the change, I'm not sure it's clear to me that we should change the tools and not the spec.

cc @lrhn @munificent

[lrhn]

The current behavior is:

• receiver.getter(argument), where receiver is typed, is evaluated int the order (receiver, argument, receiver.getter, invocation) on VM and JS, and (receiver, receiver.getter, argument, invocation) in DDC. If you change it to (receiver.getter)(argument) then receiver.getter is always evaluated before argument. (This also holds if the getter returns a callable object instead of a function).
• If receiver has type dynamic, then the order is (receiever, arg, receiver.getter, invocation) in all three compilers.

I believe the reason for the change was that DDC uses JavaScript's evaluation order, which is receiver.getter before argument, and having to comply with the original Dart ordering of getter-before-argument would make the resulting code not be canonical JS code, which was a goal at the time.
As such, the change in behavior was effectively part of strong mode. The language team at the time, after consulting the other compiler teams, agreed to change the language.
Knowing that most invocations would be typed, so you know ahead of time whether it's a getter or a method, meant that late method-lookup was still a valid implementation choice. Dynamic invocations would be complicated, but they already are.

The (abbreviated) test program used to test this was:

List<String> trace = [];
T log<T>(String entry, [T value]) {
  trace.add(entry);  
  return value;
}

class C {
 void Function(Object) get g => log("get g", (arg) { log("call g"); });
}

C get r => log("get r", C());
dynamic get d => log("get d", C());

main() {
 log("3");
 r.g(log("arg"));
 log("4");
 log("r.g", r.g)(log("arg"));

 log("7");
 d.g(log("arg"));
 log("8");
 log("d.g", d.g)(log("arg"));
  
 print(trace.join("\n"));
}

The result is:

3
get r
arg
get g
call g
4
get r
get g
r.g
arg
call g
7
get d
arg
get g
call g
8
get d
get g
d.g
arg
call g

for VM and dart2js, and the only difference for DDC is swapping get g and arg in part "3".

If we keep the current majority behavior, and change the spec back, then it means that DDC can no longer compile o.getter(arg) into the JS jsObject.jsGetter(jsArg) because that gives the wrong evaluation order (unless it knows for a fact that one of jsGetter and jsArg has no side-effects, which pretty much requires it to be constant). There is no easy work-around, you have to evaluate all arguments prior to the call, rewriting o.getter(arg1, arg2) into var a1, a2; .... (a1 = arg1, a2=arg2, o.getter(a1,a2) ....
Dart2js avoids this problem by having an extra indirection. The o.g$1(...) call to a getter is a different method from the getter, and it internally invokes this.get$g().call$1(arg), which means that the real getter is invoked after arguments are evaluated. I guess DDC can do the same, but it means extra generated code instead of just implementing a Dart getter with a JS getter. (Which sounds nice, but only works if the two language's getters are competely compatible).

If we change to the DDC/JS behavior, then even DDC needs to change because its behavior for a dynamic receiver is still wrong. The other two compilers need to change their implementation of getter invocations to delay calling the getter until after evaluating the arguments.

[eernstg]

@sigmundch wrote:

Are our tools currently consistent with one another?

@lrhn already mentioned a test. Here's another one that includes more forms:

// FILE 'scratch_test.dart'.

import "package:expect/expect.dart";
import "scratch_lib.dart" as lib;

int i;

void f() => i *= 2;

class A {
  void Function(void) get m {
    ++i;
    return (_) {};
  }

  void absentInclass(int j) {
    i = j;
    m(f()); // No syntactic receiver, calling instance getter.
    Expect.equals((j + 1) * 2, i); //# absent_inclass: ok
  }

  static void Function(void) get n {
    ++i;
    return (_) {};
  }
}

void Function(void) get m {
  ++i;
  return (_) {};
}

main() {
  i = 0;
  A().m(f()); // Type of receiver is not `dynamic`.
  Expect.equals(2, i); //# checked: ok

  dynamic d = A();
  i = 2;
  d.m(f()); // Receiver has type `dynamic`.
  Expect.equals(6, i); //# dynamic: ok

  i = 6;
  A.n(f()); // Syntactic receiver is type literal.
  Expect.equals(14, i); //# static: ok

  i = 14;
  m(f()); // No syntactic receiver.
  Expect.equals(30, i); //# absent: ok

  A().absentInclass(30);

  lib.j = 0;
  lib.m(f()); // Syntactic receiver is a prefix.
  Expect.equals(2, lib.j); //# prefix: ok
}

// FILE 'scratch_lib.dart'.

int j;

void f() => j *= 2;

void Function(void) get m {
  ++j;
  return (_) {};
}

In the current SDK this produces the following failures:

FAILED: dartdevc-chrome release_x64 language_2/scratch_test/dynamic
FAILED: dartdevc-chrome release_x64 language_2/scratch_test/prefix
FAILED: dartdevk-chrome release_x64 language_2/scratch_test/dynamic
FAILED: dartdevk-chrome release_x64 language_2/scratch_test/prefix

FAILED: dart2js-d8 release_x64 language_2/scratch_test/checked
FAILED: dart2js-d8 release_x64 language_2/scratch_test/dynamic
FAILED: dart2js-d8 release_x64 language_2/scratch_test/absent_inclass
FAILED: dart2js-d8 release_x64 language_2/scratch_test/prefix

FAILED: none-vm release_x64 language_2/scratch_test/checked
FAILED: none-vm release_x64 language_2/scratch_test/dynamic
FAILED: none-vm release_x64 language_2/scratch_test/absent_inclass
FAILED: none-vm release_x64 language_2/scratch_test/prefix

This means that DDC and DDK evaluate left-to-right except for the invocation on a receiver of type dynamic and on a library prefix, and the vm and dart2js evaluate left-to-right on the invocation of a top-level or a static getter, and args-first in all other cases.

Presumably, this means that at least many developers have been debugging their programs under the specified left-to-right semantics in the typical case of statically checked instance method invocations, and then they've deployed programs where the old args-first evaluation order was used for those same call sites. So the situation is still not crystal clear, but the difference does not seem to break many programs.

One inconsistency that stands out is that the invocation of a static getter ('static') and the plain top-level getter invocation ('absent') are treated differently from the top-level getter invocation that involves a library prefix ('prefix'), in all tools.

[eernstg]

This topic was discussed recently in the language team. It is a gnarly corner of the language semantics, but we still want to reconcile the language specification and the actual implementations (of course — no language debt should be left behind ;-).

So here's the current status. The first column shows a specific kind of invocation, and the second column characterizes the evaluation order ("GA" means that the getter is evaluated before the actual arguments, and "AG" means that the arguments are evaluated before the getter):

Configuration	Semantics
Instance getter, static invocation	AG
Instance getter, dynamic invocation	AG
Static getter	GA
Top-level getter	GA
Prefix-imported getter	GA
Extension instance getter	GA
Extension static getter	GA

The CL https://dart-review.googlesource.com/238903 confirms that the behavior of all the standard try-bots is consistent with the above table (which wasn't true the previous time we discussed this).

So we could make the decision that the current behavior is what we want, and then we would adjust the language specification accordingly; or we could decide that we really want consistent left-to-right evaluation as specified, and then we'd need to assess and handle the breakage.

There is also the question about the implementation efforts, if we decide that we want consistent left-to-right evaluation.

@sigmundch, @srujzs, I get the impression that JavaScript interop is a source of particular difficulties in this context. Another topic of interest is dynamic invocations. WDYT?

@mkustermann, @a-siva, I believe that the evaluation order for statically checked invocations could be changed in the common front end, but it might require a substantial amount of backend work to change the evaluation order for dynamic invocations. WDYT?

[sigmundch]

Thanks for following up @eernstg - you are correct, there are many difficulties on the web side for both jsinterop and dynamic invocations (the concerns mentioned in the earlier comment #36744 (comment) still apply today). Those make me hesitate here. I think we should however reevaluate this in the context of the new static interop. It may be possible for us going forward to distinguish statically all jsinterop calls, to the point that instance getters would all be non-jsinterop. That would greatly simplify things on our end implementation wise.