Skip to content

[pigeon] removes safe casting from nullables in kotlin and swift #3284

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 7 commits into from
Feb 28, 2023
Merged

[pigeon] removes safe casting from nullables in kotlin and swift #3284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f893d7
fails with bool? only
tarrinneal Feb 21, 2023
4331ba1
change most instances of [Any?] to [Any]
tarrinneal Feb 22, 2023
2772e95
kotlin, changelog, generate, format
tarrinneal Feb 23, 2023
a6fe0b3
Merge branch 'main' into unsafe
tarrinneal Feb 23, 2023
1ac0aee
gen
tarrinneal Feb 23, 2023
8d4cba5
remove Any? from maps
tarrinneal Feb 24, 2023
307128f
?
tarrinneal Feb 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
5 changes: 5 additions & 0 deletions packages/pigeon/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 9.0.2

* [swift] Removes safe casting from decode process.
* [kotlin] Removes safe casting from decode process.

## 9.0.1

* Updates links for the merge of flutter/plugins into flutter/packages.
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.0.1';
const String pigeonVersion = '9.0.2';

/// Read all the content from [stdin] to a String.
String readStdin() {
Expand Down
8 changes: 4 additions & 4 deletions packages/pigeon/lib/kotlin_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,23 +228,23 @@ class KotlinGenerator extends StructuredGenerator<KotlinOptions> {
if (!hostDatatype.isBuiltin &&
customClassNames.contains(field.type.baseName)) {
indent.write('val ${field.name}: $fieldType? = ');
indent.add('($listValue as? List<Any?>)?.let ');
indent.add('($listValue as List<Any?>?)?.let ');
indent.addScoped('{', '}', () {
indent.writeln('$fieldType.fromList(it)');
});
} else if (!hostDatatype.isBuiltin &&
customEnumNames.contains(field.type.baseName)) {
indent.write('val ${field.name}: $fieldType? = ');
indent.add('($listValue as? Int)?.let ');
indent.add('($listValue as Int?)?.let ');
indent.addScoped('{', '}', () {
indent.writeln('$fieldType.ofRaw(it)');
});
} else if (isInt) {
indent.write('val ${field.name} = $listValue');
indent.addln(
'.let { if (it is Int) it.toLong() else it as? Long }');
'.let { if (it is Int) it.toLong() else it as Long? }');
} else {
indent.writeln('val ${field.name} = $listValue as? $fieldType');
indent.writeln('val ${field.name} = $listValue as $fieldType?');
}
} else {
if (!hostDatatype.isBuiltin &&
Expand Down
16 changes: 8 additions & 8 deletions packages/pigeon/lib/swift_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,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 All @@ -185,27 +185,27 @@ import FlutterMacOS
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.write('if let ${field.name}List = $listValue as! [Any]? ');
indent.addScoped('{', '}', () {
indent.writeln(
'${field.name} = $fieldType.fromList(${field.name}List)');
'${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.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 ');
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?])!');
'let ${field.name} = $fieldType.fromList($listValue as! [Any])!');
} else if (!hostDatatype.isBuiltin &&
customEnumNames.contains(field.type.baseName)) {
indent.writeln(
Expand Down Expand Up @@ -691,9 +691,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';
}
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/mock_handler_tester/test/message.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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
2 changes: 1 addition & 1 deletion ...st_plugin/android/src/main/java/com/example/alternate_language_test_plugin/CoreTests.java
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), do not edit directly.
// See also: https://pub.dev/packages/pigeon

package com.example.alternate_language_test_plugin;
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/platform_tests/alternate_language_test_plugin/ios/Classes/CoreTests.gen.h
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), do not edit directly.
// See also: https://pub.dev/packages/pigeon

#import <Foundation/Foundation.h>
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/platform_tests/alternate_language_test_plugin/ios/Classes/CoreTests.gen.m
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), do not edit directly.
// See also: https://pub.dev/packages/pigeon

#import "CoreTests.gen.h"
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/core_tests.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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/platform_tests/flutter_null_safe_unit_tests/lib/flutter_unittests.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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/platform_tests/flutter_null_safe_unit_tests/lib/multiple_arity.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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/platform_tests/flutter_null_safe_unit_tests/lib/non_null_fields.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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/platform_tests/flutter_null_safe_unit_tests/lib/null_fields.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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/platform_tests/flutter_null_safe_unit_tests/lib/nullable_returns.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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/platform_tests/flutter_null_safe_unit_tests/lib/primitive.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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/platform_tests/shared_test_plugin_code/lib/src/generated/core_tests.gen.dart
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), 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
32 changes: 16 additions & 16 deletions ...atform_tests/test_plugin/android/src/main/kotlin/com/example/test_plugin/CoreTests.gen.kt
View file
Open in desktop
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.0.1), do not edit directly.
// Autogenerated from Pigeon (v9.0.2), do not edit directly.
// See also: https://pub.dev/packages/pigeon

