Skip to content

[pigeon] fix swift nsnull casting crash #3545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tarrinneal merged 30 commits into from
Apr 4, 2023
Merged

[pigeon] fix swift nsnull casting crash #3545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
574ee0b
crash fix
tarrinneal Mar 24, 2023
46ca75e
remove need for function
tarrinneal Mar 25, 2023
e350040
cleaner method method
tarrinneal Mar 29, 2023
3d31aff
better name
tarrinneal Mar 29, 2023
3be7eb1
fix enums and tests
tarrinneal Mar 30, 2023
ef111ee
enum fixes, and Any casting
tarrinneal Mar 30, 2023
6d5271f
gen test
tarrinneal Mar 30, 2023
a4625f8
Merge branch 'main' of github.com:flutter/packages into swift-nil
tarrinneal Mar 30, 2023
29ac0ec
simplify casting enum
tarrinneal Mar 30, 2023
4a7b408
Figured it out
tarrinneal Mar 30, 2023
d1d9c6f
Better name
tarrinneal Mar 31, 2023
241f0e3
gen tests
tarrinneal Mar 31, 2023
8d356cc
some nits
tarrinneal Mar 31, 2023
04dfdbd
writedecodecasting init
tarrinneal Mar 31, 2023
ed57271
This works, but does it explode on incorect type?
tarrinneal Apr 1, 2023
1bd3afa
cleaner with as Any
tarrinneal Apr 1, 2023
d0f9b86
gen test
tarrinneal Apr 1, 2023
fab7ca3
Merge branch 'main' of github.com:flutter/packages into swift-nil
tarrinneal Apr 1, 2023
2e2d337
comment
tarrinneal Apr 3, 2023
5b39dbe
fix NSNull issue in EchoBinaryMessenger
tarrinneal Apr 3, 2023
cbb8e0b
makes things better
tarrinneal Apr 4, 2023
7fa0f7d
gen tests
tarrinneal Apr 4, 2023
b0c7653
Improve _writeDecodeCasting
tarrinneal Apr 4, 2023
740abd8
Merge branch 'main' of github.com:flutter/packages into swift-nil
tarrinneal Apr 4, 2023
748a0bb
assert message
tarrinneal Apr 4, 2023
d8f5fe3
nits
tarrinneal Apr 4, 2023
06ab71e
comment
tarrinneal Apr 4, 2023
547ed02
nested ternary
tarrinneal Apr 4, 2023
367009e
gen test
tarrinneal Apr 4, 2023
cdf35ff
nits
tarrinneal Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
makes things better
  • Loading branch information
@tarrinneal
tarrinneal committed Apr 4, 2023
commit cbb8e0b10d38be7c93801e0b9a1268a50ae360f8
41 changes: 28 additions & 13 deletions packages/pigeon/lib/swift_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,11 @@ import FlutterMacOS
final String listValue = 'list[$index]';

