[WIP][modules] Avoid deserializing Decls from hidden (sub)modules.

[vsapsai]

Summary

The test case is for Objective-C but it is likely other languages like
C++ are affected as well, just haven't created appropriate test cases.

The test case fails with the following error

clang/test/Modules/Output/ambiguous-enum-lookup.m.tmp/test.m:7:11: error: reference to 'kEnumValue' is ambiguous
    s.x = kEnumValue;
          ^
clang/test/Modules/Output/ambiguous-enum-lookup.m.tmp/Frameworks/NonModular.framework/Headers/NonModular.h:8:5: note: candidate found by name lookup is 'kEnumValue'
    kEnumValue = 1,
    ^
clang/test/Modules/Output/ambiguous-enum-lookup.m.tmp/Frameworks/NonModular.framework/Headers/NonModular.h:8:5: note: candidate found by name lookup is 'kEnumValue'
    kEnumValue = 1,
    ^

It happens because we have 2 EnumConstantDecl for 'kEnumValue' - one from
PiecewiseVisible module, another non-modular. And the the name lookup is
ambiguous because these decls have different canonical decls.
Non-modular decl believes it is the canonical decl because that's the
behavior for Mergeable and in the source code non-modular decl comes
before other EnumConstantDecl. Modular EnumConstantDecl has no intention
of being canonical decl but it is deserialized *before* non-modular decl
is created, so as the only decl it becomes the canonical decl.

The idea for the fix is to skip deserialization of decls from hidden
(sub)modules, thus allowing to establish correct canonical decl. This is
a proof of concept, the implementation isn't optimal, and there are
failing tests. I mostly wanted to share the approach to discuss it.

Another high-level approach I've considered is to enhance handling decls
from hidden modules and to delay connecting them with redecl chains and
canonical decls till the moment they become visible. I've decided to go
with "avoid deserialization" approach because this way we should be
doing less work and it seems less error-prone compared to juggling with
existing decls.

rdar://82908206

[vsapsai]

Fix some failing tests to demonstrate the magnitued of changes better.

In some places check if a decl is deserialized successfully as it's not guaranteed.
Also tried to make sure a module is marked as visible *before* we try to deserialize its decls.

[vsapsai]

Mark identifiers as out-of-date not only on loading a new module but on making a (sub)module visible too.

[vsapsai]

Switch from checking module visibility to checking if a module is imported.

Required to handle not re-exported modules - allow to deserialize decls from
them even if the modules aren't visible.

[vsapsai]

After more investigation and comparing different entities (like structs, enums) this approach seems to be too involved and probably wrong. A better choice seems to be following the same direction as in ⦗Modules⦘ Implement ODR-like semantics for tag types in C/ObjC than inventing something new. Specifically, I plan to look into discrepancies between EnumDecl and EnumConstantDecl as we don't have problems with enums themselves, only with the constants.

clang/include/clang/Lex/Preprocessor.h

private:
   /// The names of potential module macros that we've not yet processed.
   llvm::SmallVector<const IdentifierInfo *, 32> PendingModuleMacroNames;
 
   /// The list of module macros, for each identifier, that are not overridden by
   /// any other module macro.
   llvm::DenseMap<const IdentifierInfo *, llvm::TinyPtrVector<ModuleMacro *>>
       LeafModuleMacros;
 
+  // Not the same as visible modules as ignores visibility controls and tracks
+  // transitively imported submodules. Should be tracked in SubmoduleState but
+  // for the proof of concept staying away from Local Submodule Visibility
+  // shenanigans.
+  llvm::DenseSet<const Module *> AllImportedSubModules;
+
   /// Macros that we want to warn because they are not used at the end
   /// of the translation unit.
   ///
   /// We store just their SourceLocations instead of
   /// something like MacroInfo*. The benefit of this is that when we are
   /// deserializing from PCH, we don't need to deserialize identifier & macros
   /// just so that we can report that they are unused, we just warn using
   /// the SourceLocations of this set (that will be filled by the ASTReader).
public:
   void CollectPpImportSuffix(SmallVectorImpl<Token> &Toks);
 
   void makeModuleVisible(Module *M, SourceLocation Loc);
 
   SourceLocation getModuleImportLoc(Module *M) const {
     return CurSubmoduleState->VisibleModules.getImportLoc(M);
   }
 
