Skip to content
Merged
Show file tree
Hide file tree
Changes from 25 commits
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
4 changes: 4 additions & 0 deletions packages/pigeon/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 9.2.1

* [swift] Fixes NSNull casting crash.

## 9.2.0

* [cpp] Removes experimental tags.
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/lib/generator_tools.dart
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.0';
const String pigeonVersion = '9.2.1';

/// Read all the content from [stdin] to a String.
String readStdin() {
Expand Down
160 changes: 102 additions & 58 deletions packages/pigeon/lib/swift_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -176,44 +176,17 @@ import FlutterMacOS
indent.addScoped('{', '}', () {
enumerate(getFieldsInSerializationOrder(klass),
(int index, final NamedType field) {
final HostDatatype hostDatatype = _getHostDatatype(root, field);

final String listValue = 'list[$index]';
final String fieldType = _swiftTypeForDartType(field.type);

if (field.type.isNullable) {
if (!hostDatatype.isBuiltin &&
customClassNames.contains(field.type.baseName)) {
indent.writeln('var ${field.name}: $fieldType? = nil');
indent.write('if let ${field.name}List = $listValue as! [Any]? ');
indent.addScoped('{', '}', () {
indent.writeln(
'${field.name} = $fieldType.fromList(${field.name}List as [Any])');
});
} else if (!hostDatatype.isBuiltin &&
customEnumNames.contains(field.type.baseName)) {
indent.writeln('var ${field.name}: $fieldType? = nil');
indent.write('if let ${field.name}RawValue = $listValue as! Int? ');
indent.addScoped('{', '}', () {
indent.writeln(
'${field.name} = $fieldType(rawValue: ${field.name}RawValue)');
});
} else {
indent.writeln('let ${field.name} = $listValue as! $fieldType? ');
}
} else {
if (!hostDatatype.isBuiltin &&
customClassNames.contains(field.type.baseName)) {
indent.writeln(
'let ${field.name} = $fieldType.fromList($listValue as! [Any])!');
} else if (!hostDatatype.isBuiltin &&
customEnumNames.contains(field.type.baseName)) {
indent.writeln(
'let ${field.name} = $fieldType(rawValue: $listValue as! Int)!');
} else {
indent.writeln('let ${field.name} = $listValue as! $fieldType');
}
}
_writeDecodeCasting(
root: root,
indent: indent,
value: listValue,
variableName: field.name,
type: field.type,
customClassNames: customClassNames,
customEnumNames: customEnumNames,
);
});

indent.newln();
Expand Down Expand Up @@ -345,8 +318,13 @@ import FlutterMacOS
});
} else {
indent.addScoped('{ response in', '}', () {
indent.writeln(
'let result = ${_castForceUnwrap("response", func.returnType, root)}');
_writeDecodeCasting(
root: root,
indent: indent,
value: 'response',
variableName: 'result',
type: func.returnType,
);
indent.writeln('completion(result)');
});
}
Expand Down Expand Up @@ -461,9 +439,13 @@ import FlutterMacOS
final String argName =
_getSafeArgumentName(index, arg.namedType);
final String argIndex = 'args[$index]';
indent.writeln(
'let $argName = ${_castForceUnwrap(argIndex, arg.type, root)}');

_writeDecodeCasting(
root: root,
indent: indent,
value: argIndex,
variableName: argName,
type: arg.type,
);
if (arg.label == '_') {
methodArgument.add(argName);
} else {
Expand Down Expand Up @@ -607,6 +589,75 @@ import FlutterMacOS
indent.newln();
}

