[clang][modules] Consider M affecting after mapping M.Private to M_Private

[jansvoboda11]

Summary

When Clang encounters @import M.Private during implicit build, it precompiles module M and looks through its submodules. If the Private submodule is not found, Clang assumes @import M_Private. In the dependency scanner, we don't capture the dependency on M, since it's not imported. It's an affecting module, though: compilation of the import statement will fail when implicit modules are disabled and M is not precompiled and explicitly provided. This patch fixes that.

Depends on D132430.

[benlangmuir]

clang/lib/Frontend/CompilerInstance.cpp

2016

Not sure if this is a good idea, but have you considered having the scanner provide the module with Foo.Private= syntax instead? It would be nice to keep this hack contained to implicit builds if we can.

[jansvoboda11]

clang/lib/Frontend/CompilerInstance.cpp

2016

Interesting idea, that didn't occur to me. What implementation strategy are you thinking of?

If we want to avoid this hack, it seems like we'd need to pass ModuleIdPath ImportPath instead of StringRef ModuleName to findOrCompileModuleAndReadAST() and friends. That would allow us to look up the most specific -fmodule-file= argument. I'm not convinced changing multiple functions (and then changing them back when we don't need the hack) is better than extending the hack to explicit modules.

The upside is that we wouldn't need to report M as a dependency. WDYT?

[jansvoboda11]

clang/lib/Frontend/CompilerInstance.cpp

2016

Another thing to consider: implicit build would need to propagate the information that module Foo_Private needs to be spelled on the command-line as Foo.Private to the scanner.

[benlangmuir]

LGTM once the windows test failure is fixed.

clang/lib/Frontend/CompilerInstance.cpp

2016

Okay, it sounds like the current patch is less invasive than the alternative. Thanks for talking it through.

clang/include/clang/Lex/Preprocessor.h

public:
   macros(bool IncludeExternalMacros = true) const {
     macro_iterator begin = macro_begin(IncludeExternalMacros);
     macro_iterator end = macro_end(IncludeExternalMacros);
     return llvm::make_range(begin, end);
   }
 
   /// \}
 
+  /// Mark the given module as affecting the current module or translation unit.
+  void markModuleAsAffecting(Module *M) {
+    if (!BuildingSubmoduleStack.empty()) {
+      if (M != BuildingSubmoduleStack.back().M)
+        BuildingSubmoduleStack.back().M->AffectingModules.insert(M);
+    } else {
+      AffectingModules.insert(M);
+    }
+  }
+
   /// Get the set of top-level modules that affected preprocessing, but were not
   /// imported.
   const llvm::SmallSetVector<Module *, 2> &getAffectingModules() const {
     return AffectingModules;
   }
 
   /// Mark the file as included.
   /// Returns true if this is the first time the file was included.

clang/lib/Frontend/CompilerInstance.cpp

CompilerInstance::loadModule(SourceLocation ImportLoc,
   for (unsigned I = 1, N = Path.size(); I != N; ++I) {
     StringRef Name = Path[I].first->getName();
     clang::Module *Sub = Module->findSubmodule(Name);
 
     // If the user is requesting Foo.Private and it doesn't exist, try to
     // match Foo_Private and emit a warning asking for the user to write
     // @import Foo_Private instead. FIXME: remove this when existing clients
     // migrate off of Foo.Private syntax.
-    if (!Sub && PP->getLangOpts().ImplicitModules && Name == "Private" &&
-        Module == Module->getTopLevelModule()) {
+    if (!Sub && Name == "Private" && Module == Module->getTopLevelModule()) {
       SmallString<128> PrivateModule(Module->Name);
       PrivateModule.append("_Private");
 
       SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> PrivPath;
       auto &II = PP->getIdentifierTable().get(
           PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
       PrivPath.push_back(std::make_pair(&II, Path[0].second));
 
       if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, true,
                                                  !IsInclusionDirective))
         Sub = loadModule(ImportLoc, PrivPath, Visibility, IsInclusionDirective);
       if (Sub) {
         MapPrivateSubModToTopLevel = true;
+        PP->markModuleAsAffecting(Module);
         if (!getDiagnostics().isIgnored(
                 diag::warn_no_priv_submodule_use_toplevel, ImportLoc)) {
           getDiagnostics().Report(Path[I].second,
                                   diag::warn_no_priv_submodule_use_toplevel)
               << Path[I].first << Module->getFullModuleName() << PrivateModule
               << SourceRange(Path[0].second, Path[I].second)
               << FixItHint::CreateReplacement(SourceRange(Path[0].second),
                                               PrivateModule);

clang/lib/Lex/PPDirectives.cpp

ModuleLoadResult Imported = TheModuleLoader.loadModule(
         IncludeTok.getLocation(), Path, Module::Hidden,
         /*IsInclusionDirective=*/true);
     assert((Imported == nullptr || Imported == SuggestedModule.getModule()) &&
            "the imported module is different than the suggested one");
 
     if (Imported) {
       Action = Import;
     } else if (Imported.isMissingExpected()) {
-      Module *M = static_cast<Module *>(Imported)->getTopLevelModule();
-      if (!BuildingSubmoduleStack.empty()) {
-        if (Imported != BuildingSubmoduleStack.back().M)
-          BuildingSubmoduleStack.back().M->AffectingModules.insert(M);
-      } else {
-        AffectingModules.insert(M);
-      }
+      markModuleAsAffecting(
+          static_cast<Module *>(Imported)->getTopLevelModule());
       // We failed to find a submodule that we assumed would exist (because it
       // was in the directory of an umbrella header, for instance), but no
       // actual module containing it exists (because the umbrella header is
       // incomplete).  Treat this as a textual inclusion.
       SuggestedModule = ModuleMap::KnownHeader();
       SM = nullptr;
     } else if (Imported.isConfigMismatch()) {
       // On a configuration mismatch, enter the header textually. We still know

clang/test/ClangScanDeps/modules-implicit-dot-private.m

+// This test checks that modules loaded during compilation (but not imported)
+// are still reported as dependencies.
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: split-file %s %t
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "FW.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private { umbrella header "FW_Private.h" }
+//--- frameworks/FW.framework/PrivateHeaders/FW_Private.h
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.m",
+  "directory": "DIR",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -iframework DIR/frameworks -c DIR/tu.m -o DIR/tu.o"
+}]
+//--- tu.m
+@import FW.Private;
+
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+// RUN: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+// CHECK:      {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-module-deps": [],
+// CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
+// CHECK-NEXT:       "command-line": [
+// CHECK:            ],
+// CHECK-NEXT:       "context-hash": "{{.*}}",
+// CHECK-NEXT:       "file-deps": [
+// CHECK-NEXT:         "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h",
+// CHECK-NEXT:         "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
+// CHECK-NEXT:         "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap"
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "name": "FW"
+// CHECK-NEXT:     },
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-module-deps": [],
+// CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT:       "command-line": [
+// CHECK:            ],
+// CHECK-NEXT:       "context-hash": "{{.*}}",
+// CHECK-NEXT:       "file-deps": [
+// CHECK-NEXT:         "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
+// CHECK-NEXT:         "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT:         "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h"
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "name": "FW_Private"
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-context-hash": "{{.*}}",
+// CHECK-NEXT:       "clang-module-deps": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "context-hash": "{{.*}}",
+// CHECK-NEXT:           "module-name": "FW"
+// CHECK-NEXT:         },
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "context-hash": "{{.*}}",
+// CHECK-NEXT:           "module-name": "FW_Private"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "command-line": [
+// CHECK:              "-fmodule-file={{.*}}/FW_Private-{{.*}}.pcm"
+// CHECK:              "-fmodule-file={{.*}}/FW-{{.*}}.pcm"
+// CHECK:            ],
+// CHECK-NEXT:       "file-deps": [
+// CHECK-NEXT:         "[[PREFIX]]/tu.m"
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "input-file": "[[PREFIX]]/tu.m"
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ]
+// CHECK-NEXT: }
+
+// RUN: %deps-to-rsp %t/result.json --module-name=FW > %t/FW.cc1.rsp
+// RUN: %deps-to-rsp %t/result.json --module-name=FW_Private > %t/FW_Private.cc1.rsp
+// RUN: %deps-to-rsp %t/result.json --tu-index=0 > %t/tu.rsp
+// RUN: %clang @%t/FW.cc1.rsp
+// RUN: %clang @%t/FW_Private.cc1.rsp
+// RUN: %clang @%t/tu.rsp

clang/test/Modules/implicit-map-dot-private.m

 // RUN: rm -rf %t
-// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-canonical -emit-pch -o %t-A.pch %s -Wprivate-module
+
+// Implicit modules.
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN:   -F %S/Inputs/implicit-private-canonical -fsyntax-only %s -Wprivate-module
+
+// Explicit modules.
+// RUN: %clang_cc1 -x objective-c -fmodules -emit-module -fmodule-name=A -o %t/A.pcm \
+// RUN:   %S/Inputs/implicit-private-canonical/A.framework/Modules/module.modulemap -Wprivate-module
+// RUN: %clang_cc1 -x objective-c -fmodules -emit-module -fmodule-name=A_Private -o %t/A_Private.pcm \
+// RUN:   %S/Inputs/implicit-private-canonical/A.framework/Modules/module.private.modulemap -Wprivate-module
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fno-implicit-modules \
+// RUN:   -fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm \
+// RUN:   -F %S/Inputs/implicit-private-canonical -fsyntax-only %s -Wprivate-module
 
 #ifndef HEADER
 #define HEADER
 
 @import A.Private; // expected-warning {{no submodule named 'Private' in module 'A'; using top level 'A_Private'}}
 // expected-note@Inputs/implicit-private-canonical/A.framework/Modules/module.private.modulemap:1{{module defined here}}
 
 const int *y = &APRIVATE;
 
 #endif