Skip to content

[cxx-interop] Support importing static factories as initializers #79288

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

Merged
merged 1 commit into from
Mar 3, 2025
Merged

[cxx-interop] Support importing static factories as initializers #79288

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
5 changes: 5 additions & 0 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3946,6 +3946,11 @@ namespace {
// FIXME: Poor location info.
auto nameLoc = Impl.importSourceLoc(decl->getLocation());

if (auto method = dyn_cast<clang::CXXMethodDecl>(decl);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Xazax-hun why did you choose this location in the importFunctionDecl function to implement this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the importFunctionDecl function that has most of the function importing logic. We usually calculate the Swift Name for an imported declaration relatively early on. So this logic belongs here, and relatively early in this function (but after we have the SwiftName). There is another call site of importGlobalAsInitializer in the same function to handle the non-C++ method case, which is a good indication that this is the right place.

The easiest way to find where to put some code is to step through the import process of a simple example in the debugger and see what is the earliest point where you have all the information you need and check where would the code logically belong after that point.

Copy link
Contributor Author

@Xazax-hun Xazax-hun Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another consideration is that we want to do this before we did any actual importingn (creating Swift AST) because we want to avoid importing the same function twice in two different ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks!

method && method->isStatic() && name.getBaseName().isConstructor()) {
return importGlobalAsInitializer(
decl, name, dc, importedName.getInitKind(), correctSwiftName);
}
AbstractFunctionDecl *result = nullptr;
if (auto *ctordecl = dyn_cast<clang::CXXConstructorDecl>(decl)) {
// Don't import copy constructor or move constructor -- these will be
Expand Down
42 changes: 42 additions & 0 deletions test/Interop/Cxx/foreign-reference/Inputs/constructor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#pragma once

struct __attribute__((swift_attr("import_reference")))
__attribute__((swift_attr("retain:retain")))
__attribute__((swift_attr("release:release"))) ImportWithCtor {
int value = 0;
int param1 = 0;
int param2 = 0;

__attribute__((swift_name("init()")))
__attribute__((swift_attr("returns_retained")))
static ImportWithCtor * _Nonnull create() {
return new ImportWithCtor{1};
}

__attribute__((swift_name("init(_:)")))
__attribute__((swift_attr("returns_retained")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would using returns_unretained be reasonable here in some cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Do you mean we should assume returns_retained? Or the fact that a user could pass in 0? I change the test to not pass in the initial refcount.

Copy link
Contributor

@egorzhdan egorzhdan Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not necessarily, I just wanted to check if we should test the other possible annotation, returns_unretained

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. It feels weird for a ctor to do returns_unretained. I think we should warn on that. I can do that in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we allow importing static factories as both returns_retained and returns_unretained for now? Once we have support for importing actual C++ constructors as Swift initializers then we can maybe think about a default behaviour for constructors and static factory methods. Seems like the default should be returns_retained but I am not aware if people are currently using returns_unretained for static factories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a test for the returns_unretained case. I think it might be a good idea to add diagnostics in a separate PR in this case as those can be reverted independently of the feature itself if we encounter an idiom we did not think about.

static ImportWithCtor * _Nonnull create(int x) {
return new ImportWithCtor{1, x};
}

__attribute__((swift_name("init(_:_:)")))
__attribute__((swift_attr("returns_retained")))
static ImportWithCtor * _Nonnull create(int x, int y) {
return new ImportWithCtor{1, x, y};
}

__attribute__((swift_name("init(_:_:_:)")))
__attribute__((swift_attr("returns_unretained")))
static ImportWithCtor * _Nonnull create(int x, int y, int z) {
return new ImportWithCtor{0, x, y};
}
};

inline void retain(ImportWithCtor * _Nonnull x) {
x->value++;
}

inline void release(ImportWithCtor * _Nonnull x) {
if (!--x->value)
delete x;
}
5 changes: 5 additions & 0 deletions test/Interop/Cxx/foreign-reference/Inputs/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
module Constructor {
header "constructor.h"
requires cplusplus
}

module POD {
header "pod.h"
requires cplusplus
Expand Down
25 changes: 25 additions & 0 deletions test/Interop/Cxx/foreign-reference/constructor-execution.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// RUN: %target-run-simple-swift(-I %S/Inputs -Xfrontend -enable-experimental-cxx-interop -Xfrontend -disable-availability-checking)
// REQUIRES: executable_test

import Constructor
import StdlibUnittest

var ForeignRefCtorSuite = TestSuite("ForeignRef Ctor")

ForeignRefCtorSuite.test("ImportStaticFactoryAsInitializer") {
let x = ImportWithCtor()
expectEqual(x.param1, 0)
expectEqual(x.param2, 0)
let y = x
let z = ImportWithCtor(1)
expectEqual(z.param1, 1)
expectEqual(z.param2, 0)
let z2 = ImportWithCtor(2, 3)
expectEqual(z2.param1, 2)
expectEqual(z2.param2, 3)
let z3 = ImportWithCtor(2, 3, 4)
expectEqual(z3.param1, 2)
expectEqual(z3.param2, 3)
}

runAllTests()