Merge pull request #67178 from hyp/eng/base-class-members-ambig

[cxx-interop] Do not add base class members that cause lookup ambigui…

Author: hyp

lib/ClangImporter/ClangImporter.cpp

@@ -5167,6 +5167,15 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
   // If this is a C++ record, look through any base classes.
   if (auto cxxRecord =
           dyn_cast<clang::CXXRecordDecl>(recordDecl->getClangDecl())) {
+    // 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;
+    for (const auto *valueDecl : result)
+      foundNameArities.insert(getArity(valueDecl));
+
     for (auto base : cxxRecord->bases()) {
       if (base.getAccessSpecifier() != clang::AccessSpecifier::AS_public)
         continue;
@@ -5202,6 +5211,10 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
           }
         }
         for (auto foundInBase : baseResults) {
+          // Do not add duplicate entry with the same arity,
+          // as that would cause an ambiguous lookup.
+          if (foundNameArities.count(getArity(foundInBase)))
+            continue;
           if (auto newDecl = clangModuleLoader->importBaseMemberDecl(
                   foundInBase, recordDecl)) {
             result.push_back(newDecl);
@@ -6375,6 +6388,16 @@ ValueDecl *ClangImporter::Implementation::importBaseMemberDecl(
   return known->second;
 }
 
+size_t ClangImporter::Implementation::getImportedBaseMemberDeclArity(
+    const ValueDecl *valueDecl) {
+  if (auto *func = dyn_cast<FuncDecl>(valueDecl)) {
+    if (auto *params = func->getParameters()) {
+      return params->size();
+    }
+  }
+  return 0;
+}
+
 ValueDecl *ClangImporter::importBaseMemberDecl(ValueDecl *decl,
                                                DeclContext *newContext) {
   return Impl.importBaseMemberDecl(decl, newContext);

lib/ClangImporter/ImportDecl.cpp

@@ -8890,6 +8890,23 @@ void ClangImporter::Implementation::loadAllMembersOfRecordDecl(
   for (auto member: members) {
     // This means we found a member in a C++ record's base class.
     if (swiftDecl->getClangDecl() != clangRecord) {
+      // Do not clone the base member into the derived class
+      // when the derived class already has a member of such
+      // name and arity.
+      auto memberArity =
+          getImportedBaseMemberDeclArity(cast<ValueDecl>(member));
+      bool shouldAddBaseMember = true;
+      for (const auto *currentMember : swiftDecl->getMembers()) {
+        auto vd = dyn_cast<ValueDecl>(currentMember);
+        if (vd->getName() == cast<ValueDecl>(member)->getName()) {
+          if (memberArity == getImportedBaseMemberDeclArity(vd)) {
+            shouldAddBaseMember = false;
+            break;
+          }
+        }
+      }
+      if (!shouldAddBaseMember)
+        continue;
       // So we need to clone the member into the derived class.
       if (auto newDecl = importBaseMemberDecl(cast<ValueDecl>(member), swiftDecl)) {
         swiftDecl->addMember(newDecl);

lib/ClangImporter/ImporterImpl.h

@@ -650,6 +650,8 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   ValueDecl *importBaseMemberDecl(ValueDecl *decl, DeclContext *newContext);
 
 public:
+  static size_t getImportedBaseMemberDeclArity(const ValueDecl *valueDecl);
+
   // Cache for already-specialized function templates and any thunks they may
   // have.
   llvm::DenseMap<clang::FunctionDecl *, ValueDecl *>

test/Interop/Cxx/class/inheritance/Inputs/fields.h

@@ -4,6 +4,10 @@ struct HasThreeFields {
   int c = 3;
 };
 
+struct DerivedWithSameField : HasThreeFields {
+  int a = 2;
+};
+
 struct DerivedWithOneField : HasThreeFields {
   int d = 4;
 };
@@ -59,4 +63,4 @@ struct ClassTemplate {
   T value;
 };
 
-struct DerivedFromClassTemplate : ClassTemplate<int> {};
+struct DerivedFromClassTemplate : ClassTemplate<int> {};

test/Interop/Cxx/class/inheritance/Inputs/functions.h

@@ -50,6 +50,14 @@ struct Base {
   }
 
   void pure() const __attribute__((pure)) {}
+
+  inline int sameMethodNameSameSignature() const {
+    return 42;
+  }
+
+  inline int sameMethodDifferentSignature() const {
+    return 18;
+  }
 };
 
 struct OtherBase {
@@ -65,6 +73,14 @@ struct Derived : Base, OtherBase {
       __attribute__((swift_attr("import_unsafe"))) {
     return "Derived::inDerived";
   }
+
+  inline int sameMethodNameSameSignature() const {
+    return 21;
+  }
+
+  inline int sameMethodDifferentSignature(int x) const {
+    return x + 1;
+  }
 };
 
 struct DerivedFromDerived : Derived {
@@ -92,4 +108,4 @@ struct EmptyBaseClass {
 struct DerivedFromEmptyBaseClass : EmptyBaseClass {
   int a = 42;
   int b = 42;
-};
+};

test/Interop/Cxx/class/inheritance/fields-module-interface.swift

@@ -8,6 +8,13 @@
 // CHECK-NEXT:   var c: Int32
 // CHECK-NEXT: }
 
+// CHECK-NEXT: struct DerivedWithSameField {
+// CHECK-NEXT:   init()
+// CHECK-NEXT:   var a: Int32
+// CHECK-NEXT:   var b: Int32
+// CHECK-NEXT:   var c: Int32
+// CHECK-NEXT: }
+
 // CHECK-NEXT: struct DerivedWithOneField {
 // CHECK-NEXT:   init()
 // CHECK-NEXT:   var d: Int32

test/Interop/Cxx/class/inheritance/fields.swift

@@ -79,4 +79,10 @@ FieldsTestSuite.test("Derived from class template") {
   expectEqual(derived.value, 42)
 }
 
+FieldsTestSuite.test("Same field from derived") {
+  var derived = DerivedWithSameField()
+  derived.a = 42
+  expectEqual(derived.a, 42)
+}
+
 runAllTests()

test/Interop/Cxx/class/inheritance/functions-module-interface.swift

@@ -29,6 +29,10 @@
 // CHECK-NEXT:   @available(swift, obsoleted: 3, renamed: "swiftRenamed(input:)")
 // CHECK-NEXT:   mutating func renamed(_ i: Int32) -> Int32
 // CHECK-NEXT:   @_effects(readonly) func pure()
+// CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodNameSameSignature() -> Int32
+// CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodDifferentSignature() -> Int32
 // CHECK-NEXT: }
 
 // CHECK-NEXT: struct OtherBase {
@@ -42,6 +46,10 @@
 // CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   func inDerived() -> UnsafePointer<CChar>!
 // CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodNameSameSignature() -> Int32
+// CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodDifferentSignature(_ x: Int32) -> Int32
+// CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   mutating func mutatingInBase() -> UnsafePointer<CChar>?
 // CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   func constInBase() -> UnsafePointer<CChar>?
@@ -57,6 +65,8 @@
 // CHECK-NEXT:   mutating func renamed(_ i: Int32) -> Int32
 // CHECK-NEXT:   @_effects(readonly) func pure()
 // CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodDifferentSignature() -> Int32
+// CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   func inOtherBase() -> UnsafePointer<CChar>?
 // CHECK-NEXT: }
 
@@ -67,6 +77,10 @@
 // CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   func inDerived() -> UnsafePointer<CChar>?
 // CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodNameSameSignature() -> Int32
+// CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodDifferentSignature(_ x: Int32) -> Int32
+// CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   mutating func mutatingInBase() -> UnsafePointer<CChar>?
 // CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   func constInBase() -> UnsafePointer<CChar>?
@@ -82,6 +96,8 @@
 // CHECK-NEXT:   mutating func renamed(_ i: Int32) -> Int32
 // CHECK-NEXT:   @_effects(readonly) func pure()
 // CHECK-NEXT:   @discardableResult
+// CHECK-NEXT:   func sameMethodDifferentSignature() -> Int32
+// CHECK-NEXT:   @discardableResult
 // CHECK-NEXT:   func inOtherBase() -> UnsafePointer<CChar>?
 // CHECK-NEXT: }
 

test/Interop/Cxx/class/inheritance/functions-typechecker.swift

@@ -11,3 +11,9 @@ extension Base {
 }
 
 Derived().swiftFunc() // expected-error {{value of type 'Derived' has no member 'swiftFunc'}}
+
+// ok, this calls the derived method.
+Derived().sameMethodNameSameSignature()
+Derived().sameMethodDifferentSignature(1)
+// ok, this is the base class method.
+Derived().sameMethodDifferentSignature()

test/Interop/Cxx/class/inheritance/functions.swift

@@ -38,6 +38,13 @@ FunctionsTestSuite.test("Other base member from derived") {
   expectEqual(String(cString: derived.inOtherBase()!), "OtherBase::inOtherBase")
 }
 
+FunctionsTestSuite.test("Unambiguous members from derived") {
+  let derived = Derived()
+  expectEqual(derived.sameMethodNameSameSignature(), 21)
+  expectEqual(derived.sameMethodDifferentSignature(1), 2)
+  expectEqual(derived.sameMethodDifferentSignature(), 18)
+}
+
 FunctionsTestSuite.test("Basic methods from derived * 2") {
   let dd = DerivedFromDerived()
   expectEqual(String(cString: dd.constInBase()!), "Base::constInBase")