Merge pull request swiftlang#37912 from tbkka/tbkka/sr14635-overlyAggressiveSwiftValueBoxing-Part2

SR-14635: __SwiftValue should be transparent to casting

Author: tbkka

include/swift/Runtime/Bincompat.h

@@ -38,6 +38,9 @@ bool useLegacyOptionalNilInjectionInCasting();
 /// Obj-C interop
 bool useLegacyObjCBoxingInCasting();
 
+/// Whether to use legacy semantics when unboxing __SwiftValue
+bool useLegacySwiftValueUnboxingInCasting();
+
 } // namespace bincompat
 
 } // namespace runtime

stdlib/public/runtime/Bincompat.cpp

@@ -189,7 +189,27 @@ bool useLegacyOptionalNilInjectionInCasting() {
 // by that protocol.
 bool useLegacyObjCBoxingInCasting() {
 #if BINARY_COMPATIBILITY_APPLE
-  return true; // For now, continue using the legacy behavior on Apple OSes
+  return false; // For now, always use the new behavior on Apple OSes
+#else
+  return false; // Always use the new behavior on non-Apple OSes
+#endif
+}
+
+// Should casting be strict about protocol conformance when
+// unboxing values that were boxed for Obj-C use?
+
+// Similar to `useLegacyObjCBoxingInCasting()`, but
+// this applies to the case where you have already boxed
+// some Swift non-reference-type into a `__SwiftValue`
+// and are now casting to a protocol.
+
+// For example, this cast
+// `x as! AnyObject as? NSCopying`
+// always succeeded with the legacy semantics.
+
+bool useLegacySwiftValueUnboxingInCasting() {
+#if BINARY_COMPATIBILITY_APPLE
+  return false; // For now, always use the new behavior on Apple OSes
 #else
   return false; // Always use the new behavior on non-Apple OSes
 #endif

stdlib/public/runtime/DynamicCast.cpp

@@ -425,6 +425,23 @@ tryCastUnwrappingObjCSwiftValueSource(
       destFailureType, srcFailureType,
       /*takeOnSuccess=*/ false, mayDeferChecks);
 }
+#else
+static DynamicCastResult
+tryCastUnwrappingSwiftValueSource(
+  OpaqueValue *destLocation, const Metadata *destType,
+  OpaqueValue *srcValue, const Metadata *srcType,
+  const Metadata *&destFailureType, const Metadata *&srcFailureType,
+  bool takeOnSuccess, bool mayDeferChecks)
+{
+  assert(srcType->getKind() == MetadataKind::Class);
+
+  // unboxFromSwiftValueWithType is really just a recursive casting operation...
+  if (swift_unboxFromSwiftValueWithType(srcValue, destLocation, destType)) {
+    return DynamicCastResult::SuccessViaCopy;
+  } else {
+    return DynamicCastResult::Failure;
+  }
+}
 #endif
 
 /******************************************************************************/
@@ -1474,6 +1491,18 @@ tryCastToClassExistential(
   }
 
   case MetadataKind::ObjCClassWrapper:
+#if SWIFT_OBJC_INTEROP
+    id srcObject;
+    memcpy(&srcObject, srcValue, sizeof(id));
+    if (!runtime::bincompat::useLegacySwiftValueUnboxingInCasting()) {
+      if (getAsSwiftValue(srcObject) != nullptr) {
+	// Do not directly cast a `__SwiftValue` box
+	// Return failure so our caller will unwrap and try again
+	return DynamicCastResult::Failure;
+      }
+    }
+#endif
+    SWIFT_FALLTHROUGH;
   case MetadataKind::Class:
   case MetadataKind::ForeignClass: {
     auto srcObject = getNonNullSrcObject(srcValue, srcType, destType);
@@ -2285,8 +2314,11 @@ tryCast(
   case MetadataKind::Class: {
 #if !SWIFT_OBJC_INTEROP
     // Try unwrapping native __SwiftValue implementation
-    if (swift_unboxFromSwiftValueWithType(srcValue, destLocation, destType)) {
-      return DynamicCastResult::SuccessViaCopy;
+    auto subcastResult = tryCastUnwrappingSwiftValueSource(
+      destLocation, destType, srcValue, srcType,
+      destFailureType, srcFailureType, takeOnSuccess, mayDeferChecks);
+    if (isSuccess(subcastResult)) {
+      return subcastResult;
     }
 #endif
     break;

stdlib/public/runtime/SwiftObject.mm

@@ -25,6 +25,7 @@
 #endif
 #include "llvm/ADT/StringRef.h"
 #include "swift/Basic/Lazy.h"
+#include "swift/Runtime/Bincompat.h"
 #include "swift/Runtime/Casting.h"
 #include "swift/Runtime/CustomRRABI.h"
 #include "swift/Runtime/Debug.h"
@@ -41,6 +42,7 @@
 #include "ErrorObject.h"
 #include "Private.h"
 #include "SwiftObject.h"
+#include "SwiftValue.h"
 #include "WeakReference.h"
 #if SWIFT_OBJC_INTEROP
 #include <dlfcn.h>
@@ -1269,6 +1271,12 @@ id swift_dynamicCastObjCProtocolUnconditional(id object,
 id swift_dynamicCastObjCProtocolConditional(id object,
                                             size_t numProtocols,
                                             Protocol * const *protocols) {
+  if (!runtime::bincompat::useLegacySwiftValueUnboxingInCasting()) {
+    if (getAsSwiftValue(object) != nil) {
+      // SwiftValue wrapper never holds a class object
+      return nil;
+    }
+  }
   for (size_t i = 0; i < numProtocols; ++i) {
     if (![object conformsToProtocol:protocols[i]]) {
       return nil;

test/Casting/Casts.swift

@@ -981,37 +981,41 @@ CastsTests.test("Recursive AnyHashable") {
 // https://github.com/apple/swift/issues/56987
 #if _runtime(_ObjC)
 CastsTests.test("Do not overuse __SwiftValue")
-.skip(.osxAny("Not yet fully enabled for Apple OSes"))
-.skip(.iOSAny("Not yet fully enabled for Apple OSes"))
-.skip(.iOSSimulatorAny("Not yet fully enabled for Apple OSes"))
-.skip(.tvOSAny("Not yet fully enabled for Apple OSes"))
-.skip(.tvOSSimulatorAny("Not yet fully enabled for Apple OSes"))
-.skip(.watchOSAny("Not yet fully enabled for Apple OSes"))
-.skip(.watchOSSimulatorAny("Not yet fully enabled for Apple OSes"))
+.skip(.custom({
+  if #available(SwiftStdlib 5.9, *) { return false } else { return true }
+}, reason: "Requires stdlib from Swift 5.9 or later"))
 .code {
   struct Bar {}
   // This used to succeed because of overeager __SwiftValue
   // boxing (and __SwiftValue does satisfy NSCopying)
   expectFalse(Bar() is NSCopying)
+  expectNil(runtimeCast(Bar(), to: NSCopying.self))
   expectFalse(Bar() as Any is NSCopying)
+  expectNil(runtimeCast(Bar() as Any, to: NSCopying.self))
 
-  // This seems unavoidable?
-  // `Bar() as! AnyObject` gets boxed as a __SwiftValue,
-  // and __SwiftValue does conform to NSCopying
-  expectTrue(Bar() as! AnyObject is NSCopying)
+  // `Bar() as! AnyObject` gets boxed as a __SwiftValue.
+  // __SwiftValue does conform to NSCopying, but that should
+  // not be visible here.
+  let anyBar = Bar() as! AnyObject
+  expectNil(runtimeCast(anyBar, to: NSCopying.self))
+  expectFalse(anyBar is NSCopying)
 
   class Foo {}
   // Foo does not conform to NSCopying
   // (This used to succeed due to over-eager __SwiftValue boxing)
   expectFalse(Foo() is NSCopying)
+  expectNil(runtimeCast(Foo(), to: NSCopying.self))
   expectFalse(Foo() as Any is NSCopying)
+  expectNil(runtimeCast(Foo() as Any, to: NSCopying.self))
 
   // A type that really does conform should cast to NSCopying
   class Foo2: NSCopying {
     func copy(with: NSZone?) -> Any { return self }
   }
   expectTrue(Foo2() is NSCopying)
+  expectNotNil(runtimeCast(Foo2(), to: NSCopying.self))
   expectTrue(Foo2() is AnyObject)
+  expectNotNil(runtimeCast(Foo2(), to: AnyObject.self))
 }
 #endif
 
@@ -1030,10 +1034,21 @@ CastsTests.test("Do not overuse __SwiftValue (non-ObjC)") {
   // This should succeed because this is what __SwiftValue boxing is for
   expectTrue(Bar() is AnyObject)
   expectTrue(Bar() as Any is AnyObject)
+  let a = Bar() as Any as! AnyObject
+  expectTrue(a is Bar)
 
   class Foo {}
   // Any class type can be cast to AnyObject
   expectTrue(Foo() is AnyObject)
+  let b = Foo() as! AnyObject
+  expectTrue(b is Foo)
+
+  // As above, but force use of runtime casting
+  expectNotNil(runtimeCast(Bar(), to: AnyObject.self))
+  expectNotNil(runtimeCast(Bar() as Any, to: AnyObject.self))
+  expectNotNil(runtimeCast(a, to: Bar.self))
+  expectNotNil(runtimeCast(Foo(), to: AnyObject.self))
+  expectNotNil(runtimeCast(b, to: Foo.self))
 }
 
 runAllTests()