+  bool isModuleImported(const Module *M) const {
+    return AllImportedSubModules.count(M);
+  }
+
   /// Lex a string literal, which may be the concatenation of multiple
   /// string literals and may even come from macro expansion.
   /// \returns true on success, false if a error diagnostic has been generated.
   bool LexStringLiteral(Token &Result, std::string &String,
                         const char *DiagnosticTag, bool AllowMacroExpansion) {
     if (AllowMacroExpansion)
       Lex(Result);
     else

clang/include/clang/Serialization/ASTReader.h

private:
   /// The receiver of some callbacks invoked by ASTReader.
   std::unique_ptr<ASTReaderListener> Listener;
 
   /// The receiver of deserialization events.
   ASTDeserializationListener *DeserializationListener = nullptr;
 
   bool OwnsDeserializationListener = false;
 
+  // Special types are deserialized before we have a chance to mark a submodule
+  // as visible or hidden. Add a hacky way to allow deserialization of types'
+  // decls even if we don't know if a corresponding module is visible.
+  bool HackyIsDeserializingSpecialTypes = false;
+
   SourceManager &SourceMgr;
   FileManager &FileMgr;
   const PCHContainerReader &PCHContainerRdr;
   DiagnosticsEngine &Diags;
 
   /// The semantic analysis object that will be processing the
   /// AST files and the translation unit that uses it.
   Sema *SemaObj = nullptr;

clang/lib/Lex/Preprocessor.cpp

bool Preprocessor::LexAfterModuleImport(Token &Result) {
   if (!Suffix.empty()) {
     EnterTokens(Suffix);
     return false;
   }
   return true;
 }
 
 void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc) {
+  bool HasAddedMoreModules = false;
   CurSubmoduleState->VisibleModules.setVisible(
-      M, Loc, [](Module *) {},
+      M, Loc, [&HasAddedMoreModules](Module *) { HasAddedMoreModules = true; },
       [&](ArrayRef<Module *> Path, Module *Conflict, StringRef Message) {
         // FIXME: Include the path in the diagnostic.
         // FIXME: Include the import location for the conflicting module.
         Diag(ModuleImportLoc, diag::warn_module_conflict)
             << Path[0]->getFullModuleName()
             << Conflict->getFullModuleName()
             << Message;
       });
 
   // Add this module to the imports list of the currently-built submodule.
   if (!BuildingSubmoduleStack.empty() && M != BuildingSubmoduleStack.back().M)
     BuildingSubmoduleStack.back().M->Imports.insert(M);
+
+  std::function<void(const Module*)> VisitModule = [&](const Module *VisM) {
+    if (VisM->IsUnimportable)
+      return;
+    if (AllImportedSubModules.count(VisM))
+      return;
+    AllImportedSubModules.insert(VisM);
+
+    for (const Module *ImportedM : VisM->Imports)
+      VisitModule(ImportedM);
+
+    // Need to traverse also exported modules to handle implicitly-imported
+    // submodules like
+    //     module Parent {
+    //       export *
+    //       module Child { header "child.h" }
+    //     }
+    // Where `@import Parent` also pulls in the module "Parent.Child".
+    SmallVector<Module *, 16> Exports;
+    VisM->getExportedModules(Exports);
+    for (const Module *ExportedM : Exports)
+      VisitModule(ExportedM);
+  };
+  VisitModule(M);
+
+  // EXPERIMENTAL: Setting a (sub)module visible allows to deserialize its
+  // decls. For this reason marking the identifiers out-of-date like
+  // in `ASTReader::ReadAST`.
+  if (HasAddedMoreModules && getExternalSource()) {
+    if (!getLangOpts().CPlusPlus) {
+      for (IdentifierTable::iterator Id = getIdentifierTable().begin(),
+                                     IdEnd = getIdentifierTable().end();
+           Id != IdEnd; ++Id)
+        Id->second->setOutOfDate(true);
+    }
+  }
 }
 
 bool Preprocessor::FinishLexStringLiteral(Token &Result, std::string &String,
                                           const char *DiagnosticTag,
                                           bool AllowMacroExpansion) {
   // We need at least one string literal.
   if (Result.isNot(tok::string_literal)) {
     Diag(Result, diag::err_expected_string_literal)

clang/lib/Sema/SemaModule.cpp

 }
 
 void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
   checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);
   BuildModuleInclude(DirectiveLoc, Mod);
 }
 
 void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
