Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct unconditionally bridge for a number of Foundation types #7429

Merged
merged 11 commits into from May 6, 2017

Conversation

@phausler
Copy link
Member

commented Feb 13, 2017

Foundation incorrectly implemented the unconditional bridge methods in a number of cases that could represent a reasonable fallback for undecorated or improperly decorated APIs. This changes Data to return a Data with zero bytes, AffineTransform to return identity, CharacterSet to return an empty CharacterSet, DateComponents to return a structure with no values, Decimal to return 0, IndexSet to return an empty IndexSet, IndexPath to return an empty IndexPath, Notification to return an "empty" notification (note the name is just the empty string; which is not suggested to subscribe to or post), and URLComponents to return a structure with no values when the value passed was detected as a nil value when it was not expected. This will help alleviate crashes when interoperating with APIs that have not yet been annotated with nullability (it is strongly suggested not just for the health and fitness of APIs to annotate for just swift but objective-c in general becomes more robust when nullability is marked 馃槈).

This addresses the following issues:
rdar://problem/30398990
rdar://problem/30384882
rdar://problem/30384851
rdar://problem/30384806
rdar://problem/30384727
rdar://problem/30384692

@phausler

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2017

@swift-ci please test

@jrose-apple

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

A nitpick: this doesn't affect APIs that haven't been annotated for nullability, only those that have been incorrectly annotated.

@phausler

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2017

Yea i should have been more specific. Out of the Foundation types these are the ones that directly have a sensible empty representation, I have a change for the NSValue ones but i need to test it more

_forceBridgeFromObjectiveC(source!, result: &result)
return result!
guard let src = source else { return DateComponents() }
return DateComponents(reference: src)

This comment has been minimized.

Copy link
@michael-lehew

michael-lehew Feb 13, 2017

Member

unrelated, but its a shame how many names we have for the first paraemeter (_bridged / referencing / reference / etc.)

This comment has been minimized.

Copy link
@phausler

phausler Feb 13, 2017

Author Member

most of those are internal; I think it would be interesting to perhaps make all bridgeable types have a public init method that takes a reference and a counterpart to the reference types that take the value (however that may seem too C++ to some...)

@michael-lehew
Copy link
Member

left a comment

I think this looks good to me!

@phausler

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2017

Tony requested behavior tests for these; holding off on merge until I have those ready

stdlib/public/SDK/Foundation/AffineTransform.swift Outdated
var result: AffineTransform?
_forceBridgeFromObjectiveC(x!, result: &result)
return result!
guard let src = x else { return AffineTransform.identity }

This comment has been minimized.

Copy link
@xwu

xwu Feb 14, 2017

Collaborator

World's smallest drive-by nit: extra space after return 馃槢

@phausler phausler force-pushed the phausler:correct_unconditionally_bridge branch Apr 17, 2017

@phausler

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

@swift-ci test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - c163ee30b055067cee2740acb4d60b3f2693f368
Test requested by - @phausler

@phausler phausler force-pushed the phausler:correct_unconditionally_bridge branch Apr 17, 2017

@phausler

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

@swift-ci test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - c163ee30b055067cee2740acb4d60b3f2693f368
Test requested by - @phausler

@phausler

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

@swift-ci test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - b4251b5fbc0c784bd76b6a8b3480640408f4001d
Test requested by - @phausler

@phausler phausler force-pushed the phausler:correct_unconditionally_bridge branch to 138644f Apr 18, 2017

@phausler

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

@swift-ci test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - b4251b5fbc0c784bd76b6a8b3480640408f4001d
Test requested by - @phausler

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - b4251b5fbc0c784bd76b6a8b3480640408f4001d
Test requested by - @phausler

@phausler

This comment has been minimized.

@phausler

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

@swift-ci please test and merge

@phausler

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/IRGen/indirect_argument.sil:18:14: error: expected string not found in input
16:15:02 // CHECK-32: call swiftcc void @huge_method(%T17indirect_argument4HugeV* noalias nocapture swiftself dereferenceable({{.*}})

I am quite certain this does not change the IRGen side of things

@phausler

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

@swift-ci please test

@phausler

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

Waiting for the completion of Swift Pull Request Test Linux
Swift Pull Request Test Linux #7156 completed. Result was FAILURE
Waiting for the completion of Swift Pull Request Test OS X
Swift Pull Request Test OS X #7199 completed. Result was FAILURE

swift-ci sounds really negative about my changes,
@shahmishal is there something more here that I am missing to find where the error is?

@phausler

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

lets try it again for good measure

@swift-ci please test and merge

@swift-ci swift-ci merged commit 85e20af into apple:master May 6, 2017

4 of 5 checks passed

Test and Merge Build started.
Details
Swift Test Linux Platform 9797 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 49235 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.