Skip to content

[cxx-interop] convert CXXForeignReferenceTypeInitializers into SuppressCXXForeignReferenceTypeInitializers to be an opt-out flag #80659

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

Jump to bottom
Merged
merged 2 commits into from
Apr 10, 2025
Merged

[cxx-interop] convert CXXForeignReferenceTypeInitializers into SuppressCXXForeignReferenceTypeInitializers to be an opt-out flag #80659

merged 2 commits into from
Apr 10, 2025

Conversation

fahadnayyar
Copy link
Contributor

@fahadnayyar fahadnayyar commented Apr 8, 2025 •
edited
Loading

Turning the feature "ctor of C++ foreign reference types is callable as Swift Initializers" on by default.

rdar://148285972

@fahadnayyar
[cxx-interop] convert CXXForeignReferenceTypeInitializers into Suppre…
29eb408 
…ssCXXForeignReferenceTypeInitializers to be an opt-out flag
@fahadnayyar
Fixed CI issues (abstract C++ class and SWIFT_RETURNS(UN)RETAINED dia…
bb812ff 
…gnostics in case of inferred c++ foreign reference types)
@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test

@fahadnayyar
Copy link
Contributor Author

NOTE: I am addressing rdar://145098078 as well in this patch.
SWIFT_RETURNS_(UN)RETAINED related diagnostics (specially the warning in case of unannotated C++ APIs returning SWIFT_SHARED_REFERENCE types) were not triggering for cases where the return type in the C++ API is not annotated with SWIFT_SHARED_REFERENCE directly but it derives the reference semantics from one of its parent type via C++ inheritance.

We still need to address rdar://148952864 which is a minor bug. Now we also see diagnostics for C++ APIs returning a type which was derived from a SWIFT_IMMORTAL_REFERNECE. The diagnostic does not trigger when return type is SWIFT_IMMORTAL_REFERNECE but only when IMMORTAL_REFERNECE behaviour is inferred from a base type via C++ inheritance.

@fahadnayyar fahadnayyar self-assigned this Apr 10, 2025
@fahadnayyar fahadnayyar added the c++ interop Feature: Interoperability with C++ label Apr 10, 2025
@fahadnayyar fahadnayyar marked this pull request as ready for review April 10, 2025 05:09
@fahadnayyar fahadnayyar enabled auto-merge (squash) April 10, 2025 05:10
@fahadnayyar fahadnayyar merged commit 0ae0528 into swiftlang:main Apr 10, 2025
3 checks passed
Xazax-hun
Xazax-hun reviewed Apr 10, 2025
View reviewed changes
Copy link
Contributor

@Xazax-hun Xazax-hun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I see that this is already merged. Could you address the reviews in a follow-up?

Also, I strongly discourage PRs that are changing two unrelated features at the same time. The changes to importing the FRT ctors and the changes to the diagnostics could have been two independent PRs, I don't see any dependencies between them.

Edit: my concerns are now resolved.

lib/ClangImporter/SwiftDeclSynthesizer.cpp
@@ -2538,6 +2538,9 @@ llvm::SmallVector<clang::CXXMethodDecl *, 4>
SwiftDeclSynthesizer::synthesizeStaticFactoryForCXXForeignRef(
const clang::CXXRecordDecl *cxxRecordDecl) {

if (cxxRecordDecl->isAbstract())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test covering this branch? I am ok with the change but would be nice to have it actually tested.

test/Interop/Cxx/foreign-reference/pod-module-interface.swift
@@ -1,7 +1,7 @@
// RUN: %target-swift-ide-test -print-module -module-to-print=POD -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s

// CHECK: class Empty {
// CHECK-NOT: init
// CHECK: init
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it is not the right indentation.

test/Interop/Cxx/symbolic-imports/print-symbolic-module-interface.swift
@@ -119,6 +119,7 @@ struct NonCopyable {
// CHECK-NEXT: }
// CHECK-NEXT: }
// CHECK: class MyImmortal {
// CHECK-NEXT: init
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: indent the same as the function below.

lib/ClangImporter/ImportDecl.cpp
@@ -3609,7 +3609,18 @@ namespace {
isa<clang::FunctionDecl>(decl)
? cast<clang::FunctionDecl>(decl)->getReturnType()
: cast<clang::ObjCMethodDecl>(decl)->getReturnType();
if (isForeignReferenceTypeWithoutImmortalAttrs(retType)) {
clang::QualType pointeeType = retType;
Copy link
Contributor

Choose a reason for hiding this comment

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

This was the only use of isForeignReferenceTypeWithoutImmortalAttrs. I think we could have fixed that function. Now I think it is unused so let's remove it.

@fahadnayyar fahadnayyar mentioned this pull request Apr 10, 2025
Merged
Xazax-hun
Xazax-hun reviewed Apr 10, 2025
View reviewed changes
Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

We talked offline. The seemingly unrelated and untested changes were all responses to existing tests failing so everything should have at least indirect test coverage. Moreover, every change in this PR is actually motivated. The UI does not let me approve the PR, but I consider this approved on my end. The nits can be fixed in a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xazax-hun Xazax-hun Xazax-hun left review comments

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@egorzhdan egorzhdan Awaiting requested review from egorzhdan egorzhdan is a code owner

@j-hui j-hui Awaiting requested review from j-hui j-hui is a code owner

@susmonteiro susmonteiro Awaiting requested review from susmonteiro susmonteiro is a code owner

@hnrklssn hnrklssn Awaiting requested review from hnrklssn hnrklssn is a code owner

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

@ian-twilightcoder ian-twilightcoder Awaiting requested review from ian-twilightcoder ian-twilightcoder is a code owner

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

@DougGregor DougGregor Awaiting requested review from DougGregor DougGregor is a code owner

Assignees

@fahadnayyar fahadnayyar

Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fahadnayyar @Xazax-hun