[cxx-interop] Fix ambiguous methods in long chains of inheritance

Author: susmonteiro

include/swift/AST/ClangModuleLoader.h

@@ -216,6 +216,9 @@ class ClangModuleLoader : public ModuleLoader {
                                           DeclContext *newContext,
                                           ClangInheritanceInfo inheritance) = 0;
 
+  /// Checks if \param decl is the original method or a clone from a base class
+  virtual bool isClonedMemberDecl(ValueDecl *decl) = 0;
+
   /// Emits diagnostics for any declarations named name
   /// whose direct declaration context is a TU.
   virtual void diagnoseTopLevelValue(const DeclName &name) = 0;

include/swift/ClangImporter/ClangImporter.h

@@ -657,6 +657,8 @@ class ClangImporter final : public ClangModuleLoader {
   ValueDecl *importBaseMemberDecl(ValueDecl *decl, DeclContext *newContext,
                                   ClangInheritanceInfo inheritance) override;
 
+  bool isClonedMemberDecl(ValueDecl *decl) override;
+
   /// Emits diagnostics for any declarations named name
   /// whose direct declaration context is a TU.
   void diagnoseTopLevelValue(const DeclName &name) override;

lib/ClangImporter/ClangImporter.cpp

@@ -6311,7 +6311,8 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
          cast<NominalTypeDecl>(recordDecl)->getCurrentMembersWithoutLoading()) {
       auto namedMember = dyn_cast<ValueDecl>(member);
       if (!namedMember || !namedMember->hasName() ||
-          namedMember->getName().getBaseName() != name)
+          namedMember->getName().getBaseName() != name ||
+          clangModuleLoader->isClonedMemberDecl(namedMember))
         continue;
 
       auto *imported = clangModuleLoader->importBaseMemberDecl(
@@ -7664,11 +7665,24 @@ ValueDecl *ClangImporter::Implementation::importBaseMemberDecl(
   if (known == clonedBaseMembers.end()) {
     ValueDecl *cloned = cloneBaseMemberDecl(decl, newContext, inheritance);
     known = clonedBaseMembers.insert({key, cloned}).first;
+    clonedMembers.insert(cloned);
   }
 
   return known->second;
 }
 
+bool ClangImporter::Implementation::isClonedMemberDecl(ValueDecl *decl) {
+  // If this is a cloned decl, we don't want to reclone it
+  // Otherwise, we may end up with multiple copies of the same method
+  if (!decl->hasClangNode()) {
+    // Skip decls with a clang node as those will never be a clone
+    auto result = clonedMembers.find(decl);
+    return result != clonedMembers.end();
+  }
+
+  return false;
+}
+
 size_t ClangImporter::Implementation::getImportedBaseMemberDeclArity(
     const ValueDecl *valueDecl) {
   if (auto *func = dyn_cast<FuncDecl>(valueDecl)) {
@@ -7685,6 +7699,10 @@ ClangImporter::importBaseMemberDecl(ValueDecl *decl, DeclContext *newContext,
   return Impl.importBaseMemberDecl(decl, newContext, inheritance);
 }
 
+bool ClangImporter::isClonedMemberDecl(ValueDecl *decl) {
+  return Impl.isClonedMemberDecl(decl);
+}
+
 void ClangImporter::diagnoseTopLevelValue(const DeclName &name) {
   Impl.diagnoseTopLevelValue(name);
 }

lib/ClangImporter/ImporterImpl.h

@@ -692,6 +692,9 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   llvm::DenseMap<std::pair<ValueDecl *, DeclContext *>, ValueDecl *>
       clonedBaseMembers;
 
+  // Store all methods that result from cloning a base member
+  llvm::DenseSet<ValueDecl *> clonedMembers;
+
 public:
   llvm::DenseMap<const clang::ParmVarDecl*, FuncDecl*> defaultArgGenerators;
 
@@ -700,6 +703,8 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   ValueDecl *importBaseMemberDecl(ValueDecl *decl, DeclContext *newContext,
                                   ClangInheritanceInfo inheritance);
 
+  bool isClonedMemberDecl(ValueDecl *decl);
+
   static size_t getImportedBaseMemberDeclArity(const ValueDecl *valueDecl);
 
   // Cache for already-specialized function templates and any thunks they may

test/Interop/Cxx/class/inheritance/inherited-lookup-executable.swift

@@ -6,6 +6,20 @@ import StdlibUnittest
 
 var InheritedMemberTestSuite = TestSuite("Test if inherited lookup works")
 
+// The compiler handles 
+// - let iiiOne = IIIOne()
+// - struct Foo { let iiiOne : IIIOne } 
+// differently and because of that, by introducing the struct HasOne in this test, 
+// the function importBaseMemberDecl is called twice for each pair of parent-child structs. 
+// The second time, all the structs have a clone version of the method, 
+// which was triggering a "use of ambiguous method" error.
+struct HasOne {
+  let one: One
+  let iOne: IOne
+  let iiOne: IIOne
+  let iiiOne: IIIOne
+}
+
 InheritedMemberTestSuite.test("Regular methods resolve to base classes") {
   // No inheritance (sanity check)
   let one = One()