From f1a69eb08d06bd39add1c08ab83e1ef71b8e1565 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Fri, 7 Jun 2024 15:05:53 -0400 Subject: [PATCH] [pigeon] Fix handling of null class args in C++ (#6881) The code generated for Windows (C++) crashes when `null` is passed for a nullable, class-type parameter to a Pigeon method. This fixes that generation, and adds an integration test covering this case since it was missing from the list of types where we echo a `null` in integration tests to catch this kind of issue. Fixes https://github.com/flutter/flutter/issues/149174 --- packages/pigeon/CHANGELOG.md | 4 + packages/pigeon/lib/cpp_generator.dart | 2 +- packages/pigeon/lib/generator_tools.dart | 2 +- .../lib/integration_tests.dart | 9 + .../windows/pigeon/core_tests.gen.cpp | 232 ++++++++++-------- packages/pigeon/pubspec.yaml | 2 +- packages/pigeon/test/cpp_generator_test.dart | 2 +- 7 files changed, 143 insertions(+), 110 deletions(-) diff --git a/packages/pigeon/CHANGELOG.md b/packages/pigeon/CHANGELOG.md index 4fa98cd73e21..0d5d10af3d49 100644 --- a/packages/pigeon/CHANGELOG.md +++ b/packages/pigeon/CHANGELOG.md @@ -1,3 +1,7 @@ +## 20.0.1 + +* [cpp] Fixes handling of null class arguments. + ## 20.0.0 * Moves all codec logic to single custom codec per file. diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index 5c42df0b0822..47dd2b3276bd 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -1529,7 +1529,7 @@ if (!$encodableArgName.IsNull()) { }'''); } else { indent.writeln( - 'const auto* $argName = &(${_classReferenceFromEncodableValue(hostType, encodableArgName)});'); + 'const auto* $argName = $encodableArgName.IsNull() ? nullptr : &(${_classReferenceFromEncodableValue(hostType, encodableArgName)});'); } } else { // Non-nullable arguments are either passed by value or reference, but the diff --git a/packages/pigeon/lib/generator_tools.dart b/packages/pigeon/lib/generator_tools.dart index 415113c7ff7b..7c43243b0f53 100644 --- a/packages/pigeon/lib/generator_tools.dart +++ b/packages/pigeon/lib/generator_tools.dart @@ -13,7 +13,7 @@ import 'ast.dart'; /// The current version of pigeon. /// /// This must match the version in pubspec.yaml. -const String pigeonVersion = '20.0.0'; +const String pigeonVersion = '20.0.1'; /// Prefix for all local variables in methods. /// diff --git a/packages/pigeon/platform_tests/shared_test_plugin_code/lib/integration_tests.dart b/packages/pigeon/platform_tests/shared_test_plugin_code/lib/integration_tests.dart index d108f4892f07..ba3ac04c1c95 100644 --- a/packages/pigeon/platform_tests/shared_test_plugin_code/lib/integration_tests.dart +++ b/packages/pigeon/platform_tests/shared_test_plugin_code/lib/integration_tests.dart @@ -983,6 +983,15 @@ void runPigeonIntegrationTests(TargetGenerator targetGenerator) { expect(echoEnum, sentEnum); }); + testWidgets('null classes serialize and deserialize correctly', + (WidgetTester _) async { + final HostIntegrationCoreApi api = HostIntegrationCoreApi(); + + final AllNullableTypes? echoObject = await api.echoAllNullableTypes(null); + + expect(echoObject, isNull); + }); + testWidgets('optional nullable parameter', (WidgetTester _) async { final HostIntegrationCoreApi api = HostIntegrationCoreApi(); diff --git a/packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.cpp b/packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.cpp index ec06637957d5..c97dc064a96b 100644 --- a/packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.cpp +++ b/packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.cpp @@ -2261,9 +2261,11 @@ void HostIntegrationCoreApi::SetUp(flutter::BinaryMessenger* binary_messenger, const auto& args = std::get(message); const auto& encodable_everything_arg = args.at(0); const auto* everything_arg = - &(std::any_cast( - std::get( - encodable_everything_arg))); + encodable_everything_arg.IsNull() + ? nullptr + : &(std::any_cast( + std::get( + encodable_everything_arg))); ErrorOr> output = api->EchoAllNullableTypes(everything_arg); if (output.has_error()) { @@ -2295,35 +2297,38 @@ void HostIntegrationCoreApi::SetUp(flutter::BinaryMessenger* binary_messenger, prepended_suffix, &GetCodec()); if (api != nullptr) { - channel.SetMessageHandler( - [api](const EncodableValue& message, - const flutter::MessageReply& reply) { - try { - const auto& args = std::get(message); - const auto& encodable_everything_arg = args.at(0); - const auto* everything_arg = - &(std::any_cast( - std::get( - encodable_everything_arg))); - ErrorOr> output = - api->EchoAllNullableTypesWithoutRecursion(everything_arg); - if (output.has_error()) { - reply(WrapError(output.error())); - return; - } - EncodableList wrapped; - auto output_optional = std::move(output).TakeValue(); - if (output_optional) { - wrapped.push_back( - CustomEncodableValue(std::move(output_optional).value())); - } else { - wrapped.push_back(EncodableValue()); - } - reply(EncodableValue(std::move(wrapped))); - } catch (const std::exception& exception) { - reply(WrapError(exception.what())); - } - }); + channel.SetMessageHandler([api]( + const EncodableValue& message, + const flutter::MessageReply& + reply) { + try { + const auto& args = std::get(message); + const auto& encodable_everything_arg = args.at(0); + const auto* everything_arg = + encodable_everything_arg.IsNull() + ? nullptr + : &(std::any_cast( + std::get( + encodable_everything_arg))); + ErrorOr> output = + api->EchoAllNullableTypesWithoutRecursion(everything_arg); + if (output.has_error()) { + reply(WrapError(output.error())); + return; + } + EncodableList wrapped; + auto output_optional = std::move(output).TakeValue(); + if (output_optional) { + wrapped.push_back( + CustomEncodableValue(std::move(output_optional).value())); + } else { + wrapped.push_back(EncodableValue()); + } + reply(EncodableValue(std::move(wrapped))); + } catch (const std::exception& exception) { + reply(WrapError(exception.what())); + } + }); } else { channel.SetMessageHandler(nullptr); } @@ -3459,9 +3464,11 @@ void HostIntegrationCoreApi::SetUp(flutter::BinaryMessenger* binary_messenger, const auto& args = std::get(message); const auto& encodable_everything_arg = args.at(0); const auto* everything_arg = - &(std::any_cast( - std::get( - encodable_everything_arg))); + encodable_everything_arg.IsNull() + ? nullptr + : &(std::any_cast( + std::get( + encodable_everything_arg))); api->EchoAsyncNullableAllNullableTypes( everything_arg, [reply](ErrorOr>&& output) { @@ -3495,39 +3502,41 @@ void HostIntegrationCoreApi::SetUp(flutter::BinaryMessenger* binary_messenger, prepended_suffix, &GetCodec()); if (api != nullptr) { - channel.SetMessageHandler( - [api](const EncodableValue& message, - const flutter::MessageReply& reply) { - try { - const auto& args = std::get(message); - const auto& encodable_everything_arg = args.at(0); - const auto* everything_arg = - &(std::any_cast( - std::get( - encodable_everything_arg))); - api->EchoAsyncNullableAllNullableTypesWithoutRecursion( - everything_arg, - [reply]( - ErrorOr>&& + channel.SetMessageHandler([api]( + const EncodableValue& message, + const flutter::MessageReply& + reply) { + try { + const auto& args = std::get(message); + const auto& encodable_everything_arg = args.at(0); + const auto* everything_arg = + encodable_everything_arg.IsNull() + ? nullptr + : &(std::any_cast( + std::get( + encodable_everything_arg))); + api->EchoAsyncNullableAllNullableTypesWithoutRecursion( + everything_arg, + [reply](ErrorOr>&& output) { - if (output.has_error()) { - reply(WrapError(output.error())); - return; - } - EncodableList wrapped; - auto output_optional = std::move(output).TakeValue(); - if (output_optional) { - wrapped.push_back(CustomEncodableValue( - std::move(output_optional).value())); - } else { - wrapped.push_back(EncodableValue()); - } - reply(EncodableValue(std::move(wrapped))); - }); - } catch (const std::exception& exception) { - reply(WrapError(exception.what())); - } - }); + if (output.has_error()) { + reply(WrapError(output.error())); + return; + } + EncodableList wrapped; + auto output_optional = std::move(output).TakeValue(); + if (output_optional) { + wrapped.push_back( + CustomEncodableValue(std::move(output_optional).value())); + } else { + wrapped.push_back(EncodableValue()); + } + reply(EncodableValue(std::move(wrapped))); + }); + } catch (const std::exception& exception) { + reply(WrapError(exception.what())); + } + }); } else { channel.SetMessageHandler(nullptr); } @@ -4057,9 +4066,11 @@ void HostIntegrationCoreApi::SetUp(flutter::BinaryMessenger* binary_messenger, const auto& args = std::get(message); const auto& encodable_everything_arg = args.at(0); const auto* everything_arg = - &(std::any_cast( - std::get( - encodable_everything_arg))); + encodable_everything_arg.IsNull() + ? nullptr + : &(std::any_cast( + std::get( + encodable_everything_arg))); api->CallFlutterEchoAllNullableTypes( everything_arg, [reply](ErrorOr>&& output) { @@ -4142,39 +4153,41 @@ void HostIntegrationCoreApi::SetUp(flutter::BinaryMessenger* binary_messenger, prepended_suffix, &GetCodec()); if (api != nullptr) { - channel.SetMessageHandler( - [api](const EncodableValue& message, - const flutter::MessageReply& reply) { - try { - const auto& args = std::get(message); - const auto& encodable_everything_arg = args.at(0); - const auto* everything_arg = - &(std::any_cast( - std::get( - encodable_everything_arg))); - api->CallFlutterEchoAllNullableTypesWithoutRecursion( - everything_arg, - [reply]( - ErrorOr>&& + channel.SetMessageHandler([api]( + const EncodableValue& message, + const flutter::MessageReply& + reply) { + try { + const auto& args = std::get(message); + const auto& encodable_everything_arg = args.at(0); + const auto* everything_arg = + encodable_everything_arg.IsNull() + ? nullptr + : &(std::any_cast( + std::get( + encodable_everything_arg))); + api->CallFlutterEchoAllNullableTypesWithoutRecursion( + everything_arg, + [reply](ErrorOr>&& output) { - if (output.has_error()) { - reply(WrapError(output.error())); - return; - } - EncodableList wrapped; - auto output_optional = std::move(output).TakeValue(); - if (output_optional) { - wrapped.push_back(CustomEncodableValue( - std::move(output_optional).value())); - } else { - wrapped.push_back(EncodableValue()); - } - reply(EncodableValue(std::move(wrapped))); - }); - } catch (const std::exception& exception) { - reply(WrapError(exception.what())); - } - }); + if (output.has_error()) { + reply(WrapError(output.error())); + return; + } + EncodableList wrapped; + auto output_optional = std::move(output).TakeValue(); + if (output_optional) { + wrapped.push_back( + CustomEncodableValue(std::move(output_optional).value())); + } else { + wrapped.push_back(EncodableValue()); + } + reply(EncodableValue(std::move(wrapped))); + }); + } catch (const std::exception& exception) { + reply(WrapError(exception.what())); + } + }); } else { channel.SetMessageHandler(nullptr); } @@ -5108,8 +5121,12 @@ void FlutterIntegrationCoreApi::EchoAllNullableTypes( std::get(list_return_value->at(1)), list_return_value->at(2))); } else { - const auto* return_value = &(std::any_cast( - std::get(list_return_value->at(0)))); + const auto* return_value = + list_return_value->at(0).IsNull() + ? nullptr + : &(std::any_cast( + std::get( + list_return_value->at(0)))); on_success(return_value); } } else { @@ -5191,8 +5208,11 @@ void FlutterIntegrationCoreApi::EchoAllNullableTypesWithoutRecursion( list_return_value->at(2))); } else { const auto* return_value = - &(std::any_cast( - std::get(list_return_value->at(0)))); + list_return_value->at(0).IsNull() + ? nullptr + : &(std::any_cast( + std::get( + list_return_value->at(0)))); on_success(return_value); } } else { diff --git a/packages/pigeon/pubspec.yaml b/packages/pigeon/pubspec.yaml index 0281b15cba31..413804a0691e 100644 --- a/packages/pigeon/pubspec.yaml +++ b/packages/pigeon/pubspec.yaml @@ -2,7 +2,7 @@ name: pigeon description: Code generator tool to make communication between Flutter and the host platform type-safe and easier. repository: https://github.com/flutter/packages/tree/main/packages/pigeon issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+pigeon%22 -version: 20.0.0 # This must match the version in lib/generator_tools.dart +version: 20.0.1 # This must match the version in lib/generator_tools.dart environment: sdk: ^3.2.0 diff --git a/packages/pigeon/test/cpp_generator_test.dart b/packages/pigeon/test/cpp_generator_test.dart index ecdb0ab33d28..80ffc0702243 100644 --- a/packages/pigeon/test/cpp_generator_test.dart +++ b/packages/pigeon/test/cpp_generator_test.dart @@ -1194,7 +1194,7 @@ void main() { expect( code, contains( - 'const auto* an_object_arg = &(std::any_cast(std::get(encodable_an_object_arg)));')); + 'const auto* an_object_arg = encodable_an_object_arg.IsNull() ? nullptr : &(std::any_cast(std::get(encodable_an_object_arg)));')); // "Object" requires no extraction at all since it has to use // EncodableValue directly. expect(