[package:js] Disallow generative constructors in @staticInterop

[srujzs]

This discussion spawned off of supporting generative constructors in Dart-Wasm interop in https://dart-review.googlesource.com/c/sdk/+/239009. The driving factor was that disallowing generative constructors will improve the interop story for Wasm due to the difficulties in supporting them.

Currently, @staticInterop classes can use either factory or generative constructors e.g.

@JS()
@staticInterop
class GenClass {
  external GenClass();
  external GenClass.named();
}

@JS()
@staticInterop
class FactClass {
  external factory FactClass();
  external factory FactClass.named();
  factory FactClass.nonExt() => FactClass();
}

The code for all external constructors (generative vs factory or named vs not) all seem to generate the same code in dart2js and ddc. They all resolve to the provided namespace of the library (if any) + the name of the class (with any potential renaming). Migrating off of generative constructors to factory constructors will be fairly simple from a code difference standpoint - simply add factory. Doing this will be a breaking change however, although the surface area is fairly small still since @staticInterop is fairly new. js_bindings seems to be the large user of external generative constructors, and internally there are a few packages that use them.

Making this change doesn't significantly affect the migration story when we do get views, especially if we have a migration tool making the transformation from generative to factory constructors. Manual migrations might be less confusing, however. This is all contingent on how factories are supported in views, of course. If external factories are not allowed in views, then that further complicates things (because now we're requiring users to explicitly use callConstructor in conjunction with a non-external factory). If they are, there's still a matter of resolving what that factory should point to (with @JS for example). At any rate, I don't think this is a motivator for this change, as migration to views from @staticInterop can be largely automated.

There are a couple options in migrating:

1. Refactor all internal code to use external factory instead, add a check disabling generative constructors, and release with 2.18 (or some future version), letting people know this is a breaking change.
2. Go the dart fix route and automatically convert code as this is a simple fix, and then still add the check and release once that fixer is ready. This may be excessive work and requires investigation, but is definitely an option.

cc @joshualitt @sigmundch @rileyporter

[sigmundch]

The code for all external constructors (generative vs factory or named vs not) all seem to generate the same code in dart2js and ddc.

When we first discussed this, I had a different recollection about dart2js. Turns out that my confusion was about regular (non-external) generative constructors as opposed to factory vs non-factory constructors. Apparently we still allow implicit non-external constructors like in this example below. This code prints null in dart2js, but 3 in DDC:

import 'package:js/js.dart';
@JS() class A { external get foo; } // implicit non-external constructor
@JS() external void eval(String s);
main() {
  eval('''self.A = function A() { this.foo = 3; };''');
  print(A().foo);
}

If you add an external A non-factory constructor, then dart2js prints 3 as expected.

This is all contingent on how factories are supported in views, of course. If external factories are not allowed in views, then that further complicates things (because now we're requiring users to explicitly use callConstructor in conjunction with a non-external factory). If they are, there's still a matter of resolving what that factory should point to (with @js for example)

@leafpetersen - it would be good to discuss how views will be constructed and what syntax will be available. We have two common use cases for construction: wrapping/assigning an existing value (which I believe will be a common pattern in static views in general) or creating a brand new value. For the latter, we want to make the JSInterop pattern very succinct, and in the past we've done so via external constructors.

... options in migrating:

Agree that the dart fix route seems excessive for this. The feature has very limited use at the moment, so a manual migration is much more tractable. I'd move forward and rewrite all existing uses, and send a PR for js_bindings (mostly requires a 1-line change to their generator and rebuilding the output).

[srujzs]

Oh, that's unfortunate that we don't handle synthetic constructors correctly. I think a check is honestly the best idea there, as it's best to ensure users are explicit in whether they want an external constructor or non-external that redirects.

Okay, I'll move forward on migrating google3 and js_bindings to external factorys instead.