From 844a5176ff39b4eedd64e8317b30adb99db1fa07 Mon Sep 17 00:00:00 2001 From: Tess Strickland Date: Mon, 16 Nov 2020 11:53:39 +0000 Subject: [PATCH] [vm] Fix ArgumentsDescriptor uses in DoArgumentTypesMatch. Use of PositionalCount() and PositionAt() as indices in the arguments array must account for the type arguments if present. Otherwise, we'll either skip checking the last positional argument (in the former case) or check against the wrong arguments (in the latter case). In nosuchmethod_forwarding_arguments_test.dart, add cases that check for the above mistakes. In require_named_args_strong_test.dart, use the more specific throwsTypeError or throwsNoSuchMethod checks instead of the generic throwsError to ensure the correct error is thrown. TEST=Changed tests to add extra cases for failures that can happen. Cq-Include-Trybots: luci.dart.try:vm-kernel-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try Change-Id: If5e6c310d36d244bb0650ded54e32e583732584e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/171947 Reviewed-by: Martin Kustermann Reviewed-by: Daco Harkes Commit-Queue: Tess Strickland --- runtime/vm/object.cc | 6 ++-- .../required_named_args_strong_test.dart | 12 +++---- ...osuchmethod_forwarding_arguments_test.dart | 33 +++++++++++++++++++ ...osuchmethod_forwarding_arguments_test.dart | 33 +++++++++++++++++++ 4 files changed, 75 insertions(+), 9 deletions(-) diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 91ca10fbdef..ddba562f439 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -8135,8 +8135,8 @@ ObjectPtr Function::DoArgumentTypesMatch( const intptr_t arg_offset = args_desc.FirstArgIndex(); // Only check explicit arguments. const intptr_t arg_start = arg_offset + NumImplicitParameters(); - const intptr_t num_positional_args = args_desc.PositionalCount(); - for (intptr_t arg_index = arg_start; arg_index < num_positional_args; + const intptr_t end_positional_args = arg_offset + args_desc.PositionalCount(); + for (intptr_t arg_index = arg_start; arg_index < end_positional_args; ++arg_index) { argument ^= args.At(arg_index); // Adjust for type arguments when they're present. @@ -8166,7 +8166,7 @@ ObjectPtr Function::DoArgumentTypesMatch( named_index++) { argument_name = args_desc.NameAt(named_index); ASSERT(argument_name.IsSymbol()); - argument ^= args.At(args_desc.PositionAt(named_index)); + argument ^= args.At(arg_offset + args_desc.PositionAt(named_index)); // Try to find the named parameter that matches the provided argument. // Even when annotated with @required, named parameters are still stored diff --git a/tests/language/nnbd/required_named_parameters/required_named_args_strong_test.dart b/tests/language/nnbd/required_named_parameters/required_named_args_strong_test.dart index 71390365bd6..5f7cd58bbc0 100644 --- a/tests/language/nnbd/required_named_parameters/required_named_args_strong_test.dart +++ b/tests/language/nnbd/required_named_parameters/required_named_args_strong_test.dart @@ -12,7 +12,7 @@ main() { dynamic f = func; // Invalid: Subtype may not redeclare optional parameters as required. - Expect.throws(() { + Expect.throwsTypeError(() { Function( String p0, { required int p1, @@ -21,7 +21,7 @@ main() { }); // Invalid: Subtype may not declare new required named parameters. - Expect.throws(() { + Expect.throwsTypeError(() { Function( String p0, { required int p1, @@ -29,18 +29,18 @@ main() { }); // Invalid: Invocation with explicit null required named argument. - Expect.throws(() { + Expect.throwsTypeError(() { f("", p1: null, p2: null); }); - Expect.throws(() { + Expect.throwsTypeError(() { Function.apply(f, [""], {#p1: null, #p2: null}); }); // Invalid: Invocation that omits a required named argument. - Expect.throws(() { + Expect.throwsNoSuchMethodError(() { f("", p1: 100); }); - Expect.throws(() { + Expect.throwsNoSuchMethodError(() { Function.apply(f, [""], {#p1: 100}); }); } diff --git a/tests/language/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart b/tests/language/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart index 08a79ad56bc..ab8a4498244 100644 --- a/tests/language/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart +++ b/tests/language/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart @@ -5,6 +5,9 @@ // Testing that `noSuchMethod` forwarding properly handles optional, named and // type parameters, and result type checking. +// VMOptions=--lazy-dispatchers +// VMOptions=--no-lazy-dispatchers + import 'package:expect/expect.dart'; class A { @@ -48,6 +51,16 @@ class A { } else if (invoke.memberName == #test8) { Expect.equals(1, invoke.positionalArguments.length); Expect.equals(null, invoke.positionalArguments[0]); + } else if (invoke.memberName == #test9) { + Expect.equals(invoke.typeArguments.length, 2); + Expect.equals(invoke.typeArguments[0].toString(), "num"); + Expect.equals(invoke.typeArguments[1].toString(), "double"); + + Expect.equals(1, invoke.positionalArguments.length); + Expect.equals(4.2, invoke.positionalArguments[0]); + + Expect.equals(1, invoke.namedArguments.length); + Expect.equals(3, invoke.namedArguments[#foo]); } } @@ -63,6 +76,7 @@ class A { void set test7(int x); void test8([String? x]); + void test9(S x1, {T? foo}); T allTogetherNow(S x1, {List foo: const []}); } @@ -97,6 +111,25 @@ main() { Expect.throwsTypeError(() => (a as dynamic).test7 = "hi"); a.allTogetherNow(2.0, foo: const [3, 4]); + Expect.throwsTypeError(() => + (a.allTogetherNow as dynamic)(2.0, foo: const [3, 4])); + Expect.throwsTypeError(() => + (a.allTogetherNow as dynamic)(2.0, foo: const [3, 4])); + Expect.throwsTypeError(() => (a.allTogetherNow + as dynamic)(2.0, foo: const [3, 4])); a.test8(); + + a.test9(4.2, foo: 3); + Expect.throwsTypeError(() => (a.test9 as dynamic)(3, foo: 3)); + Expect.throwsTypeError( + () => (a.test9 as dynamic)(3, foo: 3.2)); + // Added to check that uses of positions from the ArgumentsDescriptor in the + // VM properly offsets named argument positions if there are also type + // arguments. allTogetherNow doesn't work for this because the runtime type of + // the positional argument cannot be the same as the type of the named + // argument without the positional argument failing to match its own type, + // and positional argument types are usually checked first. + Expect.throwsTypeError( + () => (a.test9 as dynamic)(4.2, foo: 3.2)); } diff --git a/tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart b/tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart index 01a59e77931..3e42573fa60 100644 --- a/tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart +++ b/tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_arguments_test.dart @@ -5,6 +5,9 @@ // Testing that `noSuchMethod` forwarding properly handles optional, named and // type parameters, and result type checking. +// VMOptions=--lazy-dispatchers +// VMOptions=--no-lazy-dispatchers + import 'package:expect/expect.dart'; class A { @@ -47,6 +50,16 @@ class A { } else if (invoke.memberName == #test8) { Expect.equals(1, invoke.positionalArguments.length); Expect.equals(null, invoke.positionalArguments[0]); + } else if (invoke.memberName == #test9) { + Expect.equals(invoke.typeArguments.length, 2); + Expect.equals(invoke.typeArguments[0].toString(), "num"); + Expect.equals(invoke.typeArguments[1].toString(), "double"); + + Expect.equals(1, invoke.positionalArguments.length); + Expect.equals(4.2, invoke.positionalArguments[0]); + + Expect.equals(1, invoke.namedArguments.length); + Expect.equals(3, invoke.namedArguments[#foo]); } } @@ -62,6 +75,7 @@ class A { void set test7(int x); void test8([String x]); + void test9(S x1, {T foo}); T allTogetherNow(S x1, {List foo: const []}); } @@ -96,6 +110,25 @@ main() { Expect.throwsTypeError(() => (a as dynamic).test7 = "hi"); a.allTogetherNow(2.0, foo: const [3, 4]); + Expect.throwsTypeError(() => + (a.allTogetherNow as dynamic)(2.0, foo: const [3, 4])); + Expect.throwsTypeError(() => + (a.allTogetherNow as dynamic)(2.0, foo: const [3, 4])); + Expect.throwsTypeError(() => (a.allTogetherNow + as dynamic)(2.0, foo: const [3, 4])); a.test8(); + + a.test9(4.2, foo: 3); + Expect.throwsTypeError(() => (a.test9 as dynamic)(3, foo: 3)); + Expect.throwsTypeError( + () => (a.test9 as dynamic)(3, foo: 3.2)); + // Added to check that uses of positions from the ArgumentsDescriptor in the + // VM properly offsets named argument positions if there are also type + // arguments. allTogetherNow doesn't work for this because the runtime type of + // the positional argument cannot be the same as the type of the named + // argument without the positional argument failing to match its own type, + // and positional argument types are usually checked first. + Expect.throwsTypeError( + () => (a.test9 as dynamic)(4.2, foo: 3.2)); }