Merge pull request #29659 from slavapestov/nlml-cleanup

Small cleanup for named lazy member loading

Author: slavapestov

include/swift/AST/LazyResolver.h

@@ -81,10 +81,7 @@ class alignas(void*) LazyMemberLoader {
 
   /// Populates a vector with all members of \p IDC that have DeclName
   /// matching \p N.
-  ///
-  /// Returns None if an error occurred \em or named member-lookup
-  /// was otherwise unsupported in this implementation or Decl.
-  virtual Optional<TinyPtrVector<ValueDecl *>>
+  virtual TinyPtrVector<ValueDecl *>
   loadNamedMembers(const IterableDeclContext *IDC, DeclBaseName N,
                    uint64_t contextData) = 0;
 

include/swift/Basic/Statistics.def

@@ -243,9 +243,6 @@ FRONTEND_STATISTIC(Sema, NumLazyIterableDeclContexts)
 /// Number of member-name lookups that avoided loading all members.
 FRONTEND_STATISTIC(Sema, NamedLazyMemberLoadSuccessCount)
 
-/// Number of member-name lookups that wound up loading all members.
-FRONTEND_STATISTIC(Sema, NamedLazyMemberLoadFailureCount)
-
 /// Number of types deserialized.
 FRONTEND_STATISTIC(Sema, NumTypesDeserialized)
 

lib/AST/NameLookup.cpp

@@ -1143,34 +1143,27 @@ void ExtensionDecl::addedMember(Decl *member) {
 // MemberLookupTable is constructed (and possibly has entries in it),
 // MemberLookupTable is incrementally reconstituted with new members.
 
-static bool
+static void
 populateLookupTableEntryFromLazyIDCLoader(ASTContext &ctx,
                                           MemberLookupTable &LookupTable,
                                           DeclBaseName name,
                                           IterableDeclContext *IDC) {
   auto ci = ctx.getOrCreateLazyIterableContextData(IDC,
                                                    /*lazyLoader=*/nullptr);
-  if (auto res = ci->loader->loadNamedMembers(IDC, name, ci->memberData)) {
-    if (auto s = ctx.Stats) {
-      ++s->getFrontendCounters().NamedLazyMemberLoadSuccessCount;
-    }
-    for (auto d : *res) {
-      LookupTable.addMember(d);
-    }
-    return false;
-  } else {
-    if (auto s = ctx.Stats) {
-      ++s->getFrontendCounters().NamedLazyMemberLoadFailureCount;
-    }
-    return true;
+  auto res = ci->loader->loadNamedMembers(IDC, name, ci->memberData);
+  if (auto s = ctx.Stats) {
+    ++s->getFrontendCounters().NamedLazyMemberLoadSuccessCount;
+  }
+  for (auto d : res) {
+    LookupTable.addMember(d);
   }
 }
 
 static void
 populateLookupTableEntryFromExtensions(ASTContext &ctx,
                                        MemberLookupTable &table,
-                                       NominalTypeDecl *nominal,
-                                       DeclBaseName name) {
+                                       DeclBaseName name,
+                                       NominalTypeDecl *nominal) {
   assert(!table.isLazilyComplete(name) &&
          "Should not be searching extensions for complete name!");
 
@@ -1185,12 +1178,7 @@ populateLookupTableEntryFromExtensions(ASTContext &ctx,
            "Extension without deserializable content has lazy members!");
     assert(!e->hasUnparsedMembers());
 
-    // Try lazy loading. If that fails, then we fall back by loading the
-    // entire extension. FIXME: It's rather unfortunate that we fall off the
-    // happy path because the Clang Importer can't handle lazy import-as-member.
-    if (populateLookupTableEntryFromLazyIDCLoader(ctx, table, name, e)) {
-      e->loadAllMembers();
-    }
+    populateLookupTableEntryFromLazyIDCLoader(ctx, table, name, e);
   }
 }
 
@@ -1280,55 +1268,44 @@ DirectLookupRequest::evaluate(Evaluator &evaluator,
 
   decl->prepareLookupTable();
 
-  auto tryCacheLookup =
-      [=](MemberLookupTable &table,
-          DeclName name) -> Optional<TinyPtrVector<ValueDecl *>> {
-    // Look for a declaration with this name.
-    auto known = table.find(name);
-    if (known == table.end()) {
-      return None;
-    }
-
-    // We found something; return it.
-    return maybeFilterOutAttrImplements(known->second, name,
-                                        includeAttrImplements);
-  };
-
-  auto updateLookupTable = [&decl](MemberLookupTable &table,
-                                   bool noExtensions) {
+  auto &Table = *decl->LookupTable;
+  if (!useNamedLazyMemberLoading) {
     // Make sure we have the complete list of members (in this nominal and in
     // all extensions).
     (void)decl->getMembers();
 
-    if (noExtensions)
-      return;
-
-    for (auto E : decl->getExtensions())
-      (void)E->getMembers();
+    if (!disableAdditionalExtensionLoading) {
+      for (auto E : decl->getExtensions())
+        (void)E->getMembers();
 
-    table.updateLookupTable(decl);
-  };
-
-  auto &Table = *decl->LookupTable;
-  if (!useNamedLazyMemberLoading) {
-    updateLookupTable(Table, disableAdditionalExtensionLoading);
+      Table.updateLookupTable(decl);
+    }
   } else if (!Table.isLazilyComplete(name.getBaseName())) {
     // The lookup table believes it doesn't have a complete accounting of this
     // name - either because we're never seen it before, or another extension
     // was registered since the last time we searched. Ask the loaders to give
     // us a hand.
     DeclBaseName baseName(name.getBaseName());
-    if (populateLookupTableEntryFromLazyIDCLoader(ctx, Table, baseName, decl)) {
-      updateLookupTable(Table, disableAdditionalExtensionLoading);
-    } else if (!disableAdditionalExtensionLoading) {
-      populateLookupTableEntryFromExtensions(ctx, Table, decl, baseName);
+    populateLookupTableEntryFromLazyIDCLoader(ctx, Table, baseName, decl);
+
+    if (!disableAdditionalExtensionLoading) {
+      populateLookupTableEntryFromExtensions(ctx, Table, baseName, decl);
     }
+
+    // FIXME: If disableAdditionalExtensionLoading is true, we should
+    // not mark the entry as complete.
     Table.markLazilyComplete(baseName);
   }
 
   // Look for a declaration with this name.
-  return tryCacheLookup(Table, name)
-            .getValueOr(TinyPtrVector<ValueDecl *>());
+  auto known = Table.find(name);
+  if (known == Table.end()) {
+    return TinyPtrVector<ValueDecl *>();
+  }
+
+  // We found something; return it.
+  return maybeFilterOutAttrImplements(known->second, name,
+                                      includeAttrImplements);
 }
 
 void ClassDecl::createObjCMethodLookup() {

lib/ClangImporter/ClangImporter.cpp

@@ -3723,7 +3723,7 @@ void ClangImporter::Implementation::lookupAllObjCMembers(
   }
 }
 
-Optional<TinyPtrVector<ValueDecl *>>
+TinyPtrVector<ValueDecl *>
 ClangImporter::Implementation::loadNamedMembers(
     const IterableDeclContext *IDC, DeclBaseName N, uint64_t contextData) {
 

lib/ClangImporter/ImporterImpl.h

@@ -1238,7 +1238,7 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   virtual void
   loadAllMembers(Decl *D, uint64_t unused) override;
 
-  virtual Optional<TinyPtrVector<ValueDecl *>>
+  virtual TinyPtrVector<ValueDecl *>
   loadNamedMembers(const IterableDeclContext *IDC, DeclBaseName N,
                    uint64_t contextData) override;
 

lib/Serialization/Deserialization.cpp

@@ -2831,14 +2831,10 @@ class DeclDeserializer {
     if (numBackingProperties > 0) {
       auto backingDecl = MF.getDeclChecked(backingPropertyIDs[0]);
       if (!backingDecl) {
-        if (numBackingProperties > 1 &&
-            backingDecl.errorIsA<XRefNonLoadedModuleError>()) {
-            // A property wrapper defined behind an implementation-only import
-            // is safe to drop when it can't be deserialized.
-            // rdar://problem/56599179
-            consumeError(backingDecl.takeError());
-        } else
-          return backingDecl.takeError();
+        // FIXME: This is actually wrong. We can't just drop stored properties
+        // willy-nilly if the struct is @frozen.
+        consumeError(backingDecl.takeError());
+        return var;
       }
 
       VarDecl *backingVar = cast<VarDecl>(backingDecl.get());

lib/Serialization/ModuleFile.cpp

@@ -2376,7 +2376,7 @@ void ModuleFile::loadObjCMethods(
   }
 }
 
-Optional<TinyPtrVector<ValueDecl *>>
+TinyPtrVector<ValueDecl *>
 ModuleFile::loadNamedMembers(const IterableDeclContext *IDC, DeclBaseName N,
                              uint64_t contextData) {
   PrettyStackTraceDecl trace("loading members for", IDC->getDecl());
@@ -2399,7 +2399,7 @@ ModuleFile::loadNamedMembers(const IterableDeclContext *IDC, DeclBaseName N,
         fatalIfUnexpected(DeclMemberTablesCursor.advance());
     if (entry.Kind != llvm::BitstreamEntry::Record) {
       fatal();
-      return None;
+      return results;
     }
     SmallVector<uint64_t, 64> scratch;
     StringRef blobData;
@@ -2424,10 +2424,6 @@ ModuleFile::loadNamedMembers(const IterableDeclContext *IDC, DeclBaseName N,
         if (!getContext().LangOpts.EnableDeserializationRecovery)
           fatal(mem.takeError());
         consumeError(mem.takeError());
-
-        // Treat this as a cache-miss to the caller and let them attempt
-        // to refill through the normal loadAllMembers() path.
-        return None;
       }
     }
   }

lib/Serialization/ModuleFile.h

@@ -835,8 +835,7 @@ class ModuleFile
   virtual void loadAllMembers(Decl *D,
                               uint64_t contextData) override;
 
-  virtual
-  Optional<TinyPtrVector<ValueDecl *>>
+  virtual TinyPtrVector<ValueDecl *>
   loadNamedMembers(const IterableDeclContext *IDC, DeclBaseName N,
                    uint64_t contextData) override;