[cxx-interop] Add members to the LookupTable where possible.

If possible, add imported members to the StructDecl's LookupTable rather than adding them directly as members. This will fix the issues with ordering that #39436 poorly attempted to solve during IRGen.

This also allows us to break out most of the test changes from #39436.

Author: zoecarver

include/swift/AST/Decl.h

@@ -3073,6 +3073,10 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
   /// Prepare to traverse the list of extensions.
   void prepareExtensions();
 
+  /// Add loaded members from all extensions. Eagerly load any members that we
+  /// can't lazily load.
+  void addLoadedExtensions();
+
   /// Retrieve the conformance loader (if any), and removing it in the
   /// same operation. The caller is responsible for loading the
   /// conformances.
@@ -3195,6 +3199,11 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
   /// Add a new extension to this nominal type.
   void addExtension(ExtensionDecl *extension);
 
+  /// Add a member to this decl's lookup table.
+  ///
+  /// Calls "prepareLookupTable" as a side effect.
+  void addMemberToLookupTable(Decl *member);
+
   /// Retrieve the set of extensions of this type.
   ExtensionRange getExtensions();
 

lib/AST/DeclContext.cpp

@@ -867,6 +867,10 @@ void IterableDeclContext::addMemberSilently(Decl *member, Decl *hint,
       if (d->isImplicit())
         return true;
 
+      // Imported decls won't have complete location info.
+      if (d->hasClangNode())
+        return true;
+
       return false;
     };
 

lib/AST/NameLookup.cpp

@@ -1220,6 +1220,12 @@ void ExtensionDecl::addedMember(Decl *member) {
   }
 }
 
+void NominalTypeDecl::addMemberToLookupTable(Decl *member) {
+  prepareLookupTable();
+
+  LookupTable->addMember(member);
+}
+
 // For lack of anywhere more sensible to put it, here's a diagram of the pieces
 // involved in finding members and extensions of a NominalTypeDecl.
 //
@@ -1332,7 +1338,9 @@ void NominalTypeDecl::prepareLookupTable() {
   } else {
     LookupTable->addMembers(getMembers());
   }
+}
 
+void NominalTypeDecl::addLoadedExtensions() {
   for (auto e : getExtensions()) {
     // If we can lazy-load this extension, only take the members we've loaded
     // so far.
@@ -1424,10 +1432,10 @@ DirectLookupRequest::evaluate(Evaluator &evaluator,
 
   decl->prepareLookupTable();
 
-  // If we're allowed to load extensions, call prepareExtensions to ensure we
+  // If we're allowed to load extensions, call addLoadedExtensions to ensure we
   // properly invalidate the lazily-complete cache for any extensions brought in
   // by modules loaded after-the-fact. This can happen with the LLDB REPL.
-  decl->prepareExtensions();
+  decl->addLoadedExtensions();
 
   auto &Table = *decl->LookupTable;
   if (!useNamedLazyMemberLoading) {

lib/ClangImporter/ClangImporter.cpp

@@ -4053,7 +4053,8 @@ ClangImporter::Implementation::loadNamedMembers(
   auto table = findLookupTable(*CMO);
   assert(table && "clang module without lookup table");
 
-  assert(isa<clang::ObjCContainerDecl>(CD) || isa<clang::NamespaceDecl>(CD));
+  assert(isa<clang::ObjCContainerDecl>(CD) || isa<clang::NamespaceDecl>(CD) ||
+         isa<clang::RecordDecl>(CD));
 
   // Force the members of the entire inheritance hierarchy to be loaded and
   // deserialized before loading the named member of a class. This warms up

lib/ClangImporter/ImportDecl.cpp

@@ -2401,48 +2401,6 @@ namespace {
         if (auto *typedefForAnon = decl->getTypedefNameForAnonDecl())
           return importFullName(typedefForAnon);
       }
-
-      if (!decl->isRecord())
-        return {ImportedName(), None};
-
-      // If the type has no name and no structure name, but is not anonymous,
-      // generate a name for it. Specifically this is for cases like:
-      //   struct a {
-      //     struct {} z;
-      //   }
-      // Where the member z is an unnamed struct, but does have a member-name
-      // and is accessible as a member of struct a.
-      if (auto recordDecl = dyn_cast<clang::RecordDecl>(
-                              decl->getLexicalDeclContext())) {
-        for (auto field : recordDecl->fields()) {
-          if (field->getType()->getAsTagDecl() == decl) {
-            // Create a name for the declaration from the field name.
-            std::string Id;
-            llvm::raw_string_ostream IdStream(Id);
-
-            const char *kind;
-            if (decl->isStruct())
-              kind = "struct";
-            else if (decl->isUnion())
-              kind = "union";
-            else
-              llvm_unreachable("unknown decl kind");
-
-            IdStream << "__Unnamed_" << kind << "_";
-            if (field->isAnonymousStructOrUnion()) {
-              IdStream << "__Anonymous_field" << field->getFieldIndex();
-            } else {
-              assert(!field->getDeclName().isEmpty() &&
-                     "Microsoft anonymous struct extension?");
-              IdStream << field->getName();
-            }
-            ImportedName Result;
-            Result.setDeclName(Impl.SwiftContext.getIdentifier(IdStream.str()));
-            Result.setEffectiveContext(decl->getDeclContext());
-            return {Result, None};
-          }
-        }
-      }
 
       return {ImportedName(), None};
     }
@@ -3634,7 +3592,7 @@ namespace {
         if (auto nominalDecl =
                 dyn_cast<NominalTypeDecl>(nestedType->getDeclContext())) {
           assert(nominalDecl == result && "interesting nesting of C types?");
-          nominalDecl->addMember(nestedType);
+          nominalDecl->addMemberToLookupTable(nestedType);
         }
       }
 
@@ -3675,7 +3633,7 @@ namespace {
                                                   /*wantBody=*/true);
           ctors.push_back(valueCtor);
         }
-        result->addMember(member);
+        result->addMemberToLookupTable(member);
       }
 
       const clang::CXXRecordDecl *cxxRecordDecl =
@@ -3710,12 +3668,14 @@ namespace {
         ctors.push_back(valueCtor);
       }
 
+      // Add ctors directly as they cannot always be looked up from the clang
+      // decl (some are synthesized by Swift).
       for (auto ctor : ctors) {
         result->addMember(ctor);
       }
 
       for (auto method : methods) {
-        result->addMember(method);
+        result->addMemberToLookupTable(method);
       }
 
       result->setHasUnreferenceableStorage(hasUnreferenceableStorage);
@@ -3731,10 +3691,13 @@ namespace {
           auto getterAndSetter = subscriptInfo.second;
           auto subscript = makeSubscript(getterAndSetter.first,
                                          getterAndSetter.second);
+          // Also add subscripts directly because they won't be found from the
+          // clang decl.
           result->addMember(subscript);
         }
       }
 
+      result->setMemberLoader(&Impl, 0);
       return result;
     }
 