/// Writes decode and casting code for any type.
///
/// Optional parameters are necessary for class decoding only.
Copy link
Collaborator

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

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({
required Root root,
required Indent indent,
required String value,
required String variableName,
required TypeDeclaration type,
Set<String>? customClassNames,
Set<String>? customEnumNames,
}) {
String castForceUnwrap(String value, TypeDeclaration type, Root root) {
if (isEnum(root, type)) {
assert(!type.isNullable,
'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;
} else if (type.baseName == 'int') {
final String orString =
type.isNullable ? 'nilOrValue($value)' : '$value as! Int64';
return '($value is Int32) ? Int64($value as! Int32) : $orString';
Copy link
Collaborator

Choose a reason for hiding this comment

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

It occurs to me that we should reverse the conditional based on what I found about NSNumber; as long as the underlying value actually is an NSNumber, "casting" to Int64 directly will work, so checking for Int32 first (which will also work) will result in two conversions (NSNumber -> Int32 > Int64) instead of one.

So:

final String int64String = type.isNullable ? 'nilOrValue($value)' : '$value as! Int64';
return '($value is Int64) ? $int64String : Int64($value as! Int32);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't work for nullables though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I'm fairly certain it doesn't lower the conversion amount, since casting checking the cast type doesn't perform an actual casting

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I'm fairly certain it doesn't lower the conversion amount, since casting checking the cast type doesn't perform an actual casting

But the cast does. In the version you have in the PR now:

(args[0] is Int32) ? Int64(args[0] as! Int32) : args[0] as! Int64

The flow for a NSNumber (what the codec currently produces) with a value that fits in 32 bits (which is going to be ~all of them in most usage) is, according to the evolution doc and observed behavior:

  • Check is Int32 -> true, because the value can be safely converted to Int32 (despite appearances, this is not actually just doing a type check; NSNumber is not an Int32 or an Int64, but is will return true for it if it can convert).
  • Do as! Int32 -> does a conversion, not a cast (again, despite appearances), creating an Int32 from the NSNumber's value.
  • Do Int64(theAnonymousNewInt32) -> does a conversion from Int32 to Int64, creating an Int64 and discarding the Int32.

What we want is to convert the NSNumber directly to an Int64, which always work.

That doesn't work for nullables though

Ah, right. Maybe we should just make ints fully custom then:

non-nullable: let foo : Int64 = $value is Int64 ? $value as! Int64 : Int64($value as! Int32)
nullable: let foo : Int64? = $value is NSNull ? nil : $value is Int64? ? $value as! Int64? : Int64($value as! Int32)
(Usually I think nested ternaries are a crime against nature, but here it's short and in generated code, and we can put a comment in the Dart explaining the logic, so I think it's okay, and it avoids the mess of generating extra variables.)

} else if (type.isNullable) {
return 'nilOrValue($value)';
} else {
return '$value as! ${_swiftTypeForDartType(type)}';
}
}

final String fieldType = _swiftTypeForDartType(type);

if (type.isNullable) {
if (customClassNames != null &&
customClassNames.contains(type.baseName)) {
indent.writeln('var $variableName: $fieldType? = nil');
indent.write('if let ${variableName}List = $value as! [Any]? ');
indent.addScoped('{', '}', () {
indent.writeln(
'$variableName = $fieldType.fromList(${variableName}List)');
});
} else if (customEnumNames != null &&
customEnumNames.contains(type.baseName)) {
indent.writeln('var $variableName: $fieldType? = nil');
indent.writeln(
'let ${variableName}EnumVal: Int? = ${castForceUnwrap(value, const TypeDeclaration(baseName: 'Int', isNullable: true), root)}');
indent
.write('if let ${variableName}RawValue = ${variableName}EnumVal ');
indent.addScoped('{', '}', () {
indent.writeln(
'$variableName = $fieldType(rawValue: ${variableName}RawValue)!');
});
} else {
indent.writeln(
'let $variableName: $fieldType? = ${castForceUnwrap(value, type, root)}');
}
} else {
if (customClassNames != null &&
customClassNames.contains(type.baseName)) {
indent.writeln(
'let $variableName = $fieldType.fromList($value as! [Any])!');
} else {
indent.writeln(
'let $variableName = ${castForceUnwrap(value, type, root)}');
}
}
}

void _writeWrapResult(Indent indent) {
indent.newln();
indent.write('private func wrapResult(_ result: Any?) -> [Any?] ');
Expand Down Expand Up @@ -637,11 +688,21 @@ import FlutterMacOS
});
}

void _writeNilOrValue(Indent indent) {
indent.format('''

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

Choose a reason for hiding this comment

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

Discussed offline - this is indeed a weird Swift behavior that as Any is required to make it work. I will take a closer look but no need to block.

}''');
}

@override
void writeGeneralUtilities(
SwiftOptions generatorOptions, Root root, Indent indent) {
_writeWrapResult(indent);
_writeWrapError(indent);
_writeNilOrValue(indent);
}
}

Expand All @@ -667,23 +728,6 @@ String _camelCase(String text) {
return pascal[0].toLowerCase() + pascal.substring(1);
}

String _castForceUnwrap(String value, TypeDeclaration type, Root root) {
final String forceUnwrap = type.isNullable ? '' : '!';
final String castUnwrap = type.isNullable ? '?' : '';
if (isEnum(root, type)) {
final String nullableConditionPrefix =
type.isNullable ? '$value == nil ? nil : ' : '';
return '$nullableConditionPrefix${_swiftTypeForDartType(type)}(rawValue: $value as! Int)$forceUnwrap';
} else if (type.baseName == 'Object') {
// Special-cased to avoid warnings about using 'as' with Any.
return value;
} else if (type.baseName == 'int') {
return '($value is Int) ? Int64($value as! Int) : $value as! Int64$castUnwrap';
} else {
return '$value as! ${_swiftTypeForDartType(type)}$castUnwrap';
}
}

/// Converts a [List] of [TypeDeclaration]s to a comma separated [String] to be
/// used in Swift code.
String _flattenTypeArguments(List<TypeDeclaration> args) {
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/mock_handler_tester/test/message.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/mock_handler_tester/test/test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, unnecessary_import
// ignore_for_file: avoid_relative_lib_imports
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon

package com.example.alternate_language_test_plugin;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon

#import <Foundation/Foundation.h>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon

#import "CoreTests.gen.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1255,6 +1255,15 @@ void runPigeonIntegrationTests(TargetGenerator targetGenerator) {
expect(echoObject, sentObject);
});

testWidgets('nullable big ints serialize and deserialize correctly',
(WidgetTester _) async {
final HostIntegrationCoreApi api = HostIntegrationCoreApi();

const int sentObject = _biggerThanBigInt;
final int? echoObject = await api.callFlutterEchoNullableInt(sentObject);
expect(echoObject, sentObject);
});

testWidgets('null ints serialize and deserialize correctly',
(WidgetTester _) async {
final HostIntegrationCoreApi api = HostIntegrationCoreApi();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Autogenerated from Pigeon (v9.2.0), do not edit directly.
// Autogenerated from Pigeon (v9.2.1), do not edit directly.
// See also: https://pub.dev/packages/pigeon

package com.example.test_plugin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ class EchoBinaryMessenger: NSObject, FlutterBinaryMessenger {

guard
let args = self.codec.decode(message) as? [Any?],
let firstArg = args.first,
!(firstArg is NSNull)
let firstArg: Any? = nilOrValue(args.first)
else {
callback(self.defaultReturn.flatMap { self.codec.encode($0) })
return
Expand All @@ -49,4 +48,9 @@ class EchoBinaryMessenger: NSObject, FlutterBinaryMessenger {
func cleanUpConnection(_ connection: FlutterBinaryMessengerConnection) {
// Method not implemented because this messenger is just for echoing
}

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