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.2k stars 1.57k forks source link

dev_compiler fails when hot reloading with JS interrop #49383

Closed Schwusch closed 1 year ago

Schwusch commented 2 years ago

When hot reloading I get this error:

lib/main.dart:18:27: Error: The argument type 'void Function(USBDevice)' can't be assigned to the parameter type 'void Function(JavaScriptObject)'.
 - 'USBDevice' is from 'package:js_demonstration/other.dart' ('lib/other.dart').
 - 'JavaScriptObject' is from 'dart:_interceptors'.
    usb?.subscribeConnect(onConnect);

This is when hot reloading any change in main.dart in this Flutter project: https://github.com/Schwusch/dart_js_bug_repro. It is the smallest reproducible code example I could make. When making a change and hot reloading in other.dart, everything works as expected and the code is hot reloaded. I am using a M1 MacOSX 12.4 Monterey.

Reproduced with both:

flutter --version
Flutter 3.1.0-0.0.pre.1423 • channel master •
https://github.com/flutter/flutter.git
Framework • revision 8fe82bbe60 (2 days ago) • 2022-06-29 10:16:05 +0300
Engine • revision 8c36371fa7
Tools • Dart 2.18.0 (build 2.18.0-234.0.dev) • DevTools 2.14.1

and:

flutter --version
Flutter 3.0.4 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 85684f9300 (22 hours ago) • 2022-06-30 13:22:47 -0700
Engine • revision 6ba2af10bb
Tools • Dart 2.17.5 • DevTools 2.12.2

main.dart

import 'package:flutter/material.dart';

import 'other.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  doTheThing(USBDevice device) {}

  void onConnect(USBDevice device) => doTheThing(device);

  @override
  Widget build(BuildContext context) {
    usb?.subscribeConnect(onConnect);
    return const MaterialApp(
      home: Scaffold(
        body: Center(child: Text('hello world')),
      ),
    );
  }
}

other.dart

@JS()
library web_usb;

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

@JS('navigator.usb')
external Usb? get usb;

@JS('USB')
@staticInterop
class Usb {}

typedef USBConnectionEventListener = void Function(USBDevice event);

extension PropsUsb on Usb {
  void subscribeConnect(USBConnectionEventListener listener) {
    callMethod(this, 'addEventListener', [
      'connect',
      allowInterop((USBConnectionEvent event) => listener(event.device))
    ]);
  }
}

@JS()
@staticInterop
class USBDevice {
  external USBDevice();
}

@JS()
@staticInterop
class USBConnectionEvent {
  external USBConnectionEvent();
}

extension PropsUSBConnectionEvent on USBConnectionEvent {
  USBDevice get device => getProperty(this, 'device');
}
srujzs commented 2 years ago

It makes sense that reloading main doesn't work while reloading web_usb does since void Function(JavaScriptObject) <: void Function(USBDevice). I think this is a result of erasing the types in both libraries and then when hot reloading, doing type-checking without erasing the reloaded library.

Perhaps the right thing here is to erase before type-checking, but that may have its own issues.

srujzs commented 2 years ago

Late response to this but I've been working on a similar bug on modular compilation and erasure that I believe should fix this. The fix should be to do a global transform once the whole program is compiled (before code generation). This way, reloading modules should not result in inconsistencies. I'll respond back once that's landed.

Schwusch commented 2 years ago

Thanks a lot for the update. Are you referring to https://dart-review.googlesource.com/c/sdk/+/253000/ ?

Great work, I saw you landed a few others as well. I look forward to not working around the bug ☺️

srujzs commented 2 years ago

Yup, it has landed: https://github.com/dart-lang/sdk/commit/61abaeda3f84e6d45c4a70689b08b798713bc5c1

A dev version should be available in the next few hours. Flutter rolls might take a little while (I believe they're on the cadence of several days, but I'm not sure). Please let me know if you still come across this issue then, thanks!

Schwusch commented 2 years ago

I pulled from Flutter master release:

Flutter 3.1.0-0.0.pre.2032 • channel master • https://github.com/flutter/flutter.git
Framework • revision 195fa0b728 (2 hours ago) • 2022-08-02 05:39:06 -0400
Engine • revision 4b19256979
Tools • Dart 2.19.0 (build 2.19.0-53.0.dev) • DevTools 2.16.0

And the problem still persists. I take it the fix landed in 2.19.0-52.0.dev per this commit: https://github.com/dart-lang/sdk/commit/cdad34a0392362c0e580048918cb6ea481e84662 Am I thinking correctly?

srujzs commented 2 years ago

Thanks for trying this! I fear you're right after trying this locally as well. :(

We'll likely need to pipe this lower into DDC somewhere where we can do a global transform at the point where the different uses of DDC intersect (expression evaluation, this, etc.). I'll respond back once I have something.

Schwusch commented 2 years ago

This might not help you @srujzs, but I've worked around the problem by creating a wrapper/proxy class for the static interop code.

That fixes hot restart in the GUI code for me at least.

Schwusch commented 2 years ago

I now noticed the wrapper/proxy solution actually broke the functionality, since the callback is no longer called. Maybe the closure passed to allowInterop broke because of it?

srujzs commented 2 years ago

Yeah, that should work since that's a Dart class and we're not doing any transformations there. It is a bit cumbersome though, which is unfortunate.

I think that breaks because the listener in the allowInterop call is being passed a USBDevice, instead of the wrapper class you created. You can probably work around this using:

typedef USBConnectionEventListener = void Function(UsbDeviceWrapper event);

extension PropsUsb on Usb {
  void subscribeConnect(USBConnectionEventListener listener) {
    callMethod(this, 'addEventListener', [
      'connect',
      allowInterop((USBConnectionEvent event) => listener(UsbDeviceWrapper(event.device)))
    ]);
  }
}
srujzs commented 1 year ago

Hi @Schwusch, I think this should be fixed with 2.19.0-351.0.dev looking at some modular tests. This should be in Flutter master by now if you'd like to give it a try.

Schwusch commented 1 year ago

Hi, I just checked with latest on master, and it seems to be working in all the use cases I have, thank you 🙏 I'd say this issue is resolved. Great work 💪

srujzs commented 1 year ago

Awesome, thanks for trying it out!