[cxx-interop] Diagnose ObjC APIs returning SWIFT_SHARED_REFERENCE types

rdar://142500663

Author: fahadnayyar

lib/ClangImporter/ImportDecl.cpp

@@ -3376,7 +3376,11 @@ namespace {
 
     /// Emit diagnostics for incorrect usage of SWIFT_RETURNS_RETAINED and
     /// SWIFT_RETURNS_UNRETAINED
-    void checkBridgingAttrs(const clang::FunctionDecl *decl) {
+    void checkBridgingAttrs(const clang::NamedDecl *decl) {
+      assert(isa<clang::FunctionDecl>(decl) ||
+             isa<clang::ObjCMethodDecl>(decl) &&
+                 "checkBridgingAttrs called with a clang::NamedDecl which is "
+                 "neither clang::FunctionDecl nor clang::ObjCMethodDecl");
       bool returnsRetainedAttrIsPresent = false;
       bool returnsUnretainedAttrIsPresent = false;
       if (decl->hasAttrs()) {
@@ -3392,14 +3396,19 @@ namespace {
       }
 
       HeaderLoc loc(decl->getLocation());
-      if (isForeignReferenceTypeWithoutImmortalAttrs(decl->getReturnType())) {
+      const auto retType =
+          isa<clang::FunctionDecl>(decl)
+              ? dyn_cast<clang::FunctionDecl>(decl)->getReturnType()
+              : dyn_cast<clang::ObjCMethodDecl>(decl)->getReturnType();
+      if (isForeignReferenceTypeWithoutImmortalAttrs(retType)) {
         if (returnsRetainedAttrIsPresent && returnsUnretainedAttrIsPresent) {
           Impl.diagnose(loc, diag::both_returns_retained_returns_unretained,
                         decl);
         } else if (!returnsRetainedAttrIsPresent &&
                    !returnsUnretainedAttrIsPresent) {
-          bool unannotatedCxxAPIWarningNeeded = false;
-          if (!isa<clang::CXXDeductionGuideDecl>(decl)) {
+          bool unannotatedAPIWarningNeeded = false;
+          if (isa<clang::FunctionDecl>(decl) &&
+              !isa<clang::CXXDeductionGuideDecl>(decl)) {
             if (const auto *methodDecl = dyn_cast<clang::CXXMethodDecl>(decl)) {
               // FIXME: In the future we should support SWIFT_RETURNS_RETAINED
               // and SWIFT_RETURNS_UNRETAINED for overloaded C++ operators
@@ -3409,15 +3418,17 @@ namespace {
                   !methodDecl->isOverloadedOperator() &&
                   methodDecl->isUserProvided()) {
                 // normal c++ member method
-                unannotatedCxxAPIWarningNeeded = true;
+                unannotatedAPIWarningNeeded = true;
               }
             } else {
-              // global or top-level function
-              unannotatedCxxAPIWarningNeeded = true;
+              // global or top-level C/C++ function
+              unannotatedAPIWarningNeeded = true;
             }
+          } else if (isa<clang::ObjCMethodDecl>(decl)) {
+            unannotatedAPIWarningNeeded = true;
           }
 
-          if (unannotatedCxxAPIWarningNeeded) {
+          if (unannotatedAPIWarningNeeded) {
             HeaderLoc loc(decl->getLocation());
             Impl.diagnose(loc, diag::no_returns_retained_returns_unretained,
                           decl);
@@ -4560,6 +4571,8 @@ namespace {
       if (!dc)
         return nullptr;
 
+      checkBridgingAttrs(decl);
+
       // While importing the DeclContext, we might have imported the decl
       // itself.
       auto Known = Impl.importDeclCached(decl, getVersion());

test/Interop/Cxx/objc-correctness/Inputs/cxx-frt.h

@@ -9,6 +9,8 @@ struct CxxRefType {
 __attribute__((swift_attr("retain:retainCxxRefType")))
 __attribute__((swift_attr("release:releaseCxxRefType")));
 
+struct CxxValType {};
+
 void retainCxxRefType(CxxRefType *_Nonnull b) {}
 void releaseCxxRefType(CxxRefType *_Nonnull b) {}
 
@@ -19,18 +21,30 @@ void releaseCxxRefType(CxxRefType *_Nonnull b) {}
     __attribute__((swift_attr("returns_unretained")));
 + (struct CxxRefType *)objCMethodReturningFRTOwned
     __attribute__((swift_attr("returns_retained")));
++ (struct CxxRefType *)objCMethodReturningFRTBothAnnotations
+    __attribute__((swift_attr("returns_unretained")))
+    __attribute__((swift_attr("returns_retained")));
++ (struct CxxValType *)objCMethodReturningNonCxxFrtAnannotated
+    __attribute__((swift_attr("returns_retained")));
 
 @end
 
 @implementation Bridge
 + (struct CxxRefType *)objCMethodReturningFRTUnannotated {
-};
+}
 + (struct CxxRefType *)objCMethodReturningFRTUnowned
     __attribute__((swift_attr("returns_unretained"))) {
 }
 + (struct CxxRefType *)objCMethodReturningFRTOwned
     __attribute__((swift_attr("returns_retained"))) {
 }
++ (struct CxxRefType *)objCMethodReturningFRTBothAnnotations
+    __attribute__((swift_attr("returns_unretained")))
+    __attribute__((swift_attr("returns_retained"))) {
+}
++ (struct CxxValType *)objCMethodReturningNonCxxFrtAnannotated
+    __attribute__((swift_attr("returns_retained"))) {
+}
 
 @end
 

test/Interop/Cxx/objc-correctness/objc-returning-cxx-frt-diagnostics.swift

@@ -0,0 +1,17 @@
+// RUN: rm -rf %t
+// RUN: not %target-swift-frontend -typecheck -I %S/Inputs  %s -cxx-interoperability-mode=upcoming-swift 2>&1 | %FileCheck %s
+
+import CxxForeignRef
+
+// REQUIRES: objc_interop
+// TODO: figure out why the test no fail on wrong error messages
+func testObjCMethods() {
+    _ = Bridge.objCMethodReturningFRTBothAnnotations()
+    // CHECK: error: 'objCMethodReturningFRTBothAnnotations' cannot be annotated with both SWIFT_RETURNS_RETAINED and SWIFT_RETURNS_UNRETAINED
+
+    _ = Bridge.objCMethodReturningNonCxxFrtAnannotated()
+    // CHECK: error: 'objCMethodReturningNonCxxFrtAnannotated' cannot be annotated with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED because it is not returning a SWIFT_SHARED_REFERENCE type
+
+    _ = Bridge.objCMethodReturningFRTUnannotated()
+    // CHECK: warning: 'objCMethodReturningFRTUnannotated' should be annotated with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED as it is returning a SWIFT_SHARED_REFERENCE
+}