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

tarrinneal · 2023-02-23T19:12:45Z

Removes safe casting during decode process for kotlin and swift to avoid hiding potential data type inconsistencies across channels.

fixes flutter/flutter#116999

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

hellohuanlin · 2023-02-23T19:37:55Z

packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift

    var aNullableEnum: AnEnum? = nil
-    if let aNullableEnumRawValue = list[12] as? Int {
+    if let aNullableEnumRawValue = list[12] as! Int? {
      aNullableEnum = AnEnum(rawValue: aNullableEnumRawValue)
    }


you can avoid the var by either

let aNullableEnum: AnEnum? if ... { aNullableEnum = ... } else { aNullableEnum = nil }

or using a map:

let aNullableEnum = (list[12] as! Int?).map { AnEnum(rawValue: $0) }

hellohuanlin · 2023-02-23T19:39:17Z

packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/AllDatatypesTests.swift

@@ -37,7 +37,7 @@ class AllDatatypesTests: XCTestCase {

  func testAllEquals() throws {
    let everything = AllNullableTypes(
-      aNullableBool: false,
+      aNullableBool: true,


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

switching from Any? to Any

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

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

hellohuanlin · 2023-02-23T19:40:04Z

packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift

    let aBool = list[0] as! Bool
    let anInt = list[1] as! Int32
    let aDouble = list[2] as! Double
    let aByteArray = list[3] as! FlutterStandardTypedData
    let a4ByteArray = list[4] as! FlutterStandardTypedData
    let a8ByteArray = list[5] as! FlutterStandardTypedData
    let aFloatArray = list[6] as! FlutterStandardTypedData
-    let aList = list[7] as! [Any?]
+    let aList = list[7] as! [Any]


do we want to test out both [Any?] and [Any]

What do you mean? I'm not sure why we would need Any? for anything in this context since Any can already be nil

Oh interesting - what about the maps below? Do you also want to remove the ?

e.g.

var aNullableMap: [AnyHashable: Any?]? = nil var nullableMapWithObject: [String?: Any?]? = nil

probably a good idea

I'm not sure why we would need Any? for anything in this context since Any can already be nil

~~Actually, Any cannot represent nil in Swift:~~

Hmmm, if I provide concrete type of nil then it works. I think it's fine to keep Any then. Though I am not sure how this is related to the Bool? crash you had the other day. May be helpful if you can provide a minimal reproducible code snippet so I can try out in playground.

The Bool? crash was nil attempting to be cast to Bool?, it failed for the reasons mentioned here. I'm not sure how to replicate it without a lot of extra steps.

hellohuanlin · 2023-02-23T19:44:35Z

.../platform_tests/test_plugin/android/src/main/kotlin/com/example/test_plugin/CoreTests.gen.kt

@@ -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?>?


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

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

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?

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

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

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.)

hellohuanlin · 2023-02-24T00:29:03Z

.../platform_tests/test_plugin/android/src/main/kotlin/com/example/test_plugin/CoreTests.gen.kt

+      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?>?


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

hellohuanlin

The PR itself LGTM. Though I am still very curious why nil would fail to be casted to Bool? since my code snippet worked. I would need a minimal reproducible example to look into, but looks like you cannot create such example easily...

packages/pigeon/lib/kotlin_generator.dart

packages/pigeon/lib/swift_generator.dart

stuartmorgan-g · 2023-02-27T15:24:56Z

.../platform_tests/test_plugin/android/src/main/kotlin/com/example/test_plugin/CoreTests.gen.kt

@@ -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?>?


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.)

stuartmorgan-g

LGTM!

…ift (flutter/packages#3284)

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

tarrinneal added 3 commits February 21, 2023 15:44

fails with bool? only

1f893d7

change most instances of [Any?] to [Any]

4331ba1

kotlin, changelog, generate, format

2772e95

github-actions bot added p: pigeon platform-android platform-ios platform-macos platform-windows labels Feb 23, 2023

Merge branch 'main' into unsafe

a6fe0b3

github-actions bot removed the platform-windows label Feb 23, 2023

gen

1ac0aee

github-actions bot added the platform-windows label Feb 23, 2023

tarrinneal requested review from stuartmorgan-g and hellohuanlin February 23, 2023 19:30

gmackall requested a review from reidbaker February 23, 2023 19:37

hellohuanlin reviewed Feb 23, 2023

View reviewed changes

hellohuanlin reviewed Feb 24, 2023

View reviewed changes

remove Any? from maps

8d4cba5

hellohuanlin approved these changes Feb 24, 2023

View reviewed changes

stuartmorgan-g reviewed Feb 27, 2023

View reviewed changes

reidbaker approved these changes Feb 27, 2023

View reviewed changes

?

307128f

tarrinneal requested a review from stuartmorgan-g February 28, 2023 03:22

stuartmorgan-g approved these changes Feb 28, 2023

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2023

auto-submit bot merged commit e20c1b7 into flutter:main Feb 28, 2023

stuartmorgan-g mentioned this pull request Feb 28, 2023

[pigeon] Enable warnings as errors for remaining languages #3317

Merged

11 tasks

engine-flutter-autoroll mentioned this pull request Mar 1, 2023

Roll Packages from f39c97dbfb95 to 09544999b221 (8 revisions) flutter/flutter#121695

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2023

e20c1b7 [pigeon] removes safe casting from nullables in kotlin and sw…

a56b2d3

…ift (flutter/packages#3284)

reidbaker mentioned this pull request Mar 20, 2023

[Many] i123017 integration annotation documentation #3495

Merged

10 tasks

hellohuanlin mentioned this pull request Mar 28, 2023

[pigeon] The generated Swift object crashes during parsing. flutter/flutter#123387

Closed

stuartmorgan-g mentioned this pull request Apr 7, 2023

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

Merged

11 tasks

nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023

[pigeon] removes safe casting from nullables in kotlin and swift (flu…

1ecebea

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

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

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

Uh oh!

Conversation

tarrinneal commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tarrinneal commented Feb 23, 2023 •

edited

Loading

hellohuanlin Feb 24, 2023 •

edited

Loading