Skip to content

[cxx-interop] Enabling WarnUnannotatedReturnOfCxxFrt on by default and add it to a diagnostic group #84759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[cxx-interop] Enabling WarnUnannotatedReturnOfCxxFrt on by default and add it to a diagnostic group #84759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/swift/AST/DiagnosticGroups.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ GROUP(AlwaysAvailableDomain, "always-available-domain")
GROUP(AvailabilityUnrecognizedName, "availability-unrecognized-name")
GROUP(ClangDeclarationImport, "clang-declaration-import")
GROUP(ConformanceIsolation, "conformance-isolation")
GROUP(CxxForeignReferenceType, "cxx-foreign-reference-type")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the "cxx" from these. It's not a C++-specific feature.

GROUP(DeprecatedDeclaration, "deprecated-declaration")
GROUP(DynamicCallable, "dynamic-callable-requirements")
GROUP(EmbeddedRestrictions, "embedded-restrictions")
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2211,7 +2211,7 @@ 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,
GROUPED_WARNING(warn_unannotated_cxx_func_returning_frt, CxxForeignReferenceType, none,
"cannot infer the ownership of the returned value, annotate %0 with "
"either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED",
(const ValueDecl *))
Expand Down
5 changes: 1 addition & 4 deletions include/swift/Basic/Features.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ BASELINE_LANGUAGE_FEATURE(BuiltinUnprotectedStackAlloc, 0, "Builtin.unprotectedS
BASELINE_LANGUAGE_FEATURE(BuiltinTaskRunInline, 0, "Builtin.taskRunInline")
BASELINE_LANGUAGE_FEATURE(BuiltinUnprotectedAddressOf, 0, "Builtin.unprotectedAddressOf")
BASELINE_LANGUAGE_FEATURE(NewCxxMethodSafetyHeuristics, 0, "Only import C++ methods that return pointers (projections) on owned types as unsafe")
BASELINE_LANGUAGE_FEATURE(WarnUnannotatedReturnOfCxxFrt, 0, "Warn about unannotated C++ functions returning foreign reference types")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a feature for this at all, because nobody needs to do #if hasFeature(WarnUnannotatedReturnOfCxxFrt) or opt in to it

BASELINE_LANGUAGE_FEATURE(SpecializeAttributeWithAvailability, 0, "@_specialize attribute with availability")
BASELINE_LANGUAGE_FEATURE(BuiltinAssumeAlignment, 0, "Builtin.assumeAlignment")
BASELINE_LANGUAGE_FEATURE(BuiltinCreateTaskGroupWithFlags, 0, "Builtin.createTaskGroupWithFlags")
Expand Down Expand Up @@ -502,10 +503,6 @@ EXPERIMENTAL_FEATURE(ImportNonPublicCxxMembers, true)
/// types and importing them as Swift initializers.
EXPERIMENTAL_FEATURE(SuppressCXXForeignReferenceTypeInitializers, 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)

/// modify/read single-yield coroutines
SUPPRESSIBLE_EXPERIMENTAL_FEATURE(CoroutineAccessors, true)
Expand Down
1 change: 0 additions & 1 deletion lib/AST/FeatureSet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,6 @@ UNINTERESTING_FEATURE(SafeInteropWrappers)
UNINTERESTING_FEATURE(AssumeResilientCxxTypes)
UNINTERESTING_FEATURE(ImportNonPublicCxxMembers)
UNINTERESTING_FEATURE(SuppressCXXForeignReferenceTypeInitializers)
UNINTERESTING_FEATURE(WarnUnannotatedReturnOfCxxFrt)
UNINTERESTING_FEATURE(CoroutineAccessorsUnwindOnCallerError)
UNINTERESTING_FEATURE(AllowRuntimeSymbolDeclarations)

Expand Down
2 changes: 0 additions & 2 deletions lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6373,8 +6373,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())
Expand Down
4 changes: 1 addition & 3 deletions test/Interop/Cxx/foreign-reference/frt-reference-returns-diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// RUN: rm -rf %t
// RUN: %target-typecheck-verify-swift -I %S%{fs-sep}Inputs -cxx-interoperability-mode=default -enable-experimental-feature WarnUnannotatedReturnOfCxxFrt -verify-additional-file %S%{fs-sep}Inputs%{fs-sep}frt-reference-returns.h

