Merge pull request #83067 from susmonteiro/rename-virtual-methods

[cxx-interop] Adding swift_name attributes to virtual methods overrides

Author: susmonteiro

include/swift/AST/DiagnosticsClangImporter.def

@@ -73,6 +73,11 @@ GROUPED_WARNING(inconsistent_swift_name, ClangDeclarationImport, none,
         (bool, StringRef, StringRef, DeclName, StringRef, DeclName,
          StringRef))
 
+WARNING(swift_name_attr_ignored, none,
+        "ignoring swift_name attribute %0; swift_name attributes have no "
+        "effect on method overrides",
+        (DeclName))
+
 GROUPED_WARNING(swift_name_circular_context_import, ClangDeclarationImport, none,
         "cycle detected while resolving '%0' in swift_name attribute for '%1'",
         (StringRef, StringRef))

lib/ClangImporter/ClangImporter.cpp

@@ -6351,11 +6351,9 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
     // Capture the arity of already found members in the
     // current record, to avoid adding ambiguous members
     // from base classes.
-    const auto getArity =
-        ClangImporter::Implementation::getImportedBaseMemberDeclArity;
-    llvm::SmallSet<size_t, 4> foundNameArities;
+    llvm::SmallSet<DeclName, 4> foundMethodNames;
     for (const auto *valueDecl : result)
