Merge pull request #77837 from lhoward/lhoward/retain-return-self

[cxx-interop] allow shared ref retain function to return self

Author: DougGregor

include/swift/AST/DiagnosticsClangImporter.def

@@ -231,10 +231,14 @@ ERROR(foreign_reference_types_invalid_retain_release, none,
       "type '%2'",
       (bool, StringRef, StringRef))
 
-ERROR(foreign_reference_types_retain_release_non_void_return_type, none,
-      "specified %select{retain|release}0 function '%1' is invalid; "
-      "%select{retain|release}0 function must have 'void' return type",
-      (bool, StringRef))
+ERROR(foreign_reference_types_retain_non_void_or_self_return_type, none,
+      "specified retain function '%0' is invalid; "
+      "retain function must have 'void' or parameter return type",
+      (StringRef))
+ERROR(foreign_reference_types_release_non_void_return_type, none,
+      "specified release function '%0' is invalid; "
+      "release function must have 'void' return type",
+      (StringRef))
 ERROR(foreign_reference_types_retain_release_not_a_function_decl, none,
       "specified %select{retain|release}0 function '%1' is not a function",
       (bool, StringRef))

lib/ClangImporter/ImportDecl.cpp

@@ -2542,24 +2542,23 @@ namespace {
     void validateForeignReferenceType(const clang::CXXRecordDecl *decl,
                                       ClassDecl *classDecl) {
 
-      enum class RetainReleaseOperatonKind {
+      enum class RetainReleaseOperationKind {
         notAfunction,
-        doesntReturnVoid,
+        doesntReturnVoidOrSelf,
         invalidParameters,
         valid
       };
 
       auto getOperationValidity =
-          [&](ValueDecl *operation) -> RetainReleaseOperatonKind {
+          [&](ValueDecl *operation,
+              CustomRefCountingOperationKind operationKind)
+          -> RetainReleaseOperationKind {
         auto operationFn = dyn_cast<FuncDecl>(operation);
         if (!operationFn)
-          return RetainReleaseOperatonKind::notAfunction;
-
-        if (!operationFn->getResultInterfaceType()->isVoid())
-          return RetainReleaseOperatonKind::doesntReturnVoid;
+          return RetainReleaseOperationKind::notAfunction;
 
         if (operationFn->getParameters()->size() != 1)
-          return RetainReleaseOperatonKind::invalidParameters;
+          return RetainReleaseOperationKind::invalidParameters;
 
         Type paramType =
             operationFn->getParameters()->get(0)->getInterfaceType();
@@ -2569,20 +2568,31 @@ namespace {
         }
 
         swift::NominalTypeDecl *paramDecl = paramType->getAnyNominal();
+
+        // The return type should be void (for release functions), or void
+        // or the parameter type (for retain functions).
+        auto resultInterfaceType = operationFn->getResultInterfaceType();
+        if (!resultInterfaceType->isVoid()) {
+          if (operationKind == CustomRefCountingOperationKind::release ||
+              !resultInterfaceType->lookThroughSingleOptionalType()->isEqual(paramType))
+            return RetainReleaseOperationKind::doesntReturnVoidOrSelf;
+        }
+
         // The parameter of the retain/release function should be pointer to the
         // same FRT or a base FRT.
         if (paramDecl != classDecl) {
           if (const clang::Decl *paramClangDecl = paramDecl->getClangDecl()) {
             if (const auto *paramTypeDecl =
                     dyn_cast<clang::CXXRecordDecl>(paramClangDecl)) {
               if (decl->isDerivedFrom(paramTypeDecl)) {
-                return RetainReleaseOperatonKind::valid;
+                return RetainReleaseOperationKind::valid;
               }
             }
           }
-          return RetainReleaseOperatonKind::invalidParameters;
+          return RetainReleaseOperationKind::invalidParameters;
         }
-        return RetainReleaseOperatonKind::valid;
+
+        return RetainReleaseOperationKind::valid;
       };
 
       auto retainOperation = evaluateOrDefault(
@@ -2619,28 +2629,29 @@ namespace {
                       false, retainOperation.name, decl->getNameAsString());
       } else if (retainOperation.kind ==
                  CustomRefCountingOperationResult::foundOperation) {
-        RetainReleaseOperatonKind operationKind =
-            getOperationValidity(retainOperation.operation);
+        RetainReleaseOperationKind operationKind =
+            getOperationValidity(retainOperation.operation,
+                                 CustomRefCountingOperationKind::retain);
         HeaderLoc loc(decl->getLocation());
         switch (operationKind) {
-        case RetainReleaseOperatonKind::notAfunction:
+        case RetainReleaseOperationKind::notAfunction:
           Impl.diagnose(
               loc,
               diag::foreign_reference_types_retain_release_not_a_function_decl,
               false, retainOperation.name);
           break;
-        case RetainReleaseOperatonKind::doesntReturnVoid:
+        case RetainReleaseOperationKind::doesntReturnVoidOrSelf:
           Impl.diagnose(
               loc,
-              diag::foreign_reference_types_retain_release_non_void_return_type,
-              false, retainOperation.name);
+              diag::foreign_reference_types_retain_non_void_or_self_return_type,
+              retainOperation.name);
           break;
-        case RetainReleaseOperatonKind::invalidParameters:
+        case RetainReleaseOperationKind::invalidParameters:
           Impl.diagnose(loc,
                         diag::foreign_reference_types_invalid_retain_release,
                         false, retainOperation.name, classDecl->getNameStr());
           break;
-        case RetainReleaseOperatonKind::valid:
+        case RetainReleaseOperationKind::valid:
           break;
         }
       } else {
@@ -2683,28 +2694,29 @@ namespace {
                       true, releaseOperation.name, decl->getNameAsString());
       } else if (releaseOperation.kind ==
                  CustomRefCountingOperationResult::foundOperation) {
-        RetainReleaseOperatonKind operationKind =
-            getOperationValidity(releaseOperation.operation);
+        RetainReleaseOperationKind operationKind =
+            getOperationValidity(releaseOperation.operation,
+                                 CustomRefCountingOperationKind::release);
         HeaderLoc loc(decl->getLocation());
         switch (operationKind) {
-        case RetainReleaseOperatonKind::notAfunction:
+        case RetainReleaseOperationKind::notAfunction:
           Impl.diagnose(
               loc,
               diag::foreign_reference_types_retain_release_not_a_function_decl,
               true, releaseOperation.name);
           break;
-        case RetainReleaseOperatonKind::doesntReturnVoid:
+        case RetainReleaseOperationKind::doesntReturnVoidOrSelf:
           Impl.diagnose(
               loc,
-              diag::foreign_reference_types_retain_release_non_void_return_type,
-              true, releaseOperation.name);
+              diag::foreign_reference_types_release_non_void_return_type,
+              releaseOperation.name);
           break;
-        case RetainReleaseOperatonKind::invalidParameters:
+        case RetainReleaseOperationKind::invalidParameters:
           Impl.diagnose(loc,
                         diag::foreign_reference_types_invalid_retain_release,
                         true, releaseOperation.name, classDecl->getNameStr());
           break;
-        case RetainReleaseOperatonKind::valid:
+        case RetainReleaseOperationKind::valid:
           break;
         }
       } else {

test/Interop/Cxx/foreign-reference/invalid-retain-operation-errors.swift

@@ -46,6 +46,15 @@ GoodRetainRelease {};
 void goodRetain(GoodRetainRelease *v);
 void goodRelease(GoodRetainRelease *v);
 
+struct
+    __attribute__((swift_attr("import_reference")))
+    __attribute__((swift_attr("retain:goodRetainWithRetainReturningSelf")))
+    __attribute__((swift_attr("release:goodReleaseWithRetainReturningSelf")))
+GoodRetainReleaseWithRetainReturningSelf {};
+
+GoodRetainReleaseWithRetainReturningSelf *goodRetainWithRetainReturningSelf(GoodRetainReleaseWithRetainReturningSelf *v);
+void goodReleaseWithRetainReturningSelf(GoodRetainReleaseWithRetainReturningSelf *v);
+
 struct
     __attribute__((swift_attr("import_reference")))
     __attribute__((swift_attr("retain:goodRetainWithNullabilityAnnotations")))
@@ -226,7 +235,7 @@ public func test(x: NonExistent) { }
 @available(macOS 13.3, *)
 public func test(x: NoRetainRelease) { }
 
-// CHECK: error: specified retain function 'badRetain' is invalid; retain function must have 'void' return type
+// CHECK: error: specified retain function 'badRetain' is invalid; retain function must have 'void' or parameter return type
 // CHECK: error: specified release function 'badRelease' is invalid; release function must have exactly one argument of type 'BadRetainRelease'
 @available(macOS 13.3, *)
 public func test(x: BadRetainRelease) { }
@@ -239,6 +248,9 @@ public func test(x: BadRetainReleaseWithNullabilityAnnotations) { }
 @available(macOS 13.3, *)
 public func test(x: GoodRetainRelease) { }
 
+@available(macOS 13.3, *)
+public func test(x: GoodRetainReleaseRetainReturningSelf) { }
+
 @available(macOS 13.3, *)
 public func test(x: GoodRetainReleaseWithNullabilityAnnotations) { }