@@ -4363,21 +4326,9 @@ namespace {
       Optional<ImportedName> correctSwiftName;
       ImportedName importedName;
 
-      if (!decl->isAnonymousStructOrUnion() && !decl->getDeclName().isEmpty()) {
-        std::tie(importedName, correctSwiftName) = importFullName(decl);
-        if (!importedName) {
-          return nullptr;
-        }
-      } else {
-        // Generate a field name for anonymous fields, this will be used in
-        // order to be able to expose the indirect fields injected from there
-        // as computed properties forwarding the access to the subfield.
-        std::string Id;
-        llvm::raw_string_ostream IdStream(Id);
-
-        IdStream << "__Anonymous_field" << decl->getFieldIndex();
-        importedName.setDeclName(Impl.SwiftContext.getIdentifier(IdStream.str()));
-        importedName.setEffectiveContext(decl->getDeclContext());
+      std::tie(importedName, correctSwiftName) = importFullName(decl);
+      if (!importedName) {
+        return nullptr;
       }
 
       auto name = importedName.getDeclName().getBaseIdentifier();
@@ -9875,6 +9826,38 @@ static void loadAllMembersOfSuperclassIfNeeded(ClassDecl *CD) {
     E->loadAllMembers();
 }
 
+static void loadAllMembersOfRecordDecl(StructDecl *recordDecl) {
+  auto &ctx = recordDecl->getASTContext();
+  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;
+
+    auto name = ctx.getClangModuleLoader()->importName(namedDecl);
+    if (!name)
+      continue;
+
+    for (auto found : recordDecl->lookupDirect(name)) {
+      if (addedMembers.insert(found).second &&
+          // TODO: remove this check once PR#38675 lands.
+          found->getDeclContext() == recordDecl)
+        recordDecl->addMember(found);
+    }
+  }
+}
+
 void
 ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
 
@@ -9893,26 +9876,8 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
     return;
   }
 