+  // Make the module visible before adding a module initializer.
+  getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, DirectiveLoc);
+  VisibleModules.setVisible(Mod, DirectiveLoc);
+
   // Determine whether we're in the #include buffer for a module. The #includes
   // in that buffer do not qualify as module imports; they're just an
   // implementation detail of us building the module.
   //
   // FIXME: Should we even get ActOnModuleInclude calls for those?
   bool IsInModuleIncludes =
       TUKind == TU_Module &&
       getSourceManager().isWrittenInMainFile(DirectiveLoc);
 
   bool ShouldAddImport = !IsInModuleIncludes;
 
   // If this module import was due to an inclusion directive, create an
   // implicit import declaration to capture it in the AST.
   if (ShouldAddImport) {
     TranslationUnitDecl *TU = getASTContext().getTranslationUnitDecl();
     ImportDecl *ImportD = ImportDecl::CreateImplicit(getASTContext(), TU,
                                                      DirectiveLoc, Mod,
                                                      DirectiveLoc);
     if (!ModuleScopes.empty())
       Context.addModuleInitializer(ModuleScopes.back().Module, ImportD);
     TU->addDecl(ImportD);
     Consumer.HandleImplicitImportDecl(ImportD);
   }
-
-  getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, DirectiveLoc);
-  VisibleModules.setVisible(Mod, DirectiveLoc);
 }
 
 void Sema::ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod) {
   checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);
 
   ModuleScopes.push_back({});
   ModuleScopes.back().Module = Mod;
   if (getLangOpts().ModulesLocalVisibility)

clang/lib/Serialization/ASTReader.cpp

