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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from
+36 −6

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

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

@egorzhdan egorzhdan egorzhdan left review comments

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

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