commit 5abe9e0a9f99092a90472f3b90f9cd9e747f7f6c
author hellohuanlin <41930132+hellohuanlin@users.noreply.github.com> Tue May 02 12:48:40 2023 -0700
committer GitHub <noreply@github.com> Tue May 02 19:48:40 2023 +0000
tree 855dd6fd29bc4bbff06ab84da6c73ca5698c1bd0
parent c90dd9834ebfd3d9b06f5f99dae1dd7652de6053

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

## The problem

This PR fixes a weird casting behavior discussed [here](https://github.com/flutter/packages/pull/3545#discussion_r1155061282): 
```
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.*

https://github.com/flutter/packages/pull/3545#discussion_r1155061282

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*