From 8fda1d6cf3b4a339fd3702fd4ec48242c4f68bfb Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Thu, 24 Jun 2021 03:29:38 +0000 Subject: [PATCH] [js_runtime] Don't modify inputs to Function.apply In some cases adding default arguments modified the 'positional' arguments input List. Since there are 'accelerated' paths for small numbers of arguments, this bug did not show in any other tests. Change-Id: I5075c96ecfdc9645b85b9beeeb38305415153d20 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204747 Commit-Queue: Stephen Adams Reviewed-by: Mayank Patke --- .../_internal/js_runtime/lib/js_helper.dart | 39 +++++++---- tests/corelib/apply6_test.dart | 66 ++++++++++++++++++ tests/corelib_2/apply6_test.dart | 68 +++++++++++++++++++ 3 files changed, 158 insertions(+), 15 deletions(-) create mode 100644 tests/corelib/apply6_test.dart create mode 100644 tests/corelib_2/apply6_test.dart diff --git a/sdk/lib/_internal/js_runtime/lib/js_helper.dart b/sdk/lib/_internal/js_runtime/lib/js_helper.dart index f06c29a5f31..cb46dfaebc3 100644 --- a/sdk/lib/_internal/js_runtime/lib/js_helper.dart +++ b/sdk/lib/_internal/js_runtime/lib/js_helper.dart @@ -820,11 +820,10 @@ class Primitives { /// the value of the catch-all property. static applyFunction(Function function, List? positionalArguments, Map? namedArguments) { - // Fast shortcut for the common case. - if (JS('bool', '# instanceof Array', positionalArguments) && + // Fast path for common cases. + if (positionalArguments is JSArray && (namedArguments == null || namedArguments.isEmpty)) { - // Let the compiler know that we did a type-test. - List arguments = (JS('JSArray', '#', positionalArguments)); + JSArray arguments = positionalArguments; int argumentCount = arguments.length; if (argumentCount == 0) { String selectorName = JS_GET_NAME(JsGetName.CALL_PREFIX0); @@ -877,18 +876,17 @@ class Primitives { } } - return _genericApplyFunction2( - function, positionalArguments, namedArguments); + return _generalApplyFunction(function, positionalArguments, namedArguments); } - static _genericApplyFunction2(Function function, List? positionalArguments, + static _generalApplyFunction(Function function, List? positionalArguments, Map? namedArguments) { List arguments; if (positionalArguments != null) { - if (JS('bool', '# instanceof Array', positionalArguments)) { - arguments = JS('JSArray', '#', positionalArguments); + if (positionalArguments is JSArray) { + arguments = positionalArguments; } else { - arguments = new List.from(positionalArguments); + arguments = List.of(positionalArguments); } } else { arguments = []; @@ -934,8 +932,7 @@ class Primitives { return functionNoSuchMethod(function, arguments, namedArguments); } - bool acceptsPositionalArguments = - JS('bool', '# instanceof Array', defaultValues); + bool acceptsPositionalArguments = defaultValues is JSArray; if (acceptsPositionalArguments) { if (namedArguments != null && namedArguments.isNotEmpty) { @@ -950,9 +947,15 @@ class Primitives { // The function expects fewer arguments. return functionNoSuchMethod(function, arguments, null); } - List missingDefaults = JS('JSArray', '#.slice(#)', defaultValues, - argumentCount - requiredParameterCount); - arguments.addAll(missingDefaults); + if (argumentCount < maxArguments) { + List missingDefaults = JS('JSArray', '#.slice(#)', defaultValues, + argumentCount - requiredParameterCount); + if (identical(arguments, positionalArguments)) { + // Defensive copy to avoid modifying passed-in List. + arguments = List.of(arguments); + } + arguments.addAll(missingDefaults); + } return JS('var', '#.apply(#, #)', jsFunction, function, arguments); } else { // Handle named arguments. @@ -963,6 +966,11 @@ class Primitives { return functionNoSuchMethod(function, arguments, namedArguments); } + if (identical(arguments, positionalArguments)) { + // Defensive copy to avoid modifying passed-in List. + arguments = List.of(arguments); + } + List keys = JS('JSArray', r'Object.keys(#)', defaultValues); if (namedArguments == null) { for (String key in keys) { @@ -987,6 +995,7 @@ class Primitives { } } if (used != namedArguments.length) { + // Named argument with name not accected by function. return functionNoSuchMethod(function, arguments, namedArguments); } } diff --git a/tests/corelib/apply6_test.dart b/tests/corelib/apply6_test.dart new file mode 100644 index 00000000000..19ee0f4d4c6 --- /dev/null +++ b/tests/corelib/apply6_test.dart @@ -0,0 +1,66 @@ +// Copyright (c) 2021, 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 the List of positional arguments to [Function.apply] is not +// modified. + +import "package:expect/expect.dart"; + +class A { + foo([ + a = 10, + b = 20, + c = 30, + d = 40, + e = 50, + f = 60, + g = 70, + h = 80, + i = 90, + ]) => + '$a $b $c $d $e $f $g $h $i'; +} + +String static1(a, b, {c = 30, d = 40}) => '$a $b $c $d'; + +void test(String expected, Function function, List positional, + [Map? named = null]) { + final original = List.of(positional); + + Expect.equals(expected, Function.apply(function, positional, named)); + Expect.listEquals(original, positional); + + // Test again so there are multiple call sites for `Function.apply`. + Expect.equals(expected, Function.apply(function, positional, named)); + Expect.listEquals(original, positional); +} + +main() { + var a = A(); + + test('10 20 30 40 50 60 70 80 90', a.foo, []); + test('11 20 30 40 50 60 70 80 90', a.foo, [11]); + test('11 22 30 40 50 60 70 80 90', a.foo, [11, 22]); + test('11 22 33 40 50 60 70 80 90', a.foo, [11, 22, 33]); + test('11 22 33 44 50 60 70 80 90', a.foo, [11, 22, 33, 44]); + test('11 22 33 44 55 60 70 80 90', a.foo, [11, 22, 33, 44, 55]); + test('11 22 33 44 55 66 70 80 90', a.foo, [11, 22, 33, 44, 55, 66]); + test('11 22 33 44 55 66 77 80 90', a.foo, [11, 22, 33, 44, 55, 66, 77]); + test('11 22 33 44 55 66 77 88 90', a.foo, [11, 22, 33, 44, 55, 66, 77, 88]); + test('11 22 33 44 55 66 77 88 99', a.foo, + [11, 22, 33, 44, 55, 66, 77, 88, 99]); + + // Some unmodifiable Lists. An attempt to modify the argument would fail. + test('11 22 33 44 55 66 77 80 90', a.foo, const [11, 22, 33, 44, 55, 66, 77]); + test('65 66 67 68 69 70 71 80 90', a.foo, 'ABCDEFG'.codeUnits); + + test('11 22 30 40', static1, [11, 22]); + test('11 22 30 40', static1, [11, 22], {}); + test('11 22 33 40', static1, [11, 22], {#c: 33}); + test('11 22 30 44', static1, [11, 22], {#d: 44}); + test('11 22 66 55', static1, [11, 22], {#d: 55, #c: 66}); + + test('11 22 88 77', static1, const [11, 22], {#d: 77, #c: 88}); + test('65 66 11 22', static1, 'AB'.codeUnits, {#d: 22, #c: 11}); +} diff --git a/tests/corelib_2/apply6_test.dart b/tests/corelib_2/apply6_test.dart new file mode 100644 index 00000000000..0ba08f8617e --- /dev/null +++ b/tests/corelib_2/apply6_test.dart @@ -0,0 +1,68 @@ +// Copyright (c) 2021, 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. + +// @dart=2.9 + +// Test that the List of positional arguments to [Function.apply] is not +// modified. + +import "package:expect/expect.dart"; + +class A { + foo([ + a = 10, + b = 20, + c = 30, + d = 40, + e = 50, + f = 60, + g = 70, + h = 80, + i = 90, + ]) => + '$a $b $c $d $e $f $g $h $i'; +} + +String static1(a, b, {c = 30, d = 40}) => '$a $b $c $d'; + +void test(String expected, Function function, List positional, + [Map named = null]) { + final original = List.of(positional); + + Expect.equals(expected, Function.apply(function, positional, named)); + Expect.listEquals(original, positional); + + // Test again so there are multiple call sites for `Function.apply`. + Expect.equals(expected, Function.apply(function, positional, named)); + Expect.listEquals(original, positional); +} + +main() { + var a = A(); + + test('10 20 30 40 50 60 70 80 90', a.foo, []); + test('11 20 30 40 50 60 70 80 90', a.foo, [11]); + test('11 22 30 40 50 60 70 80 90', a.foo, [11, 22]); + test('11 22 33 40 50 60 70 80 90', a.foo, [11, 22, 33]); + test('11 22 33 44 50 60 70 80 90', a.foo, [11, 22, 33, 44]); + test('11 22 33 44 55 60 70 80 90', a.foo, [11, 22, 33, 44, 55]); + test('11 22 33 44 55 66 70 80 90', a.foo, [11, 22, 33, 44, 55, 66]); + test('11 22 33 44 55 66 77 80 90', a.foo, [11, 22, 33, 44, 55, 66, 77]); + test('11 22 33 44 55 66 77 88 90', a.foo, [11, 22, 33, 44, 55, 66, 77, 88]); + test('11 22 33 44 55 66 77 88 99', a.foo, + [11, 22, 33, 44, 55, 66, 77, 88, 99]); + + // Some unmodifiable Lists. An attempt to modify the argument would fail. + test('11 22 33 44 55 66 77 80 90', a.foo, const [11, 22, 33, 44, 55, 66, 77]); + test('65 66 67 68 69 70 71 80 90', a.foo, 'ABCDEFG'.codeUnits); + + test('11 22 30 40', static1, [11, 22]); + test('11 22 30 40', static1, [11, 22], {}); + test('11 22 33 40', static1, [11, 22], {#c: 33}); + test('11 22 30 44', static1, [11, 22], {#d: 44}); + test('11 22 66 55', static1, [11, 22], {#d: 55, #c: 66}); + + test('11 22 88 77', static1, const [11, 22], {#d: 77, #c: 88}); + test('65 66 11 22', static1, 'AB'.codeUnits, {#d: 22, #c: 11}); +}