Sdk: pkg:js typing inconsistency between DDC and dart2js (JSArray is not a subtype of type List...)

[evanweible-wf]

This behavior occurs on Dart 2.4.0 and the latest js/test/build_* packages.

---

In migrating some dart1 code to latest dart2, we noticed an inconsistency in how a nested JS interop field is typed in DDC vs dart2js. I'm not sure which is behaving as expected.

Given the following package:js-annotated class:

@JS()
@anonymous
class Node {
  external String get id;
  external List<Node> get children;
}

And the following JS that populates an instance of this class:

root = {
    'id': 'root',
    'children': [
        {
            'id': 'child1',
            children: [],
        },
        {
            'id': 'child2',
            children: [],
        },
    ],
};

And Dart code that passes an instance of Node.children to a method expecting
something typed as List<Node>:

void printNodeIds(List<Node> nodes) {
  for (final node in nodes) {
    print('Node ID: ${node.id}');
  }
}

The resulting behavior differs depending on whether the code is compiled with
DDC or dart2js. With DDC, the code runs without issue and prints the IDs. With
dart2js, a type error is encountered:

TypeError: Instance of 'JSArray': type 'JSArray' is not a subtype of type 'List<Node0>'

However, the structure of the JS interop class instance is as expected in both
scenarios and operations can be made on items in the Node.children collection.

---

A repro of this issue can be found here along with tests that demonstrate the differing behavior: https://github.com/evanweible-wf/dart_js_nested_interop_typing_issue

cc @kevmoo

[kevmoo]

cc @vsmenon @rakudrama

[vsmenon]

cc @natebosch

[evanweible-wf]

I'm not sure if this is the same issue, but it seems very similar: I just discovered that trying to pass window.navigator.languages (which is typed as List<String>) to a method that expects a param of type List<String> results in the same TypeError under dart2js:

@TestOn('browser')
import 'dart:html';

import 'package:test/test.dart';

void main() {
  test('window.navigator.languages returns List<String>', () {
    // This will pass with DDC and throw a TypeError with dart2js.
    printLanguages(window.navigator.languages);
  });
}

void printLanguages(List<String> languages) {
  for (final language in languages) {
    print('Language: $language');
  }
}

TypeError: Instance of 'JSArray': type 'JSArray' is not a subtype of type 'List<String>'

[vsmenon]

DDC is being too lax here. Both these should be typed to take List<dynamic>, not List<Node>, etc. With Dart, generics are reified, and because it's coming from JS, there is no reified type on the Array.

We're talking about ways to support this better (e.g., some notion of unreified generic for JS).

[evanweible-wf]

Thanks for the update @vsmenon – is there an issue I could follow regarding that last part?

Following up on my second comment about window.navigator.languages – should I open a separate issue about that? I'm guessing the root cause is this issue since it's JS interop, but it seems to me that the dart:html library should be handling that since it is typed as List<String> but we are getting something that is typed as JSArray at runtime, which causes an RTE under dart2js.

[evanweible-wf]

