[Clang importer] Eliminate the use of lookupDirect when getting all members.

Loading of the members of a C(++) struct/class can occur while doing a
direct lookup, so triggering a second direct lookup inside there can
introduce a request-evaluator cycle. Reimplement this operation to be
more like the way we lazily populate Objective-C classes and protocols,
walking through the record members in order and importing their
variants, then adding those. This eliminates a bunch of extraneous
lookup work, keeps the members in order (see the test case change),
and eliminates the potential for cycles.

Author: DougGregor

lib/ClangImporter/ImportDecl.cpp

@@ -9751,39 +9751,33 @@ static void loadAllMembersOfSuperclassIfNeeded(ClassDecl *CD) {
     E->loadAllMembers();
 }
 
-static void loadAllMembersOfRecordDecl(StructDecl *recordDecl) {
-  auto &ctx = recordDecl->getASTContext();
+void ClangImporter::Implementation::loadAllMembersOfRecordDecl(
+    StructDecl *recordDecl) {
   auto clangRecord = cast<clang::RecordDecl>(recordDecl->getClangDecl());
 
-  // This is only to keep track of the members we've already seen.
-  llvm::SmallPtrSet<Decl *, 16> addedMembers;
-  for (auto member : recordDecl->getCurrentMembersWithoutLoading())
-    addedMembers.insert(member);
-
-  for (auto member : clangRecord->decls()) {
-    auto namedDecl = dyn_cast<clang::NamedDecl>(member);
-    if (!namedDecl)
-      continue;
-
-    // Don't try to load ourselves (this will create an infinite loop).
-    if (auto possibleSelf = dyn_cast<clang::RecordDecl>(member))
-      if (possibleSelf->getDefinition() == clangRecord)
-        continue;
-
+  // Import all of the members.
+  llvm::SmallVector<Decl *, 16> members;
+  for (const clang::Decl *m : clangRecord->decls()) {
+    auto nd = dyn_cast<clang::NamedDecl>(m);
     // Currently, we don't import unnamed bitfields.
-    if (isa<clang::FieldDecl>(member) &&
-        cast<clang::FieldDecl>(member)->isUnnamedBitfield())
+    if (isa<clang::FieldDecl>(m) &&
+        cast<clang::FieldDecl>(m)->isUnnamedBitfield())
       continue;
 
-    auto name = ctx.getClangModuleLoader()->importName(namedDecl);
-    if (!name)
-      continue;
+    if (nd && nd == nd->getCanonicalDecl() &&
+        nd->getDeclContext() == clangRecord &&
+        isVisibleClangEntry(nd))
+      insertMembersAndAlternates(nd, members);
+  }
 
-    for (auto found : recordDecl->lookupDirect(name)) {
-      if (addedMembers.insert(found).second &&
-          found->getDeclContext() == recordDecl)
-        recordDecl->addMember(found);
-    }
+  // Add the members here.
+  for (auto member: members) {
+    // FIXME: constructors are added eagerly, but shouldn't be
+    // FIXME: subscripts are added eagerly, but shouldn't be
+    if (!isa<AccessorDecl>(member) &&
+        !isa<SubscriptDecl>(member) &&
+        !isa<ConstructorDecl>(member))
+      recordDecl->addMember(member);
   }
 }
 
@@ -9819,13 +9813,7 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
   }
 
   if (isa_and_nonnull<clang::RecordDecl>(D->getClangDecl())) {
-    // We haven't loaded any members yet, so tell our context that it still has
-    // lazy members. Otherwise, we won't be able to look up any individual
-    // members (lazily) in "loadAllMembersOfRecordDecl".
-    cast<StructDecl>(D)->setHasLazyMembers(true);
     loadAllMembersOfRecordDecl(cast<StructDecl>(D));
-    // Now that all members are loaded, mark the context as lazily complete.
-    cast<StructDecl>(D)->setHasLazyMembers(false);
     return;
   }
 

lib/ClangImporter/ImporterImpl.h

@@ -1390,6 +1390,8 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   void
   loadAllMembersOfObjcContainer(Decl *D,
                                 const clang::ObjCContainerDecl *objcContainer);
+  void loadAllMembersOfRecordDecl(StructDecl *recordDecl);
+
   void collectMembersToAdd(const clang::ObjCContainerDecl *objcContainer,
                            Decl *D, DeclContext *DC,
                            SmallVectorImpl<Decl *> &members);

test/Interop/Cxx/operators/member-inline-module-interface.swift

@@ -60,16 +60,17 @@
 // CHECK:   func __operatorSubscriptConst(_ x: Int32) -> UnsafePointer<Int32>
 
 // CHECK:   @available(*, unavailable, message: "use subscript")
-// CHECK:   func __operatorSubscriptConst(_ x: Bool) -> UnsafePointer<Bool>
+// CHECK:   mutating func __operatorSubscript(_ x: Int32) -> UnsafeMutablePointer<Int32>
 
 // CHECK:   @available(*, unavailable, message: "use subscript")
-// CHECK:   func __operatorSubscriptConst(_ x: Double) -> UnsafePointer<Double>
+// CHECK:   mutating func __operatorSubscript(_ x: Bool) -> UnsafeMutablePointer<Bool>
 
 // CHECK:   @available(*, unavailable, message: "use subscript")
-// CHECK:   mutating func __operatorSubscript(_ x: Int32) -> UnsafeMutablePointer<Int32>
+// CHECK:   func __operatorSubscriptConst(_ x: Bool) -> UnsafePointer<Bool>
 
 // CHECK:   @available(*, unavailable, message: "use subscript")
-// CHECK:   mutating func __operatorSubscript(_ x: Bool) -> UnsafeMutablePointer<Bool>
+// CHECK:   func __operatorSubscriptConst(_ x: Double) -> UnsafePointer<Double>
+
 // CHECK: }