[pigeon] Add errors for ProxyAPI callback methods and null instances when reading in a ProxyApiBaseCodec

packages/pigeon/CHANGELOG.md

@@ -1,3 +1,11 @@
+## 24.1.1
+
+* [swift, kotlin] Adds an error message when a ProxyAPI callback method that returns a non-null
+  value is nullable.
+* [swift, kotlin] Adds an error message in the `ProxyApiBaseCodec` when an instance could not be
+  retrieved when reading a value.
+* [swift, kotlin] Fixes ProxyAPI platform APIs not calling completion when creating a new instance.
+
 ## 24.1.0
 
 * [kotlin, swift] Adds annotation options to omit shared classes used in Event Channels.

packages/pigeon/lib/src/generator_tools.dart

@@ -14,7 +14,7 @@ import 'ast.dart';
 /// The current version of pigeon.
 ///
 /// This must match the version in pubspec.yaml.
-const String pigeonVersion = '24.1.0';
+const String pigeonVersion = '24.1.1';
 
 /// Read all the content from [stdin] to a String.
 String readStdin() {

packages/pigeon/lib/src/kotlin/kotlin_generator.dart

@@ -847,7 +847,15 @@ if (wrapped == null) {
           override fun readValueOfType(type: Byte, buffer: ByteBuffer): Any? {
             return when (type) {
               $proxyApiCodecInstanceManagerKey.toByte() -> {
-                return registrar.instanceManager.getInstance(readValue(buffer) as Long)
+                val identifier: Long = readValue(buffer) as Long
+                val instance: Any? = registrar.instanceManager.getInstance(identifier)
+                if (instance == null) {
+                  Log.e(
+                    "${proxyApiCodecName(const KotlinOptions())}",
+                    "Failed to find instance with identifier: \$identifier"
+                  )
+                }
+                return instance
               }
               else -> super.readValueOfType(type, buffer)
             }
@@ -1906,64 +1914,69 @@ if (wrapped == null) {
       }) {
         indent.writeScoped(
           'if (pigeonRegistrar.ignoreCallsToDart) {',
-          '}',
+          '} ',
           () {
             indent.format(
               '''
               callback(
                   Result.failure(
-                      $errorClassName("ignore-calls-error", "Calls to Dart are being ignored.", "")))
-              return''',
+                      $errorClassName("ignore-calls-error", "Calls to Dart are being ignored.", "")))''',
             );
           },
+          addTrailingNewline: false,
         );
         indent.writeScoped(
-          'if (pigeonRegistrar.instanceManager.containsInstance(${classMemberNamePrefix}instanceArg)) {',
-          '}',
+          'else if (pigeonRegistrar.instanceManager.containsInstance(${classMemberNamePrefix}instanceArg)) {',
+          '} ',
           () {
-            indent.writeln('Result.success(Unit)');
-            indent.writeln('return');
+            indent.writeln('callback(Result.success(Unit))');
           },
+          addTrailingNewline: false,
         );
-        if (api.hasCallbackConstructor()) {
-          indent.writeln(
-            'val ${classMemberNamePrefix}identifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(${classMemberNamePrefix}instanceArg)',
-          );
-          enumerate(api.unattachedFields, (int index, ApiField field) {
-            final String argName = _getSafeArgumentName(index, field);
+        indent.writeScoped('else {', '}', () {
+          if (api.hasCallbackConstructor()) {
             indent.writeln(
-              'val $argName = ${field.name}(${classMemberNamePrefix}instanceArg)',
+              'val ${classMemberNamePrefix}identifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(${classMemberNamePrefix}instanceArg)',
             );
-          });
+            enumerate(api.unattachedFields, (int index, ApiField field) {
+              final String argName = _getSafeArgumentName(index, field);
+              indent.writeln(
+                'val $argName = ${field.name}(${classMemberNamePrefix}instanceArg)',
+              );
+            });
 
-          indent
-              .writeln('val binaryMessenger = pigeonRegistrar.binaryMessenger');
-          indent.writeln('val codec = pigeonRegistrar.codec');
-          _writeFlutterMethodMessageCall(
-            indent,
-            returnType: returnType,
-            channelName: channelName,
-            errorClassName: errorClassName,
-            parameters: <Parameter>[
-              Parameter(
-                name: '${classMemberNamePrefix}identifier',
-                type: const TypeDeclaration(
-                  baseName: 'int',
-                  isNullable: false,
+            indent.writeln(
+                'val binaryMessenger = pigeonRegistrar.binaryMessenger');
+            indent.writeln('val codec = pigeonRegistrar.codec');
+            _writeFlutterMethodMessageCall(
+              indent,
+              returnType: returnType,
+              channelName: channelName,
+              errorClassName: errorClassName,
+              parameters: <Parameter>[
+                Parameter(
+                  name: '${classMemberNamePrefix}identifier',
+                  type: const TypeDeclaration(
+                    baseName: 'int',
+                    isNullable: false,
+                  ),
                 ),
-              ),
-              ...api.unattachedFields.map(
-                (ApiField field) {
-                  return Parameter(name: field.name, type: field.type);
-                },
-              ),
-            ],
-          );
-        } else {
-          indent.writeln(
-            'throw IllegalStateException("Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.")',
-          );
-        }
+                ...api.unattachedFields.map(
+                  (ApiField field) {
+                    return Parameter(name: field.name, type: field.type);
+                  },
+                ),
+              ],
+            );
+          } else {
+            indent.format(
+              '''
+              callback(
+                  Result.failure(
+                      $errorClassName("new-instance-error", "Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.", "")))''',
+            );
+          }
+        });
       },
     );
     indent.newln();

packages/pigeon/lib/src/pigeon_lib.dart

@@ -1397,11 +1397,22 @@ List<Error> _validateProxyApi(
       }
     }
 
-    if (method.location == ApiLocation.flutter && method.isStatic) {
-      result.add(Error(
-        message: 'Static callback methods are not supported: ${method.name}.',
-        lineNumber: _calculateLineNumberNullable(source, method.offset),
-      ));
+    if (method.location == ApiLocation.flutter) {
+      if (!method.returnType.isVoid &&
+          !method.returnType.isNullable &&
+          !method.isRequired) {
+        result.add(Error(
+          message:
+              'Callback methods that return a non-null value must be non-null: ${method.name}.',
+          lineNumber: _calculateLineNumberNullable(source, method.offset),
+        ));
+      }
+      if (method.isStatic) {
+        result.add(Error(
+          message: 'Static callback methods are not supported: ${method.name}.',
+          lineNumber: _calculateLineNumberNullable(source, method.offset),
+        ));
+      }
     }
   }
 

packages/pigeon/lib/src/swift/swift_generator.dart

@@ -949,6 +949,9 @@ if (wrapped == nil) {
                     let identifier = self.readValue()
                     let instance: AnyObject? = pigeonRegistrar.instanceManager.instance(
                       forIdentifier: identifier is Int64 ? identifier as! Int64 : Int64(identifier as! Int32))
+                    if instance == nil {
+                      print("Failed to find instance with identifier: \\(identifier!)")
+                    }
                     return instance
                   default:
                     return super.readValue(ofType: type)
@@ -1502,7 +1505,7 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
       }
       indent.addScoped('else {', '}', () {
         if (returnType.isVoid) {
-          indent.writeln('completion(.success(Void()))');
+          indent.writeln('completion(.success(()))');
         } else {
           final String fieldType = _swiftTypeForDartType(returnType);
           _writeGenericCasting(
@@ -2310,71 +2313,78 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
     indent.writeScoped('$methodSignature {', '}', () {
       indent.writeScoped(
         'if pigeonRegistrar.ignoreCallsToDart {',
-        '}',
+        '} ',
         () {
           indent.format(
             '''
             completion(
               .failure(
                 ${_getErrorClassName(generatorOptions)}(
                   code: "ignore-calls-error",
-                  message: "Calls to Dart are being ignored.", details: "")))
-            return''',
+                  message: "Calls to Dart are being ignored.", details: "")))''',
           );
         },
+        addTrailingNewline: false,
       );
 
       indent.writeScoped(
-        'if pigeonRegistrar.instanceManager.containsInstance(pigeonInstance as AnyObject) {',
-        '}',
+        'else if pigeonRegistrar.instanceManager.containsInstance(pigeonInstance as AnyObject) {',
+        '} ',
         () {
-          indent.writeln('completion(.success(Void()))');
-          indent.writeln('return');
+          indent.writeln('completion(.success(()))');
         },
+        addTrailingNewline: false,
       );
-      if (api.hasCallbackConstructor()) {
-        indent.writeln(
-          'let pigeonIdentifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(pigeonInstance as AnyObject)',
-        );
-        enumerate(api.unattachedFields, (int index, ApiField field) {
-          final String argName = _getSafeArgumentName(index, field);
+      indent.writeScoped('else {', '}', () {
+        if (api.hasCallbackConstructor()) {
           indent.writeln(
-            'let $argName = try! pigeonDelegate.${field.name}(pigeonApi: self, pigeonInstance: pigeonInstance)',
+            'let pigeonIdentifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(pigeonInstance as AnyObject)',
           );
-        });
-        indent.writeln(
-          'let binaryMessenger = pigeonRegistrar.binaryMessenger',
-        );
-        indent.writeln('let codec = pigeonRegistrar.codec');
-        _writeFlutterMethodMessageCall(
-          indent,
-          generatorOptions: generatorOptions,
-          parameters: <Parameter>[
-            Parameter(
-              name: 'pigeonIdentifier',
-              type: const TypeDeclaration(
-                baseName: 'int',
-                isNullable: false,
+          enumerate(api.unattachedFields, (int index, ApiField field) {
+            final String argName = _getSafeArgumentName(index, field);
+            indent.writeln(
+              'let $argName = try! pigeonDelegate.${field.name}(pigeonApi: self, pigeonInstance: pigeonInstance)',
+            );
+          });
+          indent.writeln(
+            'let binaryMessenger = pigeonRegistrar.binaryMessenger',
+          );
+          indent.writeln('let codec = pigeonRegistrar.codec');
+          _writeFlutterMethodMessageCall(
+            indent,
+            generatorOptions: generatorOptions,
+            parameters: <Parameter>[
+              Parameter(
+                name: 'pigeonIdentifier',
+                type: const TypeDeclaration(
+                  baseName: 'int',
+                  isNullable: false,
+                ),
               ),
+              ...api.unattachedFields.map(
+                (ApiField field) {
+                  return Parameter(name: field.name, type: field.type);
+                },
+              ),
+            ],
+            returnType: const TypeDeclaration.voidDeclaration(),
+            channelName: makeChannelNameWithStrings(
+              apiName: api.name,
+              methodName: newInstanceMethodName,
+              dartPackageName: dartPackageName,
             ),
-            ...api.unattachedFields.map(
-              (ApiField field) {
-                return Parameter(name: field.name, type: field.type);
-              },
-            ),
-          ],
-          returnType: const TypeDeclaration.voidDeclaration(),
-          channelName: makeChannelNameWithStrings(
-            apiName: api.name,
-            methodName: newInstanceMethodName,
-            dartPackageName: dartPackageName,
-          ),
-        );
-      } else {
-        indent.writeln(
-          'print("Error: Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.")',
-        );
-      }
+          );
+        } else {
+          indent.format(
+            '''
+            completion(
+              .failure(
+                ${_getErrorClassName(generatorOptions)}(
+                  code: "new-instance-error",
+                  message: "Error: Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.", details: "")))''',
+          );
+        }
+      });
     });
 
     if (unsupportedPlatforms != null) {

packages/pigeon/pigeons/proxy_api_tests.dart

@@ -95,42 +95,42 @@ abstract class ProxyApiTestClass extends ProxyApiSuperClass
   // ========== Non-nullable argument/return type tests ==========
 
   /// Returns the passed boolean, to test serialization and deserialization.
-  late bool Function(bool aBool)? flutterEchoBool;
+  late bool Function(bool aBool) flutterEchoBool;
 
   /// Returns the passed int, to test serialization and deserialization.
-  late int Function(int anInt)? flutterEchoInt;
+  late int Function(int anInt) flutterEchoInt;
 
   /// Returns the passed double, to test serialization and deserialization.
-  late double Function(double aDouble)? flutterEchoDouble;
+  late double Function(double aDouble) flutterEchoDouble;
 
   /// Returns the passed string, to test serialization and deserialization.
-  late String Function(String aString)? flutterEchoString;
+  late String Function(String aString) flutterEchoString;
 
   /// Returns the passed byte list, to test serialization and deserialization.
-  late Uint8List Function(Uint8List aList)? flutterEchoUint8List;
+  late Uint8List Function(Uint8List aList) flutterEchoUint8List;
 
   /// Returns the passed list, to test serialization and deserialization.
-  late List<Object?> Function(List<Object?> aList)? flutterEchoList;
+  late List<Object?> Function(List<Object?> aList) flutterEchoList;
 
   /// Returns the passed list with ProxyApis, to test serialization and
   /// deserialization.
-  late List<ProxyApiTestClass?> Function(List<ProxyApiTestClass?> aList)?
+  late List<ProxyApiTestClass?> Function(List<ProxyApiTestClass?> aList)
       flutterEchoProxyApiList;
 
   /// Returns the passed map, to test serialization and deserialization.
-  late Map<String?, Object?> Function(Map<String?, Object?> aMap)?
+  late Map<String?, Object?> Function(Map<String?, Object?> aMap)
       flutterEchoMap;
 
   /// Returns the passed map with ProxyApis, to test serialization and
   /// deserialization.
   late Map<String?, ProxyApiTestClass?> Function(
-      Map<String?, ProxyApiTestClass?> aMap)? flutterEchoProxyApiMap;
+      Map<String?, ProxyApiTestClass?> aMap) flutterEchoProxyApiMap;
 
   /// Returns the passed enum to test serialization and deserialization.
-  late ProxyApiTestEnum Function(ProxyApiTestEnum anEnum)? flutterEchoEnum;
+  late ProxyApiTestEnum Function(ProxyApiTestEnum anEnum) flutterEchoEnum;
 
   /// Returns the passed ProxyApi to test serialization and deserialization.
-  late ProxyApiSuperClass Function(ProxyApiSuperClass aProxyApi)?
+  late ProxyApiSuperClass Function(ProxyApiSuperClass aProxyApi)
       flutterEchoProxyApi;
 
   // ========== Nullable argument/return type tests ==========
@@ -177,7 +177,7 @@ abstract class ProxyApiTestClass extends ProxyApiSuperClass
 
   /// Returns the passed in generic Object asynchronously.
   @async
-  late String Function(String aString)? flutterEchoAsyncString;
+  late String Function(String aString) flutterEchoAsyncString;
 
   // ========== Synchronous host method tests ==========