Commit 5abe9e0
authored
[pigeon]fix "as Any" workaround due to nested optional (#3658)
## 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.

*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].*1 parent c90dd98 commit 5abe9e0
File tree
9 files changed
+193
-188
lines changed- packages/pigeon
- lib
- platform_tests/test_plugin
- example/ios/RunnerTests
- ios/Classes
- macos/Classes
- test
9 files changed
+193
-188
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
3 | 8 | | |
4 | 9 | | |
5 | 10 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
| 14 | + | |
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
169 | 169 | | |
170 | 170 | | |
171 | 171 | | |
172 | | - | |
| 172 | + | |
173 | 173 | | |
174 | 174 | | |
175 | 175 | | |
| |||
431 | 431 | | |
432 | 432 | | |
433 | 433 | | |
434 | | - | |
| 434 | + | |
435 | 435 | | |
436 | 436 | | |
437 | 437 | | |
| |||
524 | 524 | | |
525 | 525 | | |
526 | 526 | | |
527 | | - | |
| 527 | + | |
528 | 528 | | |
529 | 529 | | |
530 | 530 | | |
| |||
605 | 605 | | |
606 | 606 | | |
607 | 607 | | |
608 | | - | |
609 | | - | |
| 608 | + | |
610 | 609 | | |
611 | 610 | | |
612 | 611 | | |
| |||
628 | 627 | | |
629 | 628 | | |
630 | 629 | | |
631 | | - | |
| 630 | + | |
632 | 631 | | |
633 | 632 | | |
634 | 633 | | |
| |||
652 | 651 | | |
653 | 652 | | |
654 | 653 | | |
655 | | - | |
| 654 | + | |
656 | 655 | | |
657 | 656 | | |
658 | 657 | | |
| |||
695 | 694 | | |
696 | 695 | | |
697 | 696 | | |
698 | | - | |
| 697 | + | |
699 | 698 | | |
700 | 699 | | |
701 | 700 | | |
| |||
739 | 738 | | |
740 | 739 | | |
741 | 740 | | |
742 | | - | |
| 741 | + | |
743 | 742 | | |
744 | | - | |
| 743 | + | |
745 | 744 | | |
746 | 745 | | |
747 | 746 | | |
| |||
Lines changed: 3 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | | - | |
| 31 | + | |
| 32 | + | |
32 | 33 | | |
33 | 34 | | |
34 | 35 | | |
35 | 36 | | |
36 | 37 | | |
37 | | - | |
| 38 | + | |
38 | 39 | | |
39 | 40 | | |
40 | 41 | | |
| |||
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
15 | | - | |
| 14 | + | |
| 15 | + | |
16 | 16 | | |
17 | 17 | | |
18 | 18 | | |
| |||
0 commit comments