^ to follow up on this, it looks like we already have an issue open for dart:html APIs that (I'm guessing) suffer from this issue and return something of type JSArray instead of List<T>:
https://github.com/dart-lang/sdk/issues/36002

[evanweible-wf]

Another instance that we just ran into: https://github.com/dart-lang/sdk/issues/37887
dart:html's ClipboardData.types is typed as List<String> but returns List<dynamic>

[tomconnell-wf]

To get the type stronger, we're using new List<T>.from(JSArray). I wish we could have a stronger type from the get-go, because I'm concerned that we're opening ourselves up to more GC by copying the list.

We're having to support Dart1 and Dart2, so I'm unsure if cast() would handle this more efficiently when we're only in Dart2.

[natebosch]

I'm unsure if cast() would handle this more efficiently when we're only in Dart2.

Generally if you might access the elements more than once List.from is going to give better performance - it will do the cast/checks once during the copy and then be done with it.

[robbecker-wf]

Any update here @kevmoo @vsmenon @natebosch ?

[natebosch]

For the package:js use case the current workaround is to always type the APIs as List<dynamic>. In the near term we'll make DDC be as strict, or stricter, than dart2js for these argument types and it should solve the problem of being surprised in production.

https://dart-review.googlesource.com/c/sdk/+/113754 makes DDC callbacks strict about argument types when they are wrapped with allowInterop. We expect to be able to land that soon.

[natebosch]

For dart:html we'll need to investigate separately. During the Dart 2 transition we made breaking changes that typed more of these as List<dynamic> statically, I'm not sure if we can feasibly do that for the remainder of the APIs that were missed.

[evanweible-wf]

@natebosch If something in a core library (e.g. https://api.dartlang.org/stable/2.4.1/dart-html/Navigator/languages.html) is typed as List<String> but trying to use that value as a List<String> does not work in dart2js, isn't that API broken/incorrect? I can see that it would be statically breaking to change it to List<dynamic>, but leaving it as is doesn't seem right to me, either, given that it may then throw a TypeError at runtime even though it statically analyzes.

I don't know anything about how the interop bindings for these types of window APIs are generated in the SDK, but would it be feasible to add the workaround (e.g. List<String>.from(...)) at that level so that the returned value correctly matches the type?

If that's not doable, could the remainder of the APIs that were missed be corrected in the next minor SDK release as a proper breaking change?

[rakudrama]

For both JS-interop and some dart:html cases we are looking at making runtime type checks for JSArray instances more flexible for instances that come 'outside' of the program. It could work something like this:

There is a new type, any, that works a lot like like the old Dart-1.0 dynamic type. You can't name or use the type yourself, but it basically means 'ignore this part of the type' and is used by the runtime as below.

If a JavaScript Array comes from outside the program, it will be seen as implementing JSArray<any> rather than the current JSArray<dynamic>. If the Array comes from outside the program, it does not have a stored type to use for runtime checks.

JSArray<any> is a subtype of List<String> because JSArray is a subtype of List and we are told to ignore the type parameter.

This is unsound, but only to the degree that your use dynamic calls or calls that need parametric covariance checks (e.g. using the list with static type List<Object>, there is no stored type to enforce that only Strings are added when calling list.add(x)). Parametric covariance checks usually are method parameters and not return types, so the risks are much lower if the data is just read-from and not modified.

If you want to guarantee safety for any eventuality, you can still use List<String>.from(...) to create a copy that you own.

For dart:html, where we could in some places change the API to insert copying, this is compromise between forcing the users to deal with an API returning an unhelpful List<Object>, and defensively copying too much data.

@evanweible-wf @robbecker-wf Would the scheme meet your needs?

[aaronstgeorge-wf]

Thank you @rakudrama that's a great explanation!

@natebosch @rakudrama I have a follow up question regarding package:js specifically.

If I understand correctly, currently any time one writes code like the original example

@JS()
@anonymous
class Node {
  external List<Node> get children;
}

there maybe run time errors because the JsArray returned will not be annotated with the reified type that dart expects. When we have this any type we won't hit those runtime errors, basically because dart won't check the types, but we will lose a degree of type safety around calls to that returned array. So when this any type arrives we could write code like the example (with the parameterized type) and we would not need to use a from before using children but we may still want to, to recover full type safety. Does that sound accurate?

If I'm on the right track, I'm wondering if it would be reasonable/possible for package:js to generate a List<Node>.from(...)  behind the scenes for any list (or other collection type) with a parameterized type? There would be a performance impact by doing that so maybe it's not a reasonable default but there could be a new annotation @ GenerateCopy maybe. Time and resources permitting we might be willing to put up a PR to do that.

[natebosch]

If I'm on the right track, I'm wondering if it would be reasonable/possible for package:js to generate a List<Node>.from(...)  behind the scenes for any list (or other collection type) with a parameterized type?

This is not safe to do in general. List.from makes a copy and if an API is relying on communicating by mutating the list that would be broken. .cast may be safer, but would be even less performant.

there could be a new annotation @ GenerateCopy maybe.

I'm hesitant to add more magic or special case behavior until things are more stable.

My intuition is that the pain _mostly_ comes from inconsistencies between the compilers and lack of static checking. If we can check what patterns will or won't work statically and make it easier to stay "on the rails" with interop then I think the pain coming from needing extra boilerplate won't be so bad. It also will concretely push the choice of how to handle the translation to the author rather than the compiler which may make bad assumptions.

We do want to explore at some point a way to make the typing behave more like dart 1 dynamic, to where if you have an API returning a List<SomeJSClass> then we'd assume it was correct and skip the check for reified types.