From cd9c18b58331b1ba934c757fac6aa881fd38d348 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 8 Jun 2022 11:20:10 -0400 Subject: [PATCH 01/12] Initial rework of host APIs; unit tested only --- packages/pigeon/lib/cpp_generator.dart | 146 ++++--- packages/pigeon/lib/generator_tools.dart | 63 ++- packages/pigeon/lib/java_generator.dart | 25 +- packages/pigeon/lib/objc_generator.dart | 21 +- packages/pigeon/test/cpp_generator_test.dart | 397 +++++++++++++++++++ 5 files changed, 566 insertions(+), 86 deletions(-) diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index 8cf965c00bb8..167fc98e5082 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -57,8 +57,8 @@ class CppOptions { String _getCodecName(Api api) => '${api.name}CodecSerializer'; -String _pointerPrefix = 'pointer'; -String _encodablePrefix = 'encodable'; +const String _pointerPrefix = 'pointer'; +const String _encodablePrefix = 'encodable'; void _writeCodecHeader(Indent indent, Api api, Root root) { final String codecName = _getCodecName(api); @@ -134,7 +134,11 @@ void _writeCodecSource(Indent indent, Api api, Root root) { } } -void _writeErrorOr(Indent indent) { +void _writeErrorOr(Indent indent, + {Iterable friends = const []}) { + final String friendLines = friends + .map((String className) => '\tfriend class $className;') + .join('\n'); indent.format(''' class FlutterError { public: @@ -149,24 +153,31 @@ class FlutterError { \tstd::string message; \tflutter::EncodableValue details; }; + template class ErrorOr { -\tstd::variant, T, FlutterError> v; +\tstd::variant v; public: \tErrorOr(const T& rhs) { new(&v) T(rhs); } +\tErrorOr(const T&& rhs) { v = std::move(rhs) } \tErrorOr(const FlutterError& rhs) { \t\tnew(&v) FlutterError(rhs); \t} -\tstatic ErrorOr> MakeWithUniquePtr(std::unique_ptr rhs) { -\t\tErrorOr> ret = ErrorOr>(); -\t\tret.v = std::move(rhs); -\t\treturn ret; -\t} +\tErrorOr(const FlutterError&& rhs) { v = std::move(rhs) } + \tbool hasError() const { return std::holds_alternative(v); } \tconst T& value() const { return std::get(v); }; \tconst FlutterError& error() const { return std::get(v); }; +\tT TakeValue() && { return std::move(std::get(v)); }; + +\t// This object can be quite large, so require move instead of copy. +\tErrorOr(const ErrorOr&) = delete; +\tErrorOr& operator=(const ErrorOr&) = delete; + private: +$friendLines; + \tErrorOr() = default; -\tfriend class ErrorOr; +\tT TakeValue() && { return std::get(std::move(v)); } }; '''); } @@ -185,11 +196,11 @@ void _writeDataClassDeclaration(Indent indent, Class klass, Root root, indent.scoped(' public:', '', () { indent.writeln('${klass.name}();'); for (final NamedType field in klass.fields) { - final HostDatatype baseDatatype = getHostDatatype( + final HostDatatype baseDatatype = getFieldHostDatatype( field, root.classes, root.enums, - (NamedType x) => _baseCppTypeForBuiltinDartType(x.type)); + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); indent.writeln( '${_getterReturnType(baseDatatype)} ${_makeGetterName(field)}() const;'); indent.writeln( @@ -227,11 +238,11 @@ void _writeDataClassDeclaration(Indent indent, Class klass, Root root, } for (final NamedType field in klass.fields) { - final HostDatatype hostDatatype = getHostDatatype( + final HostDatatype hostDatatype = getFieldHostDatatype( field, root.classes, root.enums, - (NamedType x) => _baseCppTypeForBuiltinDartType(x.type)); + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); indent.writeln( '${_valueType(hostDatatype)} ${_makeInstanceVariableName(field)};'); } @@ -256,8 +267,8 @@ void _writeDataClassImplementation(Indent indent, Class klass, Root root) { // Getters and setters. for (final NamedType field in klass.fields) { - final HostDatatype hostDatatype = getHostDatatype(field, root.classes, - root.enums, (NamedType x) => _baseCppTypeForBuiltinDartType(x.type)); + final HostDatatype hostDatatype = getFieldHostDatatype(field, root.classes, + root.enums, (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); final String instanceVariableName = _makeInstanceVariableName(field); final String qualifiedGetterName = '${klass.name}::${_makeGetterName(field)}'; @@ -296,11 +307,11 @@ void _writeDataClassImplementation(Indent indent, Class klass, Root root) { indent.scoped('{', '}', () { indent.scoped('return flutter::EncodableMap{', '};', () { for (final NamedType field in klass.fields) { - final HostDatatype hostDatatype = getHostDatatype( + final HostDatatype hostDatatype = getFieldHostDatatype( field, root.classes, root.enums, - (NamedType x) => _baseCppTypeForBuiltinDartType(x.type)); + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); final String instanceVariable = _makeInstanceVariableName(field); @@ -346,16 +357,16 @@ void _writeDataClassImplementation(Indent indent, Class klass, Root root) { final String encodableFieldName = '${_encodablePrefix}_${_makeVariableName(field)}'; indent.writeln( - 'auto $encodableFieldName = map.at(flutter::EncodableValue("${field.name}"));'); + 'auto& $encodableFieldName = map.at(flutter::EncodableValue("${field.name}"));'); if (rootEnumNameSet.contains(field.type.baseName)) { indent.writeln( 'if (const int32_t* $pointerFieldName = std::get_if(&$encodableFieldName))\t$instanceVariableName = (${field.type.baseName})*$pointerFieldName;'); } else { - final HostDatatype hostDatatype = getHostDatatype( + final HostDatatype hostDatatype = getFieldHostDatatype( field, root.classes, root.enums, - (NamedType x) => _baseCppTypeForBuiltinDartType(x.type)); + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); if (field.type.baseName == 'int') { indent.format(''' if (const int32_t* $pointerFieldName = std::get_if(&$encodableFieldName)) @@ -385,7 +396,7 @@ else if (const int64_t* ${pointerFieldName}_64 = std::get_if(&$encodabl indent.addln(''); } -void _writeHostApiHeader(Indent indent, Api api) { +void _writeHostApiHeader(Indent indent, Api api, Root root) { assert(api.location == ApiLocation.host); indent.writeln( @@ -397,14 +408,24 @@ void _writeHostApiHeader(Indent indent, Api api) { indent.writeln('${api.name}& operator=(const ${api.name}&) = delete;'); indent.writeln('virtual ~${api.name}() { };'); for (final Method method in api.methods) { - final String returnTypeName = method.returnType.isVoid - ? 'std::optional' - : 'ErrorOr<${_nullSafeCppTypeForDartType(method.returnType, considerReference: false)}>'; + final HostDatatype returnType = getHostDatatype( + method.returnType, + root.classes, + root.enums, + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); + final String returnTypeName = _apiReturnType(returnType); final List argSignature = []; if (method.arguments.isNotEmpty) { - final Iterable argTypes = method.arguments - .map((NamedType e) => _nullSafeCppTypeForDartType(e.type)); + final Iterable argTypes = + method.arguments.map((NamedType arg) { + final HostDatatype hostType = getFieldHostDatatype( + arg, + root.classes, + root.enums, + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); + return _unownedArgumentType(hostType); + }); final Iterable argNames = method.arguments.map((NamedType e) => _makeVariableName(e)); argSignature.addAll( @@ -439,7 +460,7 @@ void _writeHostApiHeader(Indent indent, Api api) { }, nestCount: 0); } -void _writeHostApiSource(Indent indent, Api api) { +void _writeHostApiSource(Indent indent, Api api, Root root) { assert(api.location == ApiLocation.host); final String codecName = _getCodecName(api); @@ -470,9 +491,6 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { indent.write( 'channel->SetMessageHandler([api](const flutter::EncodableValue& message, const flutter::MessageReply& reply) '); indent.scoped('{', '});', () { - final String returnTypeName = method.returnType.isVoid - ? 'std::optional' - : 'ErrorOr<${_nullSafeCppTypeForDartType(method.returnType, considerReference: false)}>'; indent.writeln('flutter::EncodableMap wrapped;'); indent.write('try '); indent.scoped('{', '}', () { @@ -481,17 +499,22 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { indent.writeln( 'auto args = std::get(message);'); enumerate(method.arguments, (int index, NamedType arg) { - final String argType = _nullSafeCppTypeForDartType(arg.type); + final HostDatatype argumentType = getHostDatatype( + arg.type, + root.classes, + root.enums, + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); + final String argType = _unownedArgumentType(argumentType); final String argName = _getSafeArgumentName(index, arg); final String encodableArgName = '${_encodablePrefix}_$argName'; - indent.writeln('auto $encodableArgName = args.at($index);'); + indent.writeln('auto& $encodableArgName = args.at($index);'); if (!arg.type.isNullable) { indent.write('if ($encodableArgName.IsNull()) '); indent.scoped('{', '}', () { indent.writeln( - 'wrapped.insert(std::make_pair(flutter::EncodableValue("${Keys.error}"), WrapError("$argName unexpectedly null.")));'); + 'wrapped.emplace(flutter::EncodableValue("${Keys.error}"), WrapError("$argName unexpectedly null."));'); indent.writeln('reply(wrapped);'); indent.writeln('return;'); }); @@ -503,34 +526,39 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { } String _wrapResponse(String reply, TypeDeclaration returnType) { - final bool isReferenceReturnType = _isReferenceType( - _baseCppTypeForDartType(method.returnType)); String elseBody = ''; final String ifCondition; final String errorGetter; final String prefix = (reply != '') ? '\t' : ''; + + const String resultKey = + 'flutter::EncodableValue("${Keys.result}")'; + const String errorKey = + 'flutter::EncodableValue("${Keys.error}")'; + const String nullValue = 'flutter::EncodableValue()'; if (returnType.isVoid) { elseBody = - '$prefix\twrapped.insert(std::make_pair(flutter::EncodableValue("${Keys.result}"), flutter::EncodableValue()));${indent.newline}'; + '$prefix\twrapped.emplace($resultKey, $nullValue);${indent.newline}'; ifCondition = 'output.has_value()'; errorGetter = 'value'; } else { - if (isReferenceReturnType && !returnType.isNullable) { + if (returnType.isNullable) { elseBody = ''' -$prefix\tif (!output.value()) { -$prefix\t\twrapped.insert(std::make_pair(flutter::EncodableValue("${Keys.error}"), WrapError("output is unexpectedly null."))); +$prefix\tauto output_optional = std::move(output).TakeValue(); +$prefix\tif (output_optional) { +$prefix\t\twrapped.emplace($resultKey, std::move(output_optional).value()); $prefix\t} else { -$prefix\t\twrapped.insert(std::make_pair(flutter::EncodableValue("${Keys.result}"), flutter::CustomEncodableValue(*output.value().get()))); +$prefix\t\twrapped.emplace($resultKey, $nullValue); $prefix\t}${indent.newline}'''; } else { elseBody = - '$prefix\twrapped.insert(std::make_pair(flutter::EncodableValue("${Keys.result}"), flutter::CustomEncodableValue(output.value())));${indent.newline}'; + '$prefix\twrapped.emplace($resultKey, flutter::CustomEncodableValue(std::move(output).TakeValue()));${indent.newline}'; } ifCondition = 'output.hasError()'; errorGetter = 'error'; } return '${prefix}if ($ifCondition) {${indent.newline}' - '$prefix\twrapped.insert(std::make_pair(flutter::EncodableValue("${Keys.error}"), WrapError(output.$errorGetter())));${indent.newline}' + '$prefix\twrapped.emplace($errorKey, WrapError(output.$errorGetter()));${indent.newline}' '$prefix$reply' '$prefix} else {${indent.newline}' '$elseBody' @@ -538,6 +566,12 @@ $prefix\t}${indent.newline}'''; '$prefix}'; } + final HostDatatype returnType = getHostDatatype( + method.returnType, + root.classes, + root.enums, + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); + final String returnTypeName = _apiReturnType(returnType); if (method.isAsynchronous) { methodArgument.add( '[&wrapped, &reply]($returnTypeName output) {${indent.newline}' @@ -557,7 +591,7 @@ $prefix\t}${indent.newline}'''; indent.write('catch (const std::exception& exception) '); indent.scoped('{', '}', () { indent.writeln( - 'wrapped.insert(std::make_pair(flutter::EncodableValue("${Keys.error}"), WrapError(exception.what())));'); + 'wrapped.emplace(flutter::EncodableValue("${Keys.error}"), WrapError(exception.what()));'); if (method.isAsynchronous) { indent.writeln('reply(wrapped);'); } @@ -685,7 +719,7 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { } else { indent.writeln('$returnTypeName $output{};'); } - final String pointerVariable = '${_pointerPrefix}_$output'; + const String pointerVariable = '${_pointerPrefix}_$output'; if (func.returnType.baseName == 'int') { indent.format(''' if (const int32_t* $pointerVariable = std::get_if(&args)) @@ -767,6 +801,7 @@ bool _isPodType(HostDatatype type) { String? _baseCppTypeForBuiltinDartType(TypeDeclaration type) { const Map cppTypeForDartTypeMap = { + 'void': 'void', 'bool': 'bool', 'int': 'int64_t', 'String': 'std::string', @@ -822,6 +857,19 @@ String _getterReturnType(HostDatatype type) { return type.isNullable ? 'const $baseType*' : 'const $baseType&'; } +/// Returns the C++ type to use for the return of an API method retutrning +/// [type]. +String _apiReturnType(HostDatatype type) { + if (type.datatype == 'void') { + return 'std::optional'; + } + String valueType = type.datatype; + if (type.isNullable) { + valueType = 'std::optional<$valueType>'; + } + return 'ErrorOr<$valueType>'; +} + // TODO(stuartmorgan): Audit all uses of this and convert them to context-based // methods like those above. Code still using this method may well have bugs. String _nullSafeCppTypeForDartType(TypeDeclaration type, @@ -919,7 +967,7 @@ void generateCppHeader( indent.addln(''); - _writeErrorOr(indent); + _writeErrorOr(indent, friends: root.apis.map((Api api) => api.name)); for (final Class klass in root.classes) { _writeDataClassDeclaration(indent, klass, root, @@ -932,7 +980,7 @@ void generateCppHeader( _writeCodecHeader(indent, api, root); indent.addln(''); if (api.location == ApiLocation.host) { - _writeHostApiHeader(indent, api); + _writeHostApiHeader(indent, api, root); } else if (api.location == ApiLocation.flutter) { _writeFlutterApiHeader(indent, api); } @@ -986,13 +1034,13 @@ void generateCppSource(CppOptions options, Root root, StringSink sink) { _writeCodecSource(indent, api, root); indent.addln(''); if (api.location == ApiLocation.host) { - _writeHostApiSource(indent, api); + _writeHostApiSource(indent, api, root); indent.addln(''); indent.format(''' flutter::EncodableMap ${api.name}::WrapError(std::string_view error_message) { \treturn flutter::EncodableMap({ -\t\t{flutter::EncodableValue("${Keys.errorMessage}"), flutter::EncodableValue(std::string(error_message).data())}, +\t\t{flutter::EncodableValue("${Keys.errorMessage}"), flutter::EncodableValue(std::string(error_message))}, \t\t{flutter::EncodableValue("${Keys.errorCode}"), flutter::EncodableValue("Error")}, \t\t{flutter::EncodableValue("${Keys.errorDetails}"), flutter::EncodableValue()} \t}); diff --git a/packages/pigeon/lib/generator_tools.dart b/packages/pigeon/lib/generator_tools.dart index dd6295b8060b..ca221dc9bcfb 100644 --- a/packages/pigeon/lib/generator_tools.dart +++ b/packages/pigeon/lib/generator_tools.dart @@ -172,39 +172,58 @@ class HostDatatype { final bool isNullable; } -/// Calculates the [HostDatatype] for the provided [NamedType]. It will check -/// the field against [classes], the list of custom classes, to check if it is a -/// builtin type. [builtinResolver] will return the host datatype for the Dart -/// datatype for builtin types. [customResolver] can modify the datatype of -/// custom types. -HostDatatype getHostDatatype(NamedType field, List classes, - List enums, String? Function(NamedType) builtinResolver, +/// Calculates the [HostDatatype] for the provided [NamedType]. +/// +/// It will check the field against [classes], the list of custom classes, to +/// check if it is a builtin type. [builtinResolver] will return the host +/// datatype for the Dart datatype for builtin types. +/// +/// [customResolver] can modify the datatype of custom types. +HostDatatype getFieldHostDatatype(NamedType field, List classes, + List enums, String? Function(TypeDeclaration) builtinResolver, {String Function(String)? customResolver}) { - final String? datatype = builtinResolver(field); + return _getHostDatatype(field.type, classes, enums, builtinResolver, + customResolver: customResolver, fieldName: field.name); +} + +/// Calculates the [HostDatatype] for the provided [TypeDeclaration]. +/// +/// It will check the field against [classes], the list of custom classes, to +/// check if it is a builtin type. [builtinResolver] will return the host +/// datatype for the Dart datatype for builtin types. +/// +/// [customResolver] can modify the datatype of custom types. +HostDatatype getHostDatatype(TypeDeclaration type, List classes, + List enums, String? Function(TypeDeclaration) builtinResolver, + {String Function(String)? customResolver}) { + return _getHostDatatype(type, classes, enums, builtinResolver, + customResolver: customResolver); +} + +HostDatatype _getHostDatatype(TypeDeclaration type, List classes, + List enums, String? Function(TypeDeclaration) builtinResolver, + {String Function(String)? customResolver, String? fieldName}) { + final String? datatype = builtinResolver(type); if (datatype == null) { - if (classes.map((Class x) => x.name).contains(field.type.baseName)) { + if (classes.map((Class x) => x.name).contains(type.baseName)) { final String customName = customResolver != null - ? customResolver(field.type.baseName) - : field.type.baseName; + ? customResolver(type.baseName) + : type.baseName; return HostDatatype( - datatype: customName, - isBuiltin: false, - isNullable: field.type.isNullable); - } else if (enums.map((Enum x) => x.name).contains(field.type.baseName)) { + datatype: customName, isBuiltin: false, isNullable: type.isNullable); + } else if (enums.map((Enum x) => x.name).contains(type.baseName)) { final String customName = customResolver != null - ? customResolver(field.type.baseName) - : field.type.baseName; + ? customResolver(type.baseName) + : type.baseName; return HostDatatype( - datatype: customName, - isBuiltin: false, - isNullable: field.type.isNullable); + datatype: customName, isBuiltin: false, isNullable: type.isNullable); } else { throw Exception( - 'unrecognized datatype for field:"${field.name}" of type:"${field.type.baseName}"'); + 'unrecognized datatype ${fieldName == null ? '' : 'for field:"$fieldName" '}of type:"${type.baseName}"'); } } else { return HostDatatype( - datatype: datatype, isBuiltin: true, isNullable: field.type.isNullable); + datatype: datatype, isBuiltin: true, isNullable: type.isNullable); } } diff --git a/packages/pigeon/lib/java_generator.dart b/packages/pigeon/lib/java_generator.dart index 7e88c6497081..332fa7894094 100644 --- a/packages/pigeon/lib/java_generator.dart +++ b/packages/pigeon/lib/java_generator.dart @@ -442,8 +442,8 @@ String _nullsafeJavaTypeForDartType(TypeDeclaration type) { /// object. String _castObject( NamedType field, List classes, List enums, String varName) { - final HostDatatype hostDatatype = getHostDatatype(field, classes, enums, - (NamedType x) => _javaTypeForBuiltinDartType(x.type)); + final HostDatatype hostDatatype = getFieldHostDatatype(field, classes, enums, + (TypeDeclaration x) => _javaTypeForBuiltinDartType(x)); if (field.type.baseName == 'int') { return '($varName == null) ? null : (($varName instanceof Integer) ? (Integer)$varName : (${hostDatatype.datatype})$varName)'; } else if (!hostDatatype.isBuiltin && @@ -512,8 +512,11 @@ void generateJava(JavaOptions options, Root root, StringSink sink) { void writeDataClass(Class klass) { void writeField(NamedType field) { - final HostDatatype hostDatatype = getHostDatatype(field, root.classes, - root.enums, (NamedType x) => _javaTypeForBuiltinDartType(x.type)); + final HostDatatype hostDatatype = getFieldHostDatatype( + field, + root.classes, + root.enums, + (TypeDeclaration x) => _javaTypeForBuiltinDartType(x)); final String nullability = field.type.isNullable ? '@Nullable' : '@NonNull'; indent.writeln( @@ -538,8 +541,11 @@ void generateJava(JavaOptions options, Root root, StringSink sink) { indent.scoped('{', '}', () { indent.writeln('Map toMapResult = new HashMap<>();'); for (final NamedType field in klass.fields) { - final HostDatatype hostDatatype = getHostDatatype(field, root.classes, - root.enums, (NamedType x) => _javaTypeForBuiltinDartType(x.type)); + final HostDatatype hostDatatype = getFieldHostDatatype( + field, + root.classes, + root.enums, + (TypeDeclaration x) => _javaTypeForBuiltinDartType(x)); String toWriteValue = ''; final String fieldName = field.name; if (!hostDatatype.isBuiltin && @@ -583,8 +589,11 @@ void generateJava(JavaOptions options, Root root, StringSink sink) { indent.write('public static final class Builder '); indent.scoped('{', '}', () { for (final NamedType field in klass.fields) { - final HostDatatype hostDatatype = getHostDatatype(field, root.classes, - root.enums, (NamedType x) => _javaTypeForBuiltinDartType(x.type)); + final HostDatatype hostDatatype = getFieldHostDatatype( + field, + root.classes, + root.enums, + (TypeDeclaration x) => _javaTypeForBuiltinDartType(x)); final String nullability = field.type.isNullable ? '@Nullable' : '@NonNull'; indent.writeln( diff --git a/packages/pigeon/lib/objc_generator.dart b/packages/pigeon/lib/objc_generator.dart index dc1661b26ec9..84dc5cd8f73f 100644 --- a/packages/pigeon/lib/objc_generator.dart +++ b/packages/pigeon/lib/objc_generator.dart @@ -107,9 +107,10 @@ String _flattenTypeArguments(String? classPrefix, List args) { return result; } -String? _objcTypePtrForPrimitiveDartType(String? classPrefix, NamedType field) { - return _objcTypeForDartTypeMap.containsKey(field.type.baseName) - ? _objcTypeForDartType(classPrefix, field.type).ptr +String? _objcTypePtrForPrimitiveDartType( + String? classPrefix, TypeDeclaration type) { + return _objcTypeForDartTypeMap.containsKey(type.baseName) + ? _objcTypeForDartType(classPrefix, type).ptr : null; } @@ -170,8 +171,11 @@ void _writeInitializerDeclaration(Indent indent, Class klass, indent.write(x); }; isFirst = false; - final HostDatatype hostDatatype = getHostDatatype(field, classes, enums, - (NamedType x) => _objcTypePtrForPrimitiveDartType(prefix, x), + final HostDatatype hostDatatype = getFieldHostDatatype( + field, + classes, + enums, + (TypeDeclaration x) => _objcTypePtrForPrimitiveDartType(prefix, x), customResolver: enumNames.contains(field.type.baseName) ? (String x) => _className(prefix, x) : (String x) => '${_className(prefix, x)} *'); @@ -205,8 +209,11 @@ void _writeClassDeclarations( indent.addln(';'); } for (final NamedType field in klass.fields) { - final HostDatatype hostDatatype = getHostDatatype(field, classes, enums, - (NamedType x) => _objcTypePtrForPrimitiveDartType(prefix, x), + final HostDatatype hostDatatype = getFieldHostDatatype( + field, + classes, + enums, + (TypeDeclaration x) => _objcTypePtrForPrimitiveDartType(prefix, x), customResolver: enumNames.contains(field.type.baseName) ? (String x) => _className(prefix, x) : (String x) => '${_className(prefix, x)} *'); diff --git a/packages/pigeon/test/cpp_generator_test.dart b/packages/pigeon/test/cpp_generator_test.dart index 0b2c93c39dd7..3baf1228a05f 100644 --- a/packages/pigeon/test/cpp_generator_test.dart +++ b/packages/pigeon/test/cpp_generator_test.dart @@ -525,6 +525,403 @@ void main() { } }); + test('host nullable return types map correctly', () { + final Root root = Root(apis: [ + Api(name: 'Api', location: ApiLocation.host, methods: [ + Method( + name: 'returnNullableBool', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'bool', + isNullable: true, + ), + isAsynchronous: false, + ), + Method( + name: 'returnNullableInt', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'int', + isNullable: true, + ), + isAsynchronous: false, + ), + Method( + name: 'returnNullableString', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'String', + isNullable: true, + ), + isAsynchronous: false, + ), + Method( + name: 'returnNullableList', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'List', + typeArguments: [ + TypeDeclaration( + baseName: 'String', + isNullable: true, + ) + ], + isNullable: true, + ), + isAsynchronous: false, + ), + Method( + name: 'returnNullableMap', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'Map', + typeArguments: [ + TypeDeclaration( + baseName: 'String', + isNullable: true, + ), + TypeDeclaration( + baseName: 'String', + isNullable: true, + ) + ], + isNullable: true, + ), + isAsynchronous: false, + ), + Method( + name: 'returnNullableDataClass', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'ReturnData', + isNullable: true, + ), + isAsynchronous: false, + ), + ]) + ], classes: [ + Class(name: 'ReturnData', fields: [ + NamedType( + type: const TypeDeclaration( + baseName: 'bool', + isNullable: false, + ), + name: 'aValue', + offset: null), + ]), + ], enums: []); + { + final StringBuffer sink = StringBuffer(); + generateCppHeader('', const CppOptions(), root, sink); + final String code = sink.toString(); + expect( + code, contains('ErrorOr> ReturnNullableBool()')); + expect(code, + contains('ErrorOr> ReturnNullableInt()')); + expect( + code, + contains( + 'ErrorOr> ReturnNullableString()')); + expect( + code, + contains( + 'ErrorOr> ReturnNullableList()')); + expect( + code, + contains( + 'ErrorOr> ReturnNullableMap()')); + expect( + code, + contains( + 'ErrorOr> ReturnNullableDataClass()')); + } + }); + + test('host non-nullable return types map correctly', () { + final Root root = Root(apis: [ + Api(name: 'Api', location: ApiLocation.host, methods: [ + Method( + name: 'returnBool', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'bool', + isNullable: false, + ), + isAsynchronous: false, + ), + Method( + name: 'returnInt', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'int', + isNullable: false, + ), + isAsynchronous: false, + ), + Method( + name: 'returnString', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'String', + isNullable: false, + ), + isAsynchronous: false, + ), + Method( + name: 'returnList', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'List', + typeArguments: [ + TypeDeclaration( + baseName: 'String', + isNullable: true, + ) + ], + isNullable: false, + ), + isAsynchronous: false, + ), + Method( + name: 'returnMap', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'Map', + typeArguments: [ + TypeDeclaration( + baseName: 'String', + isNullable: true, + ), + TypeDeclaration( + baseName: 'String', + isNullable: true, + ) + ], + isNullable: false, + ), + isAsynchronous: false, + ), + Method( + name: 'returnDataClass', + arguments: [], + returnType: const TypeDeclaration( + baseName: 'ReturnData', + isNullable: false, + ), + isAsynchronous: false, + ), + ]) + ], classes: [ + Class(name: 'ReturnData', fields: [ + NamedType( + type: const TypeDeclaration( + baseName: 'bool', + isNullable: false, + ), + name: 'aValue', + offset: null), + ]), + ], enums: []); + { + final StringBuffer sink = StringBuffer(); + generateCppHeader('', const CppOptions(), root, sink); + final String code = sink.toString(); + expect(code, contains('ErrorOr ReturnBool()')); + expect(code, contains('ErrorOr ReturnInt()')); + expect(code, contains('ErrorOr ReturnString()')); + expect(code, contains('ErrorOr ReturnList()')); + expect(code, contains('ErrorOr ReturnMap()')); + expect(code, contains('ErrorOr ReturnDataClass()')); + } + }); + + test('host nullable arguments map correctly', () { + final Root root = Root(apis: [ + Api(name: 'Api', location: ApiLocation.host, methods: [ + Method( + name: 'doSomething', + arguments: [ + NamedType( + name: 'boolArg', + type: const TypeDeclaration( + baseName: 'bool', + isNullable: true, + )), + NamedType( + name: 'intArg', + type: const TypeDeclaration( + baseName: 'int', + isNullable: true, + )), + NamedType( + name: 'stringArg', + type: const TypeDeclaration( + baseName: 'String', + isNullable: true, + )), + NamedType( + name: 'listArg', + type: const TypeDeclaration( + baseName: 'List', + typeArguments: [ + TypeDeclaration(baseName: 'Object', isNullable: true) + ], + isNullable: true, + )), + NamedType( + name: 'mapArg', + type: const TypeDeclaration( + baseName: 'Map', + typeArguments: [ + TypeDeclaration(baseName: 'String', isNullable: true), + TypeDeclaration(baseName: 'Object', isNullable: true), + ], + isNullable: true, + )), + NamedType( + name: 'objectArg', + type: const TypeDeclaration( + baseName: 'ParameterObject', + isNullable: true, + )), + ], + returnType: const TypeDeclaration.voidDeclaration(), + isAsynchronous: false, + ), + ]) + ], classes: [ + Class(name: 'ParameterObject', fields: [ + NamedType( + type: const TypeDeclaration( + baseName: 'bool', + isNullable: false, + ), + name: 'aValue', + offset: null), + ]), + ], enums: []); + { + final StringBuffer sink = StringBuffer(); + generateCppHeader('', const CppOptions(), root, sink); + final String code = sink.toString(); + expect( + code, + contains('DoSomething(const bool* bool_arg, ' + 'const int64_t* int_arg, ' + 'const std::string_view* string_arg, ' + 'const flutter::EncodableList* list_arg, ' + 'const flutter::EncodableMap* map_arg, ' + 'const ParameterObject* object_arg)')); + } + }); + + test('host non-nullable arguments map correctly', () { + final Root root = Root(apis: [ + Api(name: 'Api', location: ApiLocation.host, methods: [ + Method( + name: 'doSomething', + arguments: [ + NamedType( + name: 'boolArg', + type: const TypeDeclaration( + baseName: 'bool', + isNullable: false, + )), + NamedType( + name: 'intArg', + type: const TypeDeclaration( + baseName: 'int', + isNullable: false, + )), + NamedType( + name: 'stringArg', + type: const TypeDeclaration( + baseName: 'String', + isNullable: false, + )), + NamedType( + name: 'listArg', + type: const TypeDeclaration( + baseName: 'List', + typeArguments: [ + TypeDeclaration(baseName: 'Object', isNullable: true) + ], + isNullable: false, + )), + NamedType( + name: 'mapArg', + type: const TypeDeclaration( + baseName: 'Map', + typeArguments: [ + TypeDeclaration(baseName: 'String', isNullable: true), + TypeDeclaration(baseName: 'Object', isNullable: true), + ], + isNullable: false, + )), + NamedType( + name: 'objectArg', + type: const TypeDeclaration( + baseName: 'ParameterObject', + isNullable: false, + )), + ], + returnType: const TypeDeclaration.voidDeclaration(), + isAsynchronous: false, + ), + ]) + ], classes: [ + Class(name: 'ParameterObject', fields: [ + NamedType( + type: const TypeDeclaration( + baseName: 'bool', + isNullable: false, + ), + name: 'aValue', + offset: null), + ]), + ], enums: []); + { + final StringBuffer sink = StringBuffer(); + generateCppHeader('', const CppOptions(), root, sink); + final String code = sink.toString(); + expect( + code, + contains('DoSomething(bool bool_arg, ' + 'int64_t int_arg, ' + 'std::string_view string_arg, ' + 'const flutter::EncodableList& list_arg, ' + 'const flutter::EncodableMap& map_arg, ' + 'const ParameterObject& object_arg)')); + } + }); + + test('host API argument extraction uses references', () { + final Root root = Root(apis: [ + Api(name: 'Api', location: ApiLocation.host, methods: [ + Method( + name: 'doSomething', + arguments: [ + NamedType( + name: 'anArg', + type: const TypeDeclaration( + baseName: 'int', + isNullable: false, + )), + ], + returnType: const TypeDeclaration.voidDeclaration(), + isAsynchronous: false, + ), + ]) + ], classes: [], enums: []); + + final StringBuffer sink = StringBuffer(); + generateCppSource(const CppOptions(), root, sink); + final String code = sink.toString(); + // A bare 'auto' here would create a copy, not a reference, which is + // ineffecient and triggers a warning in Visual Studio. + expect(code, contains('auto& encodable_an_arg_arg = args.at(0);')); + }); + test('enum argument', () { final Root root = Root( apis: [ From 3fc4f922a58d1defc9f63df5aa6cfd1273b0a46a Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 15 Jun 2022 15:24:53 -0400 Subject: [PATCH 02/12] ErrorOr fixes --- packages/pigeon/lib/cpp_generator.dart | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index 167fc98e5082..4bb5ee802193 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -158,24 +158,22 @@ template class ErrorOr { \tstd::variant v; public: \tErrorOr(const T& rhs) { new(&v) T(rhs); } -\tErrorOr(const T&& rhs) { v = std::move(rhs) } +\tErrorOr(const T&& rhs) { v = std::move(rhs); } \tErrorOr(const FlutterError& rhs) { \t\tnew(&v) FlutterError(rhs); \t} -\tErrorOr(const FlutterError&& rhs) { v = std::move(rhs) } +\tErrorOr(const FlutterError&& rhs) { v = std::move(rhs); } \tbool hasError() const { return std::holds_alternative(v); } \tconst T& value() const { return std::get(v); }; \tconst FlutterError& error() const { return std::get(v); }; -\tT TakeValue() && { return std::move(std::get(v)); }; \t// This object can be quite large, so require move instead of copy. \tErrorOr(const ErrorOr&) = delete; \tErrorOr& operator=(const ErrorOr&) = delete; private: -$friendLines; - +$friendLines \tErrorOr() = default; \tT TakeValue() && { return std::get(std::move(v)); } }; From 6e50f356ebe2ccd6c68070b760b38c91c98ef81c Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 15 Jun 2022 15:59:18 -0400 Subject: [PATCH 03/12] const refs --- packages/pigeon/lib/cpp_generator.dart | 5 +++-- packages/pigeon/test/cpp_generator_test.dart | 8 ++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index 4bb5ee802193..f2168f9861b7 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -495,7 +495,7 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { final List methodArgument = []; if (method.arguments.isNotEmpty) { indent.writeln( - 'auto args = std::get(message);'); + 'const auto& args = std::get(message);'); enumerate(method.arguments, (int index, NamedType arg) { final HostDatatype argumentType = getHostDatatype( arg.type, @@ -507,7 +507,8 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { final String encodableArgName = '${_encodablePrefix}_$argName'; - indent.writeln('auto& $encodableArgName = args.at($index);'); + indent.writeln( + 'const auto& $encodableArgName = args.at($index);'); if (!arg.type.isNullable) { indent.write('if ($encodableArgName.IsNull()) '); indent.scoped('{', '}', () { diff --git a/packages/pigeon/test/cpp_generator_test.dart b/packages/pigeon/test/cpp_generator_test.dart index 3baf1228a05f..9d31ceeb5204 100644 --- a/packages/pigeon/test/cpp_generator_test.dart +++ b/packages/pigeon/test/cpp_generator_test.dart @@ -918,8 +918,12 @@ void main() { generateCppSource(const CppOptions(), root, sink); final String code = sink.toString(); // A bare 'auto' here would create a copy, not a reference, which is - // ineffecient and triggers a warning in Visual Studio. - expect(code, contains('auto& encodable_an_arg_arg = args.at(0);')); + // ineffecient. + expect( + code, + contains( + 'const auto& args = std::get(message);')); + expect(code, contains('const auto& encodable_an_arg_arg = args.at(0);')); }); test('enum argument', () { From ddf9bde876cd31819cd706d7b7b596d5d8ae0b88 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 15 Jun 2022 16:06:40 -0400 Subject: [PATCH 04/12] std::string for host API args --- packages/pigeon/lib/cpp_generator.dart | 14 +++++++++++++- packages/pigeon/test/cpp_generator_test.dart | 4 ++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index f2168f9861b7..c35094b91f39 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -422,7 +422,7 @@ void _writeHostApiHeader(Indent indent, Api api, Root root) { root.classes, root.enums, (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); - return _unownedArgumentType(hostType); + return _hostApiArgumentType(hostType); }); final Iterable argNames = method.arguments.map((NamedType e) => _makeVariableName(e)); @@ -843,6 +843,18 @@ String _unownedArgumentType(HostDatatype type) { return type.isNullable ? 'const $baseType*' : 'const $baseType&'; } +/// Returns the C++ type to use for arguments to a host API. This is slightly +/// different from [_unownedArgumentType] since passing `std::string_view*` in +/// to the host API implementation when the actual type is `std::string*` is +/// needlessly complicated, so it uses `std::string` directly. +String _hostApiArgumentType(HostDatatype type) { + final String baseType = type.datatype; + if (_isPodType(type)) { + return type.isNullable ? 'const $baseType*' : baseType; + } + return type.isNullable ? 'const $baseType*' : 'const $baseType&'; +} + /// Returns the C++ type to use for the return of a getter for a field of type /// [type]. String _getterReturnType(HostDatatype type) { diff --git a/packages/pigeon/test/cpp_generator_test.dart b/packages/pigeon/test/cpp_generator_test.dart index 9d31ceeb5204..dc7d07fb6df4 100644 --- a/packages/pigeon/test/cpp_generator_test.dart +++ b/packages/pigeon/test/cpp_generator_test.dart @@ -808,7 +808,7 @@ void main() { code, contains('DoSomething(const bool* bool_arg, ' 'const int64_t* int_arg, ' - 'const std::string_view* string_arg, ' + 'const std::string* string_arg, ' 'const flutter::EncodableList* list_arg, ' 'const flutter::EncodableMap* map_arg, ' 'const ParameterObject* object_arg)')); @@ -888,7 +888,7 @@ void main() { code, contains('DoSomething(bool bool_arg, ' 'int64_t int_arg, ' - 'std::string_view string_arg, ' + 'const std::string& string_arg, ' 'const flutter::EncodableList& list_arg, ' 'const flutter::EncodableMap& map_arg, ' 'const ParameterObject& object_arg)')); From c6494dbe3a9b44cabb181e368dc3f205424b20b5 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 17 Jun 2022 13:39:10 -0400 Subject: [PATCH 05/12] Rewrite arg extraction; add unit tests for it --- packages/pigeon/lib/cpp_generator.dart | 58 ++++++++- packages/pigeon/test/cpp_generator_test.dart | 123 +++++++++++++++---- 2 files changed, 153 insertions(+), 28 deletions(-) diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index c35094b91f39..e34dfef100dd 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -496,13 +496,63 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { if (method.arguments.isNotEmpty) { indent.writeln( 'const auto& args = std::get(message);'); + + // Writes the code to declare and populate a variable called + // [argName] to use as a parameter to an API method call from + // an existing EncodablValue variable called [encodableArgName] + // which corresponds to [arg] in the API definition. + void extractEncodedArgument( + String argName, + String encodableArgName, + NamedType arg, + HostDatatype hostType) { + if (arg.type.isNullable) { + // Nullable arguments are always pointers, with nullptr + // corresponding to null. + if (hostType.datatype == 'int64_t') { + // The EncodableValue will either be an int32_t or an + // int64_t depending on the value, but the generated API + // requires an int64_t so that it can handle any case. + // Create a local variable for the 64-bit value... + final String valueVarName = '${argName}_value'; + indent.writeln( + 'const int64_t $valueVarName = $encodableArgName.IsNull() ? 0 : $encodableArgName.LongValue();'); + // ... then declare the arg as a reference to that local. + indent.writeln( + 'const auto* $argName = $encodableArgName.IsNull() ? nullptr : &$valueVarName;'); + } else if (hostType.isBuiltin) { + indent.writeln( + 'const auto* $argName = std::get_if<${hostType.datatype}>(&$encodableArgName);'); + } else { + indent.writeln( + 'const auto* $argName = &(std::any_cast(std::get($encodableArgName)));'); + } + } else { + // Non-nullable arguments are either passed by value or + // reference, but the extraction doesn't need to distinguish + // since those are the same at the call site. + if (hostType.datatype == 'int64_t') { + // The EncodableValue will either be an int32_t or an + // int64_t depending on the value, but the generated API + // requires an int64_t so that it can handle any case. + indent.writeln( + 'const int64_t $argName = $encodableArgName.LongValue();'); + } else if (hostType.isBuiltin) { + indent.writeln( + 'const auto& $argName = std::get<${hostType.datatype}>($encodableArgName);'); + } else { + indent.writeln( + 'const auto& $argName = std::any_cast(std::get($encodableArgName));'); + } + } + } + enumerate(method.arguments, (int index, NamedType arg) { - final HostDatatype argumentType = getHostDatatype( + final HostDatatype hostType = getHostDatatype( arg.type, root.classes, root.enums, (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); - final String argType = _unownedArgumentType(argumentType); final String argName = _getSafeArgumentName(index, arg); final String encodableArgName = @@ -518,8 +568,8 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { indent.writeln('return;'); }); } - indent.writeln( - '$argType $argName = std::any_cast<$argType>(std::get($encodableArgName));'); + extractEncodedArgument( + argName, encodableArgName, arg, hostType); methodArgument.add(argName); }); } diff --git a/packages/pigeon/test/cpp_generator_test.dart b/packages/pigeon/test/cpp_generator_test.dart index dc7d07fb6df4..c7205035c68e 100644 --- a/packages/pigeon/test/cpp_generator_test.dart +++ b/packages/pigeon/test/cpp_generator_test.dart @@ -742,25 +742,25 @@ void main() { name: 'doSomething', arguments: [ NamedType( - name: 'boolArg', + name: 'aBool', type: const TypeDeclaration( baseName: 'bool', isNullable: true, )), NamedType( - name: 'intArg', + name: 'anInt', type: const TypeDeclaration( baseName: 'int', isNullable: true, )), NamedType( - name: 'stringArg', + name: 'aString', type: const TypeDeclaration( baseName: 'String', isNullable: true, )), NamedType( - name: 'listArg', + name: 'aList', type: const TypeDeclaration( baseName: 'List', typeArguments: [ @@ -769,7 +769,7 @@ void main() { isNullable: true, )), NamedType( - name: 'mapArg', + name: 'aMap', type: const TypeDeclaration( baseName: 'Map', typeArguments: [ @@ -779,7 +779,7 @@ void main() { isNullable: true, )), NamedType( - name: 'objectArg', + name: 'anObject', type: const TypeDeclaration( baseName: 'ParameterObject', isNullable: true, @@ -806,12 +806,52 @@ void main() { final String code = sink.toString(); expect( code, - contains('DoSomething(const bool* bool_arg, ' - 'const int64_t* int_arg, ' - 'const std::string* string_arg, ' - 'const flutter::EncodableList* list_arg, ' - 'const flutter::EncodableMap* map_arg, ' - 'const ParameterObject* object_arg)')); + contains('DoSomething(const bool* a_bool, ' + 'const int64_t* an_int, ' + 'const std::string* a_string, ' + 'const flutter::EncodableList* a_list, ' + 'const flutter::EncodableMap* a_map, ' + 'const ParameterObject* an_object)')); + } + { + final StringBuffer sink = StringBuffer(); + generateCppSource(const CppOptions(), root, sink); + final String code = sink.toString(); + // Most types should just use get_if, since the parameter is a pointer, + // and get_if will automatically handle null values (since a null + // EncodableValue will not match the queried type, so get_if will return + // nullptr). + expect( + code, + contains( + 'const auto* a_bool_arg = std::get_if(&encodable_a_bool_arg);')); + expect( + code, + contains( + 'const auto* a_string_arg = std::get_if(&encodable_a_string_arg);')); + expect( + code, + contains( + 'const auto* a_list_arg = std::get_if(&encodable_a_list_arg);')); + expect( + code, + contains( + 'const auto* a_map_arg = std::get_if(&encodable_a_map_arg);')); + // Ints are complicated since there are two possible pointer types, but + // the paramter always needs an int64_t*. + expect( + code, + contains( + 'const int64_t an_int_arg_value = encodable_an_int_arg.IsNull() ? 0 : encodable_an_int_arg.LongValue();')); + expect( + code, + contains( + 'const auto* an_int_arg = encodable_an_int_arg.IsNull() ? nullptr : &an_int_arg_value;')); + // Custom class types require an extra layer of extraction. + expect( + code, + contains( + 'const auto* an_object_arg = &(std::any_cast(std::get(encodable_an_object_arg)));')); } }); @@ -822,25 +862,25 @@ void main() { name: 'doSomething', arguments: [ NamedType( - name: 'boolArg', + name: 'aBool', type: const TypeDeclaration( baseName: 'bool', isNullable: false, )), NamedType( - name: 'intArg', + name: 'anInt', type: const TypeDeclaration( baseName: 'int', isNullable: false, )), NamedType( - name: 'stringArg', + name: 'aString', type: const TypeDeclaration( baseName: 'String', isNullable: false, )), NamedType( - name: 'listArg', + name: 'aList', type: const TypeDeclaration( baseName: 'List', typeArguments: [ @@ -849,7 +889,7 @@ void main() { isNullable: false, )), NamedType( - name: 'mapArg', + name: 'aMap', type: const TypeDeclaration( baseName: 'Map', typeArguments: [ @@ -859,7 +899,7 @@ void main() { isNullable: false, )), NamedType( - name: 'objectArg', + name: 'anObject', type: const TypeDeclaration( baseName: 'ParameterObject', isNullable: false, @@ -886,12 +926,47 @@ void main() { final String code = sink.toString(); expect( code, - contains('DoSomething(bool bool_arg, ' - 'int64_t int_arg, ' - 'const std::string& string_arg, ' - 'const flutter::EncodableList& list_arg, ' - 'const flutter::EncodableMap& map_arg, ' - 'const ParameterObject& object_arg)')); + contains('DoSomething(bool a_bool, ' + 'int64_t an_int, ' + 'const std::string& a_string, ' + 'const flutter::EncodableList& a_list, ' + 'const flutter::EncodableMap& a_map, ' + 'const ParameterObject& an_object)')); + } + { + final StringBuffer sink = StringBuffer(); + generateCppSource(const CppOptions(), root, sink); + final String code = sink.toString(); + // Most types should extract references. Since the type is non-nullable, + // there's only one possible type. + expect( + code, + contains( + 'const auto& a_bool_arg = std::get(encodable_a_bool_arg);')); + expect( + code, + contains( + 'const auto& a_string_arg = std::get(encodable_a_string_arg);')); + expect( + code, + contains( + 'const auto& a_list_arg = std::get(encodable_a_list_arg);')); + expect( + code, + contains( + 'const auto& a_map_arg = std::get(encodable_a_map_arg);')); + // Ints use a copy since there are two possible reference types, but + // the paramter always needs an int64_t. + expect( + code, + contains( + 'const int64_t an_int_arg = encodable_an_int_arg.LongValue();', + )); + // Custom class types require an extra layer of extraction. + expect( + code, + contains( + 'const auto& an_object_arg = std::any_cast(std::get(encodable_an_object_arg));')); } }); From 2c2ed632d0bc0ecefbc06197f32169e8acf03f44 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 17 Jun 2022 14:39:50 -0400 Subject: [PATCH 06/12] Fix return wrapping --- packages/pigeon/lib/cpp_generator.dart | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index e34dfef100dd..6225dd880122 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -591,17 +591,28 @@ const flutter::StandardMessageCodec& ${api.name}::GetCodec() { ifCondition = 'output.has_value()'; errorGetter = 'value'; } else { + final HostDatatype hostType = getHostDatatype( + returnType, + root.classes, + root.enums, + (TypeDeclaration x) => _baseCppTypeForBuiltinDartType(x)); + const String extractedValue = 'std::move(output).TakeValue()'; + final String wrapperType = hostType.isBuiltin + ? 'flutter::EncodableValue' + : 'flutter::CustomEncodableValue'; if (returnType.isNullable) { + // The value is a std::optional, so needs an extra layer of + // handling. elseBody = ''' -$prefix\tauto output_optional = std::move(output).TakeValue(); +$prefix\tauto output_optional = $extractedValue; $prefix\tif (output_optional) { -$prefix\t\twrapped.emplace($resultKey, std::move(output_optional).value()); +$prefix\t\twrapped.emplace($resultKey, $wrapperType(std::move(output_optional).value())); $prefix\t} else { $prefix\t\twrapped.emplace($resultKey, $nullValue); $prefix\t}${indent.newline}'''; } else { elseBody = - '$prefix\twrapped.emplace($resultKey, flutter::CustomEncodableValue(std::move(output).TakeValue()));${indent.newline}'; + '$prefix\twrapped.emplace($resultKey, $wrapperType($extractedValue));${indent.newline}'; } ifCondition = 'output.hasError()'; errorGetter = 'error'; From 58886df935ecca1ce205ae54862d2a952979179a Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 22 Jun 2022 11:54:10 -0400 Subject: [PATCH 07/12] Add test utilities and partial primitive.dart test, and get it building and passing --- packages/pigeon/lib/cpp_generator.dart | 6 +- .../windows_unit_tests/windows/CMakeLists.txt | 8 + .../windows/test/pigeon_test.cpp | 6 +- .../windows/test/primitive_test.cpp | 158 ++++++++++++++++++ .../windows/test/utils/echo_messenger.cpp | 32 ++++ .../windows/test/utils/echo_messenger.h | 35 ++++ .../test/utils/fake_host_messenger.cpp | 46 +++++ .../windows/test/utils/fake_host_messenger.h | 51 ++++++ 8 files changed, 333 insertions(+), 9 deletions(-) create mode 100644 packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp create mode 100644 packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.cpp create mode 100644 packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.h create mode 100644 packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.cpp create mode 100644 packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.h diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index 6225dd880122..6b138e02e5ae 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -168,10 +168,6 @@ template class ErrorOr { \tconst T& value() const { return std::get(v); }; \tconst FlutterError& error() const { return std::get(v); }; -\t// This object can be quite large, so require move instead of copy. -\tErrorOr(const ErrorOr&) = delete; -\tErrorOr& operator=(const ErrorOr&) = delete; - private: $friendLines \tErrorOr() = default; @@ -634,7 +630,7 @@ $prefix\t}${indent.newline}'''; final String returnTypeName = _apiReturnType(returnType); if (method.isAsynchronous) { methodArgument.add( - '[&wrapped, &reply]($returnTypeName output) {${indent.newline}' + '[&wrapped, &reply]($returnTypeName&& output) {${indent.newline}' '${_wrapResponse('\treply(wrapped);${indent.newline}', method.returnType)}' '}', ); diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt b/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt index 73e559a2b9d7..3e663d015c65 100644 --- a/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt @@ -52,9 +52,12 @@ FetchContent_MakeAvailable(googletest) # The plugin's C API is not very useful for unit testing, so build the sources # directly into the test binary rather than using the DLL. add_executable(${TEST_RUNNER} + # Tests. test/non_null_fields_test.cpp test/null_fields_test.cpp test/pigeon_test.cpp + test/primitive_test.cpp + # Generated sources. test/all_datatypes.g.cpp test/all_datatypes.g.h test/all_void.g.cpp @@ -87,6 +90,11 @@ add_executable(${TEST_RUNNER} test/voidflutter.g.h test/voidhost.g.cpp test/voidhost.g.h + # Test utilities. + test/utils/echo_messenger.cpp + test/utils/echo_messenger.h + test/utils/fake_host_messenger.cpp + test/utils/fake_host_messenger.h ${PLUGIN_SOURCES} ) diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp index eeabc3e721b8..797719c7e307 100644 --- a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp @@ -59,8 +59,7 @@ class MockApi : public Api { ~MockApi() = default; MOCK_METHOD(std::optional, Initialize, (), (override)); - MOCK_METHOD(ErrorOr>, Search, - (const SearchRequest&), (override)); + MOCK_METHOD(ErrorOr, Search, (const SearchRequest&), (override)); }; class Writer : public flutter::ByteStreamWriter { @@ -128,8 +127,7 @@ TEST(PigeonTests, CallSearch) { .Times(1) .WillOnce(testing::SaveArg<1>(&handler)); EXPECT_CALL(mock_api, Search(testing::_)) - .WillOnce(Return(ByMove(ErrorOr::MakeWithUniquePtr( - std::make_unique())))); + .WillOnce(Return(ByMove(ErrorOr(SearchReply())))); Api::SetUp(&mock_messenger, &mock_api); bool did_call_reply = false; flutter::BinaryReply reply = [&did_call_reply](const uint8_t* data, diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp new file mode 100644 index 000000000000..4aeabb336b80 --- /dev/null +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp @@ -0,0 +1,158 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include "test/primitive.g.h" +#include "test/utils/fake_host_messenger.h" + +namespace primitive_pigeontest { + +namespace { +using flutter::EncodableList; +using flutter::EncodableMap; +using flutter::EncodableValue; +using testing::FakeHostMessenger; + +class TestHostApi : public PrimitiveHostApi { + public: + TestHostApi() {} + virtual ~TestHostApi(){}; + + protected: + ErrorOr AnInt(int64_t value) override { return value; } + ErrorOr ABool(bool value) override { return value; } + ErrorOr AString(const std::string& value) override { + return std::string(value); + } + ErrorOr ADouble(double value) override { return value; } + ErrorOr AMap( + const flutter::EncodableMap& value) override { + return value; + } + ErrorOr AList( + const flutter::EncodableList& value) override { + return value; + } + ErrorOr> AnInt32List( + const std::vector& value) override { + return value; + } + ErrorOr ABoolList( + const flutter::EncodableList& value) override { + return value; + } + ErrorOr AStringIntMap( + const flutter::EncodableMap& value) override { + return value; + } +}; + +const EncodableValue& GetResult(const EncodableValue& pigeon_response) { + return std::get(pigeon_response).at(EncodableValue("result")); +} +} // namespace + +TEST(Primitive, HostInt) { + FakeHostMessenger messenger(&PrimitiveHostApi::GetCodec()); + TestHostApi api; + PrimitiveHostApi::SetUp(&messenger, &api); + + int64_t result = 0; + messenger.SendHostMessage("dev.flutter.pigeon.PrimitiveHostApi.anInt", + EncodableValue(EncodableList({EncodableValue(7)})), + [&result](const EncodableValue& reply) { + result = GetResult(reply).LongValue(); + }); + + EXPECT_EQ(result, 7); +} + +TEST(Primitive, HostBool) { + FakeHostMessenger messenger(&PrimitiveHostApi::GetCodec()); + TestHostApi api; + PrimitiveHostApi::SetUp(&messenger, &api); + + bool result = false; + messenger.SendHostMessage( + "dev.flutter.pigeon.PrimitiveHostApi.aBool", + EncodableValue(EncodableList({EncodableValue(true)})), + [&result](const EncodableValue& reply) { + result = std::get(GetResult(reply)); + }); + + EXPECT_EQ(result, true); +} + +TEST(Primitive, HostDouble) { + FakeHostMessenger messenger(&PrimitiveHostApi::GetCodec()); + TestHostApi api; + PrimitiveHostApi::SetUp(&messenger, &api); + + double result = 0.0; + messenger.SendHostMessage( + "dev.flutter.pigeon.PrimitiveHostApi.aDouble", + EncodableValue(EncodableList({EncodableValue(3.0)})), + [&result](const EncodableValue& reply) { + result = std::get(GetResult(reply)); + }); + + EXPECT_EQ(result, 3.0); +} + +TEST(Primitive, HostString) { + FakeHostMessenger messenger(&PrimitiveHostApi::GetCodec()); + TestHostApi api; + PrimitiveHostApi::SetUp(&messenger, &api); + + std::string result; + messenger.SendHostMessage( + "dev.flutter.pigeon.PrimitiveHostApi.aString", + EncodableValue(EncodableList({EncodableValue("hello")})), + [&result](const EncodableValue& reply) { + result = std::get(GetResult(reply)); + }); + + EXPECT_EQ(result, "hello"); +} + +TEST(Primitive, HostList) { + FakeHostMessenger messenger(&PrimitiveHostApi::GetCodec()); + TestHostApi api; + PrimitiveHostApi::SetUp(&messenger, &api); + + EncodableList result; + messenger.SendHostMessage( + "dev.flutter.pigeon.PrimitiveHostApi.aList", + EncodableValue(EncodableList({EncodableValue(EncodableList({1, 2, 3}))})), + [&result](const EncodableValue& reply) { + result = std::get(GetResult(reply)); + }); + + EXPECT_EQ(result.size(), 3); + EXPECT_EQ(result[2].LongValue(), 3); +} + +TEST(Primitive, HostMap) { + FakeHostMessenger messenger(&PrimitiveHostApi::GetCodec()); + TestHostApi api; + PrimitiveHostApi::SetUp(&messenger, &api); + + EncodableMap result; + messenger.SendHostMessage( + "dev.flutter.pigeon.PrimitiveHostApi.aMap", + EncodableValue(EncodableList({EncodableValue(EncodableMap({ + {EncodableValue("foo"), EncodableValue("bar")}, + }))})), + [&result](const EncodableValue& reply) { + result = std::get(GetResult(reply)); + }); + + EXPECT_EQ(result.size(), 1); + EXPECT_EQ(result[EncodableValue("foo")], EncodableValue("bar")); +} + +// TODO(stuartmorgan): Add FlutterApi versions of the tests. + +} // namespace primitive_pigeontest diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.cpp new file mode 100644 index 000000000000..18da6c9f26e8 --- /dev/null +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.cpp @@ -0,0 +1,32 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "echo_messenger.h" + +#include +#include + +namespace testing { + +EchoMessenger::EchoMessenger( + const flutter::MessageCodec* codec) + : codec_(codec) {} +EchoMessenger::~EchoMessenger() {} + +// flutter::BinaryMessenger: +void EchoMessenger::Send(const std::string& channel, const uint8_t* message, + size_t message_size, + flutter::BinaryReply reply) const { + std::unique_ptr arg_value = + codec_->DecodeMessage(message, message_size); + const auto& args = std::get(*arg_value); + std::unique_ptr> reply_data = + codec_->EncodeMessage(args[0]); + reply(reply_data->data(), reply_data->size()); +} + +void EchoMessenger::SetMessageHandler(const std::string& channel, + flutter::BinaryMessageHandler handler) {} + +} // namespace testing diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.h b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.h new file mode 100644 index 000000000000..7bc462545a3d --- /dev/null +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/echo_messenger.h @@ -0,0 +1,35 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef PLATFORM_TESTS_WINDOWS_UNIT_TESTS_WINDOWS_TEST_UTILS_ECHO_MESSENGER_H_ +#define PLATFORM_TESTS_WINDOWS_UNIT_TESTS_WINDOWS_TEST_UTILS_ECHO_MESSENGER_H_ + +#include +#include +#include + +namespace testing { + +// A BinaryMessenger that replies with the first argument sent to it. +class EchoMessenger : public flutter::BinaryMessenger { + public: + // Creates an echo messenger that expects MessageCalls encoded with the given + // codec. + EchoMessenger(const flutter::MessageCodec* codec); + virtual ~EchoMessenger(); + + // flutter::BinaryMessenger: + void Send(const std::string& channel, const uint8_t* message, + size_t message_size, + flutter::BinaryReply reply = nullptr) const override; + void SetMessageHandler(const std::string& channel, + flutter::BinaryMessageHandler handler) override; + + private: + const flutter::MessageCodec* codec_; +}; + +} // namespace testing + +#endif // PLATFORM_TESTS_WINDOWS_UNIT_TESTS_WINDOWS_TEST_UTILS_ECHO_MESSENGER_H_ diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.cpp new file mode 100644 index 000000000000..a2b10b5e4744 --- /dev/null +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.cpp @@ -0,0 +1,46 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "fake_host_messenger.h" + +#include +#include + +#include +#include + +namespace testing { + +FakeHostMessenger::FakeHostMessenger( + const flutter::MessageCodec* codec) + : codec_(codec) {} +FakeHostMessenger::~FakeHostMessenger() {} + +void FakeHostMessenger::SendHostMessage(const std::string& channel, + + const flutter::EncodableValue& message, + HostMessageReply reply_handler) { + const auto* codec = codec_; + flutter::BinaryReply binary_handler = [reply_handler, codec, channel]( + const uint8_t* reply_data, + size_t reply_size) { + std::unique_ptr reply = + codec->DecodeMessage(reply_data, reply_size); + reply_handler(*reply); + }; + + std::unique_ptr> data = codec_->EncodeMessage(message); + handlers_[channel](data->data(), data->size(), std::move(binary_handler)); +} + +void FakeHostMessenger::Send(const std::string& channel, const uint8_t* message, + size_t message_size, + flutter::BinaryReply reply) const {} + +void FakeHostMessenger::SetMessageHandler( + const std::string& channel, flutter::BinaryMessageHandler handler) { + handlers_[channel] = std::move(handler); +} + +} // namespace testing diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.h b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.h new file mode 100644 index 000000000000..fd3142669471 --- /dev/null +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/utils/fake_host_messenger.h @@ -0,0 +1,51 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef PLATFORM_TESTS_WINDOWS_UNIT_TESTS_WINDOWS_TEST_UTILS_FAKE_HOST_MESSENGER_H_ +#define PLATFORM_TESTS_WINDOWS_UNIT_TESTS_WINDOWS_TEST_UTILS_FAKE_HOST_MESSENGER_H_ + +#include +#include +#include + +#include + +namespace testing { + +typedef std::function + HostMessageReply; + +// A BinaryMessenger that allows tests to act as the engine to call host APIs. +class FakeHostMessenger : public flutter::BinaryMessenger { + public: + // Creates an messenger that can send and receive responses with the given + // codec. + FakeHostMessenger( + const flutter::MessageCodec* codec); + virtual ~FakeHostMessenger(); + + // Calls the registered handler for the given channel, and calls reply_handler + // with the response. + // + // This allows a test to simulate a message from the Dart side, exercising the + // encoding and decoding logic generated for a host API. + void SendHostMessage(const std::string& channel, + const flutter::EncodableValue& message, + HostMessageReply reply_handler); + + // flutter::BinaryMessenger: + void Send(const std::string& channel, const uint8_t* message, + size_t message_size, + flutter::BinaryReply reply = nullptr) const override; + void SetMessageHandler(const std::string& channel, + flutter::BinaryMessageHandler handler) override; + + private: + const flutter::MessageCodec* codec_; + std::map handlers_; +}; + +} // namespace testing + +#endif // PLATFORM_TESTS_WINDOWS_UNIT_TESTS_WINDOWS_TEST_UTILS_FAKE_HOST_MESSENGER_H_ From 512c8708e13110124a4abb546f24d258f37062db Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 22 Jun 2022 12:25:01 -0400 Subject: [PATCH 08/12] Bring up more native tests --- .../windows_unit_tests/windows/CMakeLists.txt | 2 + .../windows/test/multiple_arity_test.cpp | 52 ++++++++ .../windows/test/nullable_returns_test.cpp | 112 ++++++++++++++++++ .../windows/test/primitive_test.cpp | 2 +- 4 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 packages/pigeon/platform_tests/windows_unit_tests/windows/test/multiple_arity_test.cpp create mode 100644 packages/pigeon/platform_tests/windows_unit_tests/windows/test/nullable_returns_test.cpp diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt b/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt index 3e663d015c65..db289d07055a 100644 --- a/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/CMakeLists.txt @@ -53,7 +53,9 @@ FetchContent_MakeAvailable(googletest) # directly into the test binary rather than using the DLL. add_executable(${TEST_RUNNER} # Tests. + test/multiple_arity_test.cpp test/non_null_fields_test.cpp + test/nullable_returns_test.cpp test/null_fields_test.cpp test/pigeon_test.cpp test/primitive_test.cpp diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/multiple_arity_test.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/multiple_arity_test.cpp new file mode 100644 index 000000000000..8434164349da --- /dev/null +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/multiple_arity_test.cpp @@ -0,0 +1,52 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include "test/multiple_arity.g.h" +#include "test/utils/fake_host_messenger.h" + +namespace multiple_arity_pigeontest { + +namespace { +using flutter::EncodableList; +using flutter::EncodableMap; +using flutter::EncodableValue; +using testing::FakeHostMessenger; + +class TestHostApi : public MultipleArityHostApi { + public: + TestHostApi() {} + virtual ~TestHostApi() {} + + protected: + ErrorOr Subtract(int64_t x, int64_t y) override { return x - y; } +}; + +const EncodableValue& GetResult(const EncodableValue& pigeon_response) { + return std::get(pigeon_response).at(EncodableValue("result")); +} +} // namespace + +TEST(MultipleArity, HostSimple) { + FakeHostMessenger messenger(&MultipleArityHostApi::GetCodec()); + TestHostApi api; + MultipleArityHostApi::SetUp(&messenger, &api); + + int64_t result = 0; + messenger.SendHostMessage("dev.flutter.pigeon.MultipleArityHostApi.subtract", + EncodableValue(EncodableList({ + EncodableValue(30), + EncodableValue(10), + })), + [&result](const EncodableValue& reply) { + result = GetResult(reply).LongValue(); + }); + + EXPECT_EQ(result, 20); +} + +// TODO(stuartmorgan): Add a FlutterApi version of the test. + +} // namespace multiple_arity_pigeontest diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/nullable_returns_test.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/nullable_returns_test.cpp new file mode 100644 index 000000000000..4804e1aff348 --- /dev/null +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/nullable_returns_test.cpp @@ -0,0 +1,112 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include + +#include "test/nullable_returns.g.h" +#include "test/utils/fake_host_messenger.h" + +namespace nullable_returns_pigeontest { + +namespace { +using flutter::EncodableList; +using flutter::EncodableMap; +using flutter::EncodableValue; +using testing::FakeHostMessenger; + +class TestNullableArgHostApi : public NullableArgHostApi { + public: + TestNullableArgHostApi() {} + virtual ~TestNullableArgHostApi() {} + + protected: + ErrorOr Doit(const int64_t* x) override { + return x == nullptr ? 42 : *x; + } +}; + +class TestNullableReturnHostApi : public NullableReturnHostApi { + public: + TestNullableReturnHostApi(std::optional return_value) + : value_(return_value) {} + virtual ~TestNullableReturnHostApi() {} + + protected: + ErrorOr> Doit() override { return value_; } + + private: + std::optional value_; +}; + +const EncodableValue& GetResult(const EncodableValue& pigeon_response) { + return std::get(pigeon_response).at(EncodableValue("result")); +} +} // namespace + +TEST(NullableReturns, HostNullableArgNull) { + FakeHostMessenger messenger(&NullableArgHostApi::GetCodec()); + TestNullableArgHostApi api; + NullableArgHostApi::SetUp(&messenger, &api); + + int64_t result = 0; + messenger.SendHostMessage("dev.flutter.pigeon.NullableArgHostApi.doit", + EncodableValue(EncodableList({EncodableValue()})), + [&result](const EncodableValue& reply) { + result = GetResult(reply).LongValue(); + }); + + EXPECT_EQ(result, 42); +} + +TEST(NullableReturns, HostNullableArgNonNull) { + FakeHostMessenger messenger(&NullableArgHostApi::GetCodec()); + TestNullableArgHostApi api; + NullableArgHostApi::SetUp(&messenger, &api); + + int64_t result = 0; + messenger.SendHostMessage("dev.flutter.pigeon.NullableArgHostApi.doit", + EncodableValue(EncodableList({EncodableValue(7)})), + [&result](const EncodableValue& reply) { + result = GetResult(reply).LongValue(); + }); + + EXPECT_EQ(result, 7); +} + +TEST(NullableReturns, HostNullableReturnNull) { + FakeHostMessenger messenger(&NullableReturnHostApi::GetCodec()); + TestNullableReturnHostApi api(std::nullopt); + NullableReturnHostApi::SetUp(&messenger, &api); + + // Initialize to a non-null value to ensure that it's actually set to null, + // rather than just never set. + EncodableValue result(true); + messenger.SendHostMessage( + "dev.flutter.pigeon.NullableReturnHostApi.doit", + EncodableValue(EncodableList({})), + [&result](const EncodableValue& reply) { result = GetResult(reply); }); + + EXPECT_TRUE(result.IsNull()); +} + +TEST(NullableReturns, HostNullableReturnNonNull) { + FakeHostMessenger messenger(&NullableReturnHostApi::GetCodec()); + TestNullableReturnHostApi api(42); + NullableReturnHostApi::SetUp(&messenger, &api); + + EncodableValue result; + messenger.SendHostMessage( + "dev.flutter.pigeon.NullableReturnHostApi.doit", + EncodableValue(EncodableList({})), + [&result](const EncodableValue& reply) { result = GetResult(reply); }); + + EXPECT_FALSE(result.IsNull()); + EXPECT_EQ(result.LongValue(), 42); +} + +// TODO(stuartmorgan): Add FlutterApi versions of the tests. + +} // namespace nullable_returns_pigeontest diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp index 4aeabb336b80..68b4dfe1457b 100644 --- a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/primitive_test.cpp @@ -18,7 +18,7 @@ using testing::FakeHostMessenger; class TestHostApi : public PrimitiveHostApi { public: TestHostApi() {} - virtual ~TestHostApi(){}; + virtual ~TestHostApi() {} protected: ErrorOr AnInt(int64_t value) override { return value; } From 57cf729722f2015daa67fc2dc95b1d0186fdd412 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 22 Jun 2022 12:27:46 -0400 Subject: [PATCH 09/12] Version bump --- packages/pigeon/CHANGELOG.md | 4 ++++ packages/pigeon/lib/generator_tools.dart | 2 +- packages/pigeon/pubspec.yaml | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/pigeon/CHANGELOG.md b/packages/pigeon/CHANGELOG.md index 9a73a6c1f11d..3df4bba77fc4 100644 --- a/packages/pigeon/CHANGELOG.md +++ b/packages/pigeon/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.1.8 + +* [C++] Fixes most non-class arguments and return values in host APIs. + ## 3.1.7 * [java] Adds option to add javax.annotation.Generated annotation. diff --git a/packages/pigeon/lib/generator_tools.dart b/packages/pigeon/lib/generator_tools.dart index 155592d695cf..a0b3aa860b2e 100644 --- a/packages/pigeon/lib/generator_tools.dart +++ b/packages/pigeon/lib/generator_tools.dart @@ -9,7 +9,7 @@ import 'dart:mirrors'; import 'ast.dart'; /// The current version of pigeon. This must match the version in pubspec.yaml. -const String pigeonVersion = '3.1.7'; +const String pigeonVersion = '3.1.8'; /// Read all the content from [stdin] to a String. String readStdin() { diff --git a/packages/pigeon/pubspec.yaml b/packages/pigeon/pubspec.yaml index cb09a192d19f..195c86df98a0 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%3Apigeon -version: 3.1.7 # This must match the version in lib/generator_tools.dart +version: 3.1.8 # This must match the version in lib/generator_tools.dart environment: sdk: ">=2.12.0 <3.0.0" From 72eade26795cfa2b2eb9b0f13b93e38533974210 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 22 Jun 2022 12:33:17 -0400 Subject: [PATCH 10/12] Improve changelog --- packages/pigeon/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/pigeon/CHANGELOG.md b/packages/pigeon/CHANGELOG.md index 3df4bba77fc4..2f1dfcd63f48 100644 --- a/packages/pigeon/CHANGELOG.md +++ b/packages/pigeon/CHANGELOG.md @@ -1,6 +1,9 @@ ## 3.1.8 -* [C++] Fixes most non-class arguments and return values in host APIs. +* [c++] Fixes most non-class arguments and return values in host APIs. The + types of arguments and return values have changed, so this may require updates + to existing code. + ## 3.1.7 From 1fed5aafc01e2e40d958d6d2652b78e9886b7b26 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Thu, 23 Jun 2022 11:10:59 -0400 Subject: [PATCH 11/12] Post-merge fixes --- packages/pigeon/lib/swift_generator.dart | 4 ++-- .../windows_unit_tests/windows/test/pigeon_test.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/pigeon/lib/swift_generator.dart b/packages/pigeon/lib/swift_generator.dart index 8a7d6ec09dd7..8be42784d20b 100644 --- a/packages/pigeon/lib/swift_generator.dart +++ b/packages/pigeon/lib/swift_generator.dart @@ -408,8 +408,8 @@ void generateSwift(SwiftOptions options, Root root, StringSink sink) { final Indent indent = Indent(sink); HostDatatype _getHostDatatype(NamedType field) { - return getHostDatatype(field, root.classes, root.enums, - (NamedType x) => _swiftTypeForBuiltinDartType(x.type)); + return getFieldHostDatatype(field, root.classes, root.enums, + (TypeDeclaration x) => _swiftTypeForBuiltinDartType(x)); } void writeHeader() { diff --git a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp index 06fb9f34c934..60c1f1f6355a 100644 --- a/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp +++ b/packages/pigeon/platform_tests/windows_unit_tests/windows/test/pigeon_test.cpp @@ -130,9 +130,9 @@ TEST(PigeonTests, CallSearch) { .Times(1) .WillOnce(testing::SaveArg<1>(&handler)); EXPECT_CALL(mock_api, Search(testing::_)) - .WillOnce(Return(ByMove(ErrorOr( - MessageSearchReply())))); - Api::SetUp(&mock_messenger, &mock_api); + .WillOnce( + Return(ByMove(ErrorOr(MessageSearchReply())))); + MessageApi::SetUp(&mock_messenger, &mock_api); bool did_call_reply = false; flutter::BinaryReply reply = [&did_call_reply](const uint8_t* data, size_t size) { From 17149c1e900b663d345ac3ddaa9e950e93b9679f Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Thu, 23 Jun 2022 11:34:50 -0400 Subject: [PATCH 12/12] Minor FlutterError cleanup --- packages/pigeon/lib/cpp_generator.dart | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/pigeon/lib/cpp_generator.dart b/packages/pigeon/lib/cpp_generator.dart index 6b138e02e5ae..ad4758a1330e 100644 --- a/packages/pigeon/lib/cpp_generator.dart +++ b/packages/pigeon/lib/cpp_generator.dart @@ -142,13 +142,12 @@ void _writeErrorOr(Indent indent, indent.format(''' class FlutterError { public: -\tFlutterError(); \tFlutterError(const std::string& arg_code) -\t\t: code(arg_code) {}; +\t\t: code(arg_code) {} \tFlutterError(const std::string& arg_code, const std::string& arg_message) -\t\t: code(arg_code), message(arg_message) {}; +\t\t: code(arg_code), message(arg_message) {} \tFlutterError(const std::string& arg_code, const std::string& arg_message, const flutter::EncodableValue& arg_details) -\t\t: code(arg_code), message(arg_message), details(arg_details) {}; +\t\t: code(arg_code), message(arg_message), details(arg_details) {} \tstd::string code; \tstd::string message; \tflutter::EncodableValue details;