[pigeon] fix swift nsnull casting crash #3545

tarrinneal · 2023-03-24T22:19:14Z

fixes flutter/flutter#123387 by forcing NSNull to nil
also simplifies Int casting, since it was overly complex

Pre-launch Checklist

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

stuartmorgan-g · 2023-03-27T15:00:03Z

Are you able to repro this from the pigeon file in the issue? If not we should figure out how to get an integration test for it first before we try to fix it.

packages/pigeon/lib/swift_generator.dart

stuartmorgan-g · 2023-03-30T19:18:42Z

packages/pigeon/lib/swift_generator.dart

-    final String nullableConditionPrefix =
-        type.isNullable ? '$value == nil ? nil : ' : '';
-    return '$nullableConditionPrefix${_swiftTypeForDartType(type)}(rawValue: $value as! Int)$forceUnwrap';
+    return '${_swiftTypeForDartType(type)}(rawValue: $value as! Int)!';


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?

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.

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?

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

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

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

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

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

packages/pigeon/lib/swift_generator.dart

hellohuanlin

~~I think~~

oops typo - added generic function example in separate comment. the generic function should allow re-using the casting part

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

hellohuanlin · 2023-03-30T19:28:15Z

I think you can do a generic version like this:

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

Then in the caller you can infer the type like this:

let bool: Bool? = nilOrValue(value: list[0])

tarrinneal · 2023-03-30T19:45:04Z

I think you can do a generic version like this:
  private func nilOrValue<T>(value: Any) -> T? {
    if value is NSNull {
      return nil
    }
    return value as! T?
  }
Then in the caller you can infer the type like this:
let bool: Bool? = nilOrValue(value: list[0])

I'm fairly certain I tried this exact code and it wouldn't work. I'll give it another attempt though for good measure!

hellohuanlin · 2023-03-30T20:00:20Z

I'm fairly certain I tried this exact code and it wouldn't work. I'll give it another attempt though for good measure!

In case it's helpful, you can try out this code snippet in Swift playground:

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

let nilInt: Int? = nil
let nonNilInt: Int? = 123
let list: [Any] = [NSNull(), 1, "hi", nilInt, nonNilInt]
let bool: Bool? = nilOrValue(value: list[0])
let int: Int? = nilOrValue(value: list[1])
let str: String? = nilOrValue(value: list[2])
let int2: Int? = nilOrValue(value: list[3])
let int3: Int? = nilOrValue(value: list[4])

print(bool)
print(int)
print(str)
print(int2)
print(int3)

tarrinneal · 2023-03-30T21:02:45Z

I'm fairly certain I tried this exact code and it wouldn't work. I'll give it another attempt though for good measure!

In case it's helpful, you can try out this code snippet in Swift playground:

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

let nilInt: Int? = nil
let nonNilInt: Int? = 123
let list: [Any] = [NSNull(), 1, "hi", nilInt, nonNilInt]
let bool: Bool? = nilOrValue(value: list[0])
let int: Int? = nilOrValue(value: list[1])
let str: String? = nilOrValue(value: list[2])
let int2: Int? = nilOrValue(value: list[3])
let int3: Int? = nilOrValue(value: list[4])

print(bool)
print(int)
print(str)
print(int2)
print(int3)

I just get a lot of this
Swift Compiler Error (Xcode): Generic parameter 'T' could not be inferred /Users/tarrinneal/work/packages/packages/pigeon/platform_tests/test_plugin/ios/Classes/NullFields.gen.swift:55:25
when I try that.

tarrinneal · 2023-03-30T21:04:15Z

I just get a lot of this Swift Compiler Error (Xcode): Generic parameter 'T' could not be inferred /Users/tarrinneal/work/packages/packages/pigeon/platform_tests/test_plugin/ios/Classes/NullFields.gen.swift:55:25 when I try that.

static func fromList(_ list: [Any]) -> NullFieldsSearchRequest? {
    let query: String? = nilOrValue(value: list[0]) as! String? 
    let identifier = list[1] as! Int64

    return NullFieldsSearchRequest(
      query: query,
      identifier: identifier
    )
  }

