Difference in JS interop callback behaviour between DDC and dart2js

parnham commented 6 years ago

Dart v2.0.0-dev.55.0 (linux & chrome)

If a callback function defined in Dart has a signature that does not match that being invoked in a JS library via interop then the behaviour at runtime differs between DDC and dart2js. The following is a minimal example to highlight the issue.

web/index.html

<!DOCTYPE html>
<html>
    <head>
        <base href="/">
        <title>Dart JS Callback</title>

        <script src="packages/js_callback/interop.js"></script>
        <script defer src="index.dart.js"></script>
    </head>
    <body>
        <button id="button">Hello</button>
    </body>
</html>

web/index.dart

import 'package:js_callback/interop.dart';
import 'package:js/js.dart';
import 'dart:html';

void badOnClick() => print("hello");
// void goodOnClick(Event e) => print("hello");

void main()
{
    var test = new InteropTest(
        document.getElementById('button')
    );

    test.on('click', allowInterop(badOnClick));
}

lib/interop.js

var InteropTest = function(src)
{
    this.src = src;
}

InteropTest.prototype.on = function(type, f)
{
    this.src.addEventListener(type, f, false);
}

lib/interop.dart

@JS()
library js_callback;

import 'dart:html';
import 'package:js/js.dart';

@JS()
class InteropTest
{
    external InteropTest(HtmlElement element);
    external void on(String type, Function callback);
}

When served with the build_runner hello is printed to the debug console. When built with the build_runner hello is printed to the debug console. When built with the build_runner in release mode it throws the following:

index.dart.js:557 Uncaught Error: NoSuchMethodError: method not found: 'call'
Receiver: Closure 'cs'
Arguments: [Instance of 'MouseEvent']
    at Object.c (index.dart.js:557)
    at bF.H (index.dart.js:1217)
    at Object.J.aU (index.dart.js:1310)
    at Object.av (index.dart.js:526)
    at Object.bx (index.dart.js:541)
    at Object.by (index.dart.js:532)
    at dart.bO (index.dart.js:1255)
    at HTMLButtonElement.<anonymous> (index.dart.js:1251)

Since the method signature for the callback function in Dart is incorrect, it should fail in both DDC and dart2js built code. However, it would be nice if the DDC produced a useful runtime error under these conditions.

cc @matanlurey

rakudrama commented 6 years ago

This looks similar to https://github.com/dart-lang/sdk/issues/32883

vsmenon commented 6 years ago

In general, we need to decide the right semantics here first. Options:

Methods should fail if invoked with the wrong number (or type?) arguments.
Methods should "just truck on" JS style. I.e., truncate extra arguments and fill in null / undefined for too few.
Something else.

@rakudrama @jmesserly @jacob314 - thoughts?

jmesserly commented 6 years ago

I prefer JS semantics. The call does originate from JS, after all, and we don't have any safety at the boundary between JS and Dart.

We could certainly have DDC allowInterop do checking, but it'd be quite a bit slower.

The dart2js version appears to use Function.apply, as well as some other performance costs (allocates 2 JS functions, uses arguments, has 2 expando properties so the Dart and JS function objects can point at each other).

jacob314 commented 6 years ago

+100 to what @jmesserly said.

matanlurey commented 6 years ago

👍 to @jmesserly

vsmenon commented 6 years ago

Is there a reasonable Dart2JS implementation of allowInterop that implements/mimics JS semantics? In the non-NSM case, I imagine it'd be the same as Function.apply. In the NSM case, it'd have to pick the right entry point in the function object.

rakudrama commented 6 years ago

I will look into making the dart2js version more lax.

vsmenon commented 6 years ago

Thanks!

joshprzybyszewski commented 5 years ago

Could this be related to https://github.com/dart-lang/sdk/issues/26770, which says that callbacks with typed arguments are bad news unless you're on dart 2?

jmesserly commented 5 years ago

