After all these years, should we support d() -> d.call() even in the case where call is a getter?

[eernstg]

[Edit: The language team decided on Jan 3 2024 that the answer is "No" (see this comment).]

See dart-lang/sdk#51517 for some background information.

The language specification has specified for at least 6 years that an invocation of the form e<typeArgs>(args) where e has static type dynamic must handle a callee o (the value of e) which is not a function object in the following way:

• Check that o is an instance of an interface type that has a method named call.
• Execute o.call<typeArgs>(args).

(and this includes a similar rule where there are no actual type arguments and/or no actual value arguments because "typeArgs and args can be empty".)

However, the implementations (at least the VM, the JavaScript output from dart compile js, and the executable provided by dart compile exe, on x64) supports the following scenario as well:

• Check that o is an instance of an interface type that has a getter named call.
• Invoke that getter, o.call, to obtain an object fo.
• Execute fo<typeArgs>(args) (which may run the same algorithm recursively).

The language team has had discussions about this behavior previously (a long time ago), and did not support the generalization. That is, the language team does not want the second scenario to be supported.

However, many backends (perhaps even all of them?) support the second scenario, and @mraleph suggested in dart-lang/sdk#51517 that we should change the specification such that the second scenario is allowed: (1) This is the most convenient decision because this behavior is implemented today, and (2) it would be a breaking change to stop supporting scenario 2.

I believe we had some arguments about the potential performance implications of supporting scenario 2, but at this time I can't see any reason why this would be important. In particular, I can't see why supporting scenario 2 would cause function invocation to be slower in all cases, or function objects would occupy more space in all cases; it seems more likely that it just makes dynamic function invocations more expensive, and they are already allowed to be expensive because we assume that they are rare (and should be even rarer ;-).

@dart-lang/language-team, WDYT? Should we change the specification such that scenario 2 is supported? Alternatively, should we initiate a breaking change process about scenario 2 being unsupported in the future?

@mkustermann, @mraleph, @sigmundch, @nshahan, @askeksa, @osa1, WDYT? Are you aware of any optimization opportunities that are made impossible in the case where the language supports scenario 2? And assuming that it is correct that we do this already, it's more like: Are you aware of any optimization opportunities that we could benefit from in the future if we stop supporting scenario 2?

@johnniwinther, WDYT? As far as I understand @osa1's comment, the CFE generates code where it is impossible to make the distinction between the invocation of a method and the invocation of a getter that yields a function object which is subsequently invoked. I thought that this had now been separated out into two distinct kinds of Kernel code, and also that it had been beneficial for function invocation performance in general to make this distinction explicit in Kernel code. Is this situation special because it is an invocation of an object of type dynamic?

[lrhn]

TL;DR: No.

The dynamic invocation case of special in that we don't know whether d.foo(arg) is a method invocation or a getter invocation followed by a function value invocation.

In all other cases, the type signature of the receiver makes it one or the other.

The underlying design principle of dynamic member invocations is that they should work the same way (within reason) as the same member invocation on the same receiver, if it had had its runtime type as static type.

That otherwise does not involve things happening during type inference, or any coercions.

(List.of as dynamic)(<int>[2])

will not infer a type argument, it creates a List<dynamic>.

The .call insertion for callable objects is special in that it partly works for dynamic, in calls, but not in downcast.
It would be consistent if it just didn't work. If the value is not a function, you can't call it dynamically, even if it has a call method.
But that'd be breaking.

So we have a specified behavior of calling callable non-function objects, which we apply to dynamic invocations too. Which is explainable.

And then implementations go one step further, and allow a dynamic invocation of an object which has no call method, because the syntax of .call(args), which we only "insert" if the receiver has a call method, would be valid for the type. (maybe, if the getter returns a function, but what if it returns a callable object? Then we recurse on that, to the point where dynamic get call => this; gives a stack overflow, entirely within the implementation of a single dynamic call. Preventing that was one of the reasons behind requiring a call method.)

