AliSoftware / Dip

Simple Swift Dependency container. Use protocols to resolve your dependencies and avoid singletons / sharedInstances!
MIT License
978 stars 75 forks source link

Swift 4.2 troubles with Any / Optional conversions #193

Closed stevenkramer closed 6 years ago

stevenkramer commented 6 years ago

Hi Ilya, in Swift 4.2 there are some problems with boxing/unboxing values in optionals again. E.g., after looking up resolvedInstances, the cast to T? currently always succeeds - even if resolvedInstances is empty.

I quickly hacked something together without really understanding the problems, to get stuff to work for me using Xcode 10. See https://github.com/stevenkramer/Dip/commits/develop.

mylemans commented 6 years ago

Did you report such issues here as well https://bugs.swift.org/secure/Dashboard.jspa ?

Its painful to think that every Swift update things could break like this.. Like the one time when didSet was getting called when GETting a property in certain situations..

stevenkramer commented 6 years ago

Hi @mylemans , I would, if I had a clue which behavior is correct. The 4.2 behavior seems to make more sense, because Optional is an Any, so the successful cast could be considered the correct approach…

ilyapuchka commented 6 years ago

Please test against swift42 branch

ilyapuchka commented 6 years ago

This is caused by this change in 4.2

Rather than hitting a runtime failure when bridging nil values, nil will be bridged to NSNull. https://swift.org/blog/iuo/

That results to nil values from resolvedInstances being bridged to NSNull which in turn then succeeds on as? T, where T is Any, which then fails on as! T where T is requested type.

stevenkramer commented 6 years ago

Thanks Ilya, I will.

To me it looks like the problem is due to a bug in casting with Any as a generic type. The bug is in Swift < 4.2 and was fixed in 4.2. In short, a conditional cast from Optional.none to Any succeeds in both versions, but it fails in Swift 4.1 if the code is generic and Any is used as a generic type. Unfortunately resolve functions depend on the broken behavior. I fixed it (in a few places) by first testing if the optional is .none before binding. See the isolated case in https://gist.github.com/stevenkramer/9b5afca1bbdbe17a7e1f9f05dd65360c

ilyapuchka commented 6 years ago

It's not needed to test it everywhere, only accessing resolvedInstances results in trying to cast NSNull to some other type, for reasons described in the article in Swift blog. If you find any other case - please create another issue. Resolving as Any is needed for properly resolving optional types when non optional types are registered.

stevenkramer commented 6 years ago

@ilyapuchka swift42 works for me. The updated binding code (if let previouslyResolved: T = foo instead of if let previouslyResolved = foo as? T) behaves identically on 4.1 & 4.2. I think it's a separate issue from the nil bridging though, because I found that one after making the changes for the conditional binding. Thanks for the prompt reply and fix.

@mylemans if you want to report it to on Swift's Jira, please go ahead, the Gist above is a nice isolated example. But 4.2 does the right thing, so I don't think it's worth the effort.

stevenkramer commented 6 years ago

This change is now mentioned in the Beta 4 release notes @mylemans.