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

[hellohuanlin]

The problem

This PR fixes a weird casting behavior discussed here:

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 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. 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. 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 can 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:

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

Lines 563 to 564 in c53db71

	let args = message as! [Any]
	let anObjectArg = args[0]

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 by replacing nils with NSNulls.

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.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• 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.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[hellohuanlin]

I probably also need to fix tests other than the plugin_test. Will request review when it's ready.

[stuartmorgan-g]

Thanks for digging into this!

Solution 2

Solution 2 is the opposite - avoid using Any? as much as possible.

We just did a bunch of this the last time we found a case where Any? gave us a nested optional, and concluded that Any? is a foot-gun. Given that it keeps causing us problems, this seems like an attractive option to me.

By keeping both flavors of Any, aren't we just setting ourselves up for more of these kinds of surprises?

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 dressed by immediately cast it to Any.

This seems like it would be straightforward since there's a well-defined boundary. It's there a downside to doing that?

b. Losing of semantic meaning of Any? that it can be optional. The hidden/implicit optional is the culprit here in the first place.

Are we losing semantic meaning though? It's explicitly documented that Any can be optional (e.g., here).

I'm still struggling to understand what the semantic difference between Any? and Any actually is in Swift. The warnings make it seem like there's supposed to be the one I naively expected at the beginning of this journey of discovery, (one allows optional types, the other doesn't) but it doesn't seem to match the actual behavior (wacky nested optionals) or docs (both can have optional types). Is there some benefit to using Any? in Swift that outweighs its surprising behaviors?

[stuartmorgan-g]

Alternately, maybe our purge of Any? was misguided and our problems were placed where we misused Any, and mistakenly addressed that the wrong way?

I don't fully understand what exactly the criteria are for implicit creation of nested optionals.

[hellohuanlin]

We just did a bunch of this the last time we found a case where Any? gave us a nested optional, and concluded that Any? is a foot-gun.

I would be interested to learn about that case.

This seems like it would be straightforward since there's a well-defined boundary. It's there a downside to doing that?

I don't think there's any downside, other than the optionality becomes implicit.

I don't fully understand what exactly the criteria are for implicit creation of nested optionals.

The criteria are (1) passing an Any that is implicitly optional (2) into a function with parameter of type Any?.

For example, this short code snippet:

func takesAnyOptional(_ val: Any?) {
  // (lldb) `po val` here after breakpoint
}
let nilAny: Any? = nil
let casted: Any = nilAny as Any
takesAnyOptional(casted)

So, solution 1 avoids (1) an Any that is implicitly optional, and solution 2 avoids (2) a function with parameter of type Any?.

[hellohuanlin]

The criteria are (1) passing an Any that is implicitly optional, (2) into a function with parameter of type Any?.

Here's another example that may help better understand why the wrapping:

func takeFoo(_ val: Int?) {
}
let foo = 3
takeFoo(foo) // the `Int` argument passed into `Int?` parameter will be wrapped with `Optional.some`

Similarly, the Any argument passed into Any? parameter will be wrapped with Optional.some. And if that Any argument itself is implicitly optional, we get nested optional.

The section "How did we end up with Any??" in PR description may also be helpful.

[hellohuanlin]

Are we losing semantic meaning though? It's explicitly documented that Any can be optional (e.g., here).

My initial take away from this note is that Apple discourages using Any to represent optional (hence the warning). I think this kind of "implicit" Optional is bad because (1) it hides the benefit of Optional in the first place (that is, compile check for nil case), and (2) it risks of creating a nested optional, as explained in my previous comment.

(Though this is just my initial thought. I can do more research on this)

[stuartmorgan-g]

Here's another example that may help better understand why the wrapping

Thanks, this example makes sense to me.

Similarly, the Any argument passed into Any? parameter will be wrapped with Optional.some. And if that Any argument itself is implicitly optional, we get nested optional.

That's... ugh. I guess I can see how the chain of other decisions leads to this but it seems really problematic, because it's consistent in some ways, but very inconsistent in other ways. The Int->Int? wrapping makes sense, because Int isn't an optional and can't contain an optional. Having Any->Any? would make sense if Any couldn't have optionals, but it can, so is fundamentally different.

I'd be very curious to talk to Swift developers to understand why they made Any allow optional types, but also made it behave differently than anything else that can contain optional types and also discourages that use via warnings. It feels a lot like there's some regret about the decision, but maybe like the wacky NSNumber casting-that-isn't-casting behavior and the NSNull-can-implicitly-be-nil behavior this falls out of some other required feature.

The section "How did we end up with Any??" in PR description may also be helpful.

Right, I had read that, but without the foundation of the simpler cases above I was not following why that chain caused an Any??.

[stuartmorgan-g]

My initial take away from this note is that Apple discourages using Any to represent optional (hence the warning).

That seems reasonable. So if we want to go that route, we need to sweep the entire generator and undo a bunch of the changes from #3284 that were made based on my conclusion in #3284 (comment). It seems likely that all of our use of [Any] and [...: Any] is incorrect, since Pigeon currently expresses all generics (and thus all collection types) as nullable.

[hellohuanlin]

The Int->Int? wrapping makes sense, because Int isn't an optional and can't contain an optional. Having Any->Any? would make sense if Any couldn't have optionals, but it can, so is fundamentally different.

My guess is that the optional wrapping is done at compile time. The compiler simply checks the static type of the argument vs the parameter, and insert the wrapping instruction there if needed. So at this moment dynamic type is unknown. They could have also added some extra logic specific about Any though, but they may have other concerns (e.g. runtime performance)

I'd be very curious to talk to Swift developers to understand why they made Any allow optional types

Yeah I'm also curious if Apple regrets this decision. I was surprised to learn the different behavior between Swift and Kotlin.

Right, I had read that, but without the foundation of the simpler cases above I was not following why that chain caused an Any??.

True. Yeah wasn't clear there.

That seems reasonable. So if we want to go that route, we need to sweep the entire generator and undo a bunch of the changes from #3284 that were made based on my conclusion in #3284 (comment). It seems likely that all of our use of [Any] and [...: Any] is incorrect, since Pigeon currently expresses all generics (and thus all collection types) as nullable.

Yeah. I think this PR cleans up some of them already. I can also take a look at what else is missing.

[hellohuanlin]

I'd be very curious to talk to Swift developers to understand why they made Any allow optional types

Thinking about it again, likely it all starts from the decision to use enum for Optional, and that Any is designed to represent any types, including enums.

I can see why they want to use enum for Optional - it's pretty natural to think Optional as enum with 2 cases (either present, or not present), and it gets the benefits of enums for free (e.g. works with switch statement, its value semantics and immutability). So maybe our problem of "nested Optional" was a good tradeoff to Apple when they design it.

[stuartmorgan-g]

I can also take a look at what else is missing.

From skimming the generated output here, it looks like collection fields are still wrong, and if Obj-C unknown types are in fact mapped to Any? then argument arrays would be wrong as well.

[stuartmorgan-g]

@hellohuanlin Did you still want to move forward with this?

[hellohuanlin]

@stuartmorgan yes. I plan to work on it this week.

[hellohuanlin]

it looks like collection fields are still wrong

This means that there will unfortunately be a breaking change - a raw dart List property would be translated into [Any?] now (rather than [Any] previously) in Swift.

I think it's fine, because most people will likely explicitly write List<bool?> or List<bool>, rather than a raw List type.

and if Obj-C unknown types are in fact mapped to Any? then argument arrays would be wrong as well.

To clarify a bit - since objc NSArray uses NSNull rather than nil, it's not necessary to use [Any?] (solution 3 in my PR description). However, it's still safer to use [Any?], in case the engine's encoder is migrated to Swift.

[stuartmorgan-g]

We are generally pretty relaxed about making breaking changes (as long as they affect compilation, not silently change runtime behavior) for Pigeon, since it would only show up for plugin developers when they are actively updating and re-running Pigeon generation. That's why Pigeon's major version is so high already.

As long as we're confident that we're changing to a better reflection of the types we want to express, it's fine to do a breaking change for.

[hellohuanlin]

I just found another interesting bug related to implicit optional (which does not cause "nested optional", but will still be fixed by this PR):

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

Lines 563 to 564 in c53db71

	let args = message as! [Any]
	let anObjectArg = args[0]

This is supposed to crash if args[0] is nil. However, it won't crash 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]!

This also means that the "Solution 2" in my PR description is problematic - it only fixes the nested optional issue, but it won't fix the issue above that silences the type mismatch (updating PR description to reflect that)

[hellohuanlin]

This is to fix a separate bug that silences type mismatch. more details: #3658 (comment)

[hellohuanlin]

This else block (and the corresponding else in Line 753) is not used, since the caller only call this function with List/Map types:

  if (type.baseName == 'List' || type.baseName == 'Map') {
    return _swiftTypeForBuiltinGenericDartType(type);
  }

I would either write an assert, or split it into 2 separate helpers (for list and map). But for now I'll leave it here to keep this PR focused (I can update it in separate PR)

[hellohuanlin]

args.first is another nested optional (Any??) - outer nil represents "there's no first element, or list is empty", and inner nil represents "there is first element, but it is nil".

When passing it to nilOrValue, it gets coerced into Any?, hence the compiler warning in Xcode (Bonus: we should make this an error instead of warning, will do in separate PR).

[tarrinneal]

This is very strange, I would expect the old code and the new code to behave in the same way.

[hellohuanlin]

The old code worked because it has "as Any" intermediate cast inside nilOrValue function. The new code here avoids implicitly optional completely, so no need the hack anymore.

[auto-submit]

auto label is removed for flutter/packages, pr: 3658, due to - This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.