I still prefer to keep the current specification, allow the implementations to implement it, categorize the extra behavior of existing implementations as legacy bugs, and let them remove it if they want to.

[mraleph]

Just want to check if this is also on the list of things to discuss for the language team. If language team prefers to keep the currently specified behavior we need to slate necessary work in CFE and backends to pay off this technical debt.

[eernstg]

The language team did discuss this issue today, but we did not reach a conclusion. Hopefully we will soon.

[leafpetersen]

Per @nshahan the current implemented semantics for dart2js, VM and DDC are as follows:

class C {
  Function get call => () {
        print('getter call function');
      };
}

main() {
  dynamic c = C();
  
  // ddc: NSM error 'call'
  // VM: works
  // dart2js: works
  c.call();

  // ddc: NSM error 'call'
  // VM: works
  // dart2js: works
  c();
}

So either way we have some work to do in the implementations.

I believe that the first example using c.call() we intend to work in either alternative. So this is just an unconditional bug in DDC.

The second example using c() is the issue under discussion here.

[johnniwinther]

The current CFE encoding of both c() and c.call() is DynamicInvocation(c, 'call', Arguments()). To support a distinction, we would need a new node DynamicFunctionInvocation(c, Arguments()) for c(). It shouldn't be a problem to do this.

[fishythefish]

Why is it important to prevent the dynamic get call => this; stack overflow? Is anyone writing that and expecting different behavior?

[mraleph]

Why is it important to prevent the dynamic get call => this; stack overflow? Is anyone writing that and expecting different behavior?

