[clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't

HeaderSearch::MarkFileModuleHeader is no longer properly checking for no-changes, and so sets the HeaderFileInfo for every `textual header` to non-external.

Author: ian-twilightcoder

clang/include/clang/Lex/HeaderSearch.h

@@ -90,7 +90,9 @@ struct HeaderFileInfo {
   LLVM_PREFERRED_TYPE(bool)
   unsigned isModuleHeader : 1;
 
-  /// Whether this header is a `textual header` in a module.
+  /// Whether this header is a `textual header` in a module. If a header is
+  /// textual in one module and normal in another module, this bit will not be
+  /// set, only `isModuleHeader`.
   LLVM_PREFERRED_TYPE(bool)
   unsigned isTextualModuleHeader : 1;
 

clang/lib/Lex/HeaderSearch.cpp

@@ -1313,11 +1313,19 @@ OptionalFileEntryRef HeaderSearch::LookupSubframeworkHeader(
 // File Info Management.
 //===----------------------------------------------------------------------===//
 
+static bool moduleMembershipNeedsMerge(const HeaderFileInfo *HFI,
+                                       ModuleMap::ModuleHeaderRole Role) {
+  if (ModuleMap::isModular(Role))
+    return !HFI->isModuleHeader || HFI->isTextualModuleHeader;
+  else if (!HFI->isModuleHeader && (Role & ModuleMap::TextualHeader))
+    return !HFI->isTextualModuleHeader;
+  else
+    return false;
+}
+
 static void mergeHeaderFileInfoModuleBits(HeaderFileInfo &HFI,
                                           bool isModuleHeader,
                                           bool isTextualModuleHeader) {
-  assert((!isModuleHeader || !isTextualModuleHeader) &&
-         "A header can't build with a module and be textual at the same time");
   HFI.isModuleHeader |= isModuleHeader;
   if (HFI.isModuleHeader)
     HFI.isTextualModuleHeader = false;
@@ -1432,7 +1440,7 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
     if ((Role & ModuleMap::ExcludedHeader))
       return;
     auto *HFI = getExistingFileInfo(FE);
-    if (HFI && HFI->isModuleHeader)
+    if (HFI && !moduleMembershipNeedsMerge(HFI, Role))
       return;
   }
 

clang/unittests/Lex/HeaderSearchTest.cpp

@@ -308,5 +308,73 @@ TEST_F(HeaderSearchTest, HeaderMapFrameworkLookup) {
   EXPECT_EQ(Search.getIncludeNameForHeader(FE), "Foo/Foo.h");
 }
 
+TEST_F(HeaderSearchTest, HeaderFileInfoMerge) {
+  auto AddHeader = [&](std::string HeaderPath) -> FileEntryRef {
+    VFS->addFile(HeaderPath, 0,
+                 llvm::MemoryBuffer::getMemBufferCopy("", HeaderPath),
+                 /*User=*/std::nullopt, /*Group=*/std::nullopt,
+                 llvm::sys::fs::file_type::regular_file);
+    return *FileMgr.getOptionalFileRef(HeaderPath);
+  };
+
+  class MockExternalHeaderFileInfoSource : public ExternalHeaderFileInfoSource {
+    HeaderFileInfo GetHeaderFileInfo(FileEntryRef FE) {
+      HeaderFileInfo HFI;
+      auto FileName = FE.getName();
+      if (FileName == ModularPath)
+        HFI.mergeModuleMembership(ModuleMap::NormalHeader);
+      else if (FileName == TextualPath)
+        HFI.mergeModuleMembership(ModuleMap::TextualHeader);
+      HFI.External = true;
+      HFI.IsValid = true;
+      return HFI;
+    }
+
+  public:
+    std::string ModularPath = "/modular.h";
+    std::string TextualPath = "/textual.h";
+  };
+
+  auto ExternalSource = new MockExternalHeaderFileInfoSource();
+  Search.SetExternalSource(ExternalSource);
+
+  // Everything should start out external.
+  auto ModularFE = AddHeader(ExternalSource->ModularPath);
+  auto TextualFE = AddHeader(ExternalSource->TextualPath);
+  EXPECT_TRUE(Search.getExistingFileInfo(ModularFE)->External);
+  EXPECT_TRUE(Search.getExistingFileInfo(TextualFE)->External);
+
+  // Marking the same role should keep it external
+  Search.MarkFileModuleHeader(ModularFE, ModuleMap::NormalHeader,
+                              /*isCompilingModuleHeader=*/false);
+  Search.MarkFileModuleHeader(TextualFE, ModuleMap::TextualHeader,
+                              /*isCompilingModuleHeader=*/false);
+  EXPECT_TRUE(Search.getExistingFileInfo(ModularFE)->External);
+  EXPECT_TRUE(Search.getExistingFileInfo(TextualFE)->External);
+
+  // textual -> modular should update the HFI, but modular -> textual should be
+  // a no-op.
+  Search.MarkFileModuleHeader(ModularFE, ModuleMap::TextualHeader,
+                              /*isCompilingModuleHeader=*/false);
+  Search.MarkFileModuleHeader(TextualFE, ModuleMap::NormalHeader,
+                              /*isCompilingModuleHeader=*/false);
+  auto ModularFI = Search.getExistingFileInfo(ModularFE);
+  auto TextualFI = Search.getExistingFileInfo(TextualFE);
+  EXPECT_TRUE(ModularFI->External);
+  EXPECT_TRUE(ModularFI->isModuleHeader);
+  EXPECT_FALSE(ModularFI->isTextualModuleHeader);
+  EXPECT_FALSE(TextualFI->External);
+  EXPECT_TRUE(ModularFI->isModuleHeader);
+  EXPECT_FALSE(ModularFI->isTextualModuleHeader);
+
+  // Compiling the module should make the HFI local.
+  Search.MarkFileModuleHeader(ModularFE, ModuleMap::NormalHeader,
+                              /*isCompilingModuleHeader=*/true);
+  Search.MarkFileModuleHeader(TextualFE, ModuleMap::NormalHeader,
+                              /*isCompilingModuleHeader=*/true);
+  EXPECT_FALSE(Search.getExistingFileInfo(ModularFE)->External);
+  EXPECT_FALSE(Search.getExistingFileInfo(TextualFE)->External);
+}
+
 } // namespace
 } // namespace clang