[cxx-interop] Fix spurious ambiguous member lookup (#78132)

Nested calls to importBaseMemberDecl() subvert its cache and compromise its idempotence, causing the semantic checker to spuriously report ambiguous member lookups when multiple ClangRecordMemberLookup requests are made (e.g., because of an unrelated missing member lookup).

One such scenario is documented as a test case: test/Interop/Cxx/class/inheritance/inherited-lookup-typechecker.swift fails without this patch because of the expected error from the missing member. Meanwhile, test/Interop/Cxx/class/inheritance/inherited-lookup-executable.swift works because it does not attempt to access a missing member.

This patch fixes the issue by only calling importBaseMemberDecl() in the most derived class (where the ClangRecordMemberLookup originated, i.e., not in recursive requests).

As a consequence of my patch, synthesized member accessors in the derived class directly invoke the member from the base class where the member is inherited from, rather than incurring an indirection at each level of inheritance. As such, the synthesized symbol names are different (and shorter). I've taken this opportunity to update the relevant tests to // CHECK for more of the mangled symbol, rather than only the synthesized symbol prefix, for more precise testing and slightly better readability.

rdar://141069984

Author: j-hui

include/swift/ClangImporter/ClangImporterRequests.h

@@ -135,9 +135,11 @@ class CXXNamespaceMemberLookup
 struct ClangRecordMemberLookupDescriptor final {
   NominalTypeDecl *recordDecl;
   DeclName name;
+  bool inherited;
 
-  ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name)
-      : recordDecl(recordDecl), name(name) {
+  ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name,
+                                    bool inherited = false)
+      : recordDecl(recordDecl), name(name), inherited(inherited) {
     assert(isa<clang::RecordDecl>(recordDecl->getClangDecl()));
   }
 

lib/ClangImporter/ClangImporter.cpp

@@ -63,6 +63,7 @@
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LangStandard.h"
 #include "clang/Basic/Module.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CAS/CASOptions.h"
 #include "clang/CAS/IncludeTree.h"
@@ -6147,6 +6148,7 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
     Evaluator &evaluator, ClangRecordMemberLookupDescriptor desc) const {
   NominalTypeDecl *recordDecl = desc.recordDecl;
   DeclName name = desc.name;
+  bool inherited = desc.inherited;
 
   auto &ctx = recordDecl->getASTContext();
   auto allResults = evaluateOrDefault(
@@ -6200,9 +6202,11 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
           continue;
 
         // Add Clang members that are imported lazily.
-        auto baseResults = evaluateOrDefault(
-            ctx.evaluator,
-            ClangRecordMemberLookup({cast<NominalTypeDecl>(import), name}), {});
+        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()) {
@@ -6221,6 +6225,21 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
           // 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);

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

@@ -0,0 +1,13 @@
+#pragma once
+
+struct Base1 {
+  int methodBase(void) const { return 1; }
+};
+
+struct IBase1 : Base1 {
+  int methodIBase(void) const { return 11; }
+};
+
+struct IIBase1 : IBase1 {
+  int methodIIBase(void) const { return 111; }
+};

test/Interop/Cxx/class/inheritance/Inputs/module.modulemap

@@ -43,3 +43,8 @@ module FunctionsObjC {
   requires cplusplus
   requires objc
 }
+
+module InheritedLookup {
+  header "inherited-lookup.h"
+  requires cplusplus
+}

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

@@ -9,6 +9,6 @@ func testGetX() -> CInt {
 
 let _ = testGetX()
 
-// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @{{.*}}__synthesizedBaseCall___synthesizedBaseGetterAccessor{{.*}}(ptr {{.*}} %[[THIS_PTR:.*]])
+// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @{{(.*)(30CopyTrackedDerivedDerivedClass33__synthesizedBaseGetterAccessor_x|__synthesizedBaseGetterAccessor_x@CopyTrackedDerivedDerivedClass)(.*)}}(ptr {{.*}} %[[THIS_PTR:.*]])
 // CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %{{.*}}, i{{32|64}} 4
-// CHECK: call{{.*}} i32 @{{.*}}__synthesizedBaseGetterAccessor{{.*}}(ptr {{.*}} %[[ADD_PTR]])
+// CHECK: %[[X:.*]] = getelementptr inbounds %class.CopyTrackedBaseClass, ptr %[[ADD_PTR]], i32 0, i32 0

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

@@ -0,0 +1,16 @@
+// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -cxx-interoperability-mode=default)
+//
+// REQUIRES: executable_test
+import InheritedLookup
+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)
+}
+
+runAllTests()

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

@@ -0,0 +1,24 @@
+// RUN: %target-typecheck-verify-swift -verify-ignore-unknown -I %S/Inputs -cxx-interoperability-mode=default
+import InheritedLookup
+
+extension IIBase1 {
+    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()
+    }
+}
+
+func f(v: IIBase1) {
+    v.missing() // expected-error {{'IIBase1' has no member 'missing'}}
+    v.methodBase()
+    v.methodIBase()
+    v.methodIIBase()
+}

test/Interop/Cxx/class/inheritance/subscript-irgen.swift