-  auto namespaceDecl =
-      dyn_cast_or_null<clang::NamespaceDecl>(D->getClangDecl());
-  if (namespaceDecl) {
-    auto *enumDecl = cast<EnumDecl>(D);
-    // TODO: This redecls should only match redecls that are in the same
-    // module as namespaceDecl after we import one namespace per clang module.
-    for (auto ns : namespaceDecl->redecls()) {
-      for (auto m : ns->decls()) {
-        auto nd = dyn_cast<clang::NamedDecl>(m);
-        if (!nd)
-          continue;
-        auto member = importDecl(nd, CurrentVersion);
-        if (!member)
-          continue;
-
-        // TODO: remove this change when #34706 lands.
-        if (!member->NextDecl)
-          enumDecl->addMember(member);
-      }
-    }
+  if (D->getClangDecl() && isa<clang::RecordDecl>(D->getClangDecl())) {
+    loadAllMembersOfRecordDecl(cast<StructDecl>(D));
     return;
   }
 

lib/ClangImporter/ImportName.cpp

@@ -1670,9 +1670,67 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
     }
   }
 
-  // For empty names, there is nothing to do.
-  if (D->getDeclName().isEmpty())
+  // Spcial case: unnamed/anonymous fields.
+  if (auto field = dyn_cast<clang::FieldDecl>(D)) {
+    if (field->isAnonymousStructOrUnion() || field->getDeclName().isEmpty()) {
+      // Generate a field name for anonymous fields, this will be used in
+      // order to be able to expose the indirect fields injected from there
+      // as computed properties forwarding the access to the subfield.
+      std::string name;
+      llvm::raw_string_ostream nameStream(name);
+
+      nameStream << "__Anonymous_field" << field->getFieldIndex();
+      result.setDeclName(swiftCtx.getIdentifier(nameStream.str()));
+      result.setEffectiveContext(field->getDeclContext());
+      return result;
+    }
+  }
+
+  if (D->getDeclName().isEmpty()) {
+    // If the type has no name and no structure name, but is not anonymous,
+    // generate a name for it. Specifically this is for cases like:
+    //   struct a {
+    //     struct {} z;
+    //   }
+    // Where the member z is an unnamed struct, but does have a member-name
+    // and is accessible as a member of struct a.
+    if (auto recordDecl = dyn_cast<clang::RecordDecl>(
+                            D->getLexicalDeclContext())) {
+      for (auto field : recordDecl->fields()) {
+        auto fieldTagDecl = field->getType()->getAsTagDecl();
+        if (fieldTagDecl == D) {
+          // Create a name for the declaration from the field name.
+          std::string name;
+          llvm::raw_string_ostream nameStream(name);
+
+          const char *kind;
+          if (fieldTagDecl->isStruct())
+            kind = "struct";
+          else if (fieldTagDecl->isUnion())
+            kind = "union";
+          else if  (fieldTagDecl->isEnum())
+            kind = "enum";
+          else
+            llvm_unreachable("unknown decl kind");
+
+          nameStream << "__Unnamed_" << kind << "_";
+          if (field->isAnonymousStructOrUnion()) {
+            nameStream << "__Anonymous_field" << field->getFieldIndex();
+          } else {
+            assert(!field->getDeclName().isEmpty() &&
+                   "Microsoft anonymous struct extension?");
+            nameStream << field->getName();
+          }
+          result.setDeclName(swiftCtx.getIdentifier(nameStream.str()));
+          result.setEffectiveContext(D->getDeclContext());
+          return result;
+        }
+      }
+    }
+
+    // Otherwise, for empty names, there is nothing to do.
     return result;
+  }
 
   /// Whether the result is a function name.
   bool isFunction = false;

test/ClangImporter/Inputs/SwiftPrivateAttr.txt

@@ -41,19 +41,19 @@ class __PrivFooSub : Foo {
 }
 func __privTest()
 struct S0 {
-  var __privValue: Int32
   init()
   init(__privValue: Int32)
+  var __privValue: Int32
 }
 struct __PrivS1 {
-  var value: Int32
   init()
   init(value: Int32)
+  var value: Int32
 }
 struct __PrivS2 {
-  var value: Int32
   init()
   init(value: Int32)
+  var value: Int32
 }
 var __PrivAnonymousA: Int { get }
 struct E0 : Equatable, RawRepresentable {

test/ClangImporter/ctypes_parse.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift %clang-importer-sdk
+// RUN: %target-typecheck-verify-swift %clang-importer-sdk -verify-ignore-unknown
 
 import ctypes
 

test/IDE/newtype.swift

@@ -128,9 +128,9 @@
 // PRINT-NEXT:  func takeMyABIOldTypeNonNullNS(_: String)
 //
 // PRINT-NEXT:  struct NSSomeContext {
-// PRINT-NEXT:    var i: Int32
 // PRINT-NEXT:    init()
 // PRINT-NEXT:    init(i: Int32)
+// PRINT-NEXT:    var i: Int32
 // PRINT-NEXT:  }
 // PRINT-NEXT:  extension NSSomeContext {
 // PRINT-NEXT:    struct Name : _ObjectiveCBridgeable, Hashable, Equatable, _SwiftNewtypeWrapper, RawRepresentable {