-      foundNameArities.insert(getArity(valueDecl));
+      foundMethodNames.insert(valueDecl->getName());
 
     for (auto base : cxxRecord->bases()) {
       if (skipIfNonPublic && base.getAccessSpecifier() != clang::AS_public)
@@ -6389,9 +6387,9 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
             {});
 
         for (auto foundInBase : baseResults) {
-          // Do not add duplicate entry with the same arity,
+          // Do not add duplicate entry with the same DeclName,
           // as that would cause an ambiguous lookup.
-          if (foundNameArities.count(getArity(foundInBase)))
+          if (foundMethodNames.count(foundInBase->getName()))
             continue;
 
           collector.add(foundInBase);
@@ -7694,6 +7692,7 @@ ValueDecl *ClangImporter::Implementation::importBaseMemberDecl(
   auto known = clonedBaseMembers.find(key);
   if (known == clonedBaseMembers.end()) {
     ValueDecl *cloned = cloneBaseMemberDecl(decl, newContext, inheritance);
+    handleAmbiguousSwiftName(cloned);
     known = clonedBaseMembers.insert({key, cloned}).first;
     clonedMembers.insert(std::make_pair(cloned, decl));
   }

lib/ClangImporter/ImportDecl.cpp

@@ -55,6 +55,7 @@
 #include "swift/ClangImporter/ClangImporterRequests.h"
 #include "swift/ClangImporter/ClangModule.h"
 #include "swift/Parse/Lexer.h"
+#include "swift/Parse/ParseDeclName.h"
 #include "swift/Strings.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
@@ -4426,7 +4427,43 @@ namespace {
             // Create a thunk that will perform dynamic dispatch.
             // TODO: we don't have to import the actual `method` in this case,
             // we can just synthesize a thunk and import that instead.
-            auto result = synthesizer.makeVirtualMethod(decl);
+
+            FuncDecl *result;
+            if (decl->size_overridden_methods() > 0) {
+              if (auto swiftNameAttr = decl->getAttr<clang::SwiftNameAttr>()) {
+                auto parsedDeclName = parseDeclName(swiftNameAttr->getName());
+                auto swiftDeclName =
+                    parsedDeclName.formDeclName(method->getASTContext());
+                ImportedName importedName;
+                std::tie(importedName, std::ignore) = importFullName(decl);
+
+                result = synthesizer.makeVirtualMethod(decl);
+
+                if (swiftDeclName != importedName.getDeclName()) {
+                  Impl.diagnose(HeaderLoc(swiftNameAttr->getLoc()),
+                                diag::swift_name_attr_ignored, swiftDeclName);
+
+                  Impl.markUnavailable(
+                      result, (llvm::Twine("ignoring swift_name '") +
+                               swiftNameAttr->getName() + "' in '" +
+                               decl->getParent()->getName() +
+                               "'; swift_name attributes have no effect "
+                               "on method overrides")
+                                  .str());
+                }
+              } else {
+                // If there's no swift_name attribute, we don't import this method. 
+                // This is because if the overridden method was renamed and
+                // this one is not, we want to use the overridden method's name. 
+                // This is reasonable because `makeVirtualMethod` returns
+                // a thunk that will perform dynamic dispatch, and consequently
+                // the correct instance of the method will get executed.
+                return nullptr;
+              }
+            } else {
+              result = synthesizer.makeVirtualMethod(decl);
+            }
+              
             if (result) {
               return result;
             } else {
@@ -10377,6 +10414,22 @@ ValueDecl *ClangImporter::Implementation::createUnavailableDecl(
   return var;
 }
 
+void ClangImporter::Implementation::handleAmbiguousSwiftName(ValueDecl *decl) {
+  if (!decl || decl->isUnavailable())
+    return;
+
+  auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(
+      decl->getDeclContext()->getAsDecl()->getClangDecl());
+
+  if (!cxxRecordDecl)
+    return;
+
+  if (findUnavailableMethod(cxxRecordDecl, decl->getName())) {
+    markUnavailable(decl,
+                    "overrides multiple C++ methods with different Swift names");
+  }
+}
+
 // Force the members of the entire inheritance hierarchy to be loaded and
 // deserialized before loading the members of this class. This allows the
 // decl members table to be warmed up and enables the correct identification of

lib/ClangImporter/ImportName.cpp

@@ -1706,6 +1706,12 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
       }
     }
 
+    // `swift_name` attribute is not supported in virtual methods overrides
+    if (auto method = dyn_cast<clang::CXXMethodDecl>(D)) {
+      if (method->isVirtual() && method->size_overridden_methods() > 0)
+        skipCustomName = true;
+    }
+
     if (!skipCustomName) {
       result.info.hasCustomName = true;
       result.declName = parsedName.formDeclName(
@@ -2315,6 +2321,42 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
       newName = baseName.substr(StringRef("__synthesizedVirtualCall_").size());
       baseName = newName;
     }
+    if (method->isVirtual()) {
+      // The name should be imported from the base method
+      if (method->size_overridden_methods() > 0) {
+        DeclName overriddenName;
+        bool foundDivergentMethod = false;
+        for (auto overriddenMethod : method->overridden_methods()) {
+          ImportedName importedName =
+              importName(overriddenMethod, version, givenName);
+          if (!overriddenName) {
+            overriddenName = importedName.getDeclName();
+          } else if (overriddenName.compare(importedName.getDeclName())) {
+            importerImpl->insertUnavailableMethod(method->getParent(),
+                                                  importedName.getDeclName());
+            foundDivergentMethod = true;
+          }
+        }
+
+        if (foundDivergentMethod) {
+          // The method we want to mark as unavailable will be generated
+          // lazily, when we clone the methods from base classes to the derived
+          // class method->getParent().
+          // Since we don't have the actual method here, we store this
+          // information to be accessed when we generate the actual method.
+          importerImpl->insertUnavailableMethod(method->getParent(),
+                                                overriddenName);
+          return ImportedName();
+        }
+
+        baseName = overriddenName.getBaseIdentifier().str();
+        // Also inherit argument names from base method
+        argumentNames.clear();
+        llvm::for_each(overriddenName.getArgumentNames(), [&](Identifier arg) {
+          argumentNames.push_back(arg.str());
+        });
+      }
+    }
   }
 
   // swift_newtype-ed declarations may have common words with the type name

lib/ClangImporter/ImporterImpl.h

@@ -699,6 +699,14 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   // Map all cloned methods back to the original member
   llvm::DenseMap<ValueDecl *, ValueDecl *> clonedMembers;
 
+  // Keep track of methods that are unavailale in each class.
+  // We need this set because these methods will be imported lazily. We don't
+  // have the corresponding Swift method when the availability check is
+  // performed, so instead we store the information in this set and then, when
+  // the method is finally generated, we check if it's present here
+  llvm::DenseSet<std::pair<const clang::CXXRecordDecl *, DeclName>>
+      unavailableMethods;
+
 public:
   llvm::DenseMap<const clang::ParmVarDecl*, FuncDecl*> defaultArgGenerators;
 
@@ -727,6 +735,18 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
     return getNameImporter().getEnumKind(decl);
   }
 
+  bool findUnavailableMethod(const clang::CXXRecordDecl *classDecl,
+                             DeclName name) {
+    return unavailableMethods.contains({classDecl, name});
+  }
+
+  void insertUnavailableMethod(const clang::CXXRecordDecl *classDecl,
+                               DeclName name) {
+    unavailableMethods.insert({classDecl, name});
+  }
+
+  void handleAmbiguousSwiftName(ValueDecl *decl);
+
 private:
   /// A mapping from imported declarations to their "alternate" declarations,
   /// for cases where a single Clang declaration is imported to two

lib/ClangImporter/SwiftLookupTable.cpp

@@ -22,6 +22,7 @@
 #include "swift/Basic/STLExtras.h"
 #include "swift/Basic/Version.h"
 #include "swift/ClangImporter/ClangImporter.h"
+#include "swift/Parse/ParseDeclName.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/Lex/MacroInfo.h"
@@ -1921,6 +1922,15 @@ void importer::addEntryToLookupTable(SwiftLookupTable &table,
                          named, importedName.getEffectiveContext());
         }
 
