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

dart2js: native order of arguments doesn't match correctly for jsinterop factory constructors. #35916

Open sigmundch opened 5 years ago

sigmundch commented 5 years ago

repro:

@JS()
import 'package:js/js.dart';

@JS()
@anonymous
class Margins {
  external factory Margins({int top, int start, int end, int right, int bottom, int left});
}

main() {
  print(new Margins(bottom: 21, left: 88, right: 20, top: 24));
}

produces this

H.printString(H.S({bottom: 21, left: 88, right: 20, top: 24}));

instead of

H.printString(H.S({bottom: 21, right: 88, top: 24}));

sigmundch commented 5 years ago

The issue derives from the fact that the factory was created using non-elided parameters (derived from entity.parameterStructure), but the call was passing values for all parameters (elided or not)

Because the code invoking the factory is the same used to invoke native static methods (which cannot elide parameters), I believe we need to explicitly pass all parameters.

@johnniwinther - Question for you - isn't that true also for non-static external js-interop methods? We seem to be filtering out the elided parameters there too.

sigmundch commented 5 years ago

https://dart-review.googlesource.com/c/sdk/+/92626

sigmundch commented 5 years ago

I landed a quick fix, but @johnniwinther - please take a look. Looking more closely it is possible that:

johnniwinther commented 5 years ago

Static native methods shouldn't allow named parameters but we might not currently enforce this fully; there is no check for native methods and the check for js-interop method might be incomplete.

Since named parameters are only supported for factory constructors on anonymous classes there is nothing to optimize; we already only pass the parameters given at the call site.