static func fromList(_ list: [Any]) -> NullFieldsSearchReply? {
    let result: String? = nilOrValue(value: list[0]) as! String? 
    let error: String? = nilOrValue(value: list[1]) as! String? 
    let indices: [Int64?]? = nilOrValue(value: list[2]) as! [Int64?]? 
    var request: NullFieldsSearchRequest? = nil
    if let requestList = list[3] as! [Any]? {
      request = NullFieldsSearchRequest.fromList(requestList)
    }
    var type: NullFieldsSearchReplyType? = nil
    let enumVal4: Int? = nilOrValue(value: list[4]) as! Int?
    if let typeRawValue = enumVal4 {
      type = NullFieldsSearchReplyType(rawValue: typeRawValue)
    }

    return NullFieldsSearchReply(
      result: result,
      error: error,
      indices: indices,
      request: request,
      type: type
    )
  }

hellohuanlin · 2023-03-30T21:19:00Z

let query: String? = nilOrValue(value: list[0]) as! String?

You have to remove as! String? here (see my original example code)

stuartmorgan-g

LGTM with a couple of nits.

stuartmorgan-g · 2023-04-04T02:20:12Z

packages/pigeon/lib/swift_generator.dart

+      } else if (type.baseName == 'int') {
+        final String orString =
+            type.isNullable ? 'nilOrValue($value)' : '$value as! Int64';
+        return '($value is Int32) ? Int64($value as! Int32) : $orString';


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

That doesn't work for nullables though

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

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

stuartmorgan-g · 2023-04-04T02:30:08Z

packages/pigeon/lib/swift_generator.dart

@@ -607,6 +589,75 @@ import FlutterMacOS
    indent.newln();
  }

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


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

I renamed them. And made the comment more specific.

packages/pigeon/lib/swift_generator.dart

stuartmorgan-g

LGTM!

Let's land it quick before we find any more unusual is or as behavior 😁

auto-submit · 2023-04-04T18:27:03Z

auto label is removed for flutter/packages, pr: 3545, due to This PR has not met approval requirements for merging. Changes were requested by {stuartmorgan}, please make the needed changes and resubmit this PR.
You have project association MEMBER and need 1 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already a MEMBER or two member reviews if you are not a MEMBER before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

stuartmorgan-g · 2023-04-04T18:29:40Z

Hm, this looks like flutter/flutter#99033 happening again. @godofredoc @CaseyHillers Any idea why this would be happening again?

tarrinneal · 2023-04-04T18:34:31Z

My guess is that it is related to how the pr was approved, then changes requested, then approved again.

tarrinneal · 2023-04-04T18:38:26Z

LGTM!

Let's land it quick before we find any more unusual is or as behavior 😁

The universe is against allowing this pr to ever land

stuartmorgan-g · 2023-04-04T19:17:38Z

The universe is against allowing this pr to ever land

You can land it manually once the tests runs have all finished; hopefully the autosubmit bug can be investigated after the fact (and if not, getting this particular fix in trumps the autosubmit issue in importance).

