Skip to content

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

Merged
merged 1 commit into from
Aug 13, 2016

Conversation

jckarter
Copy link
Contributor

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.

…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.
@jckarter
Copy link
Contributor Author

@swift-ci Please smoke test OS X

@jckarter
Copy link
Contributor Author

@slavapestov Mind reviewing this for Swift 3.0?

@jrose-apple
Copy link
Contributor

jrose-apple commented Aug 13, 2016

:-( Is it really more work to fix this? We get it right for id _Nonnull/Any, no?

@slavapestov
Copy link
Contributor

I don't understand what mayLieAboutNonOptionalReturn() is for, really. But looks good.

@slavapestov
Copy link
Contributor

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?

@slavapestov
Copy link
Contributor

slavapestov commented Aug 13, 2016

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

@jckarter
Copy link
Contributor Author

@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.

@jckarter
Copy link
Contributor Author

@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.

@jckarter jckarter merged commit f67cb0d into swiftlang:master Aug 13, 2016
@jckarter
Copy link
Contributor Author

@slavapestov @jrose-apple Any other comments? This look good for 3.0?

@jrose-apple
Copy link
Contributor

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 -count). That doesn't work for things like URL, though.

(This is why _forceBridgeFromObjectiveC takes an optional value.)

@jckarter
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants