Sdk: DDC outputs infinite recursive calls for external getters/setters without @JS() annotation

[nshahan]

[Edit by eernstg on Dec 1 2020: The specification was changed, cf. [this comment](https://github.com/dart-lang/sdk/issues/42730#issuecomment-725987641), such that there is an implementation defined choice: Emit a compile-time error at an external declaration which will not be connected to an implementation, or throw a NoSuchMethodError at run time each time such an external entity is invoked. The task associated with this issue is to make DDC emit said compile-time error for externals with no @JS() annotation.]

See failures in language/external_abstract_fields/external_fields_test.

By comparison dart2js produces compile time errors when the @JS() annotation is missing. My initial thought was that we should make DDC consistent with dart2js and produce the same compile time errors. Potentially we can move the checks and errors to the shared package:js_interop_checks.

@eernstg pointed out that the language actually specifies:

Attempting to invoke an external function that has not been connected to its body will throw a NoSuchMethodError or some subclass thereof.

@rakudrama @sigmundch Do you think we would actually want to support that behavior when external methods are not actually bound because they are missing the @JS() annotation? I would only suggest doing that if we implement it consistently in both compilers.

[sigmundch]

/cc @leafpetersen

mmm... In this case we know statically that there will be nothing behind that external method. So I'd prefer if we reported it as a static error.

I wonder if we should revise the spec instead.

[eernstg]

It really seems preferable to report the error at compile-time when that is possible.

But since the mechanism is implementation specific it would be possible for an external function to be bound/un-bound/re-bound at run time. For instance, it could be bound lazily, and that might be useful from a program size and performance perspective, if there are lots of external declarations and only few of them are ever used.

So maybe we should adjust the specification to say that a compile-time error as well as a dynamic error are acceptable treatments of the situation where an external function doesn't have a binding, and then any test of the specific behavior will have to be tool specific.

And language/external_abstract_fields/external_fields_test.dart would need to be marked (presumably in a status file) as "only for front-ends and configurations that throw at run time".

[srujzs]

I agree with making this a static error in package:js. I have two questions here:

1. Where besides JS interop does it make sense to have external members for DDC/dart2js? Besides Native, I had assumed all external members should belong to a JS interop context.
2. Why is this NNBD-specific?

[nshahan]

1. Where besides JS interop does it make sense to have external members for DDC/dart2js? Besides Native, I had assumed all external members should belong to a JS interop context.

AFAIK the only valid use for external in the web compilers is js interop. The SDK libraries do use external when the implementation should be backend specific and we use the backend specific patch files to provide the implementation. I _think_ those patch file replacements get preformed by the CFE and should be gone once we get the kernel.

1. Why is this NNBD-specific?

It's not related to the null safety feature but the language feature proposal to allow the external keyword in more places got accepted and is being released in the same release as null safety. I added the issue to our null safety tracking list simply because we need to implement it with the same deadline.

[nshahan]

@eernstg
Has there any progress made towards updating the specification to allow for backends to produce compile time errors when they recognize the external implementation will not be available at runtime?

[nshahan]

We should also catch the same issue for top level externals.

external int i1;

main() {
  Expect.throws(() { i1;}, (e) => e is NoSuchMethodError);
}

This is an example runtime test, but a compile time error would be preferred since we know it wont work without the JS interop annotation.

[eernstg]

The specification update is done in https://github.com/dart-lang/language/pull/1309, commit https://github.com/dart-lang/language/commit/12525ee3b7bf39a6bd8be53d2a4d3c3125dbd910.

[eernstg]

@nshahan, I edited the initial comment of this issue in order to make it explicit what needs to be done. Please adjust the text as needed if you think the description is imprecise. I haven't changed the title, because that might make it harder to find this issue for anyone who knows the current title.