void ASTReader::InitializeContext() {
   if (DeserializationListener)
     DeserializationListener->DeclRead(PREDEF_DECL_TRANSLATION_UNIT_ID,
                                       Context.getTranslationUnitDecl());
 
   // FIXME: Find a better way to deal with collisions between these
   // built-in types. Right now, we just ignore the problem.
 
   // Load the special types.
+  HackyIsDeserializingSpecialTypes = true;
   if (SpecialTypes.size() >= NumSpecialTypeIDs) {
     if (unsigned String = SpecialTypes[SPECIAL_TYPE_CF_CONSTANT_STRING]) {
       if (!Context.CFConstantStringTypeDecl)
         Context.setCFConstantStringType(GetType(String));
     }
 
     if (unsigned File = SpecialTypes[SPECIAL_TYPE_FILE]) {
       QualType FileType = GetType(File);
if (unsigned Ucontext_t = SpecialTypes[SPECIAL_TYPE_UCONTEXT_T]) {
         else {
           const TagType *Tag = Ucontext_tType->getAs<TagType>();
           assert(Tag && "Invalid ucontext_t type in AST file");
           Context.setucontext_tDecl(Tag->getDecl());
         }
       }
     }
   }
+  HackyIsDeserializingSpecialTypes = false;
 
   ReadPragmaDiagnosticMappings(Context.getDiagnostics());
 
   // If there were any CUDA special declarations, deserialize them.
   if (!CUDASpecialDeclRefs.empty()) {
     assert(CUDASpecialDeclRefs.size() == 1 && "More decl refs than expected!");
     Context.setcudaConfigureCallDecl(
                            cast<FunctionDecl>(GetDecl(CUDASpecialDeclRefs[0])));
if (It == Lookups.end())
     return false;
 
   Deserializing LookupResults(this);
 
   // Load the list of declarations.
   SmallVector<NamedDecl *, 64> Decls;
   llvm::SmallPtrSet<NamedDecl *, 8> Found;
   for (DeclID ID : It->second.Table.find(Name)) {
-    NamedDecl *ND = cast<NamedDecl>(GetDecl(ID));
+    if (NamedDecl *ND = cast_or_null<NamedDecl>(GetDecl(ID)))
       if (ND->getDeclName() == Name && Found.insert(ND).second)
         Decls.push_back(ND);
   }
 
   ++NumVisibleDeclContextsRead;
   SetExternalVisibleDeclsForName(DC, Name, Decls);
   return !Decls.empty();
 }
 
 void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) {
for (unsigned I = 0, N = DeclIDs.size(); I != N; ++I) {
     if (!SemaObj) {
       // Queue this declaration so that it will be added to the
       // translation unit scope and identifier's declaration chain
       // once a Sema object is known.
       PreloadedDeclIDs.push_back(DeclIDs[I]);
       continue;
     }
 
-    NamedDecl *D = cast<NamedDecl>(GetDecl(DeclIDs[I]));
+    if (Decl *DeserializedD = GetDecl(DeclIDs[I])) {
+      NamedDecl *D = cast<NamedDecl>(DeserializedD);
 
       // If we're simply supposed to record the declarations, do so now.
       if (Decls) {
         Decls->push_back(D);
         continue;
       }
 
       // Introduce this declaration into the translation-unit scope
       // and add it to the declaration chain for this identifier, so
       // that (unqualified) name lookup will find it.
       pushExternalDeclIntoScope(D, II);
     }
   }
+}
 
 IdentifierInfo *ASTReader::DecodeIdentifierInfo(IdentifierID ID) {
   if (ID == 0)
     return nullptr;
 
   if (IdentifiersLoaded.empty()) {
     Error("no identifier table in AST file");
     return nullptr;

clang/lib/Serialization/ASTReaderDecl.cpp

 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/Lambda.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/Linkage.h"
 #include "clang/Basic/Module.h"
 #include "clang/Basic/PragmaKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/IdentifierResolver.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTRecordReader.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
 #include "clang/Serialization/ModuleFile.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/STLExtras.h"
Decl *ASTReader::ReadDeclRecord(DeclID ID) {
 
   ASTContext &Context = getContext();
   Decl *D = nullptr;
   Expected<unsigned> MaybeDeclCode = Record.readRecord(DeclsCursor, Code);
   if (!MaybeDeclCode)
     llvm::report_fatal_error(
         Twine("ASTReader::readDeclRecord failed reading decl code: ") +
         toString(MaybeDeclCode.takeError()));
+  if (unsigned DeclSubmoduleID = getGlobalSubmoduleID(*Loc.F, Record.readInt())) {
+    if (!HackyIsDeserializingSpecialTypes) {
+      Module *DeclOwner = getSubmodule(DeclSubmoduleID);
+      if (DeclOwner && !PP.isModuleImported(DeclOwner)) {
+        // Skip deserialization of Decls from a hidden module.
+        return nullptr;
+      }
+    }
+  }
   switch ((DeclCode)MaybeDeclCode.get()) {
   case DECL_CONTEXT_LEXICAL:
   case DECL_CONTEXT_VISIBLE:
     llvm_unreachable("Record cannot be de-serialized with readDeclRecord");
   case DECL_TYPEDEF:
     D = TypedefDecl::CreateDeserialized(Context, ID);
     break;
   case DECL_TYPEALIAS:
else
     llvm::report_fatal_error(
         Twine("ASTReader::loadPendingDeclChain failed reading rec code: ") +
         toString(MaybeCode.takeError()));
 
   // FIXME: We have several different dispatches on decl kind here; maybe
   // we should instead generate one loop per kind and dispatch up-front?
   Decl *MostRecent = FirstLocal;
   for (unsigned I = 0, N = Record.size(); I != N; ++I) {
-    auto *D = GetLocalDecl(*M, Record[N - I - 1]);
+    // A local decl is from the same module but can be from a hidden submodule.
+    if (Decl *D = GetLocalDecl(*M, Record[N - I - 1])) {
       ASTDeclReader::attachPreviousDecl(*this, D, MostRecent, CanonDecl);
       MostRecent = D;
     }
+  }
   ASTDeclReader::attachLatestDecl(CanonDecl, MostRecent);
 }
 
 namespace {
 
   /// Given an ObjC interface, goes through the modules and links to the
   /// interface all the categories for it.
   class ObjCCategoriesVisitor {

clang/lib/Serialization/ASTWriterDecl.cpp

void RegisterTemplateSpecialization(const Decl *Template,
 
       Writer.DeclUpdates[Template].push_back(ASTWriter::DeclUpdate(
           UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION, Specialization));
     }
   };
 }
 
 void ASTDeclWriter::Visit(Decl *D) {
+  Record.push_back(Writer.getSubmoduleID(D->getOwningModule()));
   DeclVisitor<ASTDeclWriter>::Visit(D);
 
   // Source locations require array (variable-length) abbreviations.  The
   // abbreviation infrastructure requires that arrays are encoded last, so
   // we handle it here in the case of those classes derived from DeclaratorDecl
   if (DeclaratorDecl *DD = dyn_cast<DeclaratorDecl>(D)) {
     if (auto *TInfo = DD->getTypeSourceInfo())
       Record.AddTypeLoc(TInfo->getTypeLoc());

 void ASTWriter::WriteDeclAbbrevs() {
   using namespace llvm;
 
   std::shared_ptr<BitCodeAbbrev> Abv;
 
   // Abbreviation for DECL_FIELD
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_FIELD));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // isInvalidDecl
   Abv->Add(BitCodeAbbrevOp(0));                       // HasAttrs
   Abv->Add(BitCodeAbbrevOp(0));                       // isImplicit
   Abv->Add(BitCodeAbbrevOp(0));                       // isUsed
   Abv->Add(BitCodeAbbrevOp(0));                       // isReferenced
void ASTWriter::WriteDeclAbbrevs() {
   // Type Source Info
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // TypeLoc
   DeclFieldAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_OBJC_IVAR
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_OBJC_IVAR));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // isInvalidDecl
   Abv->Add(BitCodeAbbrevOp(0));                       // HasAttrs
   Abv->Add(BitCodeAbbrevOp(0));                       // isImplicit
   Abv->Add(BitCodeAbbrevOp(0));                       // isUsed
   Abv->Add(BitCodeAbbrevOp(0));                       // isReferenced
void ASTWriter::WriteDeclAbbrevs() {
   // Type Source Info
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // TypeLoc
   DeclObjCIvarAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_ENUM
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_ENUM));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // Redeclarable
   Abv->Add(BitCodeAbbrevOp(0));                       // No redeclaration
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // isInvalidDecl
   Abv->Add(BitCodeAbbrevOp(0));                       // HasAttrs
   Abv->Add(BitCodeAbbrevOp(0));                       // isImplicit
void ASTWriter::WriteDeclAbbrevs() {
   // DC
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // LexicalOffset
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // VisibleOffset
   DeclEnumAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_RECORD
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_RECORD));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // Redeclarable
   Abv->Add(BitCodeAbbrevOp(0));                       // No redeclaration
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // isInvalidDecl
   Abv->Add(BitCodeAbbrevOp(0));                       // HasAttrs
   Abv->Add(BitCodeAbbrevOp(0));                       // isImplicit
void ASTWriter::WriteDeclAbbrevs() {
   // DC
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // LexicalOffset
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // VisibleOffset
   DeclRecordAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_PARM_VAR
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_PARM_VAR));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // Redeclarable
   Abv->Add(BitCodeAbbrevOp(0));                       // No redeclaration
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // isInvalidDecl
   Abv->Add(BitCodeAbbrevOp(0));                       // HasAttrs
   Abv->Add(BitCodeAbbrevOp(0));                       // isImplicit
