Skip to content

Commit 94ce998

Browse files
bparrishMinesbparrishMines
authored
[pigeon] Add errors for ProxyAPI callback methods and null instances when reading in a ProxyApiBaseCodec (#8567)
* Adds an error message when a ProxyAPI callback method that returns a non-null value is nullable. Returning a null value causes error on the platform side. This ensures the Dart side prevents this from happening. * Adds an error message in the `ProxyApiBaseCodec` when an instance could not be retrieved when reading a value.
1 parent 1ecb531 commit 94ce998

File tree

20 files changed

+646
-1067
lines changed

20 files changed

+646
-1067
lines changed

‎packages/pigeon/CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
1+
## 24.1.1
2+
3+
* [swift, kotlin] Adds an error message when a ProxyAPI callback method that returns a non-null
4+
value is nullable.
5+
* [swift, kotlin] Adds an error message in the `ProxyApiBaseCodec` when an instance could not be
6+
retrieved when reading a value.
7+
* [swift, kotlin] Fixes ProxyAPI platform APIs not calling completion when creating a new instance.
8+
19
## 24.1.0
210

311
* [kotlin, swift] Adds annotation options to omit shared classes used in Event Channels.

‎packages/pigeon/lib/src/generator_tools.dart‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import 'ast.dart';
1414
/// The current version of pigeon.
1515
///
1616
/// This must match the version in pubspec.yaml.
17-
const String pigeonVersion = '24.1.0';
17+
const String pigeonVersion = '24.1.1';
1818

1919
/// Read all the content from [stdin] to a String.
2020
String readStdin() {

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

Lines changed: 54 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -847,7 +847,15 @@ if (wrapped == null) {
847847
override fun readValueOfType(type: Byte, buffer: ByteBuffer): Any? {
848848
return when (type) {
849849
$proxyApiCodecInstanceManagerKey.toByte() -> {
850-
return registrar.instanceManager.getInstance(readValue(buffer) as Long)
850+
val identifier: Long = readValue(buffer) as Long
851+
val instance: Any? = registrar.instanceManager.getInstance(identifier)
852+
if (instance == null) {
853+
Log.e(
854+
"${proxyApiCodecName(const KotlinOptions())}",
855+
"Failed to find instance with identifier: \$identifier"
856+
)
857+
}
858+
return instance
851859
}
852860
else -> super.readValueOfType(type, buffer)
853861
}
@@ -1912,58 +1920,63 @@ if (wrapped == null) {
19121920
'''
19131921
callback(
19141922
Result.failure(
1915-
$errorClassName("ignore-calls-error", "Calls to Dart are being ignored.", "")))
1916-
return''',
1923+
$errorClassName("ignore-calls-error", "Calls to Dart are being ignored.", "")))''',
19171924
);
19181925
},
1926+
addTrailingNewline: false,
19191927
);
19201928
indent.writeScoped(
1921-
'if (pigeonRegistrar.instanceManager.containsInstance(${classMemberNamePrefix}instanceArg)) {',
1929+
' else if (pigeonRegistrar.instanceManager.containsInstance(${classMemberNamePrefix}instanceArg)) {',
19221930
'}',
19231931
() {
1924-
indent.writeln('Result.success(Unit)');
1925-
indent.writeln('return');
1932+
indent.writeln('callback(Result.success(Unit))');
19261933
},
1934+
addTrailingNewline: false,
19271935
);
1928-
if (api.hasCallbackConstructor()) {
1929-
indent.writeln(
1930-
'val ${classMemberNamePrefix}identifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(${classMemberNamePrefix}instanceArg)',
1931-
);
1932-
enumerate(api.unattachedFields, (int index, ApiField field) {
1933-
final String argName = _getSafeArgumentName(index, field);
1936+
indent.writeScoped(' else {', '}', () {
1937+
if (api.hasCallbackConstructor()) {
19341938
indent.writeln(
1935-
'val $argName = ${field.name}(${classMemberNamePrefix}instanceArg)',
1939+
'val ${classMemberNamePrefix}identifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(${classMemberNamePrefix}instanceArg)',
19361940
);
1937-
});
1941+
enumerate(api.unattachedFields, (int index, ApiField field) {
1942+
final String argName = _getSafeArgumentName(index, field);
1943+
indent.writeln(
1944+
'val $argName = ${field.name}(${classMemberNamePrefix}instanceArg)',
1945+
);
1946+
});
19381947

1939-
indent
1940-
.writeln('val binaryMessenger = pigeonRegistrar.binaryMessenger');
1941-
indent.writeln('val codec = pigeonRegistrar.codec');
1942-
_writeFlutterMethodMessageCall(
1943-
indent,
1944-
returnType: returnType,
1945-
channelName: channelName,
1946-
errorClassName: errorClassName,
1947-
parameters: <Parameter>[
1948-
Parameter(
1949-
name: '${classMemberNamePrefix}identifier',
1950-
type: const TypeDeclaration(
1951-
baseName: 'int',
1952-
isNullable: false,
1948+
indent.writeln(
1949+
'val binaryMessenger = pigeonRegistrar.binaryMessenger');
1950+
indent.writeln('val codec = pigeonRegistrar.codec');
1951+
_writeFlutterMethodMessageCall(
1952+
indent,
1953+
returnType: returnType,
1954+
channelName: channelName,
1955+
errorClassName: errorClassName,
1956+
parameters: <Parameter>[
1957+
Parameter(
1958+
name: '${classMemberNamePrefix}identifier',
1959+
type: const TypeDeclaration(
1960+
baseName: 'int',
1961+
isNullable: false,
1962+
),
19531963
),
1954-
),
1955-
...api.unattachedFields.map(
1956-
(ApiField field) {
1957-
return Parameter(name: field.name, type: field.type);
1958-
},
1959-
),
1960-
],
1961-
);
1962-
} else {
1963-
indent.writeln(
1964-
'throw IllegalStateException("Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.")',
1965-
);
1966-
}
1964+
...api.unattachedFields.map(
1965+
(ApiField field) {
1966+
return Parameter(name: field.name, type: field.type);
1967+
},
1968+
),
1969+
],
1970+
);
1971+
} else {
1972+
indent.format(
1973+
'''
1974+
callback(
1975+
Result.failure(
1976+
$errorClassName("new-instance-error", "Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.", "")))''',
1977+
);
1978+
}
1979+
});
19671980
},
19681981
);
19691982
indent.newln();

‎packages/pigeon/lib/src/pigeon_lib.dart‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1397,11 +1397,22 @@ List<Error> _validateProxyApi(
13971397
}
13981398
}
13991399

1400-
if (method.location == ApiLocation.flutter && method.isStatic) {
1401-
result.add(Error(
1402-
message: 'Static callback methods are not supported: ${method.name}.',
1403-
lineNumber: _calculateLineNumberNullable(source, method.offset),
1404-
));
1400+
if (method.location == ApiLocation.flutter) {
1401+
if (!method.returnType.isVoid &&
1402+
!method.returnType.isNullable &&
1403+
!method.isRequired) {
1404+
result.add(Error(
1405+
message:
1406+
'Callback methods that return a non-null value must be non-null: ${method.name}.',
1407+
lineNumber: _calculateLineNumberNullable(source, method.offset),
1408+
));
1409+
}
1410+
if (method.isStatic) {
1411+
result.add(Error(
1412+
message: 'Static callback methods are not supported: ${method.name}.',
1413+
lineNumber: _calculateLineNumberNullable(source, method.offset),
1414+
));
1415+
}
14051416
}
14061417
}
14071418

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

Lines changed: 55 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -949,6 +949,9 @@ if (wrapped == nil) {
949949
let identifier = self.readValue()
950950
let instance: AnyObject? = pigeonRegistrar.instanceManager.instance(
951951
forIdentifier: identifier is Int64 ? identifier as! Int64 : Int64(identifier as! Int32))
952+
if instance == nil {
953+
print("Failed to find instance with identifier: \\(identifier!)")
954+
}
952955
return instance
953956
default:
954957
return super.readValue(ofType: type)
@@ -1502,7 +1505,7 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
15021505
}
15031506
indent.addScoped('else {', '}', () {
15041507
if (returnType.isVoid) {
1505-
indent.writeln('completion(.success(Void()))');
1508+
indent.writeln('completion(.success(()))');
15061509
} else {
15071510
final String fieldType = _swiftTypeForDartType(returnType);
15081511
_writeGenericCasting(
@@ -2318,63 +2321,70 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
23182321
.failure(
23192322
${_getErrorClassName(generatorOptions)}(
23202323
code: "ignore-calls-error",
2321-
message: "Calls to Dart are being ignored.", details: "")))
2322-
return''',
2324+
message: "Calls to Dart are being ignored.", details: "")))''',
23232325
);
23242326
},
2327+
addTrailingNewline: false,
23252328
);
23262329

23272330
indent.writeScoped(
2328-
'if pigeonRegistrar.instanceManager.containsInstance(pigeonInstance as AnyObject) {',
2331+
' else if pigeonRegistrar.instanceManager.containsInstance(pigeonInstance as AnyObject) {',
23292332
'}',
23302333
() {
2331-
indent.writeln('completion(.success(Void()))');
2332-
indent.writeln('return');
2334+
indent.writeln('completion(.success(()))');
23332335
},
2336+
addTrailingNewline: false,
23342337
);
2335-
if (api.hasCallbackConstructor()) {
2336-
indent.writeln(
2337-
'let pigeonIdentifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(pigeonInstance as AnyObject)',
2338-
);
2339-
enumerate(api.unattachedFields, (int index, ApiField field) {
2340-
final String argName = _getSafeArgumentName(index, field);
2338+
indent.writeScoped(' else {', '}', () {
2339+
if (api.hasCallbackConstructor()) {
23412340
indent.writeln(
2342-
'let $argName = try! pigeonDelegate.${field.name}(pigeonApi: self, pigeonInstance: pigeonInstance)',
2341+
'let pigeonIdentifierArg = pigeonRegistrar.instanceManager.addHostCreatedInstance(pigeonInstance as AnyObject)',
23432342
);
2344-
});
2345-
indent.writeln(
2346-
'let binaryMessenger = pigeonRegistrar.binaryMessenger',
2347-
);
2348-
indent.writeln('let codec = pigeonRegistrar.codec');
2349-
_writeFlutterMethodMessageCall(
2350-
indent,
2351-
generatorOptions: generatorOptions,
2352-
parameters: <Parameter>[
2353-
Parameter(
2354-
name: 'pigeonIdentifier',
2355-
type: const TypeDeclaration(
2356-
baseName: 'int',
2357-
isNullable: false,
2343+
enumerate(api.unattachedFields, (int index, ApiField field) {
2344+
final String argName = _getSafeArgumentName(index, field);
2345+
indent.writeln(
2346+
'let $argName = try! pigeonDelegate.${field.name}(pigeonApi: self, pigeonInstance: pigeonInstance)',
2347+
);
2348+
});
2349+
indent.writeln(
2350+
'let binaryMessenger = pigeonRegistrar.binaryMessenger',
2351+
);
2352+
indent.writeln('let codec = pigeonRegistrar.codec');
2353+
_writeFlutterMethodMessageCall(
2354+
indent,
2355+
generatorOptions: generatorOptions,
2356+
parameters: <Parameter>[
2357+
Parameter(
2358+
name: 'pigeonIdentifier',
2359+
type: const TypeDeclaration(
2360+
baseName: 'int',
2361+
isNullable: false,
2362+
),
23582363
),
2364+
...api.unattachedFields.map(
2365+
(ApiField field) {
2366+
return Parameter(name: field.name, type: field.type);
2367+
},
2368+
),
2369+
],
2370+
returnType: const TypeDeclaration.voidDeclaration(),
2371+
channelName: makeChannelNameWithStrings(
2372+
apiName: api.name,
2373+
methodName: newInstanceMethodName,
2374+
dartPackageName: dartPackageName,
23592375
),
2360-
...api.unattachedFields.map(
2361-
(ApiField field) {
2362-
return Parameter(name: field.name, type: field.type);
2363-
},
2364-
),
2365-
],
2366-
returnType: const TypeDeclaration.voidDeclaration(),
2367-
channelName: makeChannelNameWithStrings(
2368-
apiName: api.name,
2369-
methodName: newInstanceMethodName,
2370-
dartPackageName: dartPackageName,
2371-
),
2372-
);
2373-
} else {
2374-
indent.writeln(
2375-
'print("Error: Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.")',
2376-
);
2377-
}
2376+
);
2377+
} else {
2378+
indent.format(
2379+
'''
2380+
completion(
2381+
.failure(
2382+
${_getErrorClassName(generatorOptions)}(
2383+
code: "new-instance-error",
2384+
message: "Error: Attempting to create a new Dart instance of ${api.name}, but the class has a nonnull callback method.", details: "")))''',
2385+
);
2386+
}
2387+
});
23782388
});
23792389

23802390
if (unsupportedPlatforms != null) {

‎packages/pigeon/pigeons/proxy_api_tests.dart‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -95,42 +95,42 @@ abstract class ProxyApiTestClass extends ProxyApiSuperClass
9595
// ========== Non-nullable argument/return type tests ==========
9696

9797
/// Returns the passed boolean, to test serialization and deserialization.
98-
late bool Function(bool aBool)? flutterEchoBool;
98+
late bool Function(bool aBool) flutterEchoBool;
9999

100100
/// Returns the passed int, to test serialization and deserialization.
101-
late int Function(int anInt)? flutterEchoInt;
101+
late int Function(int anInt) flutterEchoInt;
102102

103103
/// Returns the passed double, to test serialization and deserialization.
104-
late double Function(double aDouble)? flutterEchoDouble;
104+
late double Function(double aDouble) flutterEchoDouble;
105105

106106
/// Returns the passed string, to test serialization and deserialization.
107-
late String Function(String aString)? flutterEchoString;
107+
late String Function(String aString) flutterEchoString;
108108

109109
/// Returns the passed byte list, to test serialization and deserialization.
110-
late Uint8List Function(Uint8List aList)? flutterEchoUint8List;
110+
late Uint8List Function(Uint8List aList) flutterEchoUint8List;
111111

112112
/// Returns the passed list, to test serialization and deserialization.
113-
late List<Object?> Function(List<Object?> aList)? flutterEchoList;
113+
late List<Object?> Function(List<Object?> aList) flutterEchoList;
114114

115115
/// Returns the passed list with ProxyApis, to test serialization and
116116
/// deserialization.
117-
late List<ProxyApiTestClass?> Function(List<ProxyApiTestClass?> aList)?
117+
late List<ProxyApiTestClass?> Function(List<ProxyApiTestClass?> aList)
118118
flutterEchoProxyApiList;
119119

120120
/// Returns the passed map, to test serialization and deserialization.
121-
late Map<String?, Object?> Function(Map<String?, Object?> aMap)?
121+
late Map<String?, Object?> Function(Map<String?, Object?> aMap)
122122
flutterEchoMap;
123123

124124
/// Returns the passed map with ProxyApis, to test serialization and
125125
/// deserialization.
126126
late Map<String?, ProxyApiTestClass?> Function(
127-
Map<String?, ProxyApiTestClass?> aMap)? flutterEchoProxyApiMap;
127+
Map<String?, ProxyApiTestClass?> aMap) flutterEchoProxyApiMap;
128128

129129
/// Returns the passed enum to test serialization and deserialization.
130-
late ProxyApiTestEnum Function(ProxyApiTestEnum anEnum)? flutterEchoEnum;
130+
late ProxyApiTestEnum Function(ProxyApiTestEnum anEnum) flutterEchoEnum;
131131

132132
/// Returns the passed ProxyApi to test serialization and deserialization.
133-
late ProxyApiSuperClass Function(ProxyApiSuperClass aProxyApi)?
133+
late ProxyApiSuperClass Function(ProxyApiSuperClass aProxyApi)
134134
flutterEchoProxyApi;
135135

136136
// ========== Nullable argument/return type tests ==========
@@ -177,7 +177,7 @@ abstract class ProxyApiTestClass extends ProxyApiSuperClass
177177

178178
/// Returns the passed in generic Object asynchronously.
179179
@async
180-
late String Function(String aString)? flutterEchoAsyncString;
180+
late String Function(String aString) flutterEchoAsyncString;
181181

182182
// ========== Synchronous host method tests ==========
183183

0 commit comments

Comments
 (0)