Merge pull request #7983 from jckarter/error-bridging-early-exit-fix

Runtime: Bridging an Error to NSError requires only checking its domain.

Author: jckarter

stdlib/public/SDK/Foundation/NSError.swift

@@ -202,7 +202,7 @@ internal let _errorDomainUserInfoProviderQueue = DispatchQueue(
 
 /// Retrieve the default userInfo dictionary for a given error.
 @_silgen_name("swift_Foundation_getErrorDefaultUserInfo")
-public func _swift_Foundation_getErrorDefaultUserInfo(_ error: Error)
+public func _swift_Foundation_getErrorDefaultUserInfo<T: Error>(_ error: T)
   -> AnyObject? {
   let hasUserInfoValueProvider: Bool
 

stdlib/public/core/ErrorType.swift

@@ -11,7 +11,6 @@
 //===----------------------------------------------------------------------===//
 import SwiftShims
 
-// TODO: API review
 /// A type representing an error value that can be thrown.
 ///
 /// Any type that declares conformance to the `Error` protocol can be used to
@@ -167,7 +166,7 @@ public func _stdlib_getErrorEmbeddedNSError<T : Error>(_ x: T)
 }
 
 @_silgen_name("swift_stdlib_getErrorDefaultUserInfo")
-public func _stdlib_getErrorDefaultUserInfo(_ error: Error) -> AnyObject?
+public func _stdlib_getErrorDefaultUserInfo<T: Error>(_ error: T) -> AnyObject?
 
 // Known function for the compiler to use to coerce `Error` instances
 // to `NSError`.

stdlib/public/runtime/ErrorObject.mm

@@ -397,7 +397,7 @@ static Class getSwiftNativeNSErrorClass() {
                            const WitnessTable *Error);
 
 //@_silgen_name("swift_stdlib_getErrorDefaultUserInfo")
-//public func _stdlib_getErrorDefaultUserInfo<T : Error>(_ x: UnsafePointer<T>) -> AnyObject
+//public func _stdlib_getErrorDefaultUserInfo<T : Error>(_ x: T) -> AnyObject
 SWIFT_CC(swift) SWIFT_RT_ENTRY_VISIBILITY
 NSDictionary *swift_stdlib_getErrorDefaultUserInfo(
                            OpaqueValue *error,
@@ -424,14 +424,22 @@ typedef SWIFT_CC(swift)
 static id _swift_bridgeErrorToNSError_(SwiftError *errorObject) {
   auto ns = reinterpret_cast<NSError *>(errorObject);
 
-  // If we already have a domain and userInfo set, then we've already
-  // initialized.
-  // FIXME: This might be overly strict; can we look only at the domain?
-  if (errorObject->domain.load(std::memory_order_acquire) &&
-      errorObject->userInfo.load(std::memory_order_acquire))
+  // If we already have a domain set, then we've already initialized.
+  // If this is a real NSError, then Cocoa and Core Foundation's initializers
+  // guarantee that the domain is never nil, so if this test fails, we can
+  // assume we're working with a bridged error. (Note that Cocoa and CF
+  // **will** allow the userInfo storage to be initialized to nil.)
+  //
+  // If this is a bridged error, then the domain, code, and user info are
+  // lazily computed, and the domain will be nil if they haven't been computed
+  // yet. The initialization is ordered in such a way that all other lazy
+  // initialization of the object happens-before the domain initialization so
+  // that the domain can be used alone as a flag for the initialization of the
+  // object.
+  if (errorObject->domain.load(std::memory_order_acquire))
     return ns;
 
-  // Otherwise, calculate the domain and code (TODO: and user info), and
+  // Otherwise, calculate the domain, code, and user info, and
   // initialize the NSError.
   auto value = SwiftError::getIndirectValue(&errorObject);
   auto type = errorObject->getType();
@@ -463,8 +471,8 @@ static id _swift_bridgeErrorToNSError_(SwiftError *errorObject) {
   // We also need to cmpxchg in the domain; if somebody beat us to it,
   // we need to release.
   //
-  // Storing the domain must be the LAST THING we do, since it's
-  // the signal that the NSError has been initialized.
+  // Storing the domain must be the **LAST THING** we do, since it's
+  // also the flag that the NSError has been initialized.
   CFStringRef expectedDomain = nullptr;
   if (!errorObject->domain.compare_exchange_strong(expectedDomain,
                                                    (CFStringRef)domain,

test/stdlib/ErrorBridgedStatic.swift

@@ -0,0 +1,28 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: %target-clang -fmodules -c -o %t/ErrorBridgedStaticImpl.o %S/Inputs/ErrorBridgedStaticImpl.m 
+// RUN: %target-build-swift -static-stdlib -o %t/ErrorBridgedStatic %t/ErrorBridgedStaticImpl.o %s -import-objc-header %S/Inputs/ErrorBridgedStaticImpl.h
+// RUN: strip %t/ErrorBridgedStatic
+// RUN: %t/ErrorBridgedStatic
+
+// REQUIRES: executable_test
+// REQUIRES: objc_interop
+// REQUIRES: static_stdlib
+
+import StdlibUnittest
+
+class Bar: Foo {
+  override func foo(_ x: Int32) throws {
+    try super.foo(5)
+  }
+}
+
+var ErrorBridgingStaticTests = TestSuite("ErrorBridging with static libs")
+
+ErrorBridgingStaticTests.test("round-trip Swift override of ObjC method") {
+  do {
+    try (Bar() as Foo).foo(5)
+  } catch { }
+}
+
+runAllTests()

test/stdlib/Inputs/ErrorBridgedStaticImpl.h

@@ -0,0 +1,7 @@
+@import Foundation;
+
+@interface Foo: NSObject
+
+- (BOOL)foo:(int)x error:(NSError**)error;
+
+@end

test/stdlib/Inputs/ErrorBridgedStaticImpl.m

@@ -0,0 +1,11 @@
+#include "ErrorBridgedStaticImpl.h"
+
+@implementation Foo
+
+- (BOOL)foo:(int)x error:(NSError**)error {
+  *error = nil;
+  return NO;
+}
+
+@end
+