Skip to content

[pigeon]fix "as Any" workaround due to nested optional #3658

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
merged 4 commits into from
May 2, 2023
Merged

[pigeon]fix "as Any" workaround due to nested optional #3658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
041219a
[pigeon]fix casting failure due to nested optional
hellohuanlin Apr 6, 2023
8483847
update more args
hellohuanlin Apr 25, 2023
5aa3b4e
fix breaking change
hellohuanlin Apr 28, 2023
89dca42
changelog and version bump
hellohuanlin Apr 28, 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
7 changes: 6 additions & 1 deletion packages/pigeon/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
## NEXT
## 10.0.0

* [swift] Avoids using `Any` to represent `Optional` in Swift.
* [swift] **Breaking Change** A raw `List` (without generic type argument) in Dart will be
translated into `[Any?]` (rather than `[Any]`) in Swift.
* [swift] **Breaking Change** A raw `Map` (without generic type argument) in Dart will be
translated into `[AnyHashable:Any?]` (rather than `[AnyHashable:Any]`) in Swift.
* Adds an example application that uses Pigeon directly, rather than in a plugin.

## 9.2.5
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/lib/generator_tools.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import 'ast.dart';
/// The current version of pigeon.
///
/// This must match the version in pubspec.yaml.
const String pigeonVersion = '9.2.5';
const String pigeonVersion = '10.0.0';

/// Read all the content from [stdin] to a String.
String readStdin() {
Expand Down
19 changes: 9 additions & 10 deletions packages/pigeon/lib/swift_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ import FlutterMacOS
Set<String> customEnumNames,
) {
final String className = klass.name;
indent.write('static func fromList(_ list: [Any]) -> $className? ');
indent.write('static func fromList(_ list: [Any?]) -> $className? ');

indent.addScoped('{', '}', () {
enumerate(getFieldsInSerializationOrder(klass),
Expand Down Expand Up @@ -431,7 +431,7 @@ import FlutterMacOS
indent.addScoped('{ $messageVarName, reply in', '}', () {
final List<String> methodArgument = <String>[];
if (components.arguments.isNotEmpty) {
indent.writeln('let args = message as! [Any]');
indent.writeln('let args = message as! [Any?]');
enumerate(components.arguments,
(int index, _SwiftFunctionArgument arg) {
final String argName =
Expand Down Expand Up @@ -524,7 +524,7 @@ import FlutterMacOS
indent.writeln('case ${customClass.enumeration}:');
indent.nest(1, () {
indent.writeln(
'return ${customClass.name}.fromList(self.readValue() as! [Any])');
'return ${customClass.name}.fromList(self.readValue() as! [Any?])');
});
}
indent.writeln('default:');
Expand Down Expand Up @@ -605,8 +605,7 @@ import FlutterMacOS
'nullable enums require special code that this helper does not supply');
return '${_swiftTypeForDartType(type)}(rawValue: $value as! Int)!';
} else if (type.baseName == 'Object') {
// Special-cased to avoid warnings about using 'as' with Any.
return value;
return value + (type.isNullable ? '' : '!');
Copy link
Contributor Author

@hellohuanlin hellohuanlin Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is to fix a separate bug that silences type mismatch. more details: #3658 (comment)

} else if (type.baseName == 'int') {
if (type.isNullable) {
// Nullable ints need to check for NSNull, and Int32 before casting can be done safely.
Expand All @@ -628,7 +627,7 @@ import FlutterMacOS
if (listEncodedClassNames != null &&
listEncodedClassNames.contains(type.baseName)) {
indent.writeln('var $variableName: $fieldType? = nil');
indent.write('if let ${variableName}List = $value as! [Any]? ');
indent.write('if let ${variableName}List = $value as! [Any?]? ');
indent.addScoped('{', '}', () {
indent.writeln(
'$variableName = $fieldType.fromList(${variableName}List)');
Expand All @@ -652,7 +651,7 @@ import FlutterMacOS
if (listEncodedClassNames != null &&
listEncodedClassNames.contains(type.baseName)) {
indent.writeln(
'let $variableName = $fieldType.fromList($value as! [Any])!');
'let $variableName = $fieldType.fromList($value as! [Any?])!');
} else {
indent.writeln(
'let $variableName = ${castForceUnwrap(value, type, root)}');
Expand Down Expand Up @@ -695,7 +694,7 @@ import FlutterMacOS

private func nilOrValue<T>(_ value: Any?) -> T? {
if value is NSNull { return nil }
return (value as Any) as! T?
return value as! T?
}''');
}

Expand Down Expand Up @@ -739,9 +738,9 @@ String _flattenTypeArguments(List<TypeDeclaration> args) {
String _swiftTypeForBuiltinGenericDartType(TypeDeclaration type) {
if (type.typeArguments.isEmpty) {
if (type.baseName == 'List') {
return '[Any]';
return '[Any?]';
} else if (type.baseName == 'Map') {
return '[AnyHashable: Any]';
return '[AnyHashable: Any?]';
} else {
return 'Any';
Copy link
Contributor Author

@hellohuanlin hellohuanlin Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This else block (and the corresponding else in Line 753) is not used, since the caller only call this function with List/Map types:

  if (type.baseName == 'List' || type.baseName == 'Map') {
    return _swiftTypeForBuiltinGenericDartType(type);
  }

I would either write an assert, or split it into 2 separate helpers (for list and map). But for now I'll leave it here to keep this PR focused (I can update it in separate PR)

}
Expand Down
5 changes: 3 additions & 2 deletions packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/EchoBinaryMessenger.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@ class EchoBinaryMessenger: NSObject, FlutterBinaryMessenger {

guard
let args = self.codec.decode(message) as? [Any?],
let firstArg: Any? = nilOrValue(args.first)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

args.first is another nested optional (Any??) - outer nil represents "there's no first element, or list is empty", and inner nil represents "there is first element, but it is nil".

When passing it to nilOrValue, it gets coerced into Any?, hence the compiler warning in Xcode (Bonus: we should make this an error instead of warning, will do in separate PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is very strange, I would expect the old code and the new code to behave in the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old code worked because it has "as Any" intermediate cast inside nilOrValue function. The new code here avoids implicitly optional completely, so no need the hack anymore.

let firstArg = args.first,
let castedFirstArg: Any? = nilOrValue(firstArg)
else {
callback(self.defaultReturn.flatMap { self.codec.encode($0) })
return
}

callback(self.codec.encode(firstArg))
callback(self.codec.encode(castedFirstArg))
}

func setMessageHandlerOnChannel(
Expand Down
4 changes: 2 additions & 2 deletions packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/PrimitiveTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ class MockPrimitiveHostApi: PrimitiveHostApi {
func aBool(value: Bool) -> Bool { value }
func aString(value: String) -> String { value }
func aDouble(value: Double) -> Double { value }
func aMap(value: [AnyHashable: Any]) -> [AnyHashable: Any] { value }
func aList(value: [Any]) -> [Any] { value }
func aMap(value: [AnyHashable: Any?]) -> [AnyHashable: Any?] { value }
func aList(value: [Any?]) -> [Any?] { value }
func anInt32List(value: FlutterStandardTypedData) -> FlutterStandardTypedData { value }
func aBoolList(value: [Bool?]) -> [Bool?] { value }
func aStringIntMap(value: [String?: Int64?]) -> [String?: Int64?] { value }
Expand Down
Loading