[clang][include-tree] Fix spurious dependencies

clang/include/clang/Basic/Module.h

@@ -341,6 +341,10 @@ class alignas(8) Module {
   /// Whether this is an inferred submodule (module * { ... }).
   unsigned IsInferred : 1;
 
+  /// Whether this is an inferred submodule that's missing from the umbrella
+  /// header.
+  unsigned IsInferredMissingFromUmbrellaHeader : 1;
+
   /// Whether we should infer submodules for this module based on
   /// the headers.
   ///

clang/lib/Basic/Module.cpp

@@ -42,6 +42,7 @@ Module::Module(StringRef Name, SourceLocation DefinitionLoc, Module *Parent,
       HasIncompatibleModuleFile(false), IsAvailable(true),
       IsFromModuleFile(false), IsFramework(IsFramework), IsExplicit(IsExplicit),
       IsSystem(false), IsExternC(false), IsInferred(false),
+      IsInferredMissingFromUmbrellaHeader(false),
       InferSubmodules(false), InferExplicitSubmodules(false),
       InferExportWildcard(false), ConfigMacrosExhaustive(false),
       NoUndeclaredIncludes(false), ModuleMapIsPrivate(false),

clang/lib/Frontend/CompilerInstance.cpp

@@ -2234,6 +2234,8 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
         << Module->getFullModuleName()
         << SourceRange(Path.front().second, Path.back().second);
 
+      Module->IsInferredMissingFromUmbrellaHeader = true;
+
       return ModuleLoadResult(Module, ModuleLoadResult::MissingExpected);
     }
 

clang/lib/Tooling/DependencyScanning/IncludeTreeActionController.cpp

@@ -204,12 +204,7 @@ struct IncludeTreePPCallbacks : public PPCallbacks {
                           bool ModuleImported,
                           SrcMgr::CharacteristicKind FileType) override {
     // File includes are handled by LexedFileChanged.
-    if (!SuggestedModule)
-      return;
-
-    // Modules that will not be imported and were not loaded from an AST file
-    // (e.g. the module being implemented) are also handled by LexedFileChanged.
-    if (!ModuleImported && !SuggestedModule->getASTFile())
+    if (!ModuleImported)
       return;
 
     // Calculate EndLoc for the directive
@@ -457,20 +452,31 @@ void IncludeTreeBuilder::exitedInclude(Preprocessor &PP, FileID IncludedBy,
   SourceManager &SM = PP.getSourceManager();
   std::pair<FileID, unsigned> LocInfo = SM.getDecomposedExpansionLoc(ExitLoc);
 
-  // If the file includes already has a node for the current source location, it
-  // must be an import that turned out to be spurious.
-  auto &CurIncludes = IncludeStack.back().Includes;
-  if (!CurIncludes.empty() && CurIncludes.back().Offset == LocInfo.second) {
-    assert(!IncludeTree->isSubmodule());
-    auto Import = CurIncludes.pop_back_val();
-    assert(Import.Kind == cas::IncludeTree::NodeKind::ModuleImport);
-    auto SpuriousImport = cas::IncludeTree::SpuriousImport::create(
-        DB, Import.Ref, IncludeTree->getRef());
-    if (!SpuriousImport)
+  // If the exited header belongs to a sub-module that's marked as missing from
+  // the umbrella, we must've first loaded its PCM file to find that out.
+  // We need to match this behavior with include-tree. Let's mark this as
+  // spurious import. For this node, Clang will load the top-level module, emit
+  // the appropriate diagnostics and then fall back to textual inclusion of the
+  // header itself.
+  if (auto FE = PP.getSourceManager().getFileEntryRefForID(Include)) {
+    ModuleMap &ModMap = PP.getHeaderSearchInfo().getModuleMap();
+    Module *M = ModMap.findModuleForHeader(*FE).getModule();
+    if (M && M->IsInferredMissingFromUmbrellaHeader) {
+      assert(!IncludeTree->isSubmodule() &&
+             "Include of header missing from umbrella header is modular");
+
+      moduleImport(PP, M, ExitLoc);
+      auto Import = IncludeStack.back().Includes.pop_back_val();
+
+      auto SpuriousImport = check(cas::IncludeTree::SpuriousImport::create(
+          DB, Import.Ref, IncludeTree->getRef()));
+      if (!SpuriousImport)
+        return;
+      IncludeStack.back().Includes.push_back(
+          {SpuriousImport->getRef(), LocInfo.second,
+           cas::IncludeTree::NodeKind::SpuriousImport});
       return;
-    CurIncludes.push_back({SpuriousImport->getRef(), LocInfo.second,
-                           cas::IncludeTree::NodeKind::SpuriousImport});
-    return;
+    }
   }
 
   IncludeStack.back().Includes.push_back({IncludeTree->getRef(), LocInfo.second,

clang/test/ClangScanDeps/modules-include-tree-implementation-transitive.c

@@ -1,15 +1,7 @@
 // REQUIRES: ondisk_cas
 
-// This test checks that we correctly handle situations where a spurious modular
-// dependency (1) turns otherwise textual dependency into modular (2).
-//
-// (1) For example #include <Spurious/Missing.h>, where the framework Spurious
-// has an umbrella header that does not include Missing.h, making it a textual
-// include instead.
-//
-// (2) For example when compiling the implementation file of a module Mod,
-// #included headers belonging to Mod are treated textually, unless some other
-// module already depends on Mod in its modular form.
+// This test checks that imports of transitively-loaded implementation module
+// are not marked as spurious.
 
 // RUN: rm -rf %t
 // RUN: split-file %s %t
@@ -19,56 +11,40 @@
 [{
   "file": "DIR/tu.m",
   "directory": "DIR",
-  "command": "clang -c DIR/tu.m -o DIR/tu.o -F DIR/frameworks -I DIR/include -fmodule-name=Mod -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+  "command": "clang -c DIR/tu.m -o DIR/tu.o -F DIR/frameworks -fmodules -fmodule-name=FW -fmodules-cache-path=DIR/module-cache"
 }]
 
-//--- frameworks/Spurious.framework/Modules/module.modulemap
-framework module Spurious {
-  umbrella header "Spurious.h"
-  module * { export * }
-}
-//--- frameworks/Spurious.framework/Headers/Spurious.h
-#include <Mod.h>
-//--- frameworks/Spurious.framework/Headers/Missing.h
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "FW.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+#include <FW/Sub.h>
+//--- frameworks/FW.framework/Headers/Sub.h
 
-//--- include/module.modulemap
+//--- module.modulemap
 module Mod { header "Mod.h" }
-//--- include/Mod.h
-typedef int mod_int;
-
+//--- Mod.h
+#include <FW/Sub.h>
 //--- tu.m
-#include <Spurious/Missing.h>
-#include <Mod.h>
-static mod_int x;
-
-// RUN: clang-scan-deps -compilation-database %t/cdb.json \
-// RUN:   -format experimental-full \
-// RUN:   -module-files-dir %t/outputs > %t/deps.json
-
-// RUN: %deps-to-rsp %t/deps.json --module-name=Mod      > %t/Mod.cc1.rsp
-// RUN: %deps-to-rsp %t/deps.json --module-name=Spurious > %t/Spurious.cc1.rsp
-// RUN: %deps-to-rsp %t/deps.json --tu-index=0           > %t/tu.rsp
-
-// RUN: %clang @%t/Mod.cc1.rsp
-// RUN: %clang @%t/Spurious.cc1.rsp
-// RUN: %clang @%t/tu.rsp
+#include "Mod.h"
+#include <FW/Sub.h>
 
 // RUN: clang-scan-deps -compilation-database %t/cdb.json \
 // RUN:   -format experimental-include-tree-full -cas-path %t/cas \
 // RUN:   -module-files-dir %t/cas-outputs > %t/cas-deps.json
 
-// RUN: %deps-to-rsp %t/cas-deps.json --module-name=Mod      > %t/cas-Mod.cc1.rsp
-// RUN: %deps-to-rsp %t/cas-deps.json --module-name=Spurious > %t/cas-Spurious.cc1.rsp
-// RUN: %deps-to-rsp %t/cas-deps.json --tu-index=0           > %t/cas-tu.rsp
+// RUN: %deps-to-rsp %t/cas-deps.json --module-name=FW  > %t/cas-FW.cc1.rsp
+// RUN: %deps-to-rsp %t/cas-deps.json --module-name=Mod > %t/cas-Mod.cc1.rsp
+// RUN: %deps-to-rsp %t/cas-deps.json --tu-index=0      > %t/cas-tu.rsp
 
 // RUN: cat %t/cas-tu.rsp | sed -E 's|.*"-fcas-include-tree" "(llvmcas://[[:xdigit:]]+)".*|\1|' > %t/tu.casid
 // RUN: clang-cas-test -cas %t/cas -print-include-tree @%t/tu.casid > %t/tu-include-tree.txt
 // RUN: FileCheck %s -input-file %t/tu-include-tree.txt -DPREFIX=%/t
 // CHECK:      [[PREFIX]]/tu.m llvmcas://
 // CHECK-NEXT: 1:1 <built-in> llvmcas://
-// CHECK-NEXT: 2:1 (Spurious import) (Module) Spurious.Missing [[PREFIX]]/frameworks/Spurious.framework/Headers/Missing.h llvmcas://
-// CHECK-NEXT: 3:1 (Module for visibility only) Mod
+// CHECK-NEXT: 2:1 (Module) Mod
+// CHECK-NEXT: 3:1 [[PREFIX]]/frameworks/FW.framework/Headers/Sub.h llvmcas://{{.*}}
+// CHECK-NEXT:   Submodule: FW
 
+// RUN: %clang @%t/cas-FW.cc1.rsp
 // RUN: %clang @%t/cas-Mod.cc1.rsp
-// RUN: %clang @%t/cas-Spurious.cc1.rsp
 // RUN: %clang @%t/cas-tu.rsp

clang/test/ClangScanDeps/modules-include-tree-implementation-via-spurious.c

@@ -0,0 +1,74 @@
+// REQUIRES: ondisk_cas
+
+// This test checks that we correctly handle situations where a spurious modular
+// dependency (1) turns otherwise textual dependency into modular (2).
+//
+// (1) For example #include <Spurious/Missing.h>, where the framework Spurious
+// has an umbrella header that does not include Missing.h, making it a textual
+// include instead.
+//
+// (2) For example when compiling the implementation file of a module Mod,
+// #included headers belonging to Mod are treated textually, unless some other
+// module already depends on Mod in its modular form.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+
+//--- cdb.json.in
+[{
+  "file": "DIR/tu.m",
+  "directory": "DIR",
+  "command": "clang -c DIR/tu.m -o DIR/tu.o -F DIR/frameworks -I DIR/include -fmodule-name=Mod -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+}]
+
+//--- frameworks/Spurious.framework/Modules/module.modulemap
+framework module Spurious {
+  umbrella header "Spurious.h"
+  module * { export * }
+}
+//--- frameworks/Spurious.framework/Headers/Spurious.h
+#include <Mod.h>
+//--- frameworks/Spurious.framework/Headers/Missing.h
+
+//--- include/module.modulemap
+module Mod { header "Mod.h" }
+//--- include/Mod.h
+typedef int mod_int;
+
+//--- tu.m
+#include <Spurious/Missing.h>
+#include <Mod.h>
+static mod_int x;
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full \
+// RUN:   -module-files-dir %t/outputs > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=Mod      > %t/Mod.cc1.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=Spurious > %t/Spurious.cc1.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index=0           > %t/tu.rsp
+
+// RUN: %clang @%t/Mod.cc1.rsp
+// RUN: %clang @%t/Spurious.cc1.rsp
+// RUN: %clang @%t/tu.rsp
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-include-tree-full -cas-path %t/cas \
+// RUN:   -module-files-dir %t/cas-outputs > %t/cas-deps.json
+
+// RUN: %deps-to-rsp %t/cas-deps.json --module-name=Mod      > %t/cas-Mod.cc1.rsp
+// RUN: %deps-to-rsp %t/cas-deps.json --module-name=Spurious > %t/cas-Spurious.cc1.rsp
+// RUN: %deps-to-rsp %t/cas-deps.json --tu-index=0           > %t/cas-tu.rsp
+
+// RUN: cat %t/cas-tu.rsp | sed -E 's|.*"-fcas-include-tree" "(llvmcas://[[:xdigit:]]+)".*|\1|' > %t/tu.casid
+// RUN: clang-cas-test -cas %t/cas -print-include-tree @%t/tu.casid > %t/tu-include-tree.txt
+// RUN: FileCheck %s -input-file %t/tu-include-tree.txt -DPREFIX=%/t
+// CHECK:      [[PREFIX]]/tu.m llvmcas://
+// CHECK-NEXT: 1:1 <built-in> llvmcas://
+// CHECK-NEXT: 2:1 (Spurious import) (Module) Spurious.Missing [[PREFIX]]/frameworks/Spurious.framework/Headers/Missing.h llvmcas://
+// CHECK-NEXT: 3:1 (Module for visibility only) Mod
+
+// RUN: %clang @%t/cas-Mod.cc1.rsp
+// RUN: %clang @%t/cas-Spurious.cc1.rsp
+// RUN: %clang @%t/cas-tu.rsp