dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.3k stars 1.59k forks source link

Dart2JS runtime TypeError with external collection in dart2 mode #33598

Open lexaknyazev opened 6 years ago

lexaknyazev commented 6 years ago

Dart SDK 2.0.0-dev.64.1.

JS

var Foo = function () {
    this.value = 42;
};
var foos = [new Foo(), new Foo()];

main.dart

@JS()
library js_interop;

import 'package:js/js.dart';

@JS()
@anonymous
class Foo {
  external int get value;
}

@JS()
external List<Foo> get foos;

void main() {
  print(foos.map((foo) => foo.value * 2));
}

Prints (84, 84) with DDC and Dart2JS in Dart 1 mode. Fails with Dart2JS dart2 mode:

TypeError: Closure 'main_closure': type '(Foo) => int' is not a subtype of type '(dynamic) => int'
lexaknyazev commented 6 years ago

The workaround exists but it doesn't look nice:

print(foos.map((Object foo) => (foo as Foo).value * 2));
matanlurey commented 6 years ago

I think the fundamental issue is that JS can't return a reified List of Foo. I think DDC might ignore some implicit List failures right now (but will enforce them by Dart2 final), so that's why it doesn't show up. See here:

https://github.com/matanlurey/dart_js_interop/blob/master/README.md#creating-a-wrapper-class

lexaknyazev commented 6 years ago

Btw, runtime error happens only with closures:

void main() {
  // This works everywhere
  for (final f in foos) {
    print(f.value);
  }

  // This also works everywhere
  print(foos[0].value * 2);

  // This fails only with dart2js dart2 mode
  foos.forEach((f) {
    print(f.value * 3);
  });
}

EDIT: passing a list as a reference also fails:

void main() {
  bar(foos);
}

void bar(List<Foo> v) {
  for (final f in v) {
    print(f.value);
  }
}
TypeError: Instance of 'JSArray': type 'JSArray' is not a subtype of type 'List<Foo>'
matanlurey commented 6 years ago

I think this is mostly due to inconsistencies in the compilers, but Dart2JS is likely doing "the right thing" here compared to DDC. Until/if JS interop starts automatically reifying (which might not be possible or desirable) I don't think you can assume an external List (really a JSArray) has a type parameter in Dart2.

/cc @jmesserly

matanlurey commented 6 years ago

@lexaknyazev You might be able to use --omit-implicit-checks to avoid this BTW.

lexaknyazev commented 6 years ago

Yeah, the examples above work with --omit-implicit-checks. Two more questions emerge from that, though:

rakudrama commented 6 years ago

Thank you for filing issues with Dart 2 and JS-interop. It is very helpful to have examples of real problems.

JS-interop was designed for Dart 1 where List<dynamic> was assignable to any List<T>, so we need to update the design and make DDC and dart2js more compatible.

In the meantime, think of Arrays coming from JavaScript as List<dynamic> and write code accordingly.

@JS()
external List get foos; // List<Foo>

With this change the program should work, but with more dynamic calls. It is seen as a List<dynamic> containing Foos, not a List<Foo>. I don't think we will ever be able to make it an actual List<Foo> without copying.

var copy = new List<Foo>.from(foos);

We did relax the type rules for JavaScript functions, and maybe the new design for JS-interop will do something similar, i.e. JavaScript Arrays behave like Dart 1 List<dynamic> in an otherwise Dart 2 program.

It is a bad idea to rely on --omit-implicit-checks. It is currently a global flag so it cannot be scoped. I think we will eventually need something that works a bit like a scoped version of --omit-implicit-checks, e.g. as an annotation on a method.

matanlurey commented 6 years ago

I think unless we are going to make accessing JS arrays with anything but dynamic as the type parameter a build error we need to make the out of box defaults more usable.

I agree it's not reasonable to expect all users to use --omit-implicit-checks (though that's largely the only way Dart2JS is used and tested internally).

lexaknyazev commented 6 years ago

A few more usage patterns from real code (updated with Dart 2 semantics).

An object comes from trusted source (e.g., such as well-defined browser API)

/// Returns a [List] of JS object's fields
@JS('Object.keys')
external List _objectKeys(Object value); // List<String>

void main() {
  final map = <String, int> {};
  /// We need to iterate over object's keys only once, and we can trust
  /// that [_objectKeys] elements are [String]s, so a declaration cast is enough.
  for (final String key in _objectKeys(someObject)) {
    map[key] = getProperty(incoming, key) as int;
  }
}

Suggestions for updated JS-interop:

An object comes from untrusted source (e.g., through an API exposed to user)

@JS()
external List get incoming; // List<String>

void main() {
  assert(incoming != null);
  final reifiedList = List<String>(incoming.length);
  for (var i = 0; i < incoming.length; ++i) {
    final element = incoming[i]; // local variable to enable promotion
    if (element is String) {
      reifiedList[i] = element;
    } else {
      throw ArgumentError('incoming[$i]: Value must be a String. Got $element.');
    }
  }
}

Here, we construct a reified List manually because:

Using List's methods

This case is the most tedious, imo. Sometimes, a JS API returns a List of anonymous JS objects. Since we cannot extend them or define Dart-only methods on them (see #33395), it may be useful to wrap such external objects into proper Dart objects.

This example uses a wrapper to override toString.

@JS()
@anonymous
abstract class _Foo {
  external String get bar;
  external String get baz;
}

class Foo {
  final _Foo _foo;
  Foo._(this._foo);

  String get bar => _foo.bar;
  String get baz => _foo.baz;

  @override
  String toString() => 'Foo: $bar & $baz';
}

@JS('incoming')
external List get _incoming; // List<_Foo>

/// In this particular case, we don't expect intense access to the [incoming].
/// So we create a lazy [Iterable<Foo>].
Iterable<Foo> get incoming => _incoming.map((Object foo) => Foo._(foo as _Foo));

It seems that we need the explicit cast in the last line to satisfy static types but it's unclear what it does at runtime.

jmesserly commented 6 years ago

I think this is mostly due to inconsistencies in the compilers, but Dart2JS is likely doing "the right thing" here compared to DDC. Until/if JS interop starts automatically reifying (which might not be possible or desirable) I don't think you can assume an external List (really a JSArray) has a type parameter in Dart2.

DDC is currently ignoring some cast failures to ease migration. If you look at the browser console, you should see a warning message printed, to indicate that the cast failed.

ajsergeev commented 5 years ago

It looks like we have a similar issue but not with JS interop but with MouseEvent. For example, I have MouseEvent and I need to check drag and drop types: (event as MouseEvent).dataTransfer.types((String str) {...}) Because (event as MouseEvent).dataTransfer.types is List of String DDC works ok with this, but when compile with dart2js it fails with: processItemDrop_closure': type '(String) => Null' is not a subtype of type '(dynamic) => void'. The only way I could fix it is to do standalone closure which makes forEach almost not usable: void forEachCallback(dynamic type) {} SDK 2.5.0. stable

Another class of DDC and dart2js issues we found recently is Map and HashMap incompatibility: For example, Map m = new HashMap() works in DDC but crashes in release (after dart2js).

Thanks!