## The problem This PR fixes a weird casting behavior discussed [here](#3545 (comment)): ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } return (value as Any) as! T? // <- HERE } ``` Without this intermediate `as Any` cast, [these 3 tests](https://github.com/flutter/packages/blob/5662a7e5799c723f76e9589a75c9d9310e2ba8c1/packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/RunnerTests.swift#L10-L29) would crash with `SIGABRT: Could not cast value of type 'Swift.Optional<Any>' (0x7ff865e84b38) to 'Swift.String' (0x7ff865e7de08).` ## Investigation The crash happens because `value` here is actually of type `Any??` (nested optional!). When it crashes, the debugger simply shows `value` is `nil`. But if we print in `lldb`, the `value` here is actually an inner `Optional.none` case nested by an outer `Optional.some` case. ### Why does `Any??` crash Since outer case is `some`, it fails to force cast to `T?` (e.g. `String?`) due to type mismatch. ### How did we end up with `Any??` It's related to the signature of these 3 functions: - `func toList() -> [Any?]` - `func fromList(args: [Any])` - `func nilOrValue<T>(_ value: Any?) -> T?` Firstly `toList` returns `nil` (of type `Any?`) as the first element of array. Then the type gets coerced as an `Any` type in `fromList`. Then because `nilOrValue` takes `Any?`, this `nil` value gets wrapped by an `Optional.some`. Hence the nested optional. ## Workarounds ### Workaround 1: `as Any` This is the current code [in this PR](https://github.com/flutter/packages/pull/3545/files#r1155061282). When casting `Optional.some(nil) as Any`, it erases the outer Optional, so no problem casting to `T?`. ### Workaround 2: Handle with nested optional directly: ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } // `if let` deals with "outer some" case and then erase the outer Optional if let val = value { // here returns "outer some + inner some" or "outer some + inner none" return val as! T? } // here returns "outer none" return nil } ``` A similar version of this was also [attempted in this PR](https://github.com/flutter/packages/pull/3545/files/241f0e31e32917f5501dab11f81ab0fbf064687f#diff-bfdb6a91beb03a906435e77e0168117f3f3977ee4d6f8bcaa1724156ae4dc27cR647-R650). It just that we did not know why that worked previously, and now we know! ### Workaround 3 Casting value to nested optional (`T??`), then stripe the outer optional ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } return (value as! T??) ?? nil } ``` ## Solutions These above workarounds handle nested optionals. However, **a real solution should prevent nested optionals from happening in the first place**, since they are so tricky. ### Solution 1 (This PR) The nested optional happens when we do cast from `Any?` to `Any` and then wrapped into `Any?`. (Refer to "How did we end up with Any??" section). So the easiest way is just to use `func fromList(args: [Any?])` to match the types of `func toList` and `func nilOrValue`. ### Solution 2 Solution 2 is the opposite - avoid using `Any?` as much as possible. Drawbacks compare to Solution 1: a. When inter-op with ObjC, `nullable id` is exported as `Any?`. So we can't 100% prevent `Any?` usage. Though this can be addressed by immediately cast it to `Any`. b. Losing of semantic meaning of `Any?` that it <s>can</s> must be optional. The hidden/implicit optional **is** the culprit here in the first place. c. While this solution fixes the nested optional issue, it does not address all issues related to implicit optional. For example: https://github.com/flutter/packages/blob/c53db71f496b436e48629a8f3e4152c48e63cd66/packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift#L563-L564 This is supposed to crash if `args[0]` is `nil`. However, the crash is silenced because `as! [Any]` will make `args[0]` an implicit optional! The correct codegen should instead be: ``` let args = message as! [Any?] let anObjectArg = args[0]! ``` ### Solution 3 Just remove `as Any` and update the test. The nested optional won't happen in production code, because ObjC `NSArray` contains `NSNull` rather than `nil` when exporting to Swift. We can simply fix [the tests](https://github.com/flutter/packages/blob/5662a7e5799c723f76e9589a75c9d9310e2ba8c1/packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/RunnerTests.swift#L10-L29) by replacing `nil`s with `NSNull`s. However, if we were to re-write engine's codec to Swift, it's actually better practice to use `nil` and not `NSNull` in the array. ## Additional TODO We would've caught this earlier if this were an error rather than warning in our unit test. ![Screenshot 2023-04-06 at 4 15 07 PM](https://user-images.githubusercontent.com/41930132/230510477-1505f830-2fc5-4a4d-858f-e658729fa7bf.png) *List which issues are fixed by this PR. You must list at least one issue.* #3545 (comment) *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

