Dynamic invocations incorrectly access "call" field in VM, dart2js, dart2wasm

[osa1]

The test method_must_not_be_field_test checks that dynamic invocations do not implicitly access the field "call":

sdk/tests/language/call/method_must_not_be_field_test.dart

Lines 13 to 28 in 0c32c7f

	C c = new C();
	dynamic d = c;
	// The presence of a field named `call` does not permit the class `C` to be
	// implicitly called.
	c(); //# 01: compile-time error
	// Nor does it permit an implicit tear-off of `call`.
	void Function() f = c; //# 02: compile-time error
	// Nor does it permit a dynamic invocation of `call`.
	Expect.throws(() => d()); //# 03: ok
	// However, all these things are possible if `call` is mentioned explicitly.
	c.call(); //# 04: ok
	void Function() f = c.call; //# 05: ok
	d.call(); //# 06: ok
	(d.call)(); //# 07: ok

This test was introduced in https://dart-review.googlesource.com/c/sdk/+/42020, and it currently fails in VM, dart2js (at least on dartpad), and dart2wasm.

This test is in sync with the static call semantics when the receiver has a "call" field but not a method, e.g.

class C {
  void Function() call = () {};
}

main() {
  C()();
}

Fails with compile-time error:

test.dart:6:6: Error: Cannot invoke an instance of 'C' because it declares 'call' to be something other than a method.
 - 'C' is from 'test.dart'.
Try changing 'call' to a method or explicitly invoke 'call'.
  C()();
     ^

[mraleph]

Seems like a change in the semantics that snuck under the radar. @eernstg can you confirm that this was incorporated into the spec?

[osa1]

Fixing this will probably require kernel-level changes. Currently if I compile expression d() where d : dynamic, this is the kernel I get:

node=DynamicInvocation(DynamicAccessKind.Dynamic,d.call())
node.receiver=VariableGetImpl(d)
node.name=call
node.arguments=ArgumentsImpl(())

This is the exact same kernel I get when I compile d.call(), which has different runtime semantics than d() (according to this test, but not as currently implemented). So I can't distinguish the Dart expr d() from d.call(), they're both compiled to the same kernel.

cc @chloestefantsova

[chloestefantsova]

I'm tentatively adding area-front-end.

[mraleph]

Actually I am puzzled why d.call() is okay and d() is not. That is:

1. Inconsistent.
2. Handling d.call() for field case would require just as much machinery as d() does.

So for these reasons I would have expected d.call() and d() to both throw in runtime, while (d.call)() should work.

[eernstg]

@mraleph wrote:

[confirm that] this was incorporated into the spec?

This distinction ("callable objects must have a call method, it is not sufficient to have a call getter") has been specified for a long time.

The current language specification says 'method', not 'member' or 'method or getter':

If $o$ has a method named \CALL{}
the following ordinary method invocation is evaluated,
and its result is then the result of evaluating $i$:
\code{$f$.call<$A_1, \ldots,\ A_r$>($a_1, \ldots,\ a_n,\ x_{n+1}$: $a_{n+1}, \ldots,\ x_{n+k}$: $a_{n+k}$)}.

This text occurs without changes back to 2018-09-17 (and the change at that time was a complex reorganization, which means that this particular rule might have been the same also before that time, but I didn't try to verify that).

The main rationale is that it avoids infinite loops during the method lookup step, but also that the "callable object" feature should be rather simple and limited (as opposed to an approach where we try to "do what i mean" in as many different situations as possible). Here's the loop:

class C { get call => this; }
void main() => (C() as dynamic)();

This runs, and it shows (as already mentioned in earlier comments) that the actual behavior (in dart-pad, based on SDK 2.19.2) is a dynamic error:

Uncaught RangeError: Maximum call stack size exceededError: RangeError: Maximum call stack size exceeded

This raises some questions:

• Is it a non-trivial breaking change to stop supporting dynamic invocations via a getter named call (and raising a dynamic error instead)? In that case we might want to change the specification to include that feature, and leave the implementations unchanged. Otherwise we'd need a breaking change process.
• How important is it that statically checked and dynamic invocations have the same semantics? If we do support call getters for dynamic invocations, should we then also support them for statically checked invocations?
• Optimization opportunities: Would the specified semantics (with no support for call getters) allow for invocations in a more general sense to be simpler and faster? In particular, would there be anything to gain for dynamic function invocations? ... for all function invocations? In that case we might prefer to endure the breakage.

[sigmundch]

/cc @stereotype441

[mraleph]

The test is still sitting out there and failing on all backends. If we are not planning to fix this any time soon, then maybe we need to alter the spec instead and change the test?

[eernstg]

I created dart-lang/language#3482 to manage the discussion about changing the specification.

[lrhn]

We should consider a long-time failing test a process failure.

The specification has been this way for years, as far as I remember since Dart 2.0.
No implementation has changed, even though they're not following the specification, there is a test showing it, and an open issue stating so.

That should lead to either the implementations fixing the bug, or a renewed discussion about changing the spec, until we reach a print where they all agree, and implementations match specification.

Instead we got crickets. Maybe nobody really cared, because everything that should work worked, and then a little more which shouldn't, and we had bigger and more urgent things to do anyway.

That's why it's a process failure. Nobody had the job of following up and taking the next step, and everybody could just do something else.

[alexmarkov]

VM behavior was fixed in 4305541.