_writeDecodeCasting(
root,
indent,
listValue,
field.name,
field.type,
root: root,
indent: indent,
value: listValue,
variableName: field.name,
type: field.type,
field: field,
customClassNames: customClassNames,
customEnumNames: customEnumNames,
Expand Down Expand Up @@ -320,7 +320,12 @@ import FlutterMacOS
} else {
indent.addScoped('{ response in', '}', () {
_writeDecodeCasting(
root, indent, 'response', 'result', func.returnType);
root: root,
indent: indent,
value: 'response',
variableName: 'result',
type: func.returnType,
);
indent.writeln('completion(result)');
});
}
Expand Down Expand Up @@ -436,7 +441,12 @@ import FlutterMacOS
_getSafeArgumentName(index, arg.namedType);
final String argIndex = 'args[$index]';
_writeDecodeCasting(
root, indent, argIndex, argName, arg.type);
root: root,
indent: indent,
value: argIndex,
variableName: argName,
type: arg.type,
);
if (arg.label == '_') {
methodArgument.add(argName);
} else {
Expand Down Expand Up @@ -583,12 +593,12 @@ import FlutterMacOS
/// Writes decode and casting code for any type.
///
/// Optional parameters are necessary for class decoding only.
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more subtle than that; I'm pretty sure that passing customClassNames would actively break things (creating essentially the bug I fixed for C++ in #3573). I was actually confused at first about how the PR wasn't causing a regression there until I noticed that the call sites weren't passing the class list.

If I'm correct about that, I think it would be better to make things much more explicit. E.g., you could rename customClassNames to listEncodedClassNames, and change this comment to explicitly say that a value for that must be provided only in the case of class decoding, with a link to flutter/flutter#119351 for context. (Alternately you could leave the name, but add an explicit bool like I did in the C++ generator.)

Copy link
Contributor Author

@tarrinneal tarrinneal Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed them. And made the comment more specific.

void _writeDecodeCasting(
Root root,
Indent indent,
String value,
String variableName,
TypeDeclaration type, {
void _writeDecodeCasting({
required Root root,
required Indent indent,
required String value,
required String variableName,
required TypeDeclaration type,
NamedType? field,
Set<String>? customClassNames,
Set<String>? customEnumNames,
Expand Down Expand Up @@ -713,10 +723,15 @@ String _camelCase(String text) {

String _castForceUnwrap(String value, TypeDeclaration type, Root root) {
if (isEnum(root, type)) {
assert(!type.isNullable);
return '${_swiftTypeForDartType(type)}(rawValue: $value as! Int)!';
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong for nullable types without the nullableConditionPrefix you removed; if value is nil or NSNull this cast is invalid. Are we not using this codepath?

Copy link
Contributor Author

@tarrinneal tarrinneal Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code path is already after a nil check for nullable enums, so it is irrelevant at this point.
A previous iterration of this code moved the entirety of that logic into this method, but it ended up not working properly, and really didn't match the use of this method in other instances so I relocated it.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say "this code path" do you mean the way it's currently called, rather than something in the method? Because if so, that seems very dangerous; it would be easy to violate that later without even knowing it wasn't intended to work.

If we can't make something self-contained work here, can we instead move this elsewhere, and assert that type isn't an enum here so this isn't a danger?

Copy link
Contributor Author

@tarrinneal tarrinneal Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line of code is all that's needed for non-nullable enums.

Nullable enums need extra code to check for nil before creating the enum from the int that is provided.

I had merged these lines into a single ternary before, but that was causing some other issues. I think I've resolved those now though, so I'll give it another try

Copy link
Contributor Author

@tarrinneal tarrinneal Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this in a really jank way, going to send up a better version tomorrow

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still has the issue that if you call it with a nullable enum it will happily output crashing code.

Copy link
Contributor Author

@tarrinneal tarrinneal Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, I can nest this method inside the other to make it uncallable. It is never used except in the context of _writeDecodeCasting

Copy link
Contributor Author

@tarrinneal tarrinneal Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or this function could just be removed entirely, since it is not really needed any longer.

} else if (type.baseName == 'Object') {
// Special-cased to avoid warnings about using 'as' with Any.
return value;
} else if (type.baseName == 'int') {
final String orString =
type.isNullable ? 'nilOrValue($value)' : '$value as! Int64';
return '($value is Int32) ? Int64($value as! Int32) : $orString';
} else if (type.isNullable) {
return 'nilOrValue($value)';
} else {
Expand Down
28 changes: 14 additions & 14 deletions packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ struct AllTypes {

static func fromList(_ list: [Any]) -> AllTypes? {
let aBool = list[0] as! Bool
let anInt = list[1] as! Int64
let anInt64 = list[2] as! Int64
let anInt = (list[1] is Int32) ? Int64(list[1] as! Int32) : list[1] as! Int64
let anInt64 = (list[2] is Int32) ? Int64(list[2] as! Int32) : list[2] as! Int64
let aDouble = list[3] as! Double
let aByteArray = list[4] as! FlutterStandardTypedData
let a4ByteArray = list[5] as! FlutterStandardTypedData
Expand Down Expand Up @@ -128,8 +128,8 @@ struct AllNullableTypes {

static func fromList(_ list: [Any]) -> AllNullableTypes? {
let aNullableBool: Bool? = nilOrValue(list[0])
let aNullableInt: Int64? = nilOrValue(list[1])
let aNullableInt64: Int64? = nilOrValue(list[2])
let aNullableInt: Int64? = (list[1] is Int32) ? Int64(list[1] as! Int32) : nilOrValue(list[1])
let aNullableInt64: Int64? = (list[2] is Int32) ? Int64(list[2] as! Int32) : nilOrValue(list[2])
let aNullableDouble: Double? = nilOrValue(list[3])
let aNullableByteArray: FlutterStandardTypedData? = nilOrValue(list[4])
let aNullable4ByteArray: FlutterStandardTypedData? = nilOrValue(list[5])
Expand Down Expand Up @@ -483,7 +483,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64 = args[0] as! Int64
let anIntArg: Int64 = (args[0] is Int32) ? Int64(args[0] as! Int32) : args[0] as! Int64
do {
let result = try api.echo(anIntArg)
reply(wrapResult(result))
Expand Down Expand Up @@ -662,7 +662,7 @@ class HostIntegrationCoreApiSetup {
sendMultipleNullableTypesChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let aNullableBoolArg: Bool? = nilOrValue(args[0])
let aNullableIntArg: Int64? = nilOrValue(args[1])
let aNullableIntArg: Int64? = (args[1] is Int32) ? Int64(args[1] as! Int32) : nilOrValue(args[1])
let aNullableStringArg: String? = nilOrValue(args[2])
do {
let result = try api.sendMultipleNullableTypes(aBool: aNullableBoolArg, anInt: aNullableIntArg, aString: aNullableStringArg)
Expand All @@ -679,7 +679,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoNullableIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let aNullableIntArg: Int64? = nilOrValue(args[0])
let aNullableIntArg: Int64? = (args[0] is Int32) ? Int64(args[0] as! Int32) : nilOrValue(args[0])
do {
let result = try api.echo(aNullableIntArg)
reply(wrapResult(result))
Expand Down Expand Up @@ -824,7 +824,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoAsyncIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64 = args[0] as! Int64
let anIntArg: Int64 = (args[0] is Int32) ? Int64(args[0] as! Int32) : args[0] as! Int64
api.echoAsync(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1052,7 +1052,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoAsyncNullableIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64? = nilOrValue(args[0])
let anIntArg: Int64? = (args[0] is Int32) ? Int64(args[0] as! Int32) : nilOrValue(args[0])
api.echoAsyncNullable(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1258,7 +1258,7 @@ class HostIntegrationCoreApiSetup {
callFlutterSendMultipleNullableTypesChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let aNullableBoolArg: Bool? = nilOrValue(args[0])
let aNullableIntArg: Int64? = nilOrValue(args[1])
let aNullableIntArg: Int64? = (args[1] is Int32) ? Int64(args[1] as! Int32) : nilOrValue(args[1])
let aNullableStringArg: String? = nilOrValue(args[2])
api.callFlutterSendMultipleNullableTypes(aBool: aNullableBoolArg, anInt: aNullableIntArg, aString: aNullableStringArg) { result in
switch result {
Expand Down Expand Up @@ -1293,7 +1293,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
callFlutterEchoIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64 = args[0] as! Int64
let anIntArg: Int64 = (args[0] is Int32) ? Int64(args[0] as! Int32) : args[0] as! Int64
api.callFlutterEcho(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1412,7 +1412,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
callFlutterEchoNullableIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64? = nilOrValue(args[0])
let anIntArg: Int64? = (args[0] is Int32) ? Int64(args[0] as! Int32) : nilOrValue(args[0])
api.callFlutterEchoNullable(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1636,7 +1636,7 @@ class FlutterIntegrationCoreApi {
func echo(_ anIntArg: Int64, completion: @escaping (Int64) -> Void) {
let channel = FlutterBasicMessageChannel(name: "dev.flutter.pigeon.FlutterIntegrationCoreApi.echoInt", binaryMessenger: binaryMessenger, codec: codec)
channel.sendMessage([anIntArg] as [Any?]) { response in
let result: Int64 = response as! Int64
let result: Int64 = (response is Int32) ? Int64(response as! Int32) : response as! Int64
completion(result)
}
}
Expand Down Expand Up @@ -1692,7 +1692,7 @@ class FlutterIntegrationCoreApi {
func echoNullable(_ anIntArg: Int64?, completion: @escaping (Int64?) -> Void) {
let channel = FlutterBasicMessageChannel(name: "dev.flutter.pigeon.FlutterIntegrationCoreApi.echoNullableInt", binaryMessenger: binaryMessenger, codec: codec)
channel.sendMessage([anIntArg] as [Any?]) { response in
let result: Int64? = nilOrValue(response)
let result: Int64? = (response is Int32) ? Int64(response as! Int32) : nilOrValue(response)
completion(result)
}
}
Expand Down
28 changes: 14 additions & 14 deletions packages/pigeon/platform_tests/test_plugin/macos/Classes/CoreTests.gen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ struct AllTypes {

static func fromList(_ list: [Any]) -> AllTypes? {
let aBool = list[0] as! Bool
let anInt = list[1] as! Int64
let anInt64 = list[2] as! Int64
let anInt = (list[1] is Int32) ? Int64(list[1] as! Int32) : list[1] as! Int64
let anInt64 = (list[2] is Int32) ? Int64(list[2] as! Int32) : list[2] as! Int64
let aDouble = list[3] as! Double
let aByteArray = list[4] as! FlutterStandardTypedData
let a4ByteArray = list[5] as! FlutterStandardTypedData
Expand Down Expand Up @@ -128,8 +128,8 @@ struct AllNullableTypes {

static func fromList(_ list: [Any]) -> AllNullableTypes? {
let aNullableBool: Bool? = nilOrValue(list[0])
let aNullableInt: Int64? = nilOrValue(list[1])
let aNullableInt64: Int64? = nilOrValue(list[2])
let aNullableInt: Int64? = (list[1] is Int32) ? Int64(list[1] as! Int32) : nilOrValue(list[1])
let aNullableInt64: Int64? = (list[2] is Int32) ? Int64(list[2] as! Int32) : nilOrValue(list[2])
let aNullableDouble: Double? = nilOrValue(list[3])
let aNullableByteArray: FlutterStandardTypedData? = nilOrValue(list[4])
let aNullable4ByteArray: FlutterStandardTypedData? = nilOrValue(list[5])
Expand Down Expand Up @@ -483,7 +483,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64 = args[0] as! Int64
let anIntArg: Int64 = (args[0] is Int32) ? Int64(args[0] as! Int32) : args[0] as! Int64
do {
let result = try api.echo(anIntArg)
reply(wrapResult(result))
Expand Down Expand Up @@ -662,7 +662,7 @@ class HostIntegrationCoreApiSetup {
sendMultipleNullableTypesChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let aNullableBoolArg: Bool? = nilOrValue(args[0])
let aNullableIntArg: Int64? = nilOrValue(args[1])
let aNullableIntArg: Int64? = (args[1] is Int32) ? Int64(args[1] as! Int32) : nilOrValue(args[1])
let aNullableStringArg: String? = nilOrValue(args[2])
do {
let result = try api.sendMultipleNullableTypes(aBool: aNullableBoolArg, anInt: aNullableIntArg, aString: aNullableStringArg)
Expand All @@ -679,7 +679,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoNullableIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let aNullableIntArg: Int64? = nilOrValue(args[0])
let aNullableIntArg: Int64? = (args[0] is Int32) ? Int64(args[0] as! Int32) : nilOrValue(args[0])
do {
let result = try api.echo(aNullableIntArg)
reply(wrapResult(result))
Expand Down Expand Up @@ -824,7 +824,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoAsyncIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64 = args[0] as! Int64
let anIntArg: Int64 = (args[0] is Int32) ? Int64(args[0] as! Int32) : args[0] as! Int64
api.echoAsync(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1052,7 +1052,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
echoAsyncNullableIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64? = nilOrValue(args[0])
let anIntArg: Int64? = (args[0] is Int32) ? Int64(args[0] as! Int32) : nilOrValue(args[0])
api.echoAsyncNullable(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1258,7 +1258,7 @@ class HostIntegrationCoreApiSetup {
callFlutterSendMultipleNullableTypesChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let aNullableBoolArg: Bool? = nilOrValue(args[0])
let aNullableIntArg: Int64? = nilOrValue(args[1])
let aNullableIntArg: Int64? = (args[1] is Int32) ? Int64(args[1] as! Int32) : nilOrValue(args[1])
let aNullableStringArg: String? = nilOrValue(args[2])
api.callFlutterSendMultipleNullableTypes(aBool: aNullableBoolArg, anInt: aNullableIntArg, aString: aNullableStringArg) { result in
switch result {
Expand Down Expand Up @@ -1293,7 +1293,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
callFlutterEchoIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64 = args[0] as! Int64
let anIntArg: Int64 = (args[0] is Int32) ? Int64(args[0] as! Int32) : args[0] as! Int64
api.callFlutterEcho(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1412,7 +1412,7 @@ class HostIntegrationCoreApiSetup {
if let api = api {
callFlutterEchoNullableIntChannel.setMessageHandler { message, reply in
let args = message as! [Any]
let anIntArg: Int64? = nilOrValue(args[0])
let anIntArg: Int64? = (args[0] is Int32) ? Int64(args[0] as! Int32) : nilOrValue(args[0])
api.callFlutterEchoNullable(anIntArg) { result in
switch result {
case .success(let res):
Expand Down Expand Up @@ -1636,7 +1636,7 @@ class FlutterIntegrationCoreApi {
func echo(_ anIntArg: Int64, completion: @escaping (Int64) -> Void) {
let channel = FlutterBasicMessageChannel(name: "dev.flutter.pigeon.FlutterIntegrationCoreApi.echoInt", binaryMessenger: binaryMessenger, codec: codec)
channel.sendMessage([anIntArg] as [Any?]) { response in
let result: Int64 = response as! Int64
let result: Int64 = (response is Int32) ? Int64(response as! Int32) : response as! Int64
completion(result)
}
}
Expand Down Expand Up @@ -1692,7 +1692,7 @@ class FlutterIntegrationCoreApi {
func echoNullable(_ anIntArg: Int64?, completion: @escaping (Int64?) -> Void) {
let channel = FlutterBasicMessageChannel(name: "dev.flutter.pigeon.FlutterIntegrationCoreApi.echoNullableInt", binaryMessenger: binaryMessenger, codec: codec)
channel.sendMessage([anIntArg] as [Any?]) { response in
let result: Int64? = nilOrValue(response)
let result: Int64? = (response is Int32) ? Int64(response as! Int32) : nilOrValue(response)
completion(result)
}
}
Expand Down