SE-0139: NSValue bridging #4865

jckarter · 2016-09-19T18:16:31Z

For every struct type for which the frameworks provides an NSValue category for boxing and unboxing values of that type, provide an _ObjectiveCBridgeable conformance in the Swift overlay that bridges that struct to NSValue, allowing the structs to be used naturally with id-as-Any APIs and Cocoa container classes. This is mostly a matter of gyb-ing out boilerplate using NSValue.init(bytes:objCType:) to construct the instance, NSValue.objCType to check its type when casting, and NSValue.getValue(_:) to extract the unboxed value, though there are a number of special snowflake cases that need special accommodation:

To maintain proper layering, CoreGraphics structs need to be bridged in the Foundation overlay.
AVFoundation provides the NSValue boxing categories for structs owned by CoreMedia, but it does so using its own internal subclasses of NSValue, and these subclasses do not interop properly with the standard NSValue subclasses instantiated by Foundation. To do the right thing, we therefore have to let AVFoundation provide the bridging implementation for the CoreMedia types, and we have to use its category methods to do so.
SceneKit provides NSValue categories to box and unbox SCNVector3, SCNVector4, and SCNMatrix4; however, the methods it provides do so in an unusual way. SCNVector3 and SCNVector4 are packaged into CGRects and then the CGRect is boxed using valueWithCGRect:. SCNMatrix4 is copied into a CATransform3D, which is then boxed using valueWithCATransform3D: from CoreAnimation. To be consistent with what SceneKit does, use its category methods for these types as well, and when casting, check the type against the type encoding SceneKit uses rather than the type encoding of the expected type.

We need the encode string to be able to construct NSValues using the core valueWithBytes:objCType: API. This builtin only works with concrete, @objc-representable types for now, which should be sufficient for a stdlib-internal API.

jckarter · 2016-09-19T18:16:50Z

@swift-ci Please test

swift-ci · 2016-09-19T18:33:28Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 3cd9b9b1831b658c135f735f619a270009590ef4
Test requested by - @jckarter

swift-ci · 2016-09-19T18:34:12Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 3cd9b9b1831b658c135f735f619a270009590ef4
Test requested by - @jckarter

jrose-apple · 2016-09-19T19:52:04Z

lib/AST/ASTContext.cpp

@@ -3951,8 +3951,17 @@ bool ASTContext::isStandardLibraryTypeBridgedInFoundation(
          nominal == getStringDecl() ||
          nominal == getErrorDecl() ||
          nominal == getAnyHashableDecl() ||
-          // Weird one-off case where CGFloat is bridged to NSNumber.
-          nominal->getName() == Id_CGFloat);
+          // Foundation is a dependency of CoreGraphics, but


Foundation isn't actually a dependency of CoreGraphics; IIRC the two frameworks are unrelated in iOS, and related the other way on macOS.

Interesting. The Swift overlays are set up in CMake such that Foundation's overlay depends on the CoreGraphics one on all platforms. Would it make sense to change it the other way, if that's purely a Swift-build-system layering and not a system one?

"Foundation is a dependency of CoreGraphics" means "CoreGraphics depends on Foundation", no?

This check also strikes me as incredibly general. Should we just give up and say all imported types might be bridged at some point?

Sorry, I meant "Foundation depends on CoreGraphics". Treating all imported types as possibly bridged might be prudent for ABI compatibility, if we want to reserve the right to continue adding new bridging relationships after ABI stability is declared.

I'm less concerned about ABI stability than about us either being too general or not general enough. Either we think it's worth maintaining this list, or we don't.

(You're right that it won't be possible to add cross-module bridging relationships post-stability, but that seems reasonable.)

@atrick or @aschwaighofer, how big of a perf impact do you think it'd be if we assumed all imported C value types might have an _ObjectiveCBridgeable conformance?

What kind of an impact do you have in mind? Generating a bunch of conformances bloats the binary and metadata. I guess that could be measured. I don't think the optimizer makes use of knowledge of the absence of a conformance. I don't think we could make that assumption for a public type.