@@ -9,6 +9,6 @@ func testGetX() -> CInt {
 
 let _ = testGetX()
 
-// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @{{.*}}__synthesizedBaseCall___synthesizedBaseCall_operatorSubscript{{.*}}(ptr {{.*}} %[[THIS_PTR:.*]], i32 {{.*}})
+// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @{{(.*)(30CopyTrackedDerivedDerivedClass39__synthesizedBaseCall_operatorSubscript|__synthesizedBaseCall_operatorSubscript@CopyTrackedDerivedDerivedClass)(.*)}}(ptr {{.*}} %[[THIS_PTR:.*]], i32 {{.*}})
 // CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %{{.*}}, i{{32|64}} 4
-// CHECK: call{{.*}} i32 @{{.*}}__synthesizedBaseCall_operatorSubscript{{.*}}(ptr {{.*}} %[[ADD_PTR]], i32 {{.*}})
+// CHECK: %[[CALL:.*]] = call {{.*}} i32 @{{.*}}CopyTrackedBaseClass{{.*}}(ptr {{.*}} %[[ADD_PTR]], i32 {{.*}})

test/Interop/Cxx/class/move-only/inherited-field-access-irgen.swift

@@ -17,14 +17,10 @@ func testSetX(_ x: CInt) {
 
 testSetX(2)
 
-// CHECK: define {{.*}}linkonce_odr{{.*}} ptr @{{.*}}__synthesizedBaseCall___synthesizedBaseGetterAccessor{{.*}}
-
-// CHECK: define {{.*}}linkonce_odr{{.*}} ptr @{{.*}}__synthesizedBaseCall___synthesizedBaseSetterAccessor{{.*}}
-
-// CHECK: define {{.*}}linkonce_odr{{.*}} ptr @{{.*}}__synthesizedBaseGetterAccessor{{.*}}
+// CHECK: define {{.*}}linkonce_odr{{.*}} ptr @{{(.*)(31NonCopyableHolderDerivedDerived*33__synthesizedBaseGetterAccessor_x|__synthesizedBaseGetterAccessor_x@NonCopyableHolderDerivedDerived)(.*)}}
 // CHECK: %[[VPTR:.*]] = getelementptr inbounds %struct.NonCopyableHolder
 // CHECK: ret ptr %[[VPTR]]
 
-// CHECK: define {{.*}}linkonce_odr{{.*}} ptr @{{.*}}__synthesizedBaseSetterAccessor{{.*}}
+// CHECK: define {{.*}}linkonce_odr{{.*}} ptr @{{(.*)(31NonCopyableHolderDerivedDerived33__synthesizedBaseSetterAccessor_x|__synthesizedBaseSetterAccessor_x@NonCopyableHolderDerivedDerived)(.*)}}
 // CHECK: %[[VPTRS:.*]] = getelementptr inbounds %struct.NonCopyableHolder
 // CHECK: ret ptr %[[VPTRS]]

test/Interop/Cxx/class/move-only/inherited-field-access-silgen.swift

@@ -26,8 +26,8 @@ testSetX(2)
 
 // CHECK: sil shared [transparent] @$sSo024NonCopyableHolderDerivedD0V1xSo0aB0Vvlu : $@convention(method) (@{{(in_)?}}guaranteed NonCopyableHolderDerivedDerived) -> UnsafePointer<NonCopyable>
 // CHECK: {{.*}}(%[[SELF_VAL:.*]] : ${{(\*)?}}NonCopyableHolderDerivedDerived):
-// CHECK: function_ref @{{.*}}__synthesizedBaseCall___synthesizedBaseGetterAccessor{{.*}} : $@convention(cxx_method) (@in_guaranteed NonCopyableHolderDerivedDerived) -> UnsafePointer<NonCopyable>
+// CHECK: function_ref @{{(.*)(31NonCopyableHolderDerivedDerived33__synthesizedBaseGetterAccessor_x|__synthesizedBaseGetterAccessor_x@NonCopyableHolderDerivedDerived)(.*)}} : $@convention(cxx_method) (@in_guaranteed NonCopyableHolderDerivedDerived) -> UnsafePointer<NonCopyable>
 // CHECK-NEXT: apply %{{.*}}
 
 // CHECK: sil shared [transparent] @$sSo024NonCopyableHolderDerivedD0V1xSo0aB0Vvau : $@convention(method) (@inout NonCopyableHolderDerivedDerived) -> UnsafeMutablePointer<NonCopyable>
-// CHECK: function_ref @{{.*}}__synthesizedBaseCall___synthesizedBaseSetterAccessor{{.*}} : $@convention(cxx_method) (@inout NonCopyableHolderDerivedDerived) -> UnsafeMutablePointer<NonCopyable>
+// CHECK: function_ref @{{(.*)(31NonCopyableHolderDerivedDerived33__synthesizedBaseSetterAccessor_x|__synthesizedBaseSetterAccessor_x@NonCopyableHolderDerivedDerived)(.*)}} : $@convention(cxx_method) (@inout NonCopyableHolderDerivedDerived) -> UnsafeMutablePointer<NonCopyable>