dart2js incorrectly assumes getter on a JS interop abstract class is null

[DanTup]

I suspect this is an error in how I'm trying to do this rather than a bug (in which case maybe there's missing docs or they're not easy enough to find - I can't find anything or any examples that seem to do the same). It seems strange that it works for build_runner serve code but fails for build_runner serve --release code though.

I'm trying to call the JavaScript API window.showDirectoryPicker() which returns a FileSystemDirectoryHandle that has values() async iterator to enumerate over the items in the folder.

Using this directly in JavaScript would look like this:

const dirHandle = await window.showDirectoryPicker();
for await (const entry of dirHandle.values()) {
  console.log(entry.name);
}

My attempt to call this from Dart started like this:

Future<FileSystemDirectoryHandle> showDirectoryPicker() =>
    promiseToFuture(callMethod(window, 'showDirectoryPicker', []));

@JS()
abstract class FileSystemDirectoryHandle extends FileSystemHandle {
  external Stream<FileSystemHandle> values();
}

However this fails in both debug/release output with:

Error: Expected a value of type 'Stream', but got one of type 'NativeJavaScriptObject'

and:

TypeError: Instance of 'NativeFileSystemDirectoryIterator': type 'minified:aD' is not a subtype of type 'minified:aCminified:am'

I thought I'd got a solution by calling the next() method on the iterator manually in an extension method on the class:

extension FileSystemDirectoryHandleExtensions on FileSystemDirectoryHandle {
  Stream<FileSystemHandle> values() =>
      _asyncIterator<FileSystemHandle>(callMethod(this, 'values', []));
}

Stream<T> _asyncIterator<T>(jsIterator) async* {
  while (true) {
    final next = await promiseToFuture(callMethod(jsIterator, 'next', []));
    if (getProperty(next, 'done')) {
      break;
    }
    yield getProperty(next, 'value');
  }
}

This worked great while running with build_runner serve, however when I came to try and build (or use serve --release) it fails with:

TypeError: J.hQ(...).j is not a function

Which I've been unable to get to debug or understand.

[devoncarew]

@sigmundch @natebosch - I'm not sure who's best to field JS interop related questions.

[natebosch]

cc @srujzs

[natebosch]

It's not totally surprising that we still have patterns that work in DDC but not dart2js. I'm not sure what could be happening in this case though.

I suspect the most straightforward solution would be to implement a working asyncIteratorToStream method like we have promiseToFuture in dart:js_util.

[DanTup]

I tried to dig more into this today by creating a simple JS async iterator (without the file system stuff), and was unable to reproduce at all. The dart2js output is much larger, but it seems to work.

So I tried again in my real project, and also couldn't reproduce this error - the app wasn't working properly, but it turned out to be that in dart2js output I couldn't compare the kind of the returned file as a string:

print(item.kind == 'file'); // true in ddc, but false in dart2js
print(getProperty(item, 'kind') == 'file'); // true in both

This seems to be because I'd missed the external keyword off the definition of kind in my bindings.

I suspect that the original bug I saw was the latter (the kind not being a pure string in dart2js) and when I trimmed it down to reproduce in a small sample I did something else wrong (such as using an old SDK). As far as I can tell, everything is working correctly using the latest version of everything.

Sorry for the confusion. I'll re-open this if I see it again and confirm it's not just an old SDK. Thanks!

[sigmundch]

Thanks for the update @DanTup - by any chance can you point me at the definition of item.kind?

@rileyporter and @srujzs have added several static checks to ensure that our JS-interop definitions are treated consistently between DDC and dart2js. It's possible that the issue you ran into is something that we'll be able to detect in the near future, but it would be good to know if there are more edge cases we need to consider.

[DanTup]

@sigmundch it's the kind field on FileSystemHandle. The binding was my own, as I don't think Dart has built-in bindings for these. It looked like:

@JS()
abstract class FileSystemHandle {
  String get kind;
  String get name;
}

And adding the external keyword to both resolved the problem.

Out of interest - how are the JS bindings normally created here? Is it done manually, or is it scripted from some API docs? I've found a few missing/incomplete bindings (for APIs like this file picker and media session) and have been just creating my own - though I don't know how well I'm doing it and it seems like the sort of thing that might have been automated?

[sigmundch]

The generation of dart:html is scripted, but it is seeded of chrome IDLs that may be outdated (currently our IDLs are several years old unfortunately).

Most users maintain the js-bindings by hand, but there are tools like https://github.com/dart-lang/js_facade_gen to help automate the process if you happen to have .d.ts definitions available.

As for your example above, If FileSystemHandle is also defined in dart:html but it was incomplete, I'd recommend making your definition with an extra @anonymous annotation. This is because of a thorny issue in how our compilers work: the definition in dart:html would be otherwise conflicting with yours and takes priority. Users have sometimes run into type errors as a result. The @anonymous annotation ameliorates the issue, but it's not perfect either. We do hope to make this better in the future (#44211)

[DanTup]

@sigmundch thanks! I suspect there are TypeScript bindings available for these APIs, so I might try js_facade_gen when I get back to this to see what it generates compared to mine.

As for your example above, If FileSystemHandle is also defined in dart:html but it was incomplete, I'd recommend making your definition with an extra @anonymous annotation

As far as I can tell, FileSystemHandle is not defined at all in Dart's SDK. Some of the others are but were incomplete (for example MediaSession's metadata has an untyped list for artwork - although it turns out I can't use that (my images are extracted from ID3 tags and uses createObjectURL, which won't work outside of the browser) so that's not really an issue now.

[srujzs]

Regarding your comment earlier out of curiosity:
print(item.kind == 'file'); // true in ddc, but false in dart2js
The resulting inconsistent behavior is worrying, non-external fields on JS classes should behave as expected. Is this before or after you set the non-external item.kind?

