[cxx-interop] Warning unannotated C++ APIs returning SWIFT_SHARED_REFERENCE types

fahadnayyar · fahadnayyar · commit ad4df732560b · 2024-10-28T19:37:38.000-07:00
diff --git a/include/swift/AST/DiagnosticsClangImporter.def b/include/swift/AST/DiagnosticsClangImporter.def
@@ -258,8 +258,23 @@ ERROR(failed_base_method_call_synthesis,none,
       "failed to synthesize call to the base method %0 of type %0",
       (ValueDecl *, ValueDecl *))
 
-ERROR(both_returns_retained_returns_unretained,none,
-      "%0 cannot be annotated with both swift_attr('returns_retained') and swift_attr('returns_unretained') attributes", (const clang::NamedDecl*))
+ERROR(both_returns_retained_returns_unretained, none,
+      "%0 cannot be annotated with both swift_attr('returns_retained') and "
+      "swift_attr('returns_unretained') attributes",
+      (const clang::NamedDecl *))
+
+ERROR(returns_retained_or_returns_unretained_for_non_cxx_frt_values, none,
+      "%0 cannot be annotated with either swift_attr('returns_retained') or "
+      "swift_attr('returns_unretained') attribute because it is not returning "
+      "a 'SWIFT_SHARED_REFERENCE' type",
+      (const clang::NamedDecl *))
+
+WARNING(
+    no_returns_retained_returns_unretained, none,
+    "%0 is returning a 'SWIFT_SHARED_REFERENCE' type but is not annotated "
+    "with either swift_attr('returns_retained') or "
+    "swift_attr('returns_unretained') attributes",
+    (const clang::NamedDecl *))
 
 NOTE(unsupported_builtin_type, none, "built-in type '%0' not supported", (StringRef))
 NOTE(record_field_not_imported, none, "field %0 unavailable (cannot import)", (const clang::NamedDecl*))
diff --git a/include/swift/ClangImporter/ClangImporter.h b/include/swift/ClangImporter/ClangImporter.h
@@ -705,6 +705,9 @@ bool isCxxConstReferenceType(const clang::Type *type);
 /// Determine whether this typedef is a CF type.
 bool isCFTypeDecl(const clang::TypedefNameDecl *Decl);
 
+// Determine whether type is a c++ foreign reference type.
+bool isForeignReferenceTypeWithoutImmortalAttrs(const clang::QualType type);
+
 /// Determine the imported CF type for the given typedef-name, or the empty
 /// string if this is not an imported CF type name.
 llvm::StringRef getCFTypeName(const clang::TypedefNameDecl *decl);
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -7440,6 +7440,35 @@ static bool hasImportAsRefAttr(const clang::RecordDecl *decl) {
          });
 }
 
