Skip to content

[cxx-interop] Allow virtual methods to be renamed with SWIFT_NAME #82485

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 1 commit into from
Jul 3, 2025
Merged

[cxx-interop] Allow virtual methods to be renamed with SWIFT_NAME #82485

merged 1 commit into from
Jul 3, 2025

Conversation

susmonteiro
Copy link
Contributor

The swift_name attribute can now be used to rename virtual methods.

rdar://152583825

@susmonteiro susmonteiro added the c++ interop Feature: Interoperability with C++ label Jun 25, 2025
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

egorzhdan
egorzhdan reviewed Jun 25, 2025
View reviewed changes
lib/ClangImporter/SwiftDeclSynthesizer.h Outdated
@@ -318,11 +318,13 @@ class SwiftDeclSynthesizer {
ReferenceReturnTypeBehaviorForBaseMethodSynthesis
referenceReturnTypeBehavior =
ReferenceReturnTypeBehaviorForBaseMethodSynthesis::KeepReference,
bool forceConstQualifier = false);
bool forceConstQualifier = false,
std::optional<importer::ImportedName> importedName = std::nullopt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'd say it isn't really clear here when the importedName needs to be non-null, and when it can be null.

lib/ClangImporter/SwiftDeclSynthesizer.h Outdated

/// Given an overload of a C++ virtual method on a reference type, create a
/// method that dispatches the call dynamically.
FuncDecl *makeVirtualMethod(const clang::CXXMethodDecl *clangMethodDecl);
FuncDecl *makeVirtualMethod(const clang::CXXMethodDecl *clangMethodDecl,
importer::ImportedName importedName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this parameter and call importName from the implementation of this method?

lib/ClangImporter/ImportDecl.cpp Outdated
Comment on lines 4399 to 4400
if (!importedName)
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can rely on the name not being nullptr if we got to this point.

@Xazax-hun
Copy link
Contributor

What is the model here? If someone renames a virtual function do they need to rename all the overrides? What happens if an override is not renamed?

@susmonteiro
Copy link
Contributor Author

What is the model here? If someone renames a virtual function do they need to rename all the overrides? What happens if an override is not renamed?

That's a good question! At the moment, if you don't add the attribute to the override, you can call it by using either the original name or the swift_name of the base class. If you add the attribute to both, renaming the functions to the same thing, only the new name will work. And if you add the attribute to both and rename the functions to different things, then you can call the override by the either the base swift_name or the override swift_name, but not by the original name.

It's a bit messy, we should come up with a better model.

@Xazax-hun
Copy link
Contributor

It's a bit messy, we should come up with a better model.

I see, thanks for elaborating! Maybe it would be great to have test for the cases you mentioned. I don't have a strong opinion about what we want to do here but in case we make it more strict in the future that would be a source breaking change.

I think the options we have:

  • Let users rename any overrides independently of each other. This can be helpful so that users can call every override independently. Without this, they might not have a language feature in Swift to do this for value types as we can never represent the full C++ hierarchy for value types in Swift
  • Force users to rename all the overrides explicitly
  • Only let users to rename the base virtual method but not the overrides. Apply the renaming to the overrides automatically

I do not have a strong opinion which one is the best. As long as we do not rush this to 6.2, I am OK merging this as is and figuring out what we want to do before the next stable release.

@susmonteiro susmonteiro force-pushed the susmonteiro/rename-virtual-methods branch from 9a3fd10 to c10dbc4 Compare June 27, 2025 14:12
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro force-pushed the susmonteiro/rename-virtual-methods branch from c10dbc4 to 9838de6 Compare June 27, 2025 15:35
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

egorzhdan
egorzhdan reviewed Jun 30, 2025
View reviewed changes
test/Interop/Cxx/class/inheritance/virtual-methods.swift Outdated
@@ -64,4 +64,12 @@ VirtualMethodsTestSuite.test("C++ virtual method with complex parameter") {
}
#endif

if #available(SwiftStdlib 5.8, *) {
VirtualMethodsTestSuite.test("renamed C++ virtual method") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: since this is testing foreign reference types, could you please move this to Interop/Cxx/foreign-reference/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I ended up moving all the foreign reference types tests to Interop/Cxx/foreign-reference/member-inheritance-<...>. I'm also happy to move them to Interop/Cxx/foreign-reference/inheritance-<...> if you think it's unnecessary to create 2 new test files. Since Interop/Cxx/foreign-reference/member-inheritance.swift already existed, it made sense to me to keep member inheritance separated from general inheritance.

@susmonteiro susmonteiro force-pushed the susmonteiro/rename-virtual-methods branch 2 times, most recently from 2bb8125 to 4e710c5 Compare July 2, 2025 15:06
@susmonteiro susmonteiro force-pushed the susmonteiro/rename-virtual-methods branch from 4e710c5 to b3e2288 Compare July 2, 2025 15:09
@susmonteiro
Copy link
Contributor Author

Regarding overrides of renamed virtual methods...

I think the options we have:

  • Let users rename any overrides independently of each other. This can be helpful so that users can call every override independently. Without this, they might not have a language feature in Swift to do this for value types as we can never represent the full C++ hierarchy for value types in Swift
  • Force users to rename all the overrides explicitly
  • Only let users to rename the base virtual method but not the overrides. Apply the renaming to the overrides automatically

We’re opting for the last option here. It won’t allow users to call a specific override, as you mentioned in the first option, but that can be worked around. Additionally, we can develop a way to do that in the future. However, if we allow every override to have a different name, we’d lose virtual dispatch.

I will add the overrides implementation in a future patch.

@swift-ci please smoke test

egorzhdan
egorzhdan approved these changes Jul 3, 2025
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@susmonteiro susmonteiro merged commit 7c1852e into main Jul 3, 2025
3 checks passed
@susmonteiro susmonteiro deleted the susmonteiro/rename-virtual-methods branch July 3, 2025 14:19
@susmonteiro susmonteiro mentioned this pull request Jul 15, 2025
Open
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 approved these changes

@egorzhdan egorzhdan egorzhdan approved these changes

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

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

@fahadnayyar fahadnayyar Awaiting requested review from fahadnayyar fahadnayyar 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

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

Successfully merging this pull request may close these issues.
3 participants
@susmonteiro @Xazax-hun @egorzhdan