## The problem This PR fixes a weird casting behavior discussed [here](flutter#3545 (comment)): ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } return (value as Any) as! T? // <- HERE } ``` Without this intermediate `as Any` cast, [these 3 tests](https://github.com/flutter/packages/blob/5662a7e5799c723f76e9589a75c9d9310e2ba8c1/packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/RunnerTests.swift#L10-L29) would crash with `SIGABRT: Could not cast value of type 'Swift.Optional<Any>' (0x7ff865e84b38) to 'Swift.String' (0x7ff865e7de08).` ## Investigation The crash happens because `value` here is actually of type `Any??` (nested optional!). When it crashes, the debugger simply shows `value` is `nil`. But if we print in `lldb`, the `value` here is actually an inner `Optional.none` case nested by an outer `Optional.some` case. ### Why does `Any??` crash Since outer case is `some`, it fails to force cast to `T?` (e.g. `String?`) due to type mismatch. ### How did we end up with `Any??` It's related to the signature of these 3 functions: - `func toList() -> [Any?]` - `func fromList(args: [Any])` - `func nilOrValue<T>(_ value: Any?) -> T?` Firstly `toList` returns `nil` (of type `Any?`) as the first element of array. Then the type gets coerced as an `Any` type in `fromList`. Then because `nilOrValue` takes `Any?`, this `nil` value gets wrapped by an `Optional.some`. Hence the nested optional. ## Workarounds ### Workaround 1: `as Any` This is the current code [in this PR](https://github.com/flutter/packages/pull/3545/files#r1155061282). When casting `Optional.some(nil) as Any`, it erases the outer Optional, so no problem casting to `T?`. ### Workaround 2: Handle with nested optional directly: ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } // `if let` deals with "outer some" case and then erase the outer Optional if let val = value { // here returns "outer some + inner some" or "outer some + inner none" return val as! T? } // here returns "outer none" return nil } ``` A similar version of this was also [attempted in this PR](https://github.com/flutter/packages/pull/3545/files/241f0e31e32917f5501dab11f81ab0fbf064687f#diff-bfdb6a91beb03a906435e77e0168117f3f3977ee4d6f8bcaa1724156ae4dc27cR647-R650). It just that we did not know why that worked previously, and now we know! ### Workaround 3 Casting value to nested optional (`T??`), then stripe the outer optional ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } return (value as! T??) ?? nil } ``` ## Solutions These above workarounds handle nested optionals. However, **a real solution should prevent nested optionals from happening in the first place**, since they are so tricky. ### Solution 1 (This PR) The nested optional happens when we do cast from `Any?` to `Any` and then wrapped into `Any?`. (Refer to "How did we end up with Any??" section). So the easiest way is just to use `func fromList(args: [Any?])` to match the types of `func toList` and `func nilOrValue`. ### Solution 2 Solution 2 is the opposite - avoid using `Any?` as much as possible. Drawbacks compare to Solution 1: a. When inter-op with ObjC, `nullable id` is exported as `Any?`. So we can't 100% prevent `Any?` usage. Though this can be addressed by immediately cast it to `Any`. b. Losing of semantic meaning of `Any?` that it <s>can</s> must be optional. The hidden/implicit optional **is** the culprit here in the first place. c. While this solution fixes the nested optional issue, it does not address all issues related to implicit optional. For example: https://github.com/flutter/packages/blob/c53db71f496b436e48629a8f3e4152c48e63cd66/packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift#L563-L564 This is supposed to crash if `args[0]` is `nil`. However, the crash is silenced because `as! [Any]` will make `args[0]` an implicit optional! The correct codegen should instead be: ``` let args = message as! [Any?] let anObjectArg = args[0]! ``` ### Solution 3 Just remove `as Any` and update the test. The nested optional won't happen in production code, because ObjC `NSArray` contains `NSNull` rather than `nil` when exporting to Swift. We can simply fix [the tests](https://github.com/flutter/packages/blob/5662a7e5799c723f76e9589a75c9d9310e2ba8c1/packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/RunnerTests.swift#L10-L29) by replacing `nil`s with `NSNull`s. However, if we were to re-write engine's codec to Swift, it's actually better practice to use `nil` and not `NSNull` in the array. ## Additional TODO We would've caught this earlier if this were an error rather than warning in our unit test. ![Screenshot 2023-04-06 at 4 15 07 PM](https://user-images.githubusercontent.com/41930132/230510477-1505f830-2fc5-4a4d-858f-e658729fa7bf.png) *List which issues are fixed by this PR. You must list at least one issue.* flutter#3545 (comment) *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

fixes flutter/flutter#123387 by forcing NSNull to nil also simplifies Int casting, since it was overly complex ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [x] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates [following repository CHANGELOG style]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests

