-
Notifications
You must be signed in to change notification settings - Fork 10.5k
SILGen: Don't pretend to handle ObjC APIs that "lie" about block return optionality. #4281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rn optionality. If an ObjC API claims to return a nonnull object of a bridged type, such as blocks, then we're already screwed since we don't take that possibility into account when bridging to a Swift function. Attempting to peephole that case only generates broken code attempting to bitcast () -> () to Optional<() -> ()>, which is invalid due to the abstraction change between () -> () and Optional<T>. Fixes SR-2331.
@swift-ci Please smoke test OS X |
@slavapestov Mind reviewing this for Swift 3.0? |
:-( Is it really more work to fix this? We get it right for |
I don't understand what mayLieAboutNonOptionalReturn() is for, really. But looks good. |
It appears the hack allows us to evaluate a non-null Objective-C call in optional context and get nil if the call really does return null. If this is just for failing initializers, we support those natively now don't we? |
This code has an interesting history. In 2014, Doug originally added the hack for failing initializers, which then got removed after failing initializers were implemented properly, but the original hack was re-instated after, with no explanation: 3a7b2c3 Joe's recent fixes seem to all make the 'may lie' check more conservative after users found cases where it was mis-applied which again raises the question of why it's needed in the first place :) If I remove the hack, the only tests that fail are those testing the hack -- in particular no executable tests fail: slavapestov@982e559 |
@jrose-apple We don't get this right for any bridged cases AFAIK. We would need to bitcast the original object pointer then bridge that as an optional. |
@slavapestov There have definitely been cases in the past where SDKs were incorrectly annotated. It could be that the cases we tested have since been fixed. |
@slavapestov @jrose-apple Any other comments? This look good for 3.0? |
I think we're at the point where we expect APIs to be incorrectly annotated forever. We have a different way to handle it for the value types: we treat nil as empty, on the idea that that's probably what was meant (and what the behavior would have been for iteration or (This is why |
Right, for bridged types without an obvious default, this is already broken. A better approach IMO would be to recognize when we're bridging in an optional context, do the bitcast-to-optional peephole first, then bridge as an optional. I'd say that's out of scope for Swift 3.0 at this point, though. |
If an ObjC API claims to return a nonnull object of a bridged type, such as blocks, then we're already screwed since we don't take that possibility into account when bridging to a Swift function. Attempting to peephole that case only generates broken code attempting to bitcast () -> () to Optional<() -> ()>, which is invalid due to the abstraction change between () -> () and Optional. Fixes SR-2331.