[clang][include-tree] Fix spurious dependencies

This patch fixes an incorrect assumption that only spurious imports cause both `IncludeTreeBuilder::moduleImport()` and `IncludeTreeBuilder::exitedInclude()` to be called for the same directive. This can also happen for `#include` of a header that belongs to the module being implemented that previously got transitively loaded from a PCM.

Instead of trying to guess that a submodule was a spurious import (by checking that it was not imported, is not from module file but its parent is from module file), we just mark it missing from umbrella header when we detect this. This allows us to stop putting the temporary `ModuleImport` onto the stack in one function and later popping and replacing it by `SpuriousImport` in another function.

rdar://123253616

Cherry-pick of /pull/8264

Author: jansvoboda11

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,25 @@ 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 (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