+1 to this question. (There is one case of this in VM's core libraries - _Closure has get call => this;, but it is never going to cause stack overflow because closures are handled specially by the invocation sequence anyway).

[lrhn]

The stack overflow isn't so much the problem, as it is a symptom that something more complicated than necessary is going on in what looks like a simple function call.

class C {
  static int ctr = 0;
  C get call { if (ctr & ++ctr == 0) print(ctr); return this; }
}
void main() {
  foo(C());
}
foo(dynamic f) {
  f(); // Looks innocuous. Stack-overflows, or does an infinity of work with only a single call.
}

And does stack-overflow in dart2js and the JIT VM:

1
... 2..4096 ...
8192
Unhandled exception:
Stack Overflow
#0      C.call (file:///.../so.dart:3:3)
...
...
#15943  foo (file:///.../so.dart:9:4)
#15944  main (file:///.../so.dart:6:3)
#15945  _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#15946  _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

I agree that the AoT VM doesn't seem to stack overflow.

But it's not that problem that makes me want to change implementation.

It's more important to me to follow the guiding principle of dynamic invocations, which is that the invocation works like the similar invocation would, had the receiver had its runtime type as static type. But that'll be without any compile-time coercions, since it's happening at runtime.
Even allowing (C() as dynamic)() to work is breaking with that principle, by applying a little of the compile-time coercion at runtime, but it's legacy behavior that was retained deliberately.

Allowing a call getter to get called as d() is unique to dynamic invocations. It wouldn't work if the receiver was typed at its static type. I don't want users to start using that for anything. I don't want to call it a "feature" and accept it, because it isn't. It's not something we want to work. It's something that accidentally works, even though it shouldn't.

Doing things dynamically should not be more powerful than casting to the actual type and doing the same thing.
I don't want being dynamic to be a feature people opt into, to get more capabilities, even when they do know the type.
(And yes, non-existing members calling noSuchMethod is also like that, and something I'd also personally be happy to get rid of.)

I don't want dynamic to provide a feature that they type itself doesn't.
And I don't want our next implementation to have to be bug-compatible with the current implemented behavior.

Taking the code above,

C c = C();
c(); // Compile-time error, `C` is not a function type and not a callable type. No `.call` insertion.
c.call(); // Compile-time error, c.call has type `C`, still not callable.
dynamic d = c;
d() // calls `call` method forever.
d.call() // calls `call` method forever.

To match the intended behavior, the last two should be runtime errors.

• d() when it recognizes that d is not a function type and has no call method.
• d.call() when it recognizes that d.call is not a method, then invoking the getter, and doing v() on the result dynamically, going back to the case above.

That is, the first item should match the actual .call insertion rules: If calling, not a function, the type it's called on has a call method, then invoke that method with the same arguments. Otherwise, it's just an error.

Eagerly expanding f() to f.call() is a kind of .call insertion, which is applied even where the actual .call insertion wouldn't do it, and it's done "syntactically", because it's performed as another dynamic invocation, which is why it can also apply to getters, something the actual .call insertion deliberately avoids.

[mraleph]

@lrhn I see. Thanks - this makes sense to me. I was looking at it from the perspective of d() and d.call() being equivalent to each other, but I can see now that there is another perspective which draws parallels between dynamic callsites and corresponding typed call-sites o() and o.call() which don't behave the same if o declares a getter.

To implement this behavior we need to retain the difference in Kernel AST and then retain this information in the generated code i.e. use different lookup stubs for d() and d.call() or retain this information on the side. It's a hassle but nothing prohibitively expensive - development cost is likely the only cost we will encounter here.

It is not clear to me if the same applies to JS backends: you need to carry over the difference between d() and d.call() to the JS and it is unclear if JS has some good way to express this. Currently dart2js compiles both d() and d.call into .call$0() - one of this will have to change to retain the difference. I guess the assumption is that the dynamic calls are infrequent and dynamic .call calls are even more so. Then it makes sense to change d.call() to be compile into something like d.explicit$call$0(...) and then massage the rest of object representation to support that.

But again this looks like a lot of hassle - and it is unclear if it worth it.

So while I get the argument that we might want d() and o() behave the same and d.call() and o.call() to behave the same maybe we should just say that there is no such thing as d() --- d() just means and behaves like d.call()?

[lrhn]

d() just means and behaves like d.call()?

But where does it end? Does d.call() mean and behave like d.call.call()?

I want to say no, but if d.call is a getter, then it'd have to be "yes".

Do we insert the .call during compilation? Probably not, what would we do about class C {C get call => this;} then?
Or will we just need cycle-detection? If there is a type-cycle among call getter return types (or only those that are invoked), the program won't compile?

Restricting .call insertion to a static coercion, based on type, was a way to avoid having classes that subtype function types, and to do complicated checking at runtime. If call is a method, we know it has a function type. If not, it can be anything.

[mraleph]

Does d.call() mean and behave like d.call.call()?

(d.call)() behaves like d.call.call(). But d.call() and d.call.call() are unrelated.

[eernstg]

The language team decided on Jan 3 that it is preferable to get the specified semantics implemented (that is, an implicitly invoked call will not invoke a getter, only a method). The main argument was that this semantics makes dynamic invocation semantics more similar to that of statically checked invocations, and it is desirable for consistency and comprehensibility reasons that the semantics of static and dynamic invocations do not differ gratuitously.

@mkustermann, @mraleph, @sigmundch, @nshahan, @osa1, considering the comment from @johnniwinther here, how much of an effort do you think it would be to use this new DynamicFunctionInvocation(c, Arguments()) Kernel construct?

[sigmundch]

cc @rakudrama

[nshahan]

I don't expect we will need any major changes at compile time to support the new representation from the CFE.

Supporting the new representation could also be the right time to revisit the runtime library to implement support for the dynamic invocations of .call() that DDC is currently failing on.

[osa1]

how much of an effort do you think it would be to use this new DynamicFunctionInvocation(c, Arguments()) Kernel construct?

This should be a straightforward change in dart2wasm.

I think it will require a special forwarder for the "call" method members, to be used in dynamic invocations of other members. Currently in dart2wasm, the code generated for a dynamic invocation of a method m considers both methods and fields. When m of a class is a field and the field value is not a closure, we call the "call" forwarder on the field value. This forwarder is generated the same way as the m's forwarder, but unlike the forwarder for m it shouldn't consider the fields.

[rakudrama]

My initial take is that this could be complicated change for dart2js.

For dart2js, this is effectively a new feature - splitting dynamic calls from instance calls.

Today, dart2js maintains the equivalence between dynamic an instance calls (o.f() behaving the same for either call) by not actually knowing whether the call is a dynamic or instance call. There is no distinction in large parts of the compiler between dynamic and instance calls. There is no separate calling convention or method entry point. This is a very 'Dart 1' approach, and one might argue that it should not be that way, but to make o.f() with a dynamic receiver behave differently to o.f() with an instance type receiver, even for a single selector (i.e. call), probably requires paying down a lot accumulated technical debt.

The upgrade might be worth it, the cost might be less than I initially think (e.g. by special-casing the construct - adding technical debt rather than paying it down), but please plan of this taking multiple releases and having quite a high cost. I'm busy with some more urgent issue right now but I can come back to this after those have been addressed.

[eernstg]

Thanks for the info, @rakudrama, that's very important to have in mind!

It would be great, however, if you could comment on a couple of questions about this situation.

My understanding is that the relevant terms (d() vs. d.call() in Dart) are provided in Kernel code as a an AST node like DynamicInvocation(d, 'call', Arguments()), and the plan is to update the CFE such that d.call() is still provided in that form, but d() is provided as a new node DynamicFunctionInvocation(d, Arguments()). The intention is now that the former can be executed by invoking a method named call with the given arguments, or by invoking a getter named call, obtaining a result d2, and then executing DynamicInvocation(d2, 'call', Arguments()); in contrast, DynamicFunctionInvocation(d, Arguments()) can only be executed if d has a method named call, which is then executed with said arguments. In other words, the distinction between one and the other kind of dynamic invocation will be explicit in Kernel code.

I do understand that it may be a substantial effort to handle the new kind of Kernel AST nodes and propagate the new information to all parts of dart2js, but it does seem likely to me that the required information is available when it has been propagated from the Kernel AST and into all parts of dart2js.

I don't quite understand how this change (we now have 2 kinds of dynamic invocations in Kernel rather than 1) can disrupt the relationship between dynamic invocations and statically checked instance invocations. I understand that those two actions may be modeled identically in dart2js (I'm not quite sure I understand how that's possible, but bear with me ;-).

In the end, the fact that is most surprising to me is that this difficulty is created by a change which was seen by the language team as a step in the direction of making the semantics of statically checked and dynamic invocations more consistent, not less.

Anyway, sorry about ranting about dart2js internals that I know so little about. Comments are very welcome! ;-)

[sigmundch]

After chatting w/ @rakudrama and @leafpetersen I feel OK moving forward with the language decision #3482 (comment) with the understanding that fixing this bug is not an immediate priority for backend teams (e.g. P3).

This would mean to keep the spec and tests as they are. This aligns static & dynamic semantics, but implies that backends have an implementation bug going forward.

Circling back to the cost question - I believe the cost for fixing this in dart2js can be contained by special casing what we do for .call and ignoring anything else (very much what @mraleph suggested earlier). It's not entirely trivial, and I would guess it could be done in a couple weeks of dedicated time. There is an interesting overlap between this and what's needed to better support optimizing away covariant checks in our -O2 mode (rather than omitting them like we do in -O3), but we'll consider that as a separate thing to keep this discussion honest.

[johnniwinther]

I've landed https://dart-review.googlesource.com/c/sdk/+/346240 which adds an isImplicitCall flag to the DynamicInvocation node such that backends can now distinguish d() where isImplicitCall is true from d.call() where isImplicitCall is false.

[alexmarkov]

VM is fixed to conform to the spec in dart-lang/sdk@4305541.