As a side note, most likely not an issue if we don't add a FileSystemHandle class to the SDK, but if we do, you might start seeing type errors on your JS class if it's not marked as @anonymous.

[DanTup]

It was before I added external. If that doesn't seem expected, let me know and I'll make a complete repro and file an issue when I can.

but if we do, you might start seeing type errors on your JS class if it's not marked as @anonymous.

That's fine to me - if I start seeing errors like that, it'll be a signal I can switch over to SDK-supplied classes (which would be better) :-)

[srujzs]

Hmm, it is indeed unexpected. I wrote a small test verifying behavior for non-external fields, and I don't see inconsistent behavior before and after they're modified. Can you write a small repro when you get the chance? Thanks!

Interestingly enough, however, non-external fields have the same semantics as external fields. We should clarify the semantics here since that may be unintuitive. The following passes on both compilers:

Testing non-external fields

@JS()
library field_test;

import 'package:expect/expect.dart';
import 'package:js/js.dart';
import 'package:js/js_util.dart' as js_util;

@JS()
class JSClass {
  String? field;
  static String? staticField;
}

@JS()
abstract class JSAbstractClass {
  String? field;
  static String? staticField;
}

@JS()
external void eval(String code);

@JS()
external JSClass getJSObj();

@JS()
external JSAbstractClass getJSAbstractObj();

main() {
  eval(r'''
  function JSClass() {
    this.field = 'field';
    this.staticField = 'staticField';
  }
  function JSAbstractClass() {
    this.field = 'field';
    this.staticField = 'staticField';
  }
  function getJSObj() {
    return new JSClass();
  }
  function getJSAbstractObj() {
    return new JSAbstractClass();
  }
  ''');

  var jsObj = getJSObj();
  var jsAbstractObj = getJSAbstractObj();

  // Initial values for non-static fields same as underlying property and not null.
  Expect.equals(jsObj.field, 'field');
  Expect.equals(jsAbstractObj.field, 'field');
  Expect.equals(JSClass.staticField, null);
  Expect.equals(JSAbstractClass.staticField, null);

  var fieldStr = 'modifiedField';
  var staticFieldStr = 'modifiedStaticField';

  jsObj.field = fieldStr;
  jsAbstractObj.field = fieldStr;
  JSClass.staticField = staticFieldStr;
  JSAbstractClass.staticField = staticFieldStr;

  Expect.equals(jsObj.field, fieldStr);
  Expect.equals(jsAbstractObj.field, fieldStr);
  Expect.equals(JSClass.staticField, staticFieldStr);
  Expect.equals(JSAbstractClass.staticField, staticFieldStr);

  js_util.setProperty(jsObj, 'field', 'field');
  js_util.setProperty(jsAbstractObj, 'field', 'field');

  // Modified JS property affects non-external field.
  Expect.equals(jsObj.field, 'field');
  Expect.equals(jsAbstractObj.field, 'field');
}

[DanTup]

@srujzs my repro is below. It calls window.showDirectoryPicker() and then prints the items found in that directory and whether they're files or directories. When running with pub run build_runner serve everything is fine, but when running with pub run build_runner serve --release it prints:

Adding external fixes it so both behave the same.

If this seems like a bug, feel free to move to a new issue (or let me know if you want me to raise one). Thanks!

web/index.html

<html>
<head>
<script defer src="main.dart.js"></script>
</head>
<body>
	<button id="pickMusic">Pick Music Folder</button>
</body>
</html>

web/main.dart

@JS()
library js_lib;

import 'dart:html';

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

main() {
  document.querySelector('#pickMusic').onClick.listen((_) => click());
}

click() async {
  final dir = await showDirectoryPicker();
  for (var file in await dir.values().toList()) {
    if (file.kind == 'file') {
      print('found a file!');
    } else if (file.kind == 'directory') {
      print('found a directory!');
    } else {
      print('found a ${file.kind} which is neither a file or a directory');
    }
  }
}

Future<FileSystemDirectoryHandle> showDirectoryPicker() =>
    promiseToFuture(callMethod(window, 'showDirectoryPicker', []));

Stream<T> _asyncIterator<T>(jsIterator) async* {
  while (true) {
    final next = await promiseToFuture(callMethod(jsIterator, 'next', []));
    if (getProperty(next, 'done')) {
      break;
    }
    yield getProperty(next, 'value');
  }
}

@JS()
abstract class FileSystemDirectoryHandle extends FileSystemHandle {}

@JS()
abstract class FileSystemFileHandle extends FileSystemHandle {}

@JS()
abstract class FileSystemHandle {
  String get kind;
  String get name;
}

extension FileSystemDirectoryHandleExtensions on FileSystemDirectoryHandle {
  Stream<FileSystemHandle> values() =>
      _asyncIterator<FileSystemHandle>(callMethod(this, 'values', []));
}

extension FileSystemHandleExtensions on FileSystemHandle {
  Future<File> getFile() => promiseToFuture(callMethod(this, 'getFile', []));
}

[sigmundch]

Interesting - it appears that the fact that the class is labeled abstract is confusing dart2js.

Internally it seems we believe the result of the getter is always null, and as a result we unsoundly optimize away the equality checks against field.kind.

Shrinking your example:

import 'package:js/js.dart';

@JS()
abstract class A {
  String get kind;
}

@JS()
external A get a;

main() => print(a.kind == 'hi');

This generates a main method that looks like:

    main: function() {
      J.get$kind$x(self.a);
      H.printString(H.S(C.JSBool_methods.toString$0(false)));
    },

We call the getter in case there are side-effects, but we ignore the return value in this case.

[DanTup]

@sigmundch should I file an issue about that / re-open this one?

[srujzs]

Let's re-open and rename this one.