void ASTWriter::WriteDeclAbbrevs() {
   // Type Source Info
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // TypeLoc
   DeclParmVarAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_TYPEDEF
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_TYPEDEF));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // Redeclarable
   Abv->Add(BitCodeAbbrevOp(0));                       // No redeclaration
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // isInvalidDecl
   Abv->Add(BitCodeAbbrevOp(0));                       // HasAttrs
   Abv->Add(BitCodeAbbrevOp(0));                       // isImplicit
void ASTWriter::WriteDeclAbbrevs() {
   // TypedefDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // TypeLoc
   DeclTypedefAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_VAR
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_VAR));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // Redeclarable
   Abv->Add(BitCodeAbbrevOp(0));                       // No redeclaration
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                       // isInvalidDecl
   Abv->Add(BitCodeAbbrevOp(0));                       // HasAttrs
   Abv->Add(BitCodeAbbrevOp(0));                       // isImplicit
void ASTWriter::WriteDeclAbbrevs() {
   // Type Source Info
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // TypeLoc
   DeclVarAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_CXX_METHOD
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::DECL_CXX_METHOD));
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
   // RedeclarableDecl
   Abv->Add(BitCodeAbbrevOp(0));                         // CanonicalDecl
   // Decl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // DeclContext
   Abv->Add(BitCodeAbbrevOp(0));                         // LexicalDeclContext
   Abv->Add(BitCodeAbbrevOp(0));                         // Invalid
   Abv->Add(BitCodeAbbrevOp(0));                         // HasAttrs
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Implicit

clang/test/Modules/ambiguous-enum-lookup.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -F %t/Frameworks %t/test.m
+
+//--- Frameworks/NonModular.framework/Headers/NonModular.h
+#ifndef NonModular_NonModular_h
+#define NonModular_NonModular_h
+struct SomeStruct {
+    int x;
+};
+
+enum SomeEnum {
+    kEnumValue = 1,
+};
+#endif
+
+//--- Frameworks/PiecewiseVisible.framework/Headers/InitiallyVisible.h
+// empty
+
+//--- Frameworks/PiecewiseVisible.framework/Headers/InitiallyHidden.h
+#include <NonModular/NonModular.h>
+
+//--- Frameworks/PiecewiseVisible.framework/Modules/module.modulemap
+framework module PiecewiseVisible {
+  header "InitiallyVisible.h"
+  export *
+
+  explicit module HiddenPiece {
+    header "InitiallyHidden.h"
+    export *
+  }
+}
+
+//--- test.m
+#include <PiecewiseVisible/InitiallyVisible.h>
+#include <NonModular/NonModular.h>
+#include <PiecewiseVisible/InitiallyHidden.h>
+
+void test() {
+    struct SomeStruct s;
+    s.x = kEnumValue;
+}