[cxx-interop] Fix spurious ambiguous member lookup for eagerly imported members

include/swift/ClangImporter/ClangImporterRequests.h

@@ -134,29 +134,43 @@ class CXXNamespaceMemberLookup
 /// The input type for a record member lookup request.
 struct ClangRecordMemberLookupDescriptor final {
   NominalTypeDecl *recordDecl;
+  NominalTypeDecl *inheritingDecl;
   DeclName name;
-  bool inherited;
 
-  ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name,
-                                    bool inherited = false)
-      : recordDecl(recordDecl), name(name), inherited(inherited) {
+  ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name)
+      : recordDecl(recordDecl), inheritingDecl(recordDecl), name(name) {
     assert(isa<clang::RecordDecl>(recordDecl->getClangDecl()));
   }
 
   friend llvm::hash_code
   hash_value(const ClangRecordMemberLookupDescriptor &desc) {
-    return llvm::hash_combine(desc.name, desc.recordDecl);
+    return llvm::hash_combine(desc.name, desc.recordDecl, desc.inheritingDecl);
   }
 
   friend bool operator==(const ClangRecordMemberLookupDescriptor &lhs,
                          const ClangRecordMemberLookupDescriptor &rhs) {
-    return lhs.name == rhs.name && lhs.recordDecl == rhs.recordDecl;
+    return lhs.name == rhs.name && lhs.recordDecl == rhs.recordDecl &&
+           lhs.inheritingDecl == rhs.inheritingDecl;
   }
 
   friend bool operator!=(const ClangRecordMemberLookupDescriptor &lhs,
                          const ClangRecordMemberLookupDescriptor &rhs) {
     return !(lhs == rhs);
   }
