From 8db3be75579ec9ce457b338c70fb35dbdf05b7b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sinan=20A=C4=9Facan?= Date: Tue, 10 Jan 2023 13:37:33 +0000 Subject: [PATCH] [dart2wasm] Fix invocation forwarder named argument adjustment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Invocation forwarders currently cannot distinguish a named parameter not passed from passed as `null`. Refactor `getNamedParameter` to return nullable index. When the index is not null it means that the named parameter is present, and its value may be `null`. This doesn't fix any of the existing tests so I added a regression test. (Bug originally discovered and fixed in https://dart-review.googlesource.com/c/sdk/+/278505) Change-Id: I960c674073b3d25c37e79f9836647882acaff08b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278745 Reviewed-by: Aske Simon Christensen Commit-Queue: Ömer Ağacan Reviewed-by: Joshua Litt --- pkg/dart2wasm/lib/dynamic_forwarders.dart | 39 ++++++++++++++----- pkg/dart2wasm/lib/kernel_nodes.dart | 4 +- .../_internal/wasm/lib/named_parameters.dart | 8 ++-- tests/language/dynamic/named_args_test.dart | 18 +++++++++ 4 files changed, 53 insertions(+), 16 deletions(-) create mode 100644 tests/language/dynamic/named_args_test.dart diff --git a/pkg/dart2wasm/lib/dynamic_forwarders.dart b/pkg/dart2wasm/lib/dynamic_forwarders.dart index 7a33525793f..1c57e85d739 100644 --- a/pkg/dart2wasm/lib/dynamic_forwarders.dart +++ b/pkg/dart2wasm/lib/dynamic_forwarders.dart @@ -334,8 +334,8 @@ class Forwarder { translator, function, targetMemberParamInfo.named.length); b.local_set(adjustedNamedArgsLocal); - final namedParameterValueLocal = - function.addLocal(translator.topInfo.nullableType); + final namedParameterIdxLocal = function.addLocal( + translator.classInfo[translator.boxedIntClass]!.nullableType); final remainingNamedArgsLocal = numArgsLocal; b.local_get(namedArgsLocal); @@ -371,9 +371,9 @@ class Forwarder { translator.classInfo[translator.symbolClass]!.nonNullableType); b.call(translator.functions - .getFunction(translator.getNamedParameter.reference)); + .getFunction(translator.getNamedParameterIndex.reference)); + b.local_tee(namedParameterIdxLocal); - b.local_tee(namedParameterValueLocal); b.ref_is_null(); b.i32_eqz(); b.if_(); @@ -383,14 +383,20 @@ class Forwarder { b.local_set(remainingNamedArgsLocal); b.end(); - b.local_get(namedParameterValueLocal); + b.local_get(namedParameterIdxLocal); b.ref_is_null(); if (functionNodeDefaultValue == null && paramInfoDefaultValue == null) { // Required b.br_if(topBlock); b.local_get(adjustedNamedArgsLocal); - b.local_get(namedParameterValueLocal); + b.local_get(namedArgsLocal); + translator.indexList(b, (b) { + b.local_get(namedParameterIdxLocal); + translator.convertType( + function, namedParameterIdxLocal.type, w.NumType.i64); + b.i32_wrap_i64(); + }); b.call(translator.functions .getFunction(translator.growableListAdd.reference)); } else { @@ -398,6 +404,8 @@ class Forwarder { // the member b.if_(); + b.local_get(adjustedNamedArgsLocal); + if (functionNodeDefaultValue != null) { // Used by the member, has a default value translator.constants.instantiateConstant( @@ -414,13 +422,24 @@ class Forwarder { translator.topInfo.nullableType, ); } - b.local_set(namedParameterValueLocal); - b.end(); - b.local_get(adjustedNamedArgsLocal); - b.local_get(namedParameterValueLocal); b.call(translator.functions .getFunction(translator.growableListAdd.reference)); + + b.else_(); + + b.local_get(adjustedNamedArgsLocal); + b.local_get(namedArgsLocal); + translator.indexList(b, (b) { + b.local_get(namedParameterIdxLocal); + translator.convertType( + function, namedParameterIdxLocal.type, w.NumType.i64); + b.i32_wrap_i64(); + }); + b.call(translator.functions + .getFunction(translator.growableListAdd.reference)); + + b.end(); } } diff --git a/pkg/dart2wasm/lib/kernel_nodes.dart b/pkg/dart2wasm/lib/kernel_nodes.dart index 53bfb198cdb..a2c24fa3706 100644 --- a/pkg/dart2wasm/lib/kernel_nodes.dart +++ b/pkg/dart2wasm/lib/kernel_nodes.dart @@ -196,8 +196,8 @@ mixin KernelNodes { "dart:core", "_TypeUniverse", "substituteFunctionTypeArgument"); // dart:core dynamic invocation helper procedures - late final Procedure getNamedParameter = - index.getTopLevelProcedure("dart:core", "_getNamedParameter"); + late final Procedure getNamedParameterIndex = + index.getTopLevelProcedure("dart:core", "_getNamedParameterIndex"); late final Procedure namedParameterListToMap = index.getTopLevelProcedure("dart:core", "_namedParameterListToMap"); diff --git a/sdk/lib/_internal/wasm/lib/named_parameters.dart b/sdk/lib/_internal/wasm/lib/named_parameters.dart index f830c34f1e2..bf8c8aa1bcb 100644 --- a/sdk/lib/_internal/wasm/lib/named_parameters.dart +++ b/sdk/lib/_internal/wasm/lib/named_parameters.dart @@ -5,13 +5,13 @@ part of "core_patch.dart"; /// Finds a named parameter in a named parameter list passed to a dynamic -/// forwarder and returns the value of that named parameter. Returns `null` if -/// the name is not in the list. +/// forwarder and returns the index of the value of that named parameter. +/// Returns `null` if the name is not in the list. @pragma("wasm:entry-point") -Object? _getNamedParameter(List namedArguments, Symbol paramName) { +int? _getNamedParameterIndex(List namedArguments, Symbol paramName) { for (int i = 0; i < namedArguments.length; i += 2) { if (identical(namedArguments[i], paramName)) { - return namedArguments[i + 1]; + return i + 1; } } return null; diff --git a/tests/language/dynamic/named_args_test.dart b/tests/language/dynamic/named_args_test.dart new file mode 100644 index 00000000000..212e151f8f5 --- /dev/null +++ b/tests/language/dynamic/named_args_test.dart @@ -0,0 +1,18 @@ +// Copyright (c) 2023, 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. + +import 'package:expect/expect.dart'; + +class A { + int? test({int? a = 123}) { + return a; + } +} + +void main() { + dynamic x = A(); + Expect.equals(null, x.test(a: null)); + Expect.equals(123, x.test()); + Expect.equals(456, x.test(a: 456)); +}