Merge pull request #85021 from j-hui/return-frt-diagnostics

[cxx-interop] Put RETURNS_RETAINED warnings in a diagnostic group

rdar://161932849
rdar://144976203

Author: j-hui

include/swift/AST/DiagnosticGroups.def

@@ -46,6 +46,7 @@ GROUP(AvailabilityUnrecognizedName, "availability-unrecognized-name")
 GROUP(ClangDeclarationImport, "clang-declaration-import")
 GROUP(CompilationCaching, "compilation-caching")
 GROUP(ConformanceIsolation, "conformance-isolation")
+GROUP(ForeignReferenceType, "foreign-reference-type")
 GROUP(DeprecatedDeclaration, "deprecated-declaration")
 GROUP(DynamicCallable, "dynamic-callable-requirements")
 GROUP(EmbeddedRestrictions, "embedded-restrictions")

include/swift/AST/DiagnosticsSema.def

@@ -2211,12 +2211,12 @@ ERROR(expose_nested_type_to_cxx,none,
       "nested %kind0 can not yet be represented in C++", (ValueDecl *))
 ERROR(expose_macro_to_cxx,none,
       "Swift macro can not yet be represented in C++", (ValueDecl *))
-WARNING(warn_unannotated_cxx_func_returning_frt, none,
-        "cannot infer the ownership of the returned value, annotate %0 with "
-        "either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED",
+GROUPED_WARNING(warn_unannotated_cxx_func_returning_frt, ForeignReferenceType, none,
+        "cannot infer ownership of foreign reference value returned by %0",
+        (const ValueDecl *))
+NOTE(note_unannotated_cxx_func_returning_frt, none,
+        "annotate %0 with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED",
         (const ValueDecl *))
-NOTE(note_unannotated_cxx_func_returning_frt, none, "%0 is defined here",
-     (const ValueDecl *))
 ERROR(unexposed_other_decl_in_cxx,none,
       "%kind0 is not yet exposed to C++", (ValueDecl *))
 ERROR(unsupported_other_decl_in_cxx,none,

include/swift/Basic/Features.def

@@ -498,10 +498,9 @@ EXPERIMENTAL_FEATURE(AssumeResilientCxxTypes, true)
 /// Import inherited non-public members when importing C++ classes.
 EXPERIMENTAL_FEATURE(ImportNonPublicCxxMembers, true)
 
-/// Emit a warning when a C++ API returns a SWIFT_SHARED_REFERENCE type
-/// without being explicitly annotated with either SWIFT_RETURNS_RETAINED
-/// or SWIFT_RETURNS_UNRETAINED.
-EXPERIMENTAL_FEATURE(WarnUnannotatedReturnOfCxxFrt, true)
+/// Suppress the synthesis of static factory methods for C++ foreign reference
+/// types and importing them as Swift initializers.
+EXPERIMENTAL_FEATURE(SuppressCXXForeignReferenceTypeInitializers, true)
 
 /// modify/read single-yield coroutines
 SUPPRESSIBLE_EXPERIMENTAL_FEATURE(CoroutineAccessors, true)

lib/AST/FeatureSet.cpp

@@ -333,7 +333,7 @@ UNINTERESTING_FEATURE(LibraryEvolution)
 UNINTERESTING_FEATURE(SafeInteropWrappers)
 UNINTERESTING_FEATURE(AssumeResilientCxxTypes)
 UNINTERESTING_FEATURE(ImportNonPublicCxxMembers)
-UNINTERESTING_FEATURE(WarnUnannotatedReturnOfCxxFrt)
+UNINTERESTING_FEATURE(SuppressCXXForeignReferenceTypeInitializers)
 UNINTERESTING_FEATURE(CoroutineAccessorsUnwindOnCallerError)
 UNINTERESTING_FEATURE(AllowRuntimeSymbolDeclarations)
 

lib/Sema/MiscDiagnostics.cpp

@@ -6375,8 +6375,6 @@ static bool isReturningFRT(const clang::NamedDecl *ND,
 static bool shouldDiagnoseMissingReturnsRetained(const clang::NamedDecl *ND,
                                                  clang::QualType retType,
                                                  ASTContext &Ctx) {
-  if (!Ctx.LangOpts.hasFeature(Feature::WarnUnannotatedReturnOfCxxFrt))
-    return false;
 
   auto attrInfo = importer::ReturnOwnershipInfo(ND);
   if (attrInfo.hasRetainAttr())
@@ -6454,7 +6452,7 @@ static void diagnoseCxxFunctionCalls(const Expr *E, const DeclContext *DC) {
       if (shouldDiagnoseMissingReturnsRetained(ND, retType, Ctx)) {
         SourceLoc diagnosticLoc = func->getLoc();
         if (diagnosticLoc.isInvalid() && func->getClangDecl()) {
-          // Fixme: Remove the diagnosticLoc once the source locations of the
+          // FIXME: Remove the diagnosticLoc once the source locations of the
           // objc method declarations are imported correctly.
           diagnosticLoc = Ctx.getClangModuleLoader()->importSourceLocation(
               ND->getLocation());

test/Interop/C/struct/Inputs/module.modulemap

@@ -18,3 +18,7 @@ module StructAsOptionSet {
 module NoncopyableStructs {
   header "noncopyable-struct.h"
 }
+
+module ReturnForeignReference {
+  header "return-foreign-reference.h"
+}

test/Interop/C/struct/Inputs/return-foreign-reference.h

@@ -0,0 +1,33 @@
+#ifndef __RETURN_FOREIGN_REFERENCE_H
+#define __RETURN_FOREIGN_REFERENCE_H
+
+struct
+__attribute__((swift_attr("import_reference")))
+__attribute__((swift_attr("retain:FRTRetain")))
+__attribute__((swift_attr("release:FRTRelease")))
+FRT {
+  int value;
+};
+
+struct Value { int value; };
+
+void FRTRetain(struct FRT *x);
+void FRTRelease(struct FRT *x);
+
+struct FRT *getFRTNoAnnotations(void); // expected-note {{annotate}}
+struct FRT *getFRTRetained() __attribute__((swift_attr("returns_retained")));
+struct FRT *getFRTUnretained() __attribute__((swift_attr("returns_unretained")));
+
+struct FRT *getFRTUnretained() __attribute__((swift_attr("returns_unretained")));
+
+struct FRT *getFRTConflictingAnnotations()
+    __attribute__((swift_attr("returns_retained")))
+    __attribute__((swift_attr("returns_unretained")));
+// expected-error@-3 {{cannot be annotated with both}}
+
+struct Value *getValueRetained() __attribute__((swift_attr("returns_retained")));
+// expected-warning@-1 {{should not be annotated}}
+struct Value *getValueUnretained() __attribute__((swift_attr("returns_unretained")));
+// expected-warning@-1 {{should not be annotated}}
+
+#endif /* __RETURN_FOREIGN_REFERENCE_H */

test/Interop/C/struct/return-foreign-reference-typechecker.swift

@@ -0,0 +1,17 @@
+// RUN: %target-typecheck-verify-swift -cxx-interoperability-mode=off \
+// RUN:   -disable-availability-checking \
+// RUN:   -I %S%{fs-sep}Inputs \
+// RUN:   -verify-additional-file %S%{fs-sep}Inputs%{fs-sep}return-foreign-reference.h
+
+import ReturnForeignReference
+
+let _: FRT = getFRTNoAnnotations() // expected-warning {{cannot infer ownership}}
+let _: FRT = getFRTRetained()
+let _: FRT = getFRTUnretained()
+
+
+// These calls to incorrectly annotated functions are necessary to force
+// diagnostics to appear in the header file.
+let _: FRT = getFRTConflictingAnnotations()
+let _ = getValueRetained()
+let _ = getValueUnretained()

test/Interop/Cxx/class/safe-interop-mode.swift

@@ -97,7 +97,8 @@ using TTakeUnsafeTuple = TTake<UnsafeTuple>;
 struct HoldsShared {
   SharedObject* obj;
 
-  SharedObject* getObj() const SWIFT_RETURNS_INDEPENDENT_VALUE;
+  SharedObject* getObj() const SWIFT_RETURNS_INDEPENDENT_VALUE
+                               SWIFT_RETURNS_UNRETAINED;
 };
 
 //--- test.swift

test/Interop/Cxx/foreign-reference/Inputs/cxx-functions-and-methods-returning-frt.h

@@ -179,12 +179,12 @@ __attribute__((swift_attr("unsafe")));
 
 // C++ APIs returning cxx frts (for testing diagnostics)
 struct StructWithAPIsReturningCxxFrt {
-  static FRTStruct *_Nonnull StaticMethodReturningCxxFrt(); // expected-note {{'StaticMethodReturningCxxFrt()' is defined here}}
+  static FRTStruct *_Nonnull StaticMethodReturningCxxFrt(); // expected-note {{annotate 'StaticMethodReturningCxxFrt()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
   static FRTStruct *_Nonnull StaticMethodReturningCxxFrtWithAnnotation()
       __attribute__((swift_attr("returns_retained")));
 };
 
-FRTStruct *_Nonnull global_function_returning_cxx_frt(); // expected-note {{'global_function_returning_cxx_frt()' is defined here}}
+FRTStruct *_Nonnull global_function_returning_cxx_frt(); // expected-note {{annotate 'global_function_returning_cxx_frt()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
 FRTStruct *_Nonnull global_function_returning_cxx_frt_with_annotations()
     __attribute__((swift_attr("returns_retained")));
 
@@ -303,7 +303,9 @@ __attribute__((swift_attr(
   operator-(const FRTOverloadedOperators &other);
 };
 
-FRTOverloadedOperators *_Nonnull returnFRTOverloadedOperators(); // expected-note {{'returnFRTOverloadedOperators()' is defined here}} // expected-note {{'returnFRTOverloadedOperators()' is defined here}}
+FRTOverloadedOperators *_Nonnull returnFRTOverloadedOperators();
+// expected-note@-1 {{annotate 'returnFRTOverloadedOperators()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
+// expected-note@-2 {{annotate 'returnFRTOverloadedOperators()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
 
 void retain_FRTOverloadedOperators(FRTOverloadedOperators *_Nonnull v);
 void release_FRTOverloadedOperators(FRTOverloadedOperators *_Nonnull v);
@@ -371,7 +373,7 @@ struct __attribute__((swift_attr("import_reference")))
 __attribute__((swift_attr("retain:rretain")))
 __attribute__((swift_attr("release:rrelease"))) RefType {};
 
-RefType *returnRefType() { return new RefType(); } // expected-note {{'returnRefType()' is defined here}}
+RefType *returnRefType() { return new RefType(); } // expected-note {{annotate 'returnRefType()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
 
 struct __attribute__((swift_attr("import_reference")))
 __attribute__((swift_attr("retain:dretain")))
@@ -420,10 +422,10 @@ __attribute__((swift_attr("retain:dRetain")))
 __attribute__((swift_attr("release:dRelease"))) DerivedTypeNonDefault
     : public BaseTypeNonDefault {};
 
-BaseTypeNonDefault *createBaseTypeNonDefault() { // expected-note {{'createBaseTypeNonDefault()' is defined here}}
+BaseTypeNonDefault *createBaseTypeNonDefault() { // expected-note {{annotate 'createBaseTypeNonDefault()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
   return new BaseTypeNonDefault();
 }
-DerivedTypeNonDefault *createDerivedTypeNonDefault() { // expected-note {{'createDerivedTypeNonDefault()' is defined here}}
+DerivedTypeNonDefault *createDerivedTypeNonDefault() { // expected-note {{annotate 'createDerivedTypeNonDefault()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
     return new DerivedTypeNonDefault();
 }
 
@@ -464,7 +466,9 @@ __attribute__((swift_attr("release:release_SourceLocCacheB"))) TypeB {};
 
 template <typename T>
 struct Factory {
-  static T *make() { return new T(); } // expected-note {{'make()' is defined here}} // expected-note {{'make()' is defined here}}
+  static T *make() { return new T(); }
+  // expected-note@-1 {{annotate 'make()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
+  // expected-note@-2 {{annotate 'make()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
 };
 
 using FactoryA = Factory<TypeA>;
@@ -508,7 +512,7 @@ class RefTemplate<FRTStruct> {
     return &instance;
   }
 
-  FRTStruct* value() const { // expected-note {{'value()' is defined here}}
+  FRTStruct* value() const { // expected-note {{annotate 'value()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
     return new FRTStruct;
   }
 };