+
+private:
+  friend class ClangRecordMemberLookup;
+
+  // This private constructor should only be used in ClangRecordMemberLookup,
+  // for recursively traversing base classes that inheritingDecl inherites from.
+  ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name,
+                                    NominalTypeDecl *inheritingDecl)
+      : recordDecl(recordDecl), inheritingDecl(inheritingDecl), name(name) {
+    assert(isa<clang::RecordDecl>(recordDecl->getClangDecl()));
+    assert(isa<clang::CXXRecordDecl>(inheritingDecl->getClangDecl()));
+    assert(recordDecl != inheritingDecl &&
+           "recursive calls should lookup elsewhere");
+  }
 };
 
 void simple_display(llvm::raw_ostream &out,

lib/AST/NameLookupRequests.cpp

@@ -539,6 +539,9 @@ void swift::simple_display(llvm::raw_ostream &out,
   simple_display(out, desc.name);
   out << " in ";
   simple_display(out, desc.recordDecl);
+  if (desc.recordDecl != desc.inheritingDecl)
+    out << " inherited by ";
+  simple_display(out, desc.inheritingDecl);
 }
 
 SourceLoc

lib/ClangImporter/ClangImporter.cpp

@@ -27,6 +27,7 @@
 #include "swift/AST/DiagnosticEngine.h"
 #include "swift/AST/DiagnosticsClangImporter.h"
 #include "swift/AST/DiagnosticsSema.h"
+#include "swift/AST/Evaluator.h"
 #include "swift/AST/IRGenOptions.h"
 #include "swift/AST/ImportCache.h"
 #include "swift/AST/LinkLibrary.h"
@@ -41,6 +42,7 @@
 #include "swift/AST/Types.h"
 #include "swift/Basic/Assertions.h"
 #include "swift/Basic/Defer.h"
+#include "swift/Basic/LLVM.h"
 #include "swift/Basic/Platform.h"
 #include "swift/Basic/Range.h"
 #include "swift/Basic/SourceLoc.h"
@@ -6147,28 +6149,57 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
 TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
     Evaluator &evaluator, ClangRecordMemberLookupDescriptor desc) const {
   NominalTypeDecl *recordDecl = desc.recordDecl;
+  NominalTypeDecl *inheritingDecl = desc.inheritingDecl;
   DeclName name = desc.name;
-  bool inherited = desc.inherited;
 
   auto &ctx = recordDecl->getASTContext();
-  auto allResults = evaluateOrDefault(
+  auto directResults = evaluateOrDefault(
       ctx.evaluator,
       ClangDirectLookupRequest({recordDecl, recordDecl->getClangDecl(), name}),
       {});
 
   // Find the results that are actually a member of "recordDecl".
   TinyPtrVector<ValueDecl *> result;
   ClangModuleLoader *clangModuleLoader = ctx.getClangModuleLoader();
-  for (auto found : allResults) {
-    auto named = found.get<clang::NamedDecl *>();
-    if (dyn_cast<clang::Decl>(named->getDeclContext()) ==
-        recordDecl->getClangDecl()) {
-      // Don't import constructors on foreign reference types.
-      if (isa<clang::CXXConstructorDecl>(named) && isa<ClassDecl>(recordDecl))
+  for (auto foundEntry : directResults) {
+    auto found = foundEntry.get<clang::NamedDecl *>();
+    if (dyn_cast<clang::Decl>(found->getDeclContext()) !=
+        recordDecl->getClangDecl())
+      continue;
+
+    // Don't import constructors on foreign reference types.
+    if (isa<clang::CXXConstructorDecl>(found) && isa<ClassDecl>(recordDecl))
+      continue;
+
+    auto imported = clangModuleLoader->importDeclDirectly(found);
+    if (!imported)
+      continue;
+
+    // If this member is found due to inheritance, clone it from the base class
+    // by synthesizing getters and setters.
+    if (inheritingDecl != recordDecl) {
+      imported = clangModuleLoader->importBaseMemberDecl(
+          cast<ValueDecl>(imported), inheritingDecl);
+      if (!imported)
         continue;
+    }
+    result.push_back(cast<ValueDecl>(imported));
+  }
 
-      if (auto import = clangModuleLoader->importDeclDirectly(named))
-        result.push_back(cast<ValueDecl>(import));
+  if (inheritingDecl != recordDecl) {
+    // For inheritied members, add members that are synthesized eagerly, such as
+    // subscripts. This is not necessary for non-inherited members because those
+    // should already be in the lookup table.
+    for (auto member :
+         cast<NominalTypeDecl>(recordDecl)->getCurrentMembersWithoutLoading()) {
+      auto namedMember = dyn_cast<ValueDecl>(member);
+      if (!namedMember || !namedMember->hasName() ||
+          namedMember->getName().getBaseName() != name)
+        continue;
+
+      if (auto imported = clangModuleLoader->importBaseMemberDecl(
+              namedMember, inheritingDecl))
+        result.push_back(cast<ValueDecl>(imported));
     }
   }
 
@@ -6202,48 +6233,19 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
           continue;
 
         // Add Clang members that are imported lazily.
-        auto baseResults =
-            evaluateOrDefault(ctx.evaluator,
-                              ClangRecordMemberLookup(
-                                  {cast<NominalTypeDecl>(import), name, true}),
-                              {});
-        // Add members that are synthesized eagerly, such as subscripts.
-        for (auto member :
-             cast<NominalTypeDecl>(import)->getCurrentMembersWithoutLoading()) {
-          if (auto namedMember = dyn_cast<ValueDecl>(member)) {
-            if (namedMember->hasName() &&
-                namedMember->getName().getBaseName() == name &&
-                // Make sure we don't add duplicate entries, as that would
-                // wrongly imply that lookup is ambiguous.
-                !llvm::is_contained(baseResults, namedMember)) {
-              baseResults.push_back(namedMember);
-            }
-          }
-        }
+        auto baseResults = evaluateOrDefault(
+            ctx.evaluator,
+            ClangRecordMemberLookup(
+                {cast<NominalTypeDecl>(import), name, inheritingDecl}),
+            {});
+
         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;
 
-          // Do not importBaseMemberDecl() if this is a recursive lookup into
-          // some class's superclass. importBaseMemberDecl() caches synthesized
-          // members, which does not work if we call it on its result, e.g.:
-          //
-          //    importBaseMemberDecl(importBaseMemberDecl(foundInBase,
-          //    recorDecl), recordDecl)
-          //
-          // Instead, we simply pass on the imported decl (foundInBase) as is,
-          // so that only the top-most request calls importBaseMemberDecl().
-          if (inherited) {
-            result.push_back(foundInBase);
-            continue;
-          }
-
-          if (auto newDecl = clangModuleLoader->importBaseMemberDecl(
-                  foundInBase, recordDecl)) {
-            result.push_back(newDecl);
-          }
+          result.push_back(foundInBase);
         }
       }
     }

test/Interop/Cxx/class/inheritance/Inputs/inherited-lookup.h

@@ -1,13 +1,18 @@
 #pragma once
 
-struct Base1 {
-  int methodBase(void) const { return 1; }
+struct One {
+  int method(void) const { return 1; }
+  int operator[](int i) const { return 1; }
 };
 
-struct IBase1 : Base1 {
-  int methodIBase(void) const { return 11; }
+struct IOne : One {
+  int methodI(void) const { return -1; }
 };
 
-struct IIBase1 : IBase1 {
-  int methodIIBase(void) const { return 111; }
+struct IIOne : IOne {
+  int methodII(void) const { return -11; }
+};
+
+struct IIIOne : IIOne {
+  int methodIII(void) const { return -111; }
 };

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

@@ -6,11 +6,46 @@ import StdlibUnittest
 
 var InheritedMemberTestSuite = TestSuite("Test if inherited lookup works")
 
-InheritedMemberTestSuite.test("IIBase1::method() resolves to grandparent") {
-  let iibase1 = IIBase1()
-  expectEqual(iibase1.methodBase(), 1)
-  expectEqual(iibase1.methodIBase(), 11)
-  expectEqual(iibase1.methodIIBase(), 111)
+InheritedMemberTestSuite.test("Regular methods resolve to base classes") {
+  // No inheritance (sanity check)
+  let one = One()
+  expectEqual(one.method(), 1)
+
+  // One level of inheritance
+  let iOne = IOne()
+  expectEqual(iOne.method(), 1)
+  expectEqual(iOne.methodI(), -1)
+
+  // Two levels of inheritance
+  let iiOne = IIOne()
+  expectEqual(iiOne.method(), 1)
+  expectEqual(iiOne.methodI(), -1)
+  expectEqual(iiOne.methodII(), -11)
+
+  // Three levels of inheritance
+  let iiiOne = IIIOne()
+  expectEqual(iiiOne.method(), 1)
+  expectEqual(iiiOne.methodI(), -1)
+  expectEqual(iiiOne.methodII(), -11)
+  expectEqual(iiiOne.methodIII(), -111)
+}
+
+InheritedMemberTestSuite.test("Eagerly imported methods resolve to base classes") {
+  // No inheritance (sanity check)
+  let one = One()
+  expectEqual(one[0], 1)
+
+  // One level of inheritance
+  let iOne = IOne()
+  expectEqual(iOne[0], 1)
+
+  // Two levels of inheritance
+  let iiOne = IIOne()
+  expectEqual(iiOne[0], 1)
+
+  // Three levels of inheritance
+  let iiiOne = IIIOne()
+  expectEqual(iiiOne[0], 1)
 }
 
 runAllTests()

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

@@ -1,24 +1,86 @@
 // RUN: %target-typecheck-verify-swift -verify-ignore-unknown -I %S/Inputs -cxx-interoperability-mode=default
 import InheritedLookup
 
-extension IIBase1 {
+extension One {
+    // Swift extensions of a base class should not affect its derived classes.
+    // We later attempt to call baseExt() in derived classes, which should fail.
+    func baseExt() -> Int32 { return 0 }
+
+    func ext() {
+        let _ = baseExt()
+        let _ = self[0]
+        let _ = method()
+        let _ = methodI()   // expected-error {{cannot find 'methodI' in scope}}
+        let _ = methodII()  // expected-error {{cannot find 'methodII' in scope}}
+        let _ = methodIII() // expected-error {{cannot find 'methodIII' in scope}}
+    }
+}
+
+func fOne(v: One) {
+    let _ = v.baseExt()
+    let _ = v[0]
+    let _ = v.method()
+    let _ = v.methodI()     // expected-error {{'One' has no member 'methodI'}}
+    let _ = v.methodII()    // expected-error {{'One' has no member 'methodII'}}
+    let _ = v.methodIII()   // expected-error {{'One' has no member 'methodIII'}}
+}
+
+extension IOne {
+    func ext() {
+        let _ = baseExt()   // expected-error {{cannot find 'baseExt' in scope}}
+        let _ = self[0]
+        let _ = method()
+        let _ = methodI()
+        let _ = methodII()  // expected-error {{cannot find 'methodII' in scope}}
+        let _ = methodIII() // expected-error {{cannot find 'methodIII' in scope}}
+    }
+}
+
+func fIOne(v: IOne) {
+    let _ = v.baseExt()     // expected-error {{'IOne' has no member 'baseExt'}}
+    let _ = v[0]
+    let _ = v.method()
+    let _ = v.methodI()
+    let _ = v.methodII()    // expected-error {{'IOne' has no member 'methodII'}}
+    let _ = v.methodIII()   // expected-error {{'IOne' has no member 'methodIII'}}
+}
+
+extension IIOne {
+    func ext() {
+        let _ = baseExt()   // expected-error {{cannot find 'baseExt' in scope}}
+        let _ = self[0]
+        let _ = method()
+        let _ = methodI()
+        let _ = methodII()
+        let _ = methodIII() // expected-error {{cannot find 'methodIII' in scope}}
+    }
+}
+
+func fIIOne(v: IIOne) {
+    let _ = v.baseExt()     // expected-error {{'IIOne' has no member 'baseExt'}}
+    let _ = v[0]
+    let _ = v.method()
+    let _ = v.methodI()
+    let _ = v.methodII()
+    let _ = v.methodIII()   // expected-error {{'IIOne' has no member 'methodIII'}}
+}
+
+extension IIIOne {
     func ext() {
-        // NOTE: we deliberately look up a missing member above because doing so
-        // forces multiple ClangRecordMemberLookup requests, which should be
-        // idempotent (though this hasn't always been the case, because bugs).
-        missing() // expected-error {{cannot find 'missing' in scope}}
-
-        // For instance, a non-idempotent ClangRecordMemberLookup would cause
-        // the following to appear ambiguous:
-        methodBase()
-        methodIBase()
-        methodIIBase()
+        let _ = baseExt()   // expected-error {{cannot find 'baseExt' in scope}}
+        let _ = self[0]
+        let _ = method()
+        let _ = methodI()
+        let _ = methodII()
+        let _ = methodIII()
     }
 }
 
-func f(v: IIBase1) {
-    v.missing() // expected-error {{'IIBase1' has no member 'missing'}}
-    v.methodBase()
-    v.methodIBase()
-    v.methodIIBase()
+func fIIIOne(v: IIIOne) {
+    let _ = v.baseExt()     // expected-error {{'IIIOne' has no member 'baseExt'}}
+    let _ = v[0]
+    let _ = v.method()
+    let _ = v.methodI()
+    let _ = v.methodII()
+    let _ = v.methodIII()
 }