package com.example.test_plugin
Expand Down Expand Up @@ -109,22 +109,22 @@ data class AllNullableTypes (
companion object {
@Suppress("UNCHECKED_CAST")
fun fromList(list: List<Any?>): AllNullableTypes {
val aNullableBool = list[0] as? Boolean
val aNullableInt = list[1].let { if (it is Int) it.toLong() else it as? Long }
val aNullableDouble = list[2] as? Double
val aNullableByteArray = list[3] as? ByteArray
val aNullable4ByteArray = list[4] as? IntArray
val aNullable8ByteArray = list[5] as? LongArray
val aNullableFloatArray = list[6] as? DoubleArray
val aNullableList = list[7] as? List<Any?>
val aNullableMap = list[8] as? Map<Any, Any?>
val nullableNestedList = list[9] as? List<List<Boolean?>?>
val nullableMapWithAnnotations = list[10] as? Map<String?, String?>
val nullableMapWithObject = list[11] as? Map<String?, Any?>
val aNullableEnum: AnEnum? = (list[12] as? Int)?.let {
val aNullableBool = list[0] as Boolean?
val aNullableInt = list[1].let { if (it is Int) it.toLong() else it as Long? }
val aNullableDouble = list[2] as Double?
val aNullableByteArray = list[3] as ByteArray?
val aNullable4ByteArray = list[4] as IntArray?
val aNullable8ByteArray = list[5] as LongArray?
val aNullableFloatArray = list[6] as DoubleArray?
val aNullableList = list[7] as List<Any?>?
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm curious - in Kotlin, can Any already represent a nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope

val aNullableMap = list[8] as Map<Any, Any?>?
val nullableNestedList = list[9] as List<List<Boolean?>?>?
val nullableMapWithAnnotations = list[10] as Map<String?, String?>?
val nullableMapWithObject = list[11] as Map<String?, Any?>?
val aNullableEnum: AnEnum? = (list[12] as Int?)?.let {
AnEnum.ofRaw(it)
}
val aNullableString = list[13] as? String
val aNullableString = list[13] as String?
return AllNullableTypes(aNullableBool, aNullableInt, aNullableDouble, aNullableByteArray, aNullable4ByteArray, aNullable8ByteArray, aNullableFloatArray, aNullableList, aNullableMap, nullableNestedList, nullableMapWithAnnotations, nullableMapWithObject, aNullableEnum, aNullableString)
}
}
Expand Down Expand Up @@ -179,7 +179,7 @@ data class TestMessage (
companion object {
@Suppress("UNCHECKED_CAST")
fun fromList(list: List<Any?>): TestMessage {
val testList = list[0] as? List<Any?>
val testList = list[0] as List<Any?>?
Copy link
Contributor

Choose a reason for hiding this comment

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

The kotlin version is still List<Any?>, but Swift changed from [Any?] to [Any]. Is it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swift optional Any causes issues with force casting into optional types when the data is nil. Kotlin doesn't have that problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Swift optional Any causes issues with force casting into optional types when the data is nil.

Not sure if I follow this. Can you share a short code snippet when it fails? Are you talking about this following scenario?

let list: [Any?] = [nil, 1, "Hi"]
let optionalBool = list[0] as! Bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the scenario that was happening before. The only difference now is
list: [Any]

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. This scenario actually did not crash when I try it on the playground.

Copy link
Contributor

Choose a reason for hiding this comment

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

The kotlin version is still List<Any?>, but Swift changed from [Any?] to [Any]. Is it intended?

The core answer here is that Any is fundamentally different between the two languages: Any is a root type in Swift, but not Kotlin.

This Kotlin code:

val i: Int? = null
println(i is Any)
println(i is Any?)

prints

false
true

but this Swift code:

var i: Int? = nil
print(i is Any)
print(i is Any?)

prints

true
true

We're generating different code because the expression of "any type, including nullable types" is different in the two languages.

(I wasn't aware of this difference when reviewing the initial generator code, so didn't catch the incorrect use of Any? in Swift in the first place.)

return TestMessage(testList)
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/AllDatatypesTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class AllDatatypesTests: XCTestCase {

func testAllEquals() throws {
let everything = AllNullableTypes(
aNullableBool: false,
aNullableBool: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

were you able to resolve the Bool type issue the other day? What was the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switching from Any? to Any

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide more details? Not sure which Any? you are referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the same decode issue as the other comments I've replied to

aNullableInt: 1,
aNullableDouble: 2.0,
aNullableByteArray: FlutterStandardTypedData(bytes: "1234".data(using: .utf8)!),
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?: Int32?]) -> [String?: Int32?] { value }
Expand Down
Loading