[clang][include-tree] Load even spurious modular dependencies (#8011)

With implicit modules, some modular dependencies can be "spurious". By that I mean that `#include "Header.h"` results in a PCM file load, but the header ends up being included textually anyways. (This can happen in situations that would trigger the `-Wincomplete-umbrella` warning.)

This patch ensures this behavior is preserved with include-tree. Previously, include-tree would simply textually include the header file and skip the PCM file load. This now ensures consistent behavior in edge-cases like the one in the attached test case (with `-fmodule-name=X`).

rdar://121220983

Author: jansvoboda11

clang/include/clang/CAS/IncludeTree.h

@@ -56,6 +56,7 @@ class IncludeTree : public IncludeTreeBase<IncludeTree> {
   class Node;
   class Module;
   class ModuleImport;
+  class SpuriousImport;
   class ModuleMap;
   class APINotes;
 
@@ -106,6 +107,7 @@ class IncludeTree : public IncludeTreeBase<IncludeTree> {
   enum class NodeKind : uint8_t {
     Tree,
     ModuleImport,
+    SpuriousImport,
   };
 
   /// The kind of node included at the given index.
@@ -337,7 +339,8 @@ class IncludeTree::ModuleImport : public IncludeTreeBase<ModuleImport> {
   /// Whether this module should only be "marked visible" rather than imported.
   bool visibilityOnly() const { return (bool)getData()[0]; }
 
-  llvm::Error print(llvm::raw_ostream &OS, unsigned Indent = 0);
+  llvm::Error print(llvm::raw_ostream &OS, unsigned Indent = 0,
+                    char End = '\n');
 
   static bool isValid(const ObjectProxy &Node) {
     if (!IncludeTreeBase::isValid(Node))
@@ -356,13 +359,66 @@ class IncludeTree::ModuleImport : public IncludeTreeBase<ModuleImport> {
 
 private:
   friend class IncludeTreeBase<ModuleImport>;
+  friend class SpuriousImport;
   friend class Node;
 
   explicit ModuleImport(ObjectProxy Node) : IncludeTreeBase(std::move(Node)) {
     assert(isValid(*this));
   }
 };
 
+class IncludeTree::SpuriousImport
+    : public IncludeTreeBase<SpuriousImport> {
+public:
+  static Expected<SpuriousImport>
+  create(ObjectStore &DB, ObjectRef ImportRef, ObjectRef TreeRef);
+
+  static constexpr StringRef getNodeKind() { return "SpIm"; }
+
+  Expected<ModuleImport> getModuleImport() {
+    std::optional<ObjectProxy> Proxy;
+    if (llvm::Error Err = getCAS().getProxy(getReference(0)).moveInto(Proxy))
+      return Err;
+    return ModuleImport(*Proxy);
+  }
+
+  Expected<IncludeTree> getIncludeTree() {
+    std::optional<ObjectProxy> Proxy;
+    if (llvm::Error Err = getCAS().getProxy(getReference(1)).moveInto(Proxy))
+      return Err;
+    return IncludeTree(*Proxy);
+  }
+
+  llvm::Error print(llvm::raw_ostream &OS, unsigned Indent = 0);
+
+  static bool isValid(const ObjectProxy &Node) {
+    if (!IncludeTreeBase::isValid(Node))
+      return false;
+    IncludeTreeBase Base(Node);
+    if (Base.getNumReferences() != 2 && Base.getData().size() != 0)
+      return false;
+    return ModuleImport::isValid(Base.getCAS(), Base.getReference(0)) &&
+           IncludeTree::isValid(Base.getCAS(), Base.getReference(1));
+  }
+
+  static bool isValid(ObjectStore &DB, ObjectRef Ref) {
+    auto Node = DB.getProxy(Ref);
+    if (!Node) {
+      llvm::consumeError(Node.takeError());
+      return false;
+    }
+    return isValid(*Node);
+  }
+
+private:
+  friend class IncludeTreeBase;
+  friend class Node;
+
+  explicit SpuriousImport(ObjectProxy Node) : IncludeTreeBase(std::move(Node)) {
+    assert(isValid(*this));
+  }
+};
+
 /// Represents an \c IncludeTree or \c ModuleImport.
 class IncludeTree::Node {
 public:
@@ -374,6 +430,10 @@ class IncludeTree::Node {
     assert(K == NodeKind::ModuleImport);
     return ModuleImport(N);
   }
+  SpuriousImport getSpuriousImport() const {
+    assert(K == NodeKind::SpuriousImport);
+    return SpuriousImport(N);
+  }
   NodeKind getKind() const { return K; }
 
   llvm::Error print(llvm::raw_ostream &OS, unsigned Indent = 0);

clang/include/clang/Lex/PPCachedActions.h

@@ -43,6 +43,12 @@ class PPCachedActions {
     // Whether this module should only be "marked visible" rather than imported.
     bool VisibilityOnly;
   };
+  /// The module that is loaded and discarded for an \c #include directive, and
+  /// the file that is included instead.
+  struct SpuriousImport {
+    IncludeModule IM;
+    IncludeFile IF;
+  };
 
   virtual ~PPCachedActions() = default;
 
@@ -56,7 +62,8 @@ class PPCachedActions {
   /// \returns the file that should be entered or module that should be imported
   /// for an \c #include directive. \c {} indicates that the directive
   /// should be skipped.
-  virtual std::variant<std::monostate, IncludeFile, IncludeModule>
+  virtual std::variant<std::monostate, IncludeFile, IncludeModule,
+                       SpuriousImport>
   handleIncludeDirective(Preprocessor &PP, SourceLocation IncludeLoc,
                          SourceLocation AfterDirectiveLoc) = 0;
 

clang/lib/CAS/IncludeTree.cpp

@@ -36,6 +36,12 @@ Expected<NodeT> IncludeTreeBase<NodeT>::create(ObjectStore &DB,
   return NodeT(*Proxy);
 }
 
+Expected<IncludeTree::SpuriousImport>
+IncludeTree::SpuriousImport::create(ObjectStore &DB, ObjectRef ImportRef,
+                                    ObjectRef TreeRef) {
+  return IncludeTreeBase::create(DB, {ImportRef, TreeRef}, {});
+}
+
 Expected<IncludeTree::File> IncludeTree::File::create(ObjectStore &DB,
                                                       StringRef Filename,
                                                       ObjectRef Contents) {
@@ -138,7 +144,9 @@ Expected<IncludeTree> IncludeTree::create(
     assert((Include.Kind == NodeKind::Tree &&
             IncludeTree::isValid(DB, Include.Ref)) ||
            (Include.Kind == NodeKind::ModuleImport &&
-            ModuleImport::isValid(DB, Include.Ref)));
+            ModuleImport::isValid(DB, Include.Ref)) ||
+           (Include.Kind == NodeKind::SpuriousImport &&
+            SpuriousImport::isValid(DB, Include.Ref)));
     Refs.push_back(Include.Ref);
     Writer.write(Include.Offset);
     static_assert(sizeof(uint8_t) == sizeof(Kind));
@@ -671,12 +679,28 @@ llvm::Error IncludeTree::FileList::print(llvm::raw_ostream &OS,
 }
 
 llvm::Error IncludeTree::ModuleImport::print(llvm::raw_ostream &OS,
-                                             unsigned Indent) {
+                                             unsigned Indent, char End) {
   if (visibilityOnly())
     OS << "(Module for visibility only) ";
   else
     OS << "(Module) ";
-  OS << getModuleName() << '\n';
+  OS << getModuleName() << End;
+  return llvm::Error::success();
+}
+
+llvm::Error IncludeTree::SpuriousImport::print(llvm::raw_ostream &OS,
+                                               unsigned Indent) {
+  auto MI = getModuleImport();
+  if (!MI)
+    return MI.takeError();
+  auto IT = getIncludeTree();
+  if (!IT)
+    return IT.takeError();
+  OS << "(Spurious import) ";
+  if (llvm::Error E = MI->print(OS, Indent, /*End=*/' '))
+    return E;
+  if (llvm::Error E = IT->print(OS, Indent))
+    return E;
   return llvm::Error::success();
 }
 
@@ -686,6 +710,8 @@ llvm::Error IncludeTree::Node::print(llvm::raw_ostream &OS, unsigned Indent) {
     return getIncludeTree().print(OS, Indent);
   case NodeKind::ModuleImport:
     return getModuleImport().print(OS, Indent);
+  case NodeKind::SpuriousImport:
+    return getSpuriousImport().print(OS, Indent);
   }
 }
 

clang/lib/Frontend/IncludeTreePPActions.cpp

@@ -84,7 +84,7 @@ class IncludeTreePPActions final : public PPCachedActions {
     return IncludeInfo.Tree.getCheckResult(Index);
   }
 
-  std::variant<std::monostate, IncludeFile, IncludeModule>
+  std::variant<std::monostate, IncludeFile, IncludeModule, SpuriousImport>
   handleIncludeDirective(Preprocessor &PP, SourceLocation IncludeLoc,
                          SourceLocation AfterDirectiveLoc) override {
     if (HasCASErrorOccurred)
@@ -116,65 +116,89 @@ class IncludeTreePPActions final : public PPCachedActions {
     if (!Node)
       return reportError(Node.takeError());
 
-    if (Node->getKind() == cas::IncludeTree::NodeKind::ModuleImport) {
-      cas::IncludeTree::ModuleImport Import = Node->getModuleImport();
+    auto MakeModuleImport = [&](cas::IncludeTree::ModuleImport Import) {
       SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> Path;
       SmallVector<StringRef, 2> ModuleComponents;
       Import.getModuleName().split(ModuleComponents, '.');
       for (StringRef Component : ModuleComponents)
         Path.emplace_back(PP.getIdentifierInfo(Component), IncludeLoc);
       return IncludeModule{std::move(Path), Import.visibilityOnly()};
-    }
-
-    assert(Node->getKind() == cas::IncludeTree::NodeKind::Tree);
-
-    cas::IncludeTree EnteredTree = Node->getIncludeTree();
-    auto File = EnteredTree.getBaseFile();
-    if (!File)
-      return reportError(File.takeError());
-    auto FilenameBlob = File->getFilename();
-    if (!FilenameBlob)
-      return reportError(FilenameBlob.takeError());
+    };
 
-    SourceManager &SM = PP.getSourceManager();
-    Expected<FileEntryRef> FE =
-        SM.getFileManager().getFileRef(FilenameBlob->getData(),
-                                       /*OpenFile=*/true);
-    if (!FE)
-      return reportError(FE.takeError());
-    FileID FID =
-        SM.createFileID(*FE, IncludeLoc, EnteredTree.getFileCharacteristic());
-    PP.markIncluded(*FE);
-    IncludeStack.push_back(
-        {std::move(EnteredTree), SM.getLocForStartOfFile(FID)});
-
-    Module *M = nullptr;
-    auto SubmoduleName = EnteredTree.getSubmoduleName();
-    if (!SubmoduleName)
-      return reportError(SubmoduleName.takeError());
-    if (*SubmoduleName) {
-      SmallVector<StringRef> ModuleComponents;
-      (*SubmoduleName)->split(ModuleComponents, '.');
-      M = PP.getHeaderSearchInfo().lookupModule(
-          ModuleComponents[0], IncludeLoc,
-          /*AllowSearch=*/false, /*AllowExtraModuleMapSearch=*/false);
-      if (!M)
-        return reportErrorTwine(llvm::Twine("failed to find module '") +
-                                ModuleComponents[0] + "'");
-      for (StringRef Sub : ArrayRef(ModuleComponents).drop_front()) {
-        M = M->findOrInferSubmodule(Sub);
+    auto MakeIncludeTree = [&](cas::IncludeTree EnteredTree)
+        -> std::variant<std::monostate, IncludeFile> {
+      auto File = EnteredTree.getBaseFile();
+      if (!File)
+        return reportError(File.takeError());
+      auto FilenameBlob = File->getFilename();
+      if (!FilenameBlob)
+        return reportError(FilenameBlob.takeError());
+
+      SourceManager &SM = PP.getSourceManager();
+      Expected<FileEntryRef> FE =
+          SM.getFileManager().getFileRef(FilenameBlob->getData(),
+                                         /*OpenFile=*/true);
+      if (!FE)
+        return reportError(FE.takeError());
+      FileID FID =
+          SM.createFileID(*FE, IncludeLoc, EnteredTree.getFileCharacteristic());
+      PP.markIncluded(*FE);
+      IncludeStack.push_back(
+          {std::move(EnteredTree), SM.getLocForStartOfFile(FID)});
+
+      Module *M = nullptr;
+      auto SubmoduleName = EnteredTree.getSubmoduleName();
+      if (!SubmoduleName)
+        return reportError(SubmoduleName.takeError());
+      if (*SubmoduleName) {
+        SmallVector<StringRef> ModuleComponents;
+        (*SubmoduleName)->split(ModuleComponents, '.');
+        M = PP.getHeaderSearchInfo().lookupModule(
+            ModuleComponents[0], IncludeLoc,
+            /*AllowSearch=*/false, /*AllowExtraModuleMapSearch=*/false);
         if (!M)
-          return reportErrorTwine(
-              llvm::Twine("failed to find or infer submodule '") + Sub + "'");
+          return reportErrorTwine(llvm::Twine("failed to find module '") +
+                                  ModuleComponents[0] + "'");
+        for (StringRef Sub : ArrayRef(ModuleComponents).drop_front()) {
+          M = M->findOrInferSubmodule(Sub);
+          if (!M)
+            return reportErrorTwine(
+                llvm::Twine("failed to find or infer submodule '") + Sub + "'");
+        }
+
+        // Add to known headers for the module.
+        ModuleMap &MMap = PP.getHeaderSearchInfo().getModuleMap();
+        Module::Header H{"", "", *FE};
+        MMap.addHeader(M, std::move(H), ModuleMap::NormalHeader);
       }
 
-      // Add to known headers for the module.
-      ModuleMap &MMap = PP.getHeaderSearchInfo().getModuleMap();
-      Module::Header H{"", "", *FE};
-      MMap.addHeader(M, std::move(H), ModuleMap::NormalHeader);
-    }
+      return IncludeFile{FID, M};
+    };
 
-    return IncludeFile{FID, M};
+    switch (Node->getKind()) {
+    case cas::IncludeTree::NodeKind::ModuleImport:
+      return MakeModuleImport(Node->getModuleImport());
+    case cas::IncludeTree::NodeKind::Tree: {
+      auto IncludeTree = MakeIncludeTree(Node->getIncludeTree());
+      if (std::holds_alternative<std::monostate>(IncludeTree))
+        return std::monostate{};
+      return std::get<IncludeFile>(IncludeTree);
+    }
+    case cas::IncludeTree::NodeKind::SpuriousImport: {
+      auto SpuriousImportNode = Node->getSpuriousImport();
+      auto ModuleImportNode = SpuriousImportNode.getModuleImport();
+      if (!ModuleImportNode)
+        return reportError(ModuleImportNode.takeError());
+      auto IncludeTreeNode = SpuriousImportNode.getIncludeTree();
+      if (!IncludeTreeNode)
+        return reportError(IncludeTreeNode.takeError());
+      auto ModuleImport = MakeModuleImport(*ModuleImportNode);
+      auto IncludeTree = MakeIncludeTree(*IncludeTreeNode);
+      if (std::holds_alternative<std::monostate>(IncludeTree))
+        return std::monostate{};
+      return SpuriousImport{ModuleImport, std::get<IncludeFile>(IncludeTree)};
+    }
+    }
   }
 
   void exitedFile(Preprocessor &PP, FileID FID) override {