Need spec and compile-time errors of incorrect JS interop usage

[jmesserly]

There are many invalid things that can be done with JS interop, but we don't have static errors for this.

Here are some examples of things that should be reported (or else, need to be specified and fixed in the DDC/dart2js):

• an anonymous JS class with positional arguments in the constructor. This is invalid, because it's supposed to create an object literal, and causes a DDC crash (see Error compiling dartdevc module #32031)
• use of generic functions/methods (currently unsupported in JS interop)
• use of generic classes (what is intended behavior?)
• use of @JS on a class/library member without a @JS on the library
• use of @JS on a class member instead of external
• declaring Dart members on a @JS class (Dart code is silently ignored)
• extends/mixin@JS classes from a Dart class (also unclear if implements is supported)
• extends/mixin/implements Dart class from @JS class

Errors should be reported from Analyzer so people can see the problem sooner. (Once DDC migrates to Kernel, need a way of calling into Analyzer to produce these kinds of errors. Or we just have shared code between dart2js/DDC for validating JS interop usage.)

---

Also it would be lovely to have a specification. These would be useful:

• where is @JS allowed to appear?
• where is external allowed to appear?
• inheritance implications of the various permutations of Dart and JS classes and member kinds
• interaction with Dart generic classes
• interaction with Dart generic methods/functions
• meaning of Dart members on a JS class
• semantics of anonymous JS types
• how JS types fit into the Dart subtyping relation and Dart Type equality
• how JS functions related to the Dart type system
• static and dynamic call/get/set/index/operator/tearoff semantics
• behavior of dart:core Object members (toString, hashCode, operator ==, noSuchMethod, runtimeType) for JS objects
• interaction with the JS global namespace
• interaction with JS modules

[sigmundch]

Once DDC migrates to Kernel, need a way of calling into Analyzer to produce these kinds of errors. Or we just have shared code between dart2js/DDC for validating JS interop usage.

My preference is for the latter: I can imagine we'd have a js-interop checker implemented as a kernel-visitor that we can both share.

[matanlurey]

It would be ideal for the analyzer to give errors/warnings before a compilation failure, if possible.

(It's not clear to me we have a good strategy yet for web/JS-specific analyzer checks, so maybe we need a thread/doc/discussion about that separate from js-interop checking in the compilation pipeline)

[jmesserly]

Yeah for JS interop, we can have shared checks between DDC/dart2js/Analyzer. That way they show up in IDE as well as compilers.

At the moment I don't really care where the checks live :) ... I'm mostly concerned that JS interop is completely unchecked and there's all kinds of undefined behavior people can, and do stumble into.

(And personally, when I have to touch DDC's implementation code for JS interop, I have no idea what's supposed to be legal or not, so it's really difficult to refactor any of the code that touches that.)

[matanlurey]

@vsmenon and I just ran into a client internally with another edge, non-`abstract classes, i.e.:

@JS()
library js_interop;

import 'package:js/js.dart';

@JS()
@anonymous
class Animal {
  external String get name;
  external set name(String name);
}

This should probably be a compile-time error, as it implicitly creates a default constructor.

The correct implementation would be:

@JS()
@anonymous
abstract class Animal {
  external String get name;
  external set name(String name);
}

Or, with an external factory constructor.

• In DDC, everything above appears to work as if this was abstract
• In Dart2JS above creates a Dart class with a JS-semantics getter/setter.

[lrhn]

Nitpick: The second class also gets a default constructor, you just can't use it to instantiate the abstract class. You can extend the class, though, which should probably also be disallowed.

[sigmundch]

additional checks that we have discussed recently:

• instance method renames: we can warn that these are not supported. Today this means that instance method @JS declarations are unnecessary, so we can also warn about any use of them, but we can be a bit more lenient with benign annotations and only warn when there the rename is clearly the intent.
• conflict between @JS and native types: to help users detect issues like Allow users to access hidden and incomplete HTML APIs #42200, we can pre-build a list of blacklisted names that cannot be bound via @JS and issue a warning on those.

[srujzs]

I'm curious: what's blocking us from allowing instance method renames? I was talking a bit about this last week and figured it would be a good way to wrap overloading.

[natebosch]

what's blocking us from allowing instance method renames?

The current interop design in dart2js does not allow it - though we can do it easily in ddc.

In dart2js all @JS() interop classes share effectively a single implementation. When the dart code has foo.bar() that ends up going through to a Dart method that forwards to the JS bar() method. If any JS interop needs to forward from the Dart name renamed to the JS named bar, then all JS interop would have the same rename.

So the renames that work in dart2js are only the static ones.

[sigmundch]

If I recall correctly, ddc can only handle it when the invocation is typed. When invocations are dynamic (via a dcall) it will not apply the rename either.

I should note that even though the issue is more visible because of dart2js's implementation, there is an actual composition problem here. We allow users to define JS-interop classes without coordination. If two libraries expose the same JS class, they are both allowed and equally usable. Because dart2js dispatches based on the underlying value (in this case JS-instance), it needs to ensure that all JS-interop definitions for that JS class are consistent with each other. Because we want to avoid link-time errors, we enforce this modularly by limiting what the user can do. When we disallow renames (and method bodies among other things), we allow the declarations to be combined without conflict.

[natebosch]

If I recall correctly, ddc can only handle it when the invocation is typed. When invocations are dynamic (via a dcall) it will not apply the rename either.

Correct.

Because dart2js dispatches based on the underlying value (in this case JS-instance), it needs to ensure that all JS-interop definitions for that JS class are consistent with each other.

Right, that's why we ideally would like a more statically predictable JS interop. If we can trust the calling side then we can do the rename there. That's what the DDC implementation for instance member renames looked like.

[franklinyow]

What is the status on this one?

[sigmundch]

This one issue is a long arc, and wont fit entirely into the September release, so we will be moving it to the October release soon (likely next week? We can move it now if you prefer).

Many checks have landed, and we'll continue to land them before and after the next milestone.

[srujzs]

Adding on to what Siggi said, should we continue to move the milestone until everything we want here has been added? We'll likely continue to add things here and stretch this across multiple milestones.

[franklinyow]

Consider making this an Epic, with sub-issue inside it. So each sub-issue can be place in different milestone. Continue rolling this over to the next milestone defeat the purpose of the milestone.

[srujzs]

I agree with making it an Epic. We'll need to break this down into bits that will make sense to include in milestones.

[vsmenon]

@srujzs @sigmundch - are we still using this as a tracking issue? If so, it is P1 but no 2021 updates. Should we drop to P2?

[srujzs]

Edit: A lot of these have since spawned their own bugs or we aren't planning on handling, and we're currently use the above epic to track the work. We've kind of treated this as a tracking bug for static checks in the past but also not all of them, so it's a bit messy, which makes me think we should just close this and use the separate issues as needed. @sigmundch and @rileyporter , let me know if you want this reopened.