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

[package:js] @JS() Mixins can no longer be used with other @JS() classes since Dart SDK 2.12.0+ #47536

Open ganigeorgiev opened 3 years ago

ganigeorgiev commented 3 years ago

@JS() annotated mixins (or classes used as mixins) can no longer be used together with other @JS() classes since Dart >= 2.12.0-stable (I'm testing with the Stable channel builds for Linux from https://dart.dev/get-dart/archive).

Consider the following example:

// pubspec.yaml
name: demo
version: 1.0.0
description: @JS() Mixins

environment:
  sdk: '>=2.7.0 <3.0.0'

dependencies:
  js: ^0.6.2

dev_dependencies:
  build_runner: ^1.10.0
  build_web_compilers: ^2.9.0
// web/main.dart
@JS()
library main;

import 'package:js/js.dart';

@JS()
@anonymous
mixin DemoMixin {
  external bool demo();
}

@JS()
@anonymous
class DemoClass with DemoMixin {
}

void main() {}

The above fails with the following error:

[SEVERE] build_web_compilers:entrypoint on web/main.dart (cached): AssetNotFoundException: demo|web/main.unsound.ddc.js.metadata

[SEVERE] build_web_compilers:ddc on web/main.ddc.module (cached): Error compiling dartdevc module:demo|web/main.unsound.ddc.js

web/main.dart:15:7: Error: JS interop class 'DemoClass' cannot extend Dart class 'Object with DemoMixin'.
Try removing the JS interop annotation or adding it to the parent class.
class DemoClass with DemoMixin {
      ^

Everything compiles OK and without errors when using Dart <= 2.10.5-stable.

After some digging through the commits history, It seems that the change was introduced with commit https://github.com/dart-lang/sdk/commit/ed009afc4cb6ac6f3a088895ad1de5b8554026d8 but as far as I understand it is intended to prevent extending JS interop classes with plain Dart classes (and the opposite - extending plain Dart classes with JS interop ones). Also note that the test case from the above commit doesn't take mixins in consideration.

Am I missing something? Is this an intended behavior?

srujzs commented 3 years ago

Thanks for filing! Since mixin classes are generated, this makes sense, as the generated classes aren't annotated, and therefore our static checker will signal an error.

This could be resolved in the checker itself, but this opens up the question about whether we should allow mixins with package:js in general and support them. In the past, we've decided against it, as there are other issues around it, and we've (as you noted) excluded it from our analysis. However, it might be useful to make a concrete decision here and plan accordingly i.e. if we want to not support it, disallow it, and if we do want to support it, add testing to make sure it works. I'm not sure if the web compilers handle all the mixin cases correctly today, unfortunately, so I think this might be on the back-burner.

cc @sigmundch @rileyporter , do we have any thoughts on what we want to do with mixins?

sigmundch commented 2 years ago

Since we are headed towards static js-interop in the near future, it may be worth deciding first what we want to do in that context and then circle back to the non-static case.

For static interop, the value of allowing @JS mixins is pretty minimal since it is no different than using an interface with static extension methods. For example, this could work:

@JS() @staticInterop
class A {}

@JS() @staticInterop
mixin M{}

extension Me on M {
  external bool foo;
}

@JS() @staticInterop
class B extends A with M {
  factory B()...
}

but, the same will hold after s/mixin/class/ and s/with/implements/.

Once we have views/extension-types I can see us trying to move further away from modeling js-interop types as Dart classes. I sense mixins will not come into play at that point, so disallowing mixins would be aligned with that future.

I currently have a small leaning towards disallowing it. Allowing it would be a small leap from our current implementation, but the use cases are rare and disallowing it would prevent breaking changes down the line when we migrate towards static interop everywhere.

Thoughts?

srujzs commented 2 years ago

The work needed for that example might be simple enough (e.g. just make sure we walk up the mixin hierarchy in the checks) - I think the rest should come for free. I can see some value in defining mixins, but I also think they generally provide a lower benefit in the interop case compared to their non-interop use case, as we usually have external methods.

The extension types case is interesting - from a language feature POV, can they extend multiple other extension types, essentially replicating mixin functionality? If this is true and there is some value in allowing static interop mixins, I think supporting that would be easier and would keep it aligned with the future extension types model. If it is not true, I think it makes less sense to do so as the migration story would be even harder then. The non-static interop case is an unknown to me: it might not work in some cases on one or more of the compilers and I'm not sure if they're tested. Planning work there could be a bigger effort than we currently expect, so I'm also leaning towards disallowing it in that case.

At any rate, it might be worth having a separate error mentioning mixins aren't allowed in all or some use cases. The above error is unfortunate and unclear. Before that error, mixins were possibly being used because they worked in some cases and we didn't communicate that we didn't support them.

Edit: I asked Erik about this and he said you can extend multiple extension types.

KealJones commented 1 year ago

I have done a very large amount of work with Dart's JS interop and pushed it to its limits many times. I have started investigating the new @staticInterop extension types and as awesome as it is, I do think that static interop mixins would be an absolutely amazing feature to support officially. Because, although you can get inheritance via extension types it can be a bit more complicated to control, simply because of extensions scoping and lack of explicit verbosity. The advantages that static interop mixins offer seem perfectly poised for JS types.

srujzs commented 1 year ago

For what it's worth, @staticInterop is a prototype before we get inline classes (formerly known as extension types or views). Now that we have an inline class prototype and the language is working to evolve it into a fully-fledged feature, I'm less in favor of expanding @staticInterop features (but we still want to resolve bugs!), and focusing on inline classes instead if inline classes come with the feature in question.

That being said, I do agree that the abilities mixins provide are useful for static interop, as it's desirable to be able to mix a set of grouped interop members in. With inline classes, you can do the following:

Because members are within the inline class and not in an extension, scoping shouldn't be an issue, as the members are attached to the type. I'm not entirely sure what you're referring to with "lack of explicit verbosity". Is this referring to naming the mixin?

KealJones commented 1 year ago

I was just introduced to the proposal of inline classes today right as you replied. It does sound quite exciting so I agree with your stance. Thanks for the reply!