Invocations of callable classes should be desugared in Kernel

[mraleph]

For simplicity of the backends I would expect that the method Something.invoke below would be represented in the Kernel AST as (this.delegate).call(value). However currently it is represented as a method invocation of this.delegate(value) instead.

class Callable<T> {
  void call(T value) {} 
}

class Something<T> {
  Callable<T> delegate;
 
  void invoke(T value) { delegate(value); }
}

For simplicity on the backend site

[kmillikin]

I agree. I'm surprised we don't have an issue open to track it. We generate

MethodInvocation(This, 'delegate', [VariableGet(...)])

we should generate:

MethodInvocation(PropertyGet(This, 'delegate'), 'call', [VariableGet(...)])

Additionally, we should introduce FunctionInvocation for invoking an expression with a function static type, and then for this code:

class Something<T> {
  void Function(T) delegate;
 
  void invoke(T value) { delegate(value); }
}

we should generate:

FunctionInvocation(PropertyGet(This, 'delegate'), [VariableGet(...)])

/cc @johnniwinther

[kmillikin]

Yuck, we are even confusing our own selves over this. I've started work on fixing it.

[alexmarkov]

Note that in general case receiver, getter and argument(s) could have side-effects and we should be careful in keeping order of evaluation.

For the call via field/getter

receiver.foo(arg)

It would be incorrect to generate

MethodInvocation(PropertyGet(receiver, 'foo'), 'call', [arg])

Because it evaluates expressions in a wrong order:

• receiver
• receiver.foo
• arg
• receiver.foo.call(arg)

The correct order is

• receiver
• arg
• receiver.foo
• receiver.foo.call(arg)

So we might need to generate something a little bit more sophisticated here.

[kmillikin]

I have begun working on this. Obviously we will implement the correct order of evaluation and getter invocation, but it is not as described in #34497 (comment). It was changed by CL 51323 in September of 2018.

[alexmarkov]

I didn't know about that spec change, and in the comment above I assumed that we would like to keep the current behavior. The spec change you mentioned is obviously a breaking change. As far as I understand the wording of the new spec, current implementation of calls in the VM does not adhere to the new spec.

How it should work in case of dynamic calls (when receiver is dynamic in receiver.foo(arg))?
Currently, on the caller side receiver and arg are evaluated and the call is performed (so arguments are evaluated before receiver.foo is looked up). When resolving a target for a call, if receiver.foo happens to be a method, the method is invoked. If it happens to be a field/getter, then it is evaluated and getter_result.call(arg) is called (so getter is called after arguments are evaluated). If I understand the changed spec correctly, new semantics requires that getter is called before arguments are evaluated. This would require changing how dynamic calls are implemented on the caller side.

/cc @lrhn @leafpetersen @a-siva @rmacnak-google @mraleph

[lrhn]

Dynamic invocations are tricky, but should always match the non-dynamic invocation if the object has a matching method or getter.
I'll try summarizing my understanding (but please do correct me if I'm wrong, I'll read the spec again too).

The evaluation of receiver.foo(arg), if receiver has a static type where foo is not a method and the static type of receiver.foo is a callable class type, is the same as receiver.foo.call(arg). You should evaluate receiver.foo before arg.

If receiver.foo has static type dynamic or Function, but is still not a method on receiver (so it's a getter), then it's a dynamic invocation of the value of receiver.foo to the argument list ([arg]), and receiver.foo should still be evaluated before arg.

If receiver has type dynamic, then receiver.foo(arg) must first do a lookup of foo on receiver to see what it finds.

• If foo is a method, evaluate arg and call the method with that result (checking arity and types as usual)
• If foo is a getter, evaluate receiver.foo to a value v, then continue to perform a dynamic invocation of v with arg as argument.
  • If v is a function, then evaluate arg and call the function with the result.
  • If v is a callable object, dynamically look up call on v (which must be a method), and call the call method of v with the result.
  • Otherwise evaluate arg to a value a and invoke the noSuchMethod of v with Invocation.method(#call, [a]).
• otherwise evaluate arg to a value a and invoke the noSuchMethod method of receiver with Invocation.method(#foo, [a]).

[eernstg]

@alexmarkov wrote:

The spec change you mentioned is obviously a breaking change

The change to the evaluation order which is specified in the language specification as of CL 51323 was actually about a year older: It was accepted by the language team in 2017 and reported here (thanks, @kmillikin for helping to track that down!).

CL 51323 was a rather large update that fixed a number of known issues (for instance, the given specification of lookups didn't work for superinvocations, it wasn't specified that things like int x = 'Hello' as dynamic; should cause a dynamic error when x is an instance or local variable, initializer list elements like this.x = 42 weren't specified to cause a compile-time error when x doesn't exist or when it has a type that int isn't assignable to, and so on).

Adjusting the evaluation order to match that 2017 decision was one of these required corrections. It should probably have been singled out and announced explicitly as a breaking change, but it was in any case not an accident to specify the left-to-right evaluation order.

The newsletter mentions that

Our implementations (VM, dart2js and DDC) all behave differently,
and dart2js and DDC aren't even internally consistent. Dart2js'
behavior depends on optimizations, and DDC treats dynamic and
non-dynamic calls (on the same member) differently.

which would suggest that some changes in some tools may be needed in any case (unless all tools have since then been fixed, such that they precisely implement the pre-51323 semantics).

@alexmarkov, do you think there is a need to initiate a more elaborate breaking-change procedure for handling this?

[alexmarkov]

I'm not very familiar with breaking change process. According to https://github.com/dart-lang/sdk/blob/master/docs/process/breaking-changes.md we need to follow the procedure for all breaking changes:

Anyone wishing to make a breaking change to Dart is expected to perform the following steps. It is expected that all of these steps are followed prior to a change being released in a dev channel release.

@kmillikin I'm sorry for hijacking your issue for the discussion about the language change.

@eernstg Is there a VM implementation issue corresponding to that language change?

[kmillikin]

@alexmarkov the breaking change process is awesome but it is from January 2019. It was not followed in 2017, the language newsletters were used for those announcements back then.

Note that the code that was broken by this change is code that (1) invokes a function-valued getter and (2) relies on side effects of an argument expression to change the value returned by that getter. Also note that our compilers themselves weren't consistent then (and still aren't, I guess).

[eernstg]

I've created #36744 as a meta-issue for the implementation of the left-to-right evaluation order, plus tool specific subissues. Turns out that at least the vm and dart2js do not have this behavior today (but perhaps DDC has it). I believe that the right way ahead is to get it implemented.

If we need to take additional steps because this is a breaking change it can all be handled in the context of #36744. So please put comments on that topic into that issue.

(@kmillikin, sorry about temporarily hijacking this issue for the evaluation order thing, it should all be encapsulated in #36744 from now on ;-).

[johnniwinther]

With https://dart-review.googlesource.com/c/sdk/+/129702 field/getter invocation is now encoded as a .call on a property get. The VM uses this encoded whereas ddc/dart2js currently opts out because breakage wrt. js-interop. This implementation hoists the arguments to preserve the old evaluation order. It is a separate (breaking) step to change the evaluation order on these to left-to-right.