+        if (auto swiftNameAttr = named->getAttr<clang::SwiftNameAttr>()) {
+          auto parsedDeclName = parseDeclName(swiftNameAttr->getName());
+          auto swiftDeclName =
+              parsedDeclName.formDeclName(nameImporter.getContext());
+          if (importedName.getDeclName() != swiftDeclName)
+            table.addEntry(swiftDeclName, named,
+                           importedName.getEffectiveContext());
+        }
+
         return true;
       });
   if (failed) {

test/IDE/dump_swift_lookup_tables.swift

@@ -41,19 +41,19 @@ import ImportAsMember
 // CHECK-NEXT:      TU: __swift
 // CHECK-NEXT:    adding:
 // CHECK-NEXT:      SNSomeStruct: SNAdding
-// CHECK-NEXT:    blue:
+// CHECK:         blue:
 // CHECK-NEXT:      SNColorChoice: SNColorBlue
 // CHECK-NEXT:    defaultValue:
 // CHECK-NEXT:      SNSomeStruct: SNSomeStructGetDefault, SNSomeStructSetDefault
-// CHECK-NEXT:    defaultX:
+// CHECK:         defaultX:
 // CHECK-NEXT:      SNSomeStruct: DefaultXValue
-// CHECK-NEXT:    foo:
+// CHECK:         foo:
 // CHECK-NEXT:      SNSomeStruct: SNSomeStructGetFoo, SNSomeStructSetFoo
-// CHECK-NEXT:    green:
+// CHECK:         green:
 // CHECK-NEXT:      SNColorChoice: SNColorGreen
 // CHECK-NEXT:    init:
 // CHECK-NEXT:      SNSomeStruct: SNCreate
-// CHECK-NEXT:    makeSomeStruct:
+// CHECK:         makeSomeStruct:
 // CHECK-NEXT:      TU: SNMakeSomeStruct, SNMakeSomeStructForX
 // CHECK-NEXT:    x:
 // CHECK-NEXT:      SNSomeStruct: X

test/Interop/Cxx/foreign-reference/Inputs/member-inheritance.h

@@ -139,3 +139,138 @@ inline const Immortal *_Nonnull castToImmortal(
     const DerivedFromImmortal *_Nonnull immortal) {
   return static_cast<const Immortal *>(immortal);
 }
+
+//       A1
+//      /  \
+//     B1   B2
+//    /  \
+//  C1    C2
+
+struct IMMORTAL_FRT A1 {
+  virtual int virtualMethod() const { return 111; }
+
+  __attribute__((swift_name("swiftFooRename()"))) 
+  virtual int fooRename() const { return 112; }
+
+  __attribute__((swift_name("swiftBarRename()"))) 
+  virtual int barRename() const { return 113; }
+
+  __attribute__((swift_name("swiftParamsRename(a1:)"))) 
+  virtual int paramsRename(int i) const { return i; }
+
+  static A1 *_Nonnull create() { return new A1(); }
+};
+
+struct B1 : A1 {
+  __attribute__((swift_name("swiftVirtualMethod()")))
+  virtual int virtualMethod() const override { return 211; }
+
+  virtual int fooRename() const override { return 212; }
+  
+  __attribute__((swift_name("B1BarRename()"))) 
+  virtual int barRename() const override { return 213; }
+
+  __attribute__((swift_name("swiftParamsRename(b1:)"))) 
+  virtual int paramsRename(int i) const override { return i; }
+
+  static B1 *_Nonnull create() { return new B1(); }
+};
+
+struct B2 : A1 {
+  int virtualMethod() const { return 221; }
+
+  int fooRename() const { return 222; }
+
+  __attribute__((swift_name("B2BarRename()"))) int barRename() const {
+    return 223;
+  }
+
+  static B2 *_Nonnull create() { return new B2(); }
+};
+
+struct C1 : B1 {
+  __attribute__((swift_name("swiftFooRename()")))
+  virtual int fooRename() const override { return 312; }
+
+  __attribute__((swift_name("swiftBarRename()")))
+  virtual int barRename() const override { return 313; }
+
+  virtual int paramsRename(int i) const override { return i; }
+
+  static C1 *_Nonnull create() { return new C1(); }
+};
+
+struct C2 : B1 {
+  __attribute__((swift_name("swiftVirtualMethod()")))
+  virtual int virtualMethod() const override { return 321; }
+
+  __attribute__((swift_name("C2FooRename()")))
+  virtual int fooRename() const override { return 322; }
+
+  __attribute__((swift_name("B1BarRename()")))
+  virtual int barRename() const override { return 323; }
+
+  __attribute__((swift_name("swiftParamsRename(b1:)"))) 
+  virtual int paramsRename(int i) const override { return i; }
+
+  static C2 *_Nonnull create() { return new C2(); }
+};
+
+struct IMMORTAL_FRT A2 {
+  __attribute__((swift_name("swiftVirtualMethod()")))
+  virtual int virtualMethod() const { return 121; }
+
+  __attribute__((swift_name("swiftFooRename()")))
+  virtual int fooRename() const { return 122; }
+
+  __attribute__((swift_name("A2BarRename()"))) 
+  virtual int barRename() const { return 123; }
+
+  __attribute__((swift_name("swiftParamsRename(a2:)"))) virtual int
+  paramsRename(int i) const {
+    return i + 1;
+  }
+
+  static A2 *_Nonnull create() { return new A2(); }
+};
+
+//  A1    A2   
+//   \    /
+//     D1
+
+struct D1 : A1, A2 {
+  static D1 *_Nonnull create() { return new D1(); }
+};
+
+//  A1       A2   
+//   \       /
+//    B1    / 
+//      \  /
+//       D2
+
+struct D2 : B1, A2 {
+   __attribute__((swift_name("swiftVirtualMethod()")))
+  virtual int virtualMethod() const override { return 411; }
+
+  virtual int fooRename() const override { return 412; }
+  
+  virtual int barRename() const override { return 413; }
+
+  virtual int paramsRename(int i) const override { return i; }
+};
+
+//       A1
+//      /  \
+//     /    \
+//    B1    B2
+//    |\    /|
+//    | \  / |
+//    |  D3  |
+//    C1     |
+//     \     |
+//      \   /
+//        D4
+
+struct D3 : B1, B2 {};
+
+struct D4 : C1, B2 {};