Fix Js-interop factory calls which broke when eliding optional parameters.
The issue derives from the fact that `parameterStructure` has only the preserved parameters,
however the SSA was emitting values for all arguments.
I fixed this by emitting all parameters. I also noticed that the parameter order should
probably be the native-ordering here as well (it doesn't matter for the factory, but it may
matter for other static native calls, if any)
Bug: https://github.com/dart-lang/sdk/issues/35916
Change-Id: I2e93c7f8e3baf5c9285107d23786bb3bf4f3a511
Reviewed-on: https://dart-review.googlesource.com/c/92626
Commit-Queue: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Stephen Adams <sra@google.com>
diff --git a/pkg/compiler/lib/src/ssa/builder_kernel.dart b/pkg/compiler/lib/src/ssa/builder_kernel.dart
index 607fbeb..6ceb352 100644
--- a/pkg/compiler/lib/src/ssa/builder_kernel.dart
+++ b/pkg/compiler/lib/src/ssa/builder_kernel.dart
@@ -3266,12 +3266,8 @@
// Visit named arguments in parameter-position order, selecting provided
// or default value.
- // TODO(sra): Ensure the stored order is canonical so we don't have to
- // sort. The old builder uses CallStructure.makeArgumentList which
- // depends on the old element model.
- var namedParameters = target.namedParameters.toList()
- ..sort((ir.VariableDeclaration a, ir.VariableDeclaration b) =>
- a.name.compareTo(b.name));
+ var namedParameters = target.namedParameters.toList();
+ namedParameters.sort(nativeOrdering);
for (ir.VariableDeclaration parameter in namedParameters) {
HInstruction value = namedValues[parameter.name];
values.add(value);
@@ -4363,12 +4359,22 @@
// Factory constructor that is syntactic sugar for creating a JavaScript
// object literal.
ConstructorEntity constructor = element;
- ParameterStructure params = constructor.parameterStructure;
int i = 0;
int positions = 0;
var filteredArguments = <HInstruction>[];
var parameterNameMap = new Map<String, js.Expression>();
- params.namedParameters.forEach((String parameterName) {
+
+ // Note: we don't use `constructor.parameterStructure` here because
+ // we don't elide parameters to js-interop external static targets
+ // (including factory constructors.)
+ // TODO(johnniwinther): can we elide those parameters? This should be
+ // consistent with what we do with instance methods.
+ ir.Procedure node = _elementMap.getMemberDefinition(constructor).node;
+ List<ir.VariableDeclaration> namedParameters =
+ node.function.namedParameters.toList();
+ namedParameters.sort(nativeOrdering);
+ for (ir.VariableDeclaration variable in namedParameters) {
+ String parameterName = variable.name;
// TODO(jacobr): consider throwing if parameter names do not match
// names of properties in the class.
HInstruction argument = arguments[i];
@@ -4378,7 +4384,7 @@
parameterNameMap[jsName] = new js.InterpolatedExpression(positions++);
}
i++;
- });
+ }
var codeTemplate =
new js.Template(null, js.objectLiteral(parameterNameMap));
diff --git a/tests/compiler/dart2js_extra/js_interop_no_elide_optional_arg_test.dart b/tests/compiler/dart2js_extra/js_interop_no_elide_optional_arg_test.dart
new file mode 100644
index 0000000..5d6d913
--- /dev/null
+++ b/tests/compiler/dart2js_extra/js_interop_no_elide_optional_arg_test.dart
@@ -0,0 +1,31 @@
+// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/// Test that optional arguments of js-interop factory constructors are not
+/// elided.
+/// This is a regression test for issue 35916
+@JS()
+library test;
+
+import "package:js/js.dart";
+import "package:expect/expect.dart";
+
+@JS()
+@anonymous
+class Margins {
+ external factory Margins(
+ {int top, int start, int end, int right, int bottom, int left});
+ external int get top;
+ external int get right;
+ external int get left;
+ external int get bottom;
+}
+
+main() {
+ var m = new Margins(bottom: 21, left: 88, right: 20, top: 24);
+ Expect.equals(m.top, 24);
+ Expect.equals(m.bottom, 21);
+ Expect.equals(m.left, 88);
+ Expect.equals(m.right, 20);
+}