+// TODO: Move all these utility functions in a new file ClangImporterUtils.h
+// rdar://138803759
+static bool hasImmortalAtts(const clang::RecordDecl *decl) {
+  return decl->hasAttrs() && llvm::any_of(decl->getAttrs(), [](auto *attr) {
+           if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr))
+             return swiftAttr->getAttribute() == "retain:immortal" ||
+                    swiftAttr->getAttribute() == "release:immortal";
+           return false;
+         });
+}
+
+namespace swift {
+namespace importer {
+// Is this a pointer to a foreign reference type.
+bool isForeignReferenceTypeWithoutImmortalAttrs(const clang::QualType type) {
+  if (!type->isPointerType())
+    return false;
+
+  auto pointeeType =
+      dyn_cast<clang::RecordType>(type->getPointeeType().getCanonicalType());
+  if (pointeeType == nullptr)
+    return false;
+
+  return hasImportAsRefAttr(pointeeType->getDecl()) &&
+         !hasImmortalAtts(pointeeType->getDecl());
+}
+} // namespace importer
+} // namespace swift
+
 // Is this a pointer to a foreign reference type.
 static bool isForeignReferenceType(const clang::QualType type) {
   if (!type->isPointerType())
diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp
@@ -25,6 +25,7 @@
 #include "swift/AST/ConformanceLookup.h"
 #include "swift/AST/Decl.h"
 #include "swift/AST/DiagnosticsClangImporter.h"
+#include "swift/ClangImporter/ClangImporter.h"
 #include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/Expr.h"
 #include "swift/AST/GenericEnvironment.h"
@@ -96,6 +97,8 @@ struct AccessorInfo {
 
 } // end anonymous namespace
 
+// extern bool isForeignReferenceType(const clang::QualType type);
+
 static bool isInSystemModule(const DeclContext *D) {
   return cast<ClangModuleUnit>(D->getModuleScopeContext())->isSystemModule();
 }
@@ -2685,7 +2688,7 @@ namespace {
           Impl.diagnoseTopLevelValue(
               DeclName(Impl.SwiftContext.getIdentifier(releaseOperation.name)));
         }
-      }else if (releaseOperation.kind ==
+      } else if (releaseOperation.kind ==
                  CustomRefCountingOperationResult::tooManyFound) {
         HeaderLoc loc(decl->getLocation());
         Impl.diagnose(loc,
@@ -3353,13 +3356,11 @@ namespace {
         return property->getParsedAccessor(AccessorKind::Set);
       }
 
-      // If a C++ decl is annotated with both swift_attr("returns_retained") and
-      // swift_attr("returns_unretained") then emit an error in the swift
-      // compiler. Note: this error is not emitted in the clang compiler because
-      // these attributes are used only for swift interop.
+      // Emit diagnostics for incorrect usage of "returns_unretained" and
+      // "returns_unretained" attributes
+      bool returnsRetainedAttrIsPresent = false;
+      bool returnsUnretainedAttrIsPresent = false;
       if (decl->hasAttrs()) {
-        bool returnsRetainedAttrIsPresent = false;
-        bool returnsUnretainedAttrIsPresent = false;
         for (const auto *attr : decl->getAttrs()) {
           if (const auto *swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr)) {
             if (swiftAttr->getAttribute() == "returns_unretained") {
@@ -3369,11 +3370,27 @@ namespace {
             }
           }
         }
+      }
 
+      HeaderLoc loc(decl->getLocation());
+      if (isForeignReferenceTypeWithoutImmortalAttrs(decl->getReturnType())) {
         if (returnsRetainedAttrIsPresent && returnsUnretainedAttrIsPresent) {
-          HeaderLoc loc(decl->getLocation());
           Impl.diagnose(loc, diag::both_returns_retained_returns_unretained,
                         decl);
+        } else if (!returnsRetainedAttrIsPresent &&
+                   !returnsUnretainedAttrIsPresent) {
+          Impl.diagnose(loc, diag::no_returns_retained_returns_unretained,
+                        decl);
+          // TODO: make this case an error in next cxx-interop versions
+          // rdar://138806722
+        }
+      } else {
+        if (returnsRetainedAttrIsPresent || returnsUnretainedAttrIsPresent) {
+          Impl.diagnose(
+              loc,
+              diag::
+                  returns_retained_or_returns_unretained_for_non_cxx_frt_values,
+              decl);
         }
       }
 
diff --git a/test/Interop/Cxx/foreign-reference/Inputs/cxx-functions-and-methods-returning-frt.h b/test/Interop/Cxx/foreign-reference/Inputs/cxx-functions-and-methods-returning-frt.h
@@ -160,26 +160,31 @@ struct
 
 struct NonFRTStruct {};
 
+// C++ APIs returning FRTs but not having annotations
+struct
+    StructWithoutReturnsAnnotations {
+  static FRTStruct *_Nonnull StaticMethodReturningFRT();
+};
+
+FRTStruct *_Nonnull global_function_returning_FRT_WithoutReturnsAnnotations();
+
+// C++ APIs not returning FRTs but having annotations
+struct
+    StructWithReturnsAnnotation {
+  static NonFRTStruct *_Nonnull StaticMethodReturningNonFRT() __attribute__((swift_attr("returns_retained")));
+};
+
+NonFRTStruct *_Nonnull global_function_returning_NON_FRT_WithReturnsAnnotations() __attribute__((swift_attr("returns_retained")));
+
+
 // Global/free C++ functions returning non-FRT
 NonFRTStruct *_Nonnull global_function_returning_non_FRT();
-NonFRTStruct
-    *_Nonnull global_function_returning_non_FRT_with_attr_returns_retained()
-        __attribute__((swift_attr("returns_retained")));
-NonFRTStruct
-    *_Nonnull global_function_returning_non_FRT_with_attr_returns_unretained()
-        __attribute__((swift_attr("returns_unretained")));
 NonFRTStruct *_Nonnull global_function_returning_non_FRT_create();
 NonFRTStruct *_Nonnull global_function_returning_non_FRT_copy();
 
 // Struct having static method returning non-FRT
 struct StructWithStaticMethodsReturningNonFRT {
   static NonFRTStruct *_Nonnull StaticMethodReturningNonFRT();
-  static NonFRTStruct
-      *_Nonnull StaticMethodReturningNonFRTWithAttrReturnsRetained()
-          __attribute__((swift_attr("returns_retained")));
-  static NonFRTStruct
-      *_Nonnull StaticMethodReturningNonFRTWithAttrReturnsUnretained()
-          __attribute__((swift_attr("returns_unretained")));
   static NonFRTStruct *_Nonnull StaticMethodReturningNonFRT_create();
   static NonFRTStruct *_Nonnull StaticMethodReturningNonFRT_copy();
 };
diff --git a/test/Interop/Cxx/foreign-reference/frt-retained-unretained-attributes-error.swift b/test/Interop/Cxx/foreign-reference/frt-retained-unretained-attributes-error.swift
@@ -8,3 +8,18 @@ let frtLocalVar1 = global_function_returning_FRT_with_both_attrs_returns_retaine
 
 let frtLocalVar2 = StructWithStaticMethodsReturningFRTWithBothAttributesReturnsRetainedAndReturnsUnretained.StaticMethodReturningFRT()
 // CHECK: error: 'StaticMethodReturningFRT' cannot be annotated with both swift_attr('returns_retained') and swift_attr('returns_unretained') attributes
+
+let frtLocalVar3 = global_function_returning_FRT_WithoutReturnsAnnotations()
+// CHECK: warning: 'global_function_returning_FRT_WithoutReturnsAnnotations' is returning a 'SWIFT_SHARED_REFERENCE' type but is not annotated with either swift_attr('returns_retained') or swift_attr('returns_unretained') attributes
+
+let frtLocalVar4 = StructWithoutReturnsAnnotations.StaticMethodReturningFRT()
+// CHECK: warning: 'StaticMethodReturningFRT' is returning a 'SWIFT_SHARED_REFERENCE' type but is not annotated with either swift_attr('returns_retained') or swift_attr('returns_unretained') attributes
+
+let frtLocalVar5 = global_function_returning_NON_FRT_WithReturnsAnnotations()
+// CHECK: error: 'global_function_returning_NON_FRT_WithReturnsAnnotations' cannot be annotated with either swift_attr('returns_retained') or swift_attr('returns_unretained') attribute because it is not returning a 'SWIFT_SHARED_REFERENCE' type
+
+let frtLocalVar6 = StructWithReturnsAnnotation.StaticMethodReturningNonFRT()
+// CHECK: error: 'StaticMethodReturningNonFRT' cannot be annotated with either swift_attr('returns_retained') or swift_attr('returns_unretained') attribute because it is not returning a 'SWIFT_SHARED_REFERENCE' type
+
+// TODO: Add tests to check the absence of diagnostics when retrun type is SWIFT_IMMORTAL_REFERENCE or SWIFT_UNSAFE_REFERENCE
+// TODO: Add test to check the absence of diagnostics for certain cases when return type is not a C++ FRT
diff --git a/test/Interop/Cxx/foreign-reference/frt-retained-unretained-attributes.swift b/test/Interop/Cxx/foreign-reference/frt-retained-unretained-attributes.swift
@@ -167,12 +167,6 @@ func testFreeFunctionsReturningNonFRT() {
     let frtLocalVar1 = global_function_returning_non_FRT()
     // CHECK: function_ref @{{.*}}global_function_returning_non_FRT{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
 
-    let frtLocalVar2 = global_function_returning_non_FRT_with_attr_returns_retained()
-    // CHECK: function_ref @{{.*}}global_function_returning_non_FRT_with_attr_returns_retained{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
-
-    let frtLocalVar3 = global_function_returning_non_FRT_with_attr_returns_unretained()
-    // CHECK: function_ref @{{.*}}global_function_returning_non_FRT_with_attr_returns_unretained{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
-
     let frtLocalVar4 = global_function_returning_non_FRT_create()
     // CHECK: function_ref @{{.*}}global_function_returning_non_FRT_create{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
 
@@ -184,12 +178,6 @@ func testStaticMethodsReturningNonFRT() {
     let frtLocalVar1 = StructWithStaticMethodsReturningNonFRT.StaticMethodReturningNonFRT()
     // CHECK: function_ref @{{.*}}StaticMethodReturningNonFRT{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
 
-    let frtLocalVar2 = StructWithStaticMethodsReturningNonFRT.StaticMethodReturningNonFRTWithAttrReturnsRetained()
-    // CHECK: function_ref @{{.*}}StaticMethodReturningNonFRTWithAttrReturnsRetained{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
-
-    let frtLocalVar3 = StructWithStaticMethodsReturningNonFRT.StaticMethodReturningNonFRTWithAttrReturnsUnretained()
-    // CHECK: function_ref @{{.*}}StaticMethodReturningNonFRTWithAttrReturnsUnretained{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
-
     let frtLocalVar4 = StructWithStaticMethodsReturningNonFRT.StaticMethodReturningNonFRT_create()
     // CHECK: function_ref @{{.*}}StaticMethodReturningNonFRT_create{{.*}} : $@convention(c) () -> UnsafeMutablePointer<NonFRTStruct> 
 
