Dart2+JS Interop: Discrepancies in support/cast errors

[matanlurey]

(I consider this a HIGH/P1 issue due to production bugs hit by internal teams)

Both internal and external users have been suffering a bit due to the lack of (a) clear documentation of intended use of package:js and (b) slightly different runtime and compile-time errors and functionality. In one particular recent case, DDC worked fine, and our internal clients hit production bugs requiring multiple cherrypicks because Dart2JS threw exceptions (at runtime).

Here is a reproduction case of a production P0:
https://github.com/matanlurey/dart2_js_interop_casts

// lib/chart.dart
@JS()
library dart2_js_interop_casts.chart;

import 'package:js/js.dart';

@JS()
@anonymous
abstract class ChartData<T> {
  external factory ChartData({T data});
  T get data;
}

class TimeSeries {
  String get domain => 'Hello World';
}

// test/chart_test.dart
// Internally, this was production code, *not* a test.

@TestOn('browser')
import 'package:dart2_js_interop_casts/chart.dart';
import 'package:test/test.dart';

void main() {
  test('should fail in both DDC and Dart2JS', () {
    // Start with a ChartData<TimeSeries>.
    final explicitTypeChart = new ChartData<TimeSeries>(
      data: new TimeSeries(),
    );
    
    // Downcast to a ChartData<dynamic>.
    final ChartData<dynamic> downcastTypeChart = explicitTypeChart;

    // "As" cast back to ChartData<TimeSeries>.
    expect(
      () {
        downcastTypeChart as ChartData<TimeSeries>;  
      },
      returnsNormally,
    );
  });
}

With DDC:

$ pub run build_runner test

00:01 +1: All tests passed!

With Dart2JS:

$ pub run build_runner test --release

  Expected: return normally
    Actual: <Closure 'main__closure'>
     Which: threw CastErrorImplementation:<CastError: Instance of 'PlainJavaScriptObject': type 'PlainJavaScriptObject' is not a subtype of type 'ChartData<TimeSeries>'>

(I know there are some meta issues around generic types/JS interop already)

[matanlurey]

/cc @jmesserly @vsmenon @sigmundch as this hit an escalation internally.

[vsmenon]

@jacob314 @jmesserly @sigmundch @rakudrama - what do you all think is the right behavior here?

I believe we're effectively treating the generic as unreified / Dart-1 ish in DDC.

[jmesserly]

I added this to #32929

To my knowledge there is no spec for JS interop, nor errors for incorrect usage. DDC can go pretty far off the rails in some cases, and that's not even getting into DDC/dart2js differences.

[sigmundch]

@matanlurey - what version of dart2js was used here?

This seems to be the same as #32929 (I discovered the issue also running this in an internal app with a very similar error message, possibly the same). We landed a fix in dart2js in 54b8ebd which is avalable in -dev.52 onwards.

[matanlurey]

The latest internally (was hit yesterday).

…

On Tue, May 15, 2018, 11:25 AM sigmundch ***@***.***> wrote: @matanlurey <https://github.com/matanlurey> - what version of dart2js was used here. This seems to be the same as #32929 <#32929> (I discovered the issue also running this in an internal app with a very similar error message, possibly the same). We landed a fix in dart2js in 54b8ebd <54b8ebd> which is avalable in -dev.52 onwards. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKQ7msO_Qwmqs-MeFo4_LxsoevCVPPhks5tyx2egaJpZM4T__6h> .

[sigmundch]

I'm a bit puzzled, "--preview-dart-2" is not enabled internally yet, let's chat offline when you have a chance to better understand what's happening here.

[matanlurey]

Talked offline with @sigmundch, this occurs in Dart1-mode.

The ChartData class being manually reified, i.e. an explicit new ChartData<TimeSeries>, which does work in Dart1 (class type). I actually tried this with --preview-dart-2 and it works (no error). Does that change how we should proceed?

[sigmundch]

My inclination is that yes: I hope we can skip fixing this in dart2js-dart1 mode and avoid the problem until projects switch to use dart2.

[matanlurey]

Great. I'm happy to close this (though I think #32929 will cover "spec" issues and hopefully avoid more of these in the future).

[matanlurey]

Lowering priority to P2. @sigmundch feel free to close if needed.

[sigmundch]

Thanks @matanlurey, I'll close this as not-planned, but please do reopen or ping me if this becomes a pain point and we need to reevaluate.