(take 2) SR-0140: Bridge Optionals to nonnull ObjC objects by bridging their payload, or using a sentinel. #4869

jckarter · 2016-09-19T21:24:39Z

id-as-Any lets you pass Optional to an ObjC API that takes nonnull id, and also lets you bridge containers of Optional to NSArray etc. When this occurs, we can unwrap the value and bridge it so that inhabited optionals still pass into ObjC in the expected way, but we need something to represent none other than the nil pointer. Cocoa provides NSNull as the canonical "null for containers" object, which is the least bad of many possible answers. If we happen to have the rare nested optional T??, there is no precedented analog for these in Cocoa, so just generate a unique sentinel object to preserve the nil-ness depth so we at least don't lose information round-tripping across the ObjC-Swift bridge.

Making Optional conform to _ObjectiveCBridgeable is more or less enough to make this all work, though there are a few additional edge case things that need to be fixed up. We don't want to accept AnyObject?? as an @objc-compatible type, so special-case Optional in getForeignRepresentable.

Implements SR-0140 (rdar://problem/27905315).

jckarter · 2016-09-19T21:24:47Z

@swift-ci Please test

jckarter · 2016-09-19T21:25:30Z

@jrose-apple Do you mind reviewing this for inclusion in 3.0.1?

jrose-apple · 2016-09-20T00:54:39Z

stdlib/public/core/Optional.swift

+#if _runtime(_ObjC)
+extension Optional : _ObjectiveCBridgeable {
+  // The object that represents `none` for an Optional of this type.
+  internal static var _nilSentinel : AnyObject {


Is this worth caching, for SIL optimization purposes?

internal static let _nilSentinel = _swift_Foundation_getOptionalNilSentinelObject()

Unfortunately, we don't support stored static properties on generic types. We have to rely on the runtime's caching.

jrose-apple · 2016-09-20T00:55:50Z

stdlib/public/core/Optional.swift

+  }
+
+  public static func _forceBridgeFromObjectiveC(_ source: AnyObject,
+                                         result: inout Optional<Wrapped>?) {


Nitpick: unconventional alignment here. Standard library style would be to treat the parameters as a block bound with parens.

jrose-apple · 2016-09-20T00:56:09Z

stdlib/public/core/Optional.swift

+    // NB that the signature of _forceBridgeFromObjectiveC adds another level
+    // of optionality, so we need to wrap the immediate result of the conversion
+    // in `.some`.
+    if source === _nilSentinel {


Are all NSNulls guaranteed to be identical?

It is documented as a singleton, but I would double-check with someone from Foundation. @phausler?

There's example ObjC code that uses x == [NSNull null] too.

NSNull in Darwin is a singleton (swift-corelibs-foundation it cannot be since we don't have factory patterns)

jrose-apple · 2016-09-20T00:58:30Z

stdlib/public/runtime/Casting.cpp

@@ -2513,6 +2513,18 @@ bool swift::swift_dynamicCast(OpaqueValue *dest,
    // unwrapping the target. This handles an optional source wrapped within an
    // existential that Optional conforms to (Any).
    if (auto srcExistentialType = dyn_cast<ExistentialTypeMetadata>(srcType)) {
+#if SWIFT_OBJC_INTEROP
+      // If coming from AnyObject, we may want to bridge.


Why is this specific to AnyObject?

I suppose we could also support it from NSNull. AnyObject is the only Swift-level formal type that'd make sense to cast back to Optional otherwise.

Oh, I somehow missed that this was only when casting to Optional. Thanks for clarifying.

jrose-apple · 2016-09-20T00:59:12Z

stdlib/public/stubs/OptionalBridgingHelper.mm

+  return [NSString stringWithFormat:@"<_SwiftNull %u>", self->depth];
+}
+
+- (void)dealloc {


Nitpick: This is conventionally omitted when empty (and may allow run-time optimizations).

Oops, I had put this in so I could put breakpoints there, but forgot to remove it.

jrose-apple · 2016-09-20T01:02:30Z

test/stdlib/OptionalBridge.swift

+  }
+}
+
+tests.test("wrapped value") {


Any tests for optionals of non-bridged types? For optionals of reference types?

jrose-apple · 2016-09-20T01:05:19Z

stdlib/public/stubs/OptionalBridgingHelper.mm

+//
+//===----------------------------------------------------------------------===//
+
+#include <Foundation/Foundation.h>


Nitpick: I think this counts as a system include, and so it should go at the bottom. Also, #import would be more appropriate. (ObjC headers typically don't have include guards.)

jrose-apple · 2016-09-20T01:06:13Z

Some comments made, but mostly seems fine to me…except that someone else should take a look at the lib/SIL/ changes.

atrick · 2016-09-20T02:23:49Z

lib/SIL looks superficially ok to me. Question: Does SILGenBridging implicitly assume that the return value can't be reabstracted? If so, a comment would be helpful.

jckarter · 2016-09-20T03:48:29Z

@atrick ~~Yeah, it looks like SILGenBridging doesn't handle reabstraction (or address-only types, or indirect arguments, which @shajrawi is fixing). I can add that as a comment.~~ Sorry, confused the optimizer and codegen layers. Yeah, we don't currently bridge anything via protocols that requires reabstraction.

phausler · 2016-09-20T17:10:50Z

stdlib/public/stubs/OptionalBridgingHelper.mm

+
+/// Class of sentinel objects used to represent the `nil` value of nested
+/// optionals.
+@interface _SwiftNull : NSObject {


this should'd probably be __attribute__((visibility("hidden")))

Good idea. We aren't making any of the runtime-private classes hidden yet. I think we should do that as a separate patch, though, just to keep the risk of unintentional breakage minimal for this as a 3.0.1 patch.

phausler · 2016-09-20T17:12:43Z

stdlib/public/stubs/OptionalBridgingHelper.mm

+@implementation _SwiftNull : NSObject
+
+- (NSString*)description {
+  return [NSString stringWithFormat:@"<_SwiftNull %u>", self->depth];


Usually NSObjects have the format more like:

[NSString stringWithFormat:@"<%@ %p depth = %u>", [self class], self, self->depth]

That way if there is ever a problem you have the pointer which can be used in a debugger if need be.

phausler · 2016-09-20T17:16:48Z

test/stdlib/OptionalBridge.swift

+}
+
+tests.test("collection of Optional") {
+  let holeyArray: [LifetimeTracked?] = [LifetimeTracked(0), nil, LifetimeTracked(1)]


Does the reverse work? someNSArray as [LifetimeTracked?]

This pattern if ever used will never really be portable to swift-corelibs-foundation which is a bit unfortunate.

That's true of all bridging conversions, but ideally the bridging is never necessary to begin with in corelibs.

phausler · 2016-09-20T17:20:08Z

stdlib/public/stubs/OptionalBridgingHelper.mm

+static id getSentinelForDepth(unsigned depth) {
+  // For unnested optionals, use NSNull.
+  if (depth == 1)
+    return SWIFT_LAZY_CONSTANT([NSNull null]);


What does this macro do?

It inlines a lambda that dispatch_onces the expression in the macro argument.

I am fairly certain we have a full obligation from Foundation to maintain the return value as implemented:

+ (NSNull *)null { return (id)kCFNull; }

So the dispatch_once here is not really needed at all.

Good idea, I'll just use kCFNull directly.

phausler · 2016-09-20T17:21:19Z

stdlib/public/stubs/OptionalBridgingHelper.mm

+    if (!cached) {
+      auto sentinel = [[_SwiftNull alloc] init];
+      sentinel->depth = depth;
+      cached = sentinel;


Am I not reading this correctly? On cache miss you are not storing a value into the cache?

cached is a reference, so this will store the value into the cache.

jckarter · 2016-09-20T19:49:54Z

@jrose-apple @phausler @atrick Thanks for the review comments! I've revised the patch in response, if you all don't mind taking another look.

jckarter · 2016-09-20T19:50:05Z

@swift-ci Please test

jrose-apple · 2016-09-20T19:51:01Z

lib/AST/Builtins.cpp

@@ -754,6 +754,7 @@ static ValueDecl *getIsOptionalOperation(ASTContext &Context, Identifier Id) {
  return builder.build(Id);
 }

+


Nitpick: this is the only change in this file now.

Thanks, fixed.

jrose-apple · 2016-09-20T19:53:16Z

LGTM

…ayload, or using a sentinel. id-as-Any lets you pass Optional to an ObjC API that takes `nonnull id`, and also lets you bridge containers of `Optional` to `NSArray` etc. When this occurs, we can unwrap the value and bridge it so that inhabited optionals still pass into ObjC in the expected way, but we need something to represent `none` other than the `nil` pointer. Cocoa provides `NSNull` as the canonical "null for containers" object, which is the least bad of many possible answers. If we happen to have the rare nested optional `T??`, there is no precedented analog for these in Cocoa, so just generate a unique sentinel object to preserve the `nil`-ness depth so we at least don't lose information round-tripping across the ObjC-Swift bridge. Making Optional conform to _ObjectiveCBridgeable is more or less enough to make this all work, though there are a few additional edge case things that need to be fixed up. We don't want to accept `AnyObject??` as an @objc-compatible type, so special-case Optional in `getForeignRepresentable`. Implements SR-0140 (rdar://problem/27905315).

jckarter · 2016-09-20T20:04:52Z

@swift-ci Please test

jrose-apple reviewed Sep 20, 2016

View reviewed changes

phausler reviewed Sep 20, 2016

View reviewed changes

jckarter force-pushed the optional-bridging branch from ec7a28c to 36453df Compare September 20, 2016 19:49

jrose-apple reviewed Sep 20, 2016

View reviewed changes

jckarter force-pushed the optional-bridging branch from 36453df to 1a52e3f Compare September 20, 2016 20:04

jckarter merged commit 2bfe921 into swiftlang:master Sep 20, 2016

		@@ -754,6 +754,7 @@ static ValueDecl *getIsOptionalOperation(ASTContext &Context, Identifier Id) {
		return builder.build(Id);
		}

(take 2) SR-0140: Bridge Optionals to nonnull ObjC objects by bridging their payload, or using a sentinel. #4869

(take 2) SR-0140: Bridge Optionals to nonnull ObjC objects by bridging their payload, or using a sentinel. #4869

Conversation

jckarter commented Sep 19, 2016

Uh oh!

jckarter commented Sep 19, 2016

Uh oh!

jckarter 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!

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

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!

jrose-apple commented Sep 20, 2016

Uh oh!

atrick commented Sep 20, 2016

Uh oh!

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

Uh oh!

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 20, 2016

Uh oh!

jckarter commented Sep 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

jckarter commented Sep 20, 2016

Uh oh!

phausler Sep 20, 2016 •

edited

Loading

jckarter commented Sep 20, 2016 •

edited

Loading