[cxx-interop] Fix spurious ambiguous member lookup for eagerly imported members (#78673)

Follow-up from #78132, which did not fix issues related to eagerly imported members like subscripts.

This patch restructures recursive ClangRecordMemberLookup requests to importBaseMemberDecl() in the recursive calls, rather than propagating base member decls up to the initial lookup request and doing the import. Doing so seems to fix lingering resolution issues (which I've added to the regression tests).

rdar://141069984

Author: j-hui

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()
 }