Dart 2.8.0-dev.17.0 breaks Dart Sass Node.js tests

[nex3]

I haven't had time to dive in and find a minimal reproduction, but Dart Sass's Node.js tests started failing with the release of Dart 2.8.0-dev.17.0 with errors like the following:

  NoSuchMethodError: method not found: 'get$$call' (t2.get$$call is not a function)
  test/node_api/value/null_test.dart 23:47  main.<fn>.<fn>

See this Travis job for an example of the failure, and this Travis job for an example of a successful run with Dart 2.8.0-dev.16.0.

To reproduce:

$ git clone git://github.com/sass/dart-sass
$ cd dart-sass
$ pub get
$ pub run grinder before-test
$ pub run test test/node_api/value/null_test.dart -n "from a parameter is instanceof Null"

You can inspect the generated JS code in build/npm/sass.dart.js and regenerate it with pub run grinder pkg-js-dev.

This is blocking Dart Sass's release process.

[davidmorgan]

Culprit[1] is https://dart-review.googlesource.com/c/sdk/+/140042 by @johnniwinther.

This changed how implements Function is handled, which should have been a no-op, but apparently had an effect on JS interop.

This makes sense as the error relates to call. Natalie, I suspect you can make the code work by writing the signature for call yourself on the class(es) that currently implement Function. I will leave figuring out the SDK fix to those more knowledgeable than I :)

[1] found by bisecting through SDK changes in range 41fa72e..e3af7e7 which corresponds to dev.16.0 to dev.17.0: git checkout <revision> && git clean -ffd && gclient sync -D --force --reset && ./tools/build.py --mode release --arch x64 create_sdk

[nex3]

Unfortunately manually implementing call() doesn't work here—this is coming from type definitions for JS classes, e.g.:

@JS()
class NodeSassNullClass implements Function {
  external Object call();
  external NodeSassNull get NULL;
}

@JS()
class NodeSassNull {
  external Constructor get constructor;
}

[johnniwinther]

@sigmundch @fishythefish Is this related to the problem you found with js-interop and Function ?

[sigmundch]

@nex3 TL;DR - I believe a quick fix on your end would be to remove the Function type in the parameter of isJSInstanceOf in utils.dart.

@johnniwinther I think it's a different issue.

It appears that removing Function while lowering is causing downstream changes to implicit tear-offs of the call method. Before the change, if something implemented Function we allowed assigning it to the Function type, even though a cast to that type would fail. After the change, we'd tear-off the call method instead.

Consider this example:

class B implements Function {
  void call() {}
}

m1(Function f) => m2(f);
m2(f) => print(
    'arg: $f\n'
    'type: ${f.runtimeType}\n'
    'is function: ${(f as dynamic) is Function}');

main() {
  m1(B());
  m2(B());
}

Before the change, this would print twice:

  arg: Instance of 'B'
  type: B
  is function: false

After the change, the call to m2 continues to print that, but the call to m1 would print:

  arg:  Closure 'call$0' of Instance of 'B'
  type: () => void
  is function: true

(Note: this is also the case on other backends)

The new behavior is what was specified back in 2.0.0, which is that implementing Function was supposed to have no effect whatsoever. Nevertheless, this bug fix is a breaking change so we should decide whether to move forward with it and, if so, send out a breaking change announcement about it. My inclination is that we do release it.

[johnniwinther]

I assumed that the fix only had impact on backends that (mis)used the references to Function wrongfully included in the AST. With your example, Siggi, it turns out the CFE was inconsistent in its own handling of Function - it excluded Function in only some of its internal processing.

[nex3]

@nex3 TL;DR - I believe a quick fix on your end would be to remove the Function type in the parameter of isJSInstanceOf in utils.dart.

Unfortunately this doesn't work because the js package also types instanceof()'s parameter as Function.

[sigmundch]

quick update: we discussed this more broadly and are going to move ahead with the breaking change. We'll send out a proper announcement soon.

@nex3 I tried the suggestion above and it allows the test to pass. However, I believe it is not enough. I didn't notice earlier that the utility method calls js_utils's instanceof, which is also typed to receive a Function parameter.

I believe it is a mistake that such API expects a Dart Function (for the same reason behind @johnniwinther's fix), it should instead accept any js-interop type that could be representing a constructor (and not require those to implement Function). We don't have a type to describe that, so we'll likely turn it in to Object.

I think we need to relax the API in js-util. Until we do so, maybe the immediate fix for your test would be to add a copy of instanceof in your utility library instead, or skip that step in the unit test.

[sigmundch]

Unfortunately this doesn't work because the js package also types instanceof()'s parameter as Function.

Sorry I had the tab open since yesterday and I had not seen your response until after I replied. Seems you had encountered the same issue.

I sent out a change to fix instanceof and callConstructor: https://dart-review.googlesource.com/c/sdk/+/142321

[nex3]

I sent out a change to fix instanceof and callConstructor: https://dart-review.googlesource.com/c/sdk/+/142321

Please let me know once this change makes it into a dev release.

[sigmundch]

Sure thing! Merge request is tracked at #41326

[sigmundch]

Thanks again @nex3 for following up offline.

I'm closing this issue now that the fix has landed in the beta channel (1e81d67).