(assigned to @natebosch and me, as we're likely to be the ones fixing this)

This is a frequent pain point for Flutter devtools (https://github.com/flutter/devtools/issues/204) and it comes up in a lot of other places too. So marking it P1.

ochafik commented 5 years ago

This has just bitten us in prod. A library we're using started adding passing an extra parameter to our callback, which from their perspective seemed like a non-breaking API change (as in, it typically wouldn't have broken any JavaScript clients).

I had to write the ugliest of hacks as a workaround:

JsObject allowSafeInterop0(void callback()) => context['Function'].apply([
      '''
        var callback = arguments[0];
        return function() {
          callback();
        };
      '''
    ]).apply([allowInterop(callback)]);

Can this be prioritized?

vsmenon commented 5 years ago

@rakudrama @natebosch - do we have consensus on the more lax semantics? @ochafik 's case is motivation for that.

natebosch commented 5 years ago

After some exploration we want to make dart2js looser, and keep ddc as is.

We can always decide later to make DDC more strict, but we'll skip it for now since

We haven't seen a strong motivating use case for strictness. The user pain is caused by the difference between the compilers, not the looseness of DDC.
Since JS library authors may consider adding new arguments they use to call passed functions non-breaking it makes the most sense to us to be loose in production and match general JS expectations.

rakudrama commented 5 years ago

I think it is unwise to allow too few arguments - what do you do if the function has arguments with a non-nullable type?

dart2js/js_runtime can be made to discard extra arguments by using a modified version of Function.apply that ignores extra arguments. The new version can have any name, it just can't be part for the patch for class Function.

The detection of isFunctionApplyUsed needs to recognize the new method to ensure all the analysis understands that the allowInterop functions find their way some form of apply. BackendUsageBuilderImpl.registerUsedMember should have an additional test to set the flag, and closure_tracer.dart should also recognize direct and indirect calls the the new apply method (though most calls are 'recognized' as reaching Function.apply because they escape).

natebosch commented 5 years ago

I think it is unwise to allow too few arguments - what do you do if the function has arguments with a non-nullable type?

This is a really good point, and a justification for making DDC at least a little more strict than it is today. We'll end up wanting changes in both compilers.

natebosch commented 5 years ago

We're going to start by making DDC maximally strict.

https://dart-review.googlesource.com/c/sdk/+/113754

This will make it strict on both argument counts and argument types. This will only apply when allowInterop is used - we'll separately investigate how to enforce that allowInterop is used as much as we can catch.

Once we are able to make dart2js more loose on accepting extra arguments, we'll also look at doing the same for DDC.

insinfo commented 5 years ago

I am having this problem, I am using the c3js library to draw charts, when I am developing my AngularDart app with "webdev serve" (DDC) everything is ok, but when I compile for production with webdev build (dart2js) the application crashes with excepprion " Uncaught TypeError: o.tooltip_contents.call is not a function "

    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3/5.7.0/d3.min.js" charset="utf-8"></script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/c3/0.6.12/c3.min.js"></script>

c3.Data initChart() {
    var data = c3.Data();
    data.columns = [
      ['x', '2013-01-01', '2013-01-02', '2013-01-03', '2013-01-04', '2013-01-05', '2013-01-06'], //datas
      ['Valor', 30, 200, 100, 400, 150, 250], //valor
      ['Custo Previsto', 130, 340, 200, 500, 250, 350], //custoPrevisto
    ];
    data.x = 'x';    
    data.xFormat = '%Y-%m-%d'; // 'xFormat' can be used as custom format of 'x'
    var config = c3.ChartConfiguration();
    config.data = data;
    config.bindto = containerChartEconomicidade;
    config.axis = c3.Axis(
        x: c3.XAxisConfiguration(show: true, type: 'timeseries', tick: c3.XTickConfiguration(format: '%Y-%m-%d')));
    config.legend = c3.LegendOptions(
        show: true, position: "right", inset: {"anchor": 'bottom-right', "x": "5", "y": "5", "step": "5"});
    var tooltipOptions = c3.TooltipOptions();

    ///***************** Where does the problem possibly occur? ************************/

    tooltipOptions.contents = (dynamic d, String defaultTitleFormat,
      String defaultValueFormat, dynamic color) {
      String text;    
      var bgcolor = '#eee';

      for (var i = 0; i < d.length; i++) {
        var name = d[i]?.name; 
        var value = d[i]?.value;
        var titleIn = d[i]?.x;
        var title = "";

        if (titleIn != null) {
          //"EEE MMM dd yyyy HH:mm:ss 'GMT-0300' '(Horário Padrão de Brasília)'"
          var format = DateFormat("EEE MMM dd yyyy HH:mm:ss");
          var parsedDate = format.parse(titleIn.toString().substring(0,24));
          var format2 = DateFormat("dd/MM/yyyy");
          title = format2.format(parsedDate);
        }
        bgcolor = color(d[i]?.id);
        value = formatCurrency.format(value);

        if (text == null) {
          text = "<table class='c3-tooltip'><tr><th colspan='2'>$title</th></tr>";
        }
        text += "<tr class='c3-tooltip-name'>";
        text += "<td class='name'><span style='background-color:$bgcolor;'></span>$name</td>";
        text += "<td class='value'>$value</td>";
        text += "</tr>";
      }
      return text + "</table>";
    };
    config.tooltip = tooltipOptions;
    chartEconomicidade = c3.generate(config);
    return data;
  }

https://pub.dev/packages/c3

name: siscec2browser
description: A web app that uses AngularDart Components

environment:
  sdk: '>=2.4.0 <3.0.0'

dependencies:
  angular: ^5.3.0
  angular_components: ^0.13.0
  angular_router: ^2.0.0-alpha+19
  http: ^0.12.0
  stream_transform: ^0.0.19
  sass_builder: ^2.1.3
  firebase: ^5.0.4
  firebase_dart_ui: ^0.1.1
  js: ^0.6.1+1
  money: ^0.2.1
  decimal: ^0.3.4
  ng_bootstrap: any
  ng_fontawesome: ^5.7.2+2
  #modern_charts: ^0.1.17
  chartjs: ^0.5.1
  c3: ^0.1.0
  essential_components: 0.1.19

rakudrama commented 5 years ago

@insinfo I think you are missing allowInterop:

tooltipOptions.contents = (dynamic d, String defaultTitleFormat,
      String defaultValueFormat, dynamic color) {
...
};

--->


tooltipOptions.contents = allowInterop((dynamic d, String defaultTitleFormat,
      String defaultValueFormat, dynamic color) {
...
});

insinfo commented 5 years ago

@rakudrama I wrap the function with "allowInterop" and then I started getting another error, in the following part

 var name = d[i]?.name; 
 var value = d[i]?.value;
 var titleIn = d[i]?.x;

bgcolor = color(d[i]?.id);

I could not access the properties of the native javascript object .

My temporary solution was this, does anyone know a better solution. dart

@JS('getObjInstancePropertyValue')
external dynamic getObjInstancePropertyValue(jsObject,key);

js

 function getObjInstancePropertyValue(objInstance, key) {
            try {
                return objInstance[key];
            }
            catch (err) {
                print('getObjInstancePropertyValue');
                return null;
            }
}

 name = getObjInstancePropertyValue(selectedData[i], 'name');
  value = getObjInstancePropertyValue(selectedData[i], 'value');
  titleIn = getObjInstancePropertyValue(selectedData[i], 'x');

The strange thing is that everything works perfectly in DDC

natebosch commented 5 years ago

I could not access the properties of the native javascript object .

The way to access specific properties is to write an @JS() annotated class:

@JS()
class SomeMeaningfulName {
  external String get name;
  external String get value;
  external String get x;
}

List<SomeMeaningfulName> d; // initialize somehow.

var name = d[i]?.name; // This should work.

See https://github.com/dart-lang/sdk/blob/master/pkg/js/README.md

My temporary solution was this, does anyone know a better solution.

That looks a lot like getProperty from dart:js_util. (Also exported as package:js/js_util.dart)

The strange thing is that everything works perfectly in DDC

DDC has patterns that are much closer to how we'd normally write JS so it's not too surprising that things that don't work in dart2js do work in DDC. We are trying to close some of those gaps and make DDC more strict.

dart-lang / sdk

Difference in JS interop callback behaviour between DDC and dart2js #33134