## The problem This PR fixes a weird casting behavior discussed [here](flutter#3545 (comment)): ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } return (value as Any) as! T? // <- HERE } ``` Without this intermediate `as Any` cast, [these 3 tests](https://github.com/flutter/packages/blob/5662a7e5799c723f76e9589a75c9d9310e2ba8c1/packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/RunnerTests.swift#L10-L29) would crash with `SIGABRT: Could not cast value of type 'Swift.Optional<Any>' (0x7ff865e84b38) to 'Swift.String' (0x7ff865e7de08).` ## Investigation The crash happens because `value` here is actually of type `Any??` (nested optional!). When it crashes, the debugger simply shows `value` is `nil`. But if we print in `lldb`, the `value` here is actually an inner `Optional.none` case nested by an outer `Optional.some` case. ### Why does `Any??` crash Since outer case is `some`, it fails to force cast to `T?` (e.g. `String?`) due to type mismatch. ### How did we end up with `Any??` It's related to the signature of these 3 functions: - `func toList() -> [Any?]` - `func fromList(args: [Any])` - `func nilOrValue<T>(_ value: Any?) -> T?` Firstly `toList` returns `nil` (of type `Any?`) as the first element of array. Then the type gets coerced as an `Any` type in `fromList`. Then because `nilOrValue` takes `Any?`, this `nil` value gets wrapped by an `Optional.some`. Hence the nested optional. ## Workarounds ### Workaround 1: `as Any` This is the current code [in this PR](https://github.com/flutter/packages/pull/3545/files#r1155061282). When casting `Optional.some(nil) as Any`, it erases the outer Optional, so no problem casting to `T?`. ### Workaround 2: Handle with nested optional directly: ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } // `if let` deals with "outer some" case and then erase the outer Optional if let val = value { // here returns "outer some + inner some" or "outer some + inner none" return val as! T? } // here returns "outer none" return nil } ``` A similar version of this was also [attempted in this PR](https://github.com/flutter/packages/pull/3545/files/241f0e31e32917f5501dab11f81ab0fbf064687f#diff-bfdb6a91beb03a906435e77e0168117f3f3977ee4d6f8bcaa1724156ae4dc27cR647-R650). It just that we did not know why that worked previously, and now we know! ### Workaround 3 Casting value to nested optional (`T??`), then stripe the outer optional ``` private func nilOrValue<T>(_ value: Any?) -> T? { if value is NSNull { return nil } return (value as! T??) ?? nil } ``` ## Solutions These above workarounds handle nested optionals. However, **a real solution should prevent nested optionals from happening in the first place**, since they are so tricky. ### Solution 1 (This PR) The nested optional happens when we do cast from `Any?` to `Any` and then wrapped into `Any?`. (Refer to "How did we end up with Any??" section). So the easiest way is just to use `func fromList(args: [Any?])` to match the types of `func toList` and `func nilOrValue`. ### Solution 2 Solution 2 is the opposite - avoid using `Any?` as much as possible. Drawbacks compare to Solution 1: a. When inter-op with ObjC, `nullable id` is exported as `Any?`. So we can't 100% prevent `Any?` usage. Though this can be addressed by immediately cast it to `Any`. b. Losing of semantic meaning of `Any?` that it <s>can</s> must be optional. The hidden/implicit optional **is** the culprit here in the first place. c. While this solution fixes the nested optional issue, it does not address all issues related to implicit optional. For example: https://github.com/flutter/packages/blob/c53db71f496b436e48629a8f3e4152c48e63cd66/packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift#L563-L564 This is supposed to crash if `args[0]` is `nil`. However, the crash is silenced because `as! [Any]` will make `args[0]` an implicit optional! The correct codegen should instead be: ``` let args = message as! [Any?] let anObjectArg = args[0]! ``` ### Solution 3 Just remove `as Any` and update the test. The nested optional won't happen in production code, because ObjC `NSArray` contains `NSNull` rather than `nil` when exporting to Swift. We can simply fix [the tests](https://github.com/flutter/packages/blob/5662a7e5799c723f76e9589a75c9d9310e2ba8c1/packages/pigeon/platform_tests/test_plugin/example/ios/RunnerTests/RunnerTests.swift#L10-L29) by replacing `nil`s with `NSNull`s. However, if we were to re-write engine's codec to Swift, it's actually better practice to use `nil` and not `NSNull` in the array. ## Additional TODO We would've caught this earlier if this were an error rather than warning in our unit test. ![Screenshot 2023-04-06 at 4 15 07 PM](https://user-images.githubusercontent.com/41930132/230510477-1505f830-2fc5-4a4d-858f-e658729fa7bf.png) *List which issues are fixed by this PR. You must list at least one issue.* flutter#3545 (comment) *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

tarrinneal requested a review from stuartmorgan-g March 24, 2023 22:19

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

tarrinneal mentioned this pull request Mar 24, 2023

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

Closed

crash fix

574ee0b

tarrinneal force-pushed the swift-nil branch from db721c0 to 574ee0b Compare March 24, 2023 23:31

remove need for function

46ca75e

tarrinneal force-pushed the swift-nil branch from a4a8315 to 46ca75e Compare March 25, 2023 03:47

stuartmorgan-g requested changes Mar 29, 2023

View reviewed changes

packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved

packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved

cleaner method method

e350040

tarrinneal requested a review from stuartmorgan-g March 29, 2023 22:59

tarrinneal added 3 commits March 29, 2023 16:00

better name

3d31aff

fix enums and tests

3be7eb1

enum fixes, and Any casting

ef111ee

tarrinneal force-pushed the swift-nil branch from 313c849 to ef111ee Compare March 30, 2023 04:20

tarrinneal added 3 commits March 29, 2023 21:23

gen test

6d5271f

Merge branch 'main' of github.com:flutter/packages into swift-nil

a4625f8

simplify casting enum

29ac0ec

stuartmorgan-g reviewed Mar 30, 2023

View reviewed changes

hellohuanlin reviewed Mar 30, 2023

View reviewed changes

packages/pigeon/platform_tests/test_plugin/macos/Classes/CoreTests.gen.swift Outdated Show resolved Hide resolved

packages/pigeon/platform_tests/test_plugin/macos/Classes/CoreTests.gen.swift Outdated Show resolved Hide resolved

tarrinneal added 2 commits April 3, 2023 17:53

Improve _writeDecodeCasting

b0c7653

Merge branch 'main' of github.com:flutter/packages into swift-nil

740abd8

tarrinneal requested a review from stuartmorgan-g April 4, 2023 01:16

assert message

748a0bb

stuartmorgan-g approved these changes Apr 4, 2023

View reviewed changes

tarrinneal added 4 commits April 3, 2023 21:00

nits

d8f5fe3

comment

06ab71e

nested ternary

547ed02

gen test

367009e

stuartmorgan-g requested changes Apr 4, 2023

View reviewed changes

packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved

packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved

nits

cdf35ff

tarrinneal requested a review from stuartmorgan-g April 4, 2023 18:17

stuartmorgan-g approved these changes Apr 4, 2023

View reviewed changes

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2023

tarrinneal merged commit 7b4a543 into flutter:main Apr 4, 2023

engine-flutter-autoroll mentioned this pull request Apr 5, 2023

Roll Packages from 3df3ba528e29 to f224eea858f2 (8 revisions) flutter/flutter#124211

Closed

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

7b4a543 [pigeon] fix swift nsnull casting crash (flutter/packages#3545)

c6290b3

engine-flutter-autoroll mentioned this pull request Apr 5, 2023

Roll Packages from 3df3ba528e29 to f224eea858f2 (8 revisions) flutter/flutter#124242

Merged

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

7b4a543 [pigeon] fix swift nsnull casting crash (flutter/packages#3545)

1e3ccb8

hellohuanlin mentioned this pull request Apr 6, 2023

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

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pigeon] fix swift nsnull casting crash #3545

[pigeon] fix swift nsnull casting crash #3545

tarrinneal commented Mar 24, 2023

stuartmorgan-g commented Mar 27, 2023

stuartmorgan-g Mar 30, 2023

tarrinneal Mar 30, 2023

stuartmorgan-g Mar 31, 2023

tarrinneal Mar 31, 2023

tarrinneal Mar 31, 2023

stuartmorgan-g Apr 3, 2023

tarrinneal Apr 3, 2023

tarrinneal Apr 3, 2023

hellohuanlin left a comment •

edited

Loading

hellohuanlin commented Mar 30, 2023 •

edited

Loading

tarrinneal commented Mar 30, 2023

hellohuanlin commented Mar 30, 2023

tarrinneal commented Mar 30, 2023 •

edited

Loading

tarrinneal commented Mar 30, 2023 •

edited

Loading

hellohuanlin commented Mar 30, 2023

stuartmorgan-g left a comment

stuartmorgan-g Apr 4, 2023

tarrinneal Apr 4, 2023

tarrinneal Apr 4, 2023

stuartmorgan-g Apr 4, 2023

stuartmorgan-g Apr 4, 2023

tarrinneal Apr 4, 2023

stuartmorgan-g left a comment

auto-submit bot commented Apr 4, 2023

stuartmorgan-g commented Apr 4, 2023

tarrinneal commented Apr 4, 2023

tarrinneal commented Apr 4, 2023

stuartmorgan-g commented Apr 4, 2023

[pigeon] fix swift nsnull casting crash #3545

[pigeon] fix swift nsnull casting crash #3545

Conversation

tarrinneal commented Mar 24, 2023

Pre-launch Checklist

stuartmorgan-g commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin left a comment • edited Loading

Choose a reason for hiding this comment

hellohuanlin commented Mar 30, 2023 • edited Loading

tarrinneal commented Mar 30, 2023

hellohuanlin commented Mar 30, 2023

tarrinneal commented Mar 30, 2023 • edited Loading

tarrinneal commented Mar 30, 2023 • edited Loading

hellohuanlin commented Mar 30, 2023

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

auto-submit bot commented Apr 4, 2023

stuartmorgan-g commented Apr 4, 2023

tarrinneal commented Apr 4, 2023

tarrinneal commented Apr 4, 2023

stuartmorgan-g commented Apr 4, 2023

hellohuanlin left a comment •

edited

Loading

hellohuanlin commented Mar 30, 2023 •

edited

Loading

tarrinneal commented Mar 30, 2023 •

edited

Loading

tarrinneal commented Mar 30, 2023 •

edited

Loading