// REQUIRES: swift_feature_WarnUnannotatedReturnOfCxxFrt
// RUN: %target-typecheck-verify-swift -I %S%{fs-sep}Inputs -cxx-interoperability-mode=default -verify-additional-file %S%{fs-sep}Inputs%{fs-sep}frt-reference-returns.h

import FRTReferenceReturns

Expand Down
3 changes: 1 addition & 2 deletions test/Interop/Cxx/foreign-reference/frt-retained-unretained-attributes-error.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// RUN: rm -rf %t
// RUN: %target-swift-frontend -typecheck -verify -I %S/Inputs %s -cxx-interoperability-mode=upcoming-swift -enable-experimental-feature WarnUnannotatedReturnOfCxxFrt -verify-additional-file %S/Inputs/cxx-functions-and-methods-returning-frt.h -Xcc -Wno-return-type -Xcc -Wno-nullability-completeness
// RUN: %target-swift-frontend -typecheck -verify -I %S/Inputs %s -cxx-interoperability-mode=upcoming-swift -verify-additional-file %S/Inputs/cxx-functions-and-methods-returning-frt.h -Xcc -Wno-return-type -Xcc -Wno-nullability-completeness

// XFAIL: OS=windows-msvc
// TODO: Enable this on windows when -verify-additional-file issue on Windows Swift CI is resolved

// REQUIRES: swift_feature_WarnUnannotatedReturnOfCxxFrt

import FunctionsAndMethodsReturningFRT
import CxxStdlib
Expand Down
3 changes: 1 addition & 2 deletions test/Interop/Cxx/foreign-reference/inheritance-diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// RUN: rm -rf %t
// RUN: %target-swift-frontend -typecheck -verify -I %S%{fs-sep}Inputs %s -cxx-interoperability-mode=upcoming-swift -enable-experimental-feature WarnUnannotatedReturnOfCxxFrt -verify-additional-file %S%{fs-sep}Inputs%{fs-sep}inheritance.h -Xcc -Wno-return-type -Xcc -Wno-inaccessible-base
// RUN: %target-swift-frontend -typecheck -verify -I %S%{fs-sep}Inputs %s -cxx-interoperability-mode=upcoming-swift -verify-additional-file %S%{fs-sep}Inputs%{fs-sep}inheritance.h -Xcc -Wno-return-type -Xcc -Wno-inaccessible-base

// REQUIRES: swift_feature_WarnUnannotatedReturnOfCxxFrt

import Inheritance

Expand Down
3 changes: 1 addition & 2 deletions test/Interop/Cxx/objc-correctness/objc-returning-cxx-frt-diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// RUN: rm -rf %t
// RUN: %target-swift-frontend -typecheck -verify -I %S/Inputs %s -cxx-interoperability-mode=upcoming-swift -enable-experimental-feature WarnUnannotatedReturnOfCxxFrt -verify-additional-file %S/Inputs/cxx-frt.h -Xcc -Wno-return-type
// RUN: %target-swift-frontend -typecheck -verify -I %S/Inputs %s -cxx-interoperability-mode=upcoming-swift -verify-additional-file %S/Inputs/cxx-frt.h -Xcc -Wno-return-type

// REQUIRES: swift_feature_WarnUnannotatedReturnOfCxxFrt

import CxxForeignRef

Expand Down
14 changes: 7 additions & 7 deletions test/Interop/CxxToSwiftToCxx/consuming-cxx-struct-parameter-back-to-cxx-execution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// RUN: %empty-directory(%t)
// RUN: split-file %s %t

// RUN: %target-swift-frontend %t/use-cxx-types.swift -module-name UseCxx -typecheck -verify -emit-clang-header-path %t/UseCxx.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
// RUN: %target-swift-frontend %t%{fs-sep}use-cxx-types.swift -module-name UseCxx -typecheck -verify -verify-additional-file %t%{fs-sep}header.h -emit-clang-header-path %t%{fs-sep}UseCxx.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking

// RUN: %target-interop-build-clangxx -std=c++20 -c %t/use-swift-cxx-types.cpp -I %t -o %t/swift-cxx-execution.o
// RUN: %target-interop-build-swift %t/use-cxx-types.swift -o %t/swift-cxx-execution -Xlinker %t/swift-cxx-execution.o -module-name UseCxx -Xfrontend -entry-point-function-name -Xfrontend swiftMain -I %t -O -Xfrontend -disable-availability-checking
// RUN: %target-interop-build-clangxx -std=c++20 -c %t%{fs-sep}use-swift-cxx-types.cpp -I %t -o %t%{fs-sep}swift-cxx-execution.o
// RUN: %target-interop-build-swift %t%{fs-sep}use-cxx-types.swift -o %t%{fs-sep}swift-cxx-execution -Xlinker %t%{fs-sep}swift-cxx-execution.o -module-name UseCxx -Xfrontend -entry-point-function-name -Xfrontend swiftMain -I %t -O -Xfrontend -disable-availability-checking

// RUN: %target-codesign %t/swift-cxx-execution
// RUN: %target-run %t/swift-cxx-execution | %FileCheck %s
// RUN: %target-codesign %t%{fs-sep}swift-cxx-execution
// RUN: %target-run %t%{fs-sep}swift-cxx-execution | %FileCheck %s

// REQUIRES: executable_test

Expand Down Expand Up @@ -63,7 +63,7 @@ __attribute__((swift_attr("release:releaseShared")));
inline void retainShared(SharedFRT *r) { puts("retainShared"); }
inline void releaseShared(SharedFRT *r) { puts("releaseShared"); }

inline SharedFRT* createSharedFRT() { return new SharedFRT(); }
inline SharedFRT* createSharedFRT() { return new SharedFRT(); } // expected-note {{'createSharedFRT()' is defined here}}

//--- module.modulemap
module CxxTest {
Expand Down Expand Up @@ -109,7 +109,7 @@ public func returnSharedFRT(_ x : SharedFRT) -> SharedFRT {
}

public func returnSharedFRT2() -> SharedFRT {
return createSharedFRT()
return createSharedFRT() // expected-warning {{cannot infer the ownership of the returned value, annotate 'createSharedFRT()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED}}
}

public struct ValueWrapper {
Expand Down
44 changes: 44 additions & 0 deletions userdocs/diagnostics/cxx-foreign-reference-type.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# C++ Foreign Reference Type warnings (CxxForeignReferenceType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I noted earlier, SWIFT_SHARED_REFERENCE isn't a C++-specific feature. There are C types that get imported as shared references, too. Please remove the C++-specific bits; generally speaking, replacing C++ with C in this markdown file looks sufficient.


Warnings related to C++ APIs returning foreign reference types that lack proper ownership annotations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the descriptive documentation group. It's very helpful to our users.


## Overview

When importing C++ types marked with `SWIFT_SHARED_REFERENCE` into Swift, the compiler needs to understand the ownership semantics of functions returning these types. Without explicit annotations, the compiler cannot determine whether the returned object should be retained or not, which can lead to memory management issues.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth saying that these types are imported as classes in Swift, and showing an example declaration like

typedef struct SWIFT_SHARED_REFERENCE(MyFRT_retain, MyFRT_release) MyFRT;


## The Warning

The compiler emits a warning when it encounters a C++ function or Objective-C method that:
1. Returns a type marked with `SWIFT_SHARED_REFERENCE` (foreign reference type)
2. Lacks either `SWIFT_RETURNS_RETAINED` or `SWIFT_RETURNS_UNRETAINED` annotation

Example warning:
```
warning: cannot infer the ownership of the returned value, annotate 'functionName()' with either SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED
```

## How to Fix

### For C++ Functions

Add the appropriate annotation to your C++ function declaration:

```cpp
// If the function returns a retained value
SWIFT_RETURNS_RETAINED MyFRT* createObject();

// If the function returns an unretained value
SWIFT_RETURNS_UNRETAINED MyFRT* getExistingObject();
```

### For Objective-C Methods

Similarly, annotate your Objective-C methods:

```objc
// Retained value
- (MyFRT *)createObject SWIFT_RETURNS_RETAINED;

// Unretained value
- (MyFRT *)getExistingObject SWIFT_RETURNS_UNRETAINED;
```