@atrick It looks like we do try to make optimization assumptions for the lack of _ObjectiveCBridgeable conformances by disallowing conformances to that protocol outside of the type's defining module, except for this hardcoded list of exceptions. I'm not sure how valuable that is anymore in the world of id-as-Any, since there's at least one way to bridge everything to and from a class type now.

jrose-apple · 2016-09-19T19:56:37Z

test/IRGen/builtins_objc.swift

+
+
+func getObjCTypeEncoding<T>(_: T) {
+  // CHECK: call void @use({{.* i8]\*}} [[INT32:@[0-9]+]],


You should be able to check the actual encode strings too, right?

I could, but I wasn't sure that the churn of tracking down and conditionalizing the exact encode strings for each type on each supported platform really gave meaningful coverage over just verifying that the builtin works at all. Clang ought to exercise the underlying type encoding code fairly well already.

Sure, but nothing is checking that the builtin works at all.

jrose-apple · 2016-09-19T21:07:53Z

stdlib/public/SDK/SceneKit/CMakeLists.txt

-  SWIFT_MODULE_DEPENDS_WATCHOS Foundation
-  SWIFT_MODULE_DEPENDS simd
+  SWIFT_MODULE_DEPENDS_WATCHOS
+  SWIFT_MODULE_DEPENDS Foundation simd


You can probably just remove the previous line, but it wasn't incorrect the old way—the non-watchOS platforms already indirectly depend on Foundation.

Good catch.

jckarter · 2016-09-19T22:01:08Z

@swift-ci Please test

swift-ci · 2016-09-19T22:27:41Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 853afe50b09dcc1e16aa59e0205731647b0c0ee0
Test requested by - @jckarter

jckarter · 2016-09-19T22:33:12Z

@swift-ci Please test

jckarter · 2016-09-19T22:58:02Z

@jrose-apple You've already glanced over this, but do you mind reviewing for inclusion in 3.0.1 as well?

jrose-apple · 2016-09-19T23:47:32Z

I'll do another pass tonight; the first pass wasn't too thorough.

swift-ci · 2016-09-19T23:53:26Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - edf0aec59a52824aedb3564e0320471744d966a8
Test requested by - @jckarter

jrose-apple · 2016-09-20T17:01:41Z

Seems good for now. Still a few open questions but nothing that would affect the 3.0 branch.

jckarter · 2016-09-20T21:34:11Z

@swift-ci Please test

jckarter · 2016-09-20T22:01:19Z

@jrose-apple I updated the comment in ASTContext.cpp and added some tests for specific type encodings that ought to be stable across platforms. How's this look for 3.0 now?

jrose-apple · 2016-09-20T22:06:47Z

Ship it!

swift-ci · 2016-09-20T22:26:14Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - c58492cc32720577c8c0adbdf1c06e7202fa8916
Test requested by - @jckarter

jckarter · 2016-09-21T17:43:32Z

@swift-ci Please test

swift-ci · 2016-09-21T18:04:49Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - c58492cc32720577c8c0adbdf1c06e7202fa8916
Test requested by - @jckarter

jckarter · 2016-09-21T18:11:28Z

@phausler Is this Foundation test failure expected, or transient?

TestFoundation/TestNSURLSession.swift:40: error: TestURLSession.test_dataTaskWithURL : Asynchronous wait failed - Exceeded timeout of 12.0 seconds, with unfulfilled expectations: data task
11:05:05 TestFoundation/TestNSURLSession.swift:42: error: TestURLSession.test_dataTaskWithURL : XCTAssertEqual failed: ("unknown") is not equal to ("Kathmandu") - test_dataTaskWithURLRequest returned an unexpected result
11:05:05 Test Case 'TestURLSession.test_dataTaskWithURL' failed (12.006 seconds).

jckarter · 2016-09-21T18:11:38Z

@swift-ci Please test

phausler · 2016-09-21T18:12:53Z

yea that failure is transient, we have seen it flutter a bit and it really needs to be replaced with loopback testing

swift-ci · 2016-09-21T18:29:23Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - c69d35d67b7530a3dcc0cbd49a98e1b45698a779
Test requested by - @jckarter

swift-ci · 2016-09-21T20:40:01Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - c69d35d67b7530a3dcc0cbd49a98e1b45698a779
Test requested by - @jckarter

For every struct type for which the frameworks provides an NSValue category for boxing and unboxing values of that type, provide an _ObjectiveCBridgeable conformance in the Swift overlay that bridges that struct to NSValue, allowing the structs to be used naturally with id-as-Any APIs and Cocoa container classes. This is mostly a matter of gyb-ing out boilerplate using `NSValue.init(bytes:objCType:)` to construct the instance, `NSValue.objCType` to check its type when casting, and `NSValue.getValue(_:)` to extract the unboxed value, though there are a number of special snowflake cases that need special accommodation: - To maintain proper layering, CoreGraphics structs need to be bridged in the Foundation overlay. - AVFoundation provides the NSValue boxing categories for structs owned by CoreMedia, but it does so using its own internal subclasses of NSValue, and these subclasses do not interop properly with the standard `NSValue` subclasses instantiated by Foundation. To do the right thing, we therefore have to let AVFoundation provide the bridging implementation for the CoreMedia types, and we have to use its category methods to do so. - SceneKit provides NSValue categories to box and unbox SCNVector3, SCNVector4, and SCNMatrix4; however, the methods it provides do so in an unusual way. SCNVector3 and SCNVector4 are packaged into `CGRect`s and then the CGRect is boxed using `valueWithCGRect:`. SCNMatrix4 is copied into a CATransform3D, which is then boxed using `valueWithCATransform3D:` from CoreAnimation. To be consistent with what SceneKit does, use its category methods for these types as well, and when casting, check the type against the type encoding SceneKit uses rather than the type encoding of the expected type.

jckarter · 2016-09-22T02:26:50Z

@swift-ci Please test

jrose-apple reviewed Sep 19, 2016

View reviewed changes

jckarter force-pushed the nsvalue-bridging branch from 3cd9b9b to 853afe5 Compare September 19, 2016 21:54

jckarter force-pushed the nsvalue-bridging branch from 853afe5 to edf0aec Compare September 19, 2016 22:32

jckarter force-pushed the nsvalue-bridging branch from edf0aec to c58492c Compare September 20, 2016 21:33

jckarter force-pushed the nsvalue-bridging branch from c58492c to c69d35d Compare September 21, 2016 17:42

jckarter force-pushed the nsvalue-bridging branch from c69d35d to 86fbeee Compare September 22, 2016 02:26

jckarter merged commit b0e3c0b into swiftlang:master Sep 22, 2016

jckarter deleted the nsvalue-bridging branch September 22, 2016 16:13



		func getObjCTypeEncoding<T>(_: T) {
		// CHECK: call void @use({{.* i8]\*}} [[INT32:@[0-9]+]],

SE-0139: NSValue bridging #4865

SE-0139: NSValue bridging #4865

Uh oh!

Conversation

jckarter commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Sep 19, 2016

Uh oh!

swift-ci commented Sep 19, 2016

Uh oh!

swift-ci commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Sep 19, 2016

Uh oh!

swift-ci commented Sep 19, 2016

Uh oh!

jckarter commented Sep 19, 2016

Uh oh!

jckarter commented Sep 19, 2016

Uh oh!

jrose-apple commented Sep 19, 2016

Uh oh!

swift-ci commented Sep 19, 2016

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

jckarter commented Sep 20, 2016

Uh oh!

jckarter commented Sep 20, 2016

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

swift-ci commented Sep 20, 2016

Uh oh!

jckarter commented Sep 21, 2016

Uh oh!

swift-ci commented Sep 21, 2016

Uh oh!

jckarter commented Sep 21, 2016

Uh oh!

jckarter commented Sep 21, 2016

Uh oh!

phausler commented Sep 21, 2016

Uh oh!

swift-ci commented Sep 21, 2016

Uh oh!

swift-ci commented Sep 21, 2016

Uh oh!

jckarter commented Sep 22, 2016

jckarter commented Sep 19, 2016 •

edited

Loading