Enable optional unowned/unowned(unsafe) references

John okayed this change in a comment on GitHub pull request: #16237

Author: davezarzycki

CHANGELOG.md

@@ -24,9 +24,14 @@ CHANGELOG
 Swift 5.0
 ---------
 
-- [SR-419][]
+* Notable bug fix: unowned and unowned(unsafe) variables now support optional
+  types.
 
-  In Swift 5 mode, when setting a property from within its own `didSet` or `willSet` observer, the observer will now only avoid being recursively called if the property is set on `self` (either implicitly or explicitly).
+* [SR-419][]
+
+  In Swift 5 mode, when setting a property from within its own `didSet` or
+  `willSet` observer, the observer will now only avoid being recursively called
+  if the property is set on `self` (either implicitly or explicitly).
 
   For example:
   ```swift

include/swift/AST/Ownership.h

@@ -97,7 +97,6 @@ static inline bool isLessStrongThan(ReferenceOwnership left,
 enum class ReferenceOwnershipOptionality : uint8_t {
   Disallowed,
   Allowed,
-  AllowedIfImporting,
   Required,
 
   Last_Kind = Required
@@ -109,13 +108,11 @@ static inline ReferenceOwnershipOptionality
 optionalityOf(ReferenceOwnership ownership) {
   switch (ownership) {
   case ReferenceOwnership::Strong:
+  case ReferenceOwnership::Unowned:
+  case ReferenceOwnership::Unmanaged:
     return ReferenceOwnershipOptionality::Allowed;
   case ReferenceOwnership::Weak:
     return ReferenceOwnershipOptionality::Required;
-  case ReferenceOwnership::Unowned:
-    return ReferenceOwnershipOptionality::Disallowed;
-  case ReferenceOwnership::Unmanaged:
-    return ReferenceOwnershipOptionality::AllowedIfImporting;
   }
   llvm_unreachable("impossible");
 }

lib/AST/ASTContext.cpp

@@ -3395,8 +3395,6 @@ ReferenceStorageType *ReferenceStorageType::get(Type T,
     break;
   case ReferenceOwnershipOptionality::Allowed:
     break;
-  case ReferenceOwnershipOptionality::AllowedIfImporting:
-    break;
   case ReferenceOwnershipOptionality::Required:
     assert(T->getOptionalObjectType() && "optional type is required");
     break;

lib/IRGen/IRGenSIL.cpp

@@ -3882,12 +3882,22 @@ static const ReferenceTypeInfo &getReferentTypeInfo(IRGenFunction &IGF,
   void IRGenSILFunction::visitCopy##Name##ValueInst( \
                                             swift::Copy##Name##ValueInst *i) { \
     Explosion in = getLoweredExplosion(i->getOperand()); \
-    auto &ti = getReferentTypeInfo(*this, i->getOperand()->getType()); \
+    auto silTy = i->getOperand()->getType(); \
+    auto ty = cast<Name##StorageType>(silTy.getASTType()); \
+    auto isOptional = bool(ty.getReferentType()->getOptionalObjectType()); \
+    auto &ti = getReferentTypeInfo(*this, silTy); \
     ti.strongRetain##Name(*this, in, irgen::Atomicity::Atomic); \
     /* Semantically we are just passing through the input parameter but as a */\
     /* strong reference... at LLVM IR level these type differences don't */ \
     /* matter. So just set the lowered explosion appropriately. */ \
     Explosion output = getLoweredExplosion(i->getOperand()); \
+    if (isOptional) { \
+      auto values = output.claimAll(); \
+      output.reset(); \
+      for (auto value : values) { \
+        output.add(Builder.CreatePtrToInt(value, IGM.IntPtrTy)); \
+      } \
+    } \
     setLoweredExplosion(i, output); \
   }
 #define SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, name, ...) \

lib/Sema/CSGen.cpp

@@ -2058,7 +2058,6 @@ namespace {
             ty = CS.createTypeVariable(CS.getConstraintLocator(locator));
           return CS.getTypeChecker().getOptionalType(var->getLoc(), ty);
         case ReferenceOwnershipOptionality::Allowed:
-        case ReferenceOwnershipOptionality::AllowedIfImporting:
         case ReferenceOwnershipOptionality::Disallowed:
           break;
         }

lib/Sema/TypeCheckAttr.cpp

@@ -2134,12 +2134,6 @@ void TypeChecker::checkReferenceOwnershipAttr(VarDecl *var,
   auto isOptional = bool(underlyingType);
 
   switch (optionalityOf(ownershipKind)) {
-  case ReferenceOwnershipOptionality::AllowedIfImporting:
-    // Allow SIL to emulate importing testing and debugging.
-    if (auto sourceFile = var->getDeclContext()->getParentSourceFile())
-      if (sourceFile->Kind == SourceFileKind::SIL)
-        break;
-    LLVM_FALLTHROUGH;
   case ReferenceOwnershipOptionality::Disallowed:
     if (isOptional) {
       diagnose(var->getStartLoc(), diag::invalid_ownership_with_optional,

test/Interpreter/opt_unowned.swift

@@ -0,0 +1,98 @@
+// RUN: %target-run-simple-swift | %FileCheck %s
+// REQUIRES: executable_test
+
+protocol Protocol : class {
+  func noop()
+}
+
+//========================== Test pure Swift classes ==========================
+
+class SwiftClassBase : Protocol {
+  func noop() { print("noop") }
+}
+
+class SwiftClass : SwiftClassBase {
+  override init() {
+    print("SwiftClass Created")
+  }
+
+  deinit {
+    print("SwiftClass Destroyed")
+  }
+}
+
+func printState(_ x : SwiftClassBase?) {
+  print((x != nil) ? "is present" : "is nil")
+}
+
+func testSwiftClass() {
+  print("testSwiftClass")               // CHECK: testSwiftClass
+  
+  unowned var w : SwiftClassBase?
+  printState(w)                           // CHECK-NEXT: is nil
+  var c : SwiftClassBase = SwiftClass()   // CHECK: SwiftClass Created
+  printState(w)                           // CHECK-NEXT: is nil
+  w = c
+  printState(w)                           // CHECK-NEXT: is present
+  c.noop()                                // CHECK-NEXT: noop
+  c = SwiftClassBase()                    // CHECK-NEXT: SwiftClass Destroyed
+}
+
+testSwiftClass()
+
+
+
+func testSwiftImplicitOptionalClass() {
+  print("testSwiftImplicitOptionalClass") // CHECK: testSwiftImplicitOptionalClass
+  
+  unowned var w : SwiftClassBase!
+  printState(w)                           // CHECK-NEXT: is nil
+  var c : SwiftClassBase = SwiftClass()   // CHECK: SwiftClass Created
+  printState(w)                           // CHECK-NEXT: is nil
+  w = c
+  printState(w)                           // CHECK-NEXT: is present
+  c.noop()                                // CHECK-NEXT: noop
+  c = SwiftClassBase()                    // CHECK-NEXT: SwiftClass Destroyed
+}
+
+testSwiftImplicitOptionalClass()
+
+
+func testWeakInLet() {
+  print("testWeakInLet") // CHECK-LABEL: testWeakInLet
+
+  struct WeakBox {
+    unowned var value: SwiftClassBase?
+  }
+
+  var obj: SwiftClassBase? = SwiftClass() // CHECK: SwiftClass Created
+  let box = WeakBox(value: obj)
+  printState(box.value) // CHECK-NEXT: is present
+  obj = nil // CHECK-NEXT: SwiftClass Destroyed
+}
+
+testWeakInLet()
+
+
+//======================== Test Classbound Protocols ========================
+
+
+  
+func printState(_ x : Protocol?) {
+  print((x != nil) ? "is present" : "is nil")
+}
+
+func testProtocol() {
+  print("testProtocol")                 // CHECK: testProtocol
+  
+  unowned var w : Protocol?
+  printState(w)                           // CHECK-NEXT: is nil
+  var c : SwiftClassBase = SwiftClass()   // CHECK: SwiftClass Created
+  printState(w)                           // CHECK-NEXT: is nil
+  w = c
+  printState(w)                           // CHECK-NEXT: is present
+  c.noop()                                // CHECK-NEXT: noop
+  c = SwiftClassBase()                    // CHECK-NEXT: SwiftClass Destroyed
+}
+
+testProtocol()

test/Interpreter/opt_unowned_objc.swift

@@ -0,0 +1,47 @@
+// RUN: %target-build-swift %s -Xfrontend -disable-objc-attr-requires-foundation-module -o %t-main
+// RUN: %target-run %t-main | %FileCheck %s
+// REQUIRES: executable_test
+// REQUIRES: objc_interop
+
+import Foundation
+
+protocol Protocol : class {
+  func noop()
+}
+
+//========================== Test ObjC classes ==========================
+
+@objc
+class ObjCClassBase : Protocol {
+  func noop() { print("noop") }
+}
+
+@objc
+class ObjCClass : ObjCClassBase {
+  override init() {
+    print("ObjCClass Created")
+  }
+
+  deinit {
+    print("ObjCClass Destroyed")
+  }
+}
+
+func printState(_ x : ObjCClassBase?) {
+  print((x != nil) ? "is present" : "is nil")
+}
+
+func testObjCClass() {
+  print("testObjCClass")                // CHECK: testObjCClass
+  
+  unowned var w : ObjCClassBase?
+  printState(w)                           // CHECK-NEXT: is nil
+  var c : ObjCClassBase = ObjCClass()     // CHECK: ObjCClass Created
+  printState(w)                           // CHECK-NEXT: is nil
+  w = c
+  printState(w)                           // CHECK-NEXT: is present
+  c.noop()                                // CHECK-NEXT: noop
+  c = ObjCClassBase()                     // CHECK-NEXT: ObjCClass Destroyed
+}
+
+testObjCClass()

test/attr/attr_iboutlet.swift

@@ -135,8 +135,8 @@ class NonObjC {}
 // Check reference storage types
 @objc class RSX {
   @IBOutlet weak var rsx1: RSX?
-  @IBOutlet unowned var rsx2: RSX? // expected-error {{'unowned' variable cannot have optional type}}
-  @IBOutlet unowned(unsafe) var rsx3: RSX? // expected-error {{'unowned(unsafe)' variable cannot have optional type}}
+  @IBOutlet unowned var rsx2: RSX?
+  @IBOutlet unowned(unsafe) var rsx3: RSX?
   init() { }
 }
 

test/decl/var/properties.swift

@@ -1068,11 +1068,13 @@ class OwnershipBase {
   class var defaultObject: AnyObject { fatalError("") }
 
   var strongVar: AnyObject? // expected-note{{overridden declaration is here}}
-  weak var weakVar: AnyObject?
+  weak var weakVar: AnyObject? // expected-note{{overridden declaration is here}}
 
   // FIXME: These should be optional to properly test overriding.
   unowned var unownedVar: AnyObject = defaultObject
+  unowned var optUnownedVar: AnyObject? = defaultObject
   unowned(unsafe) var unownedUnsafeVar: AnyObject = defaultObject // expected-note{{overridden declaration is here}}
+  unowned(unsafe) var optUnownedUnsafeVar: AnyObject? = defaultObject
 }
 
 class OwnershipExplicitSub : OwnershipBase {
@@ -1085,9 +1087,15 @@ class OwnershipExplicitSub : OwnershipBase {
   override unowned var unownedVar: AnyObject {
     didSet {}
   }
+  override unowned var optUnownedVar: AnyObject? {
+    didSet {}
+  }
   override unowned(unsafe) var unownedUnsafeVar: AnyObject {
     didSet {}
   }
+  override unowned(unsafe) var optUnownedUnsafeVar: AnyObject? {
+    didSet {}
+  }
 }
 
 class OwnershipImplicitSub : OwnershipBase {
@@ -1100,16 +1108,22 @@ class OwnershipImplicitSub : OwnershipBase {
   override unowned var unownedVar: AnyObject {
     didSet {}
   }
+  override unowned var optUnownedVar: AnyObject? {
+    didSet {}
+  }
   override unowned(unsafe) var unownedUnsafeVar: AnyObject {
     didSet {}
   }
+  override unowned(unsafe) var optUnownedUnsafeVar: AnyObject? {
+    didSet {}
+  }
 }
 
 class OwnershipBadSub : OwnershipBase {
   override weak var strongVar: AnyObject? { // expected-error {{cannot override 'strong' property with 'weak' property}}
     didSet {}
   }
-  override unowned var weakVar: AnyObject? { // expected-error {{'unowned' variable cannot have optional type}}
+  override unowned var weakVar: AnyObject? { // expected-error {{cannot override 'weak' property with 'unowned' property}}
     didSet {}
   }
   override weak var unownedVar: AnyObject { // expected-error {{'weak' variable should have optional type 'AnyObject?'}}