[clang][modules] Disallow importing private framework in the implementation

[jansvoboda11]

Summary

Whenever we are compiling implementation of a framework (with the -fmodule-name=FW option), we never translate #import <FW/Header.h> to an import, regardless of whether "Header.h" belongs to "FW" or "FW_Private". For the same reasons, we also disallow @import FW. However, we still allow @import FW_Private. This patch disallows that a well, to be consistent with the rest of the rules.

[jansvoboda11]

clang/lib/Sema/SemaModule.cpp

544

This is the functional change.

clang/include/clang/Basic/Module.h

public:
   /// requirement, this parameter will be set to one of the requirements that
   /// is not met for use of this module.
   ///
   /// \param ShadowingModule If this module is unimportable because it is
   /// shadowed, this parameter will be set to the shadowing module.
   bool isUnimportable(const LangOptions &LangOpts, const TargetInfo &Target,
                       Requirement &Req, Module *&ShadowingModule) const;
 
+  /// Determine whether this module can be built in this compilation.
+  bool isForBuilding(const LangOptions &LangOpts) const;
+
   /// Determine whether this module is available for use within the
   /// current translation unit.
   bool isAvailable() const { return IsAvailable; }
 
   /// Determine whether this module is available for use within the
   /// current translation unit.
   ///
   /// \param LangOpts The language options used for the current

clang/lib/Basic/Module.cpp

for (unsigned I = 0, N = Current->Requirements.size(); I != N; ++I) {
         return true;
       }
     }
   }
 
   llvm_unreachable("could not find a reason why module is unimportable");
 }
 
+// The -fmodule-name option tells the compiler to textually include headers in
+// the specified module, meaning Clang won't build the specified module. This
+// is useful in a number of situations, for instance, when building a library
+// that vends a module map, one might want to avoid hitting intermediate build
+// products containing the module map or avoid finding the system installed
+// modulemap for that library.
+bool Module::isForBuilding(const LangOptions &LangOpts) const {
+  StringRef TopLevelName = getTopLevelModuleName();
+  StringRef CurrentModule = LangOpts.CurrentModule;
+
+  // When building framework Foo, we want to make sure that Foo *and*
+  // Foo_Private are textually included and no modules are built for both.
+  if (getTopLevelModule()->IsFramework &&
+      CurrentModule == LangOpts.ModuleName &&
+      !CurrentModule.endswith("_Private") && TopLevelName.endswith("_Private"))
+    TopLevelName = TopLevelName.drop_back(8);
+
+  return TopLevelName == CurrentModule;
+}
+
 bool Module::isAvailable(const LangOptions &LangOpts, const TargetInfo &Target,
                          Requirement &Req,
                          UnresolvedHeaderDirective &MissingHeader,
                          Module *&ShadowingModule) const {
   if (IsAvailable)
     return true;
 
   if (isUnimportable(LangOpts, Target, Req, ShadowingModule))

clang/lib/Lex/PPDirectives.cpp

 /// Enumerates possible %select values for the pp_err_elif_after_else and
 /// pp_err_elif_without_if diagnostics.
 enum PPElifDiag {
   PED_Elif,
   PED_Elifdef,
   PED_Elifndef
 };
 
-// The -fmodule-name option tells the compiler to textually include headers in
-// the specified module, meaning clang won't build the specified module. This is
-// useful in a number of situations, for instance, when building a library that
-// vends a module map, one might want to avoid hitting intermediate build
-// products containimg the module map or avoid finding the system installed
-// modulemap for that library.
-static bool isForModuleBuilding(Module *M, StringRef CurrentModule,
-                                StringRef ModuleName) {
-  StringRef TopLevelName = M->getTopLevelModuleName();
-
-  // When building framework Foo, we wanna make sure that Foo *and* Foo_Private
-  // are textually included and no modules are built for both.
-  if (M->getTopLevelModule()->IsFramework && CurrentModule == ModuleName &&
-      !CurrentModule.endswith("_Private") && TopLevelName.endswith("_Private"))
-    TopLevelName = TopLevelName.drop_back(8);
-
-  return TopLevelName == CurrentModule;
-}
-
 static MacroDiag shouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {
   const LangOptions &Lang = PP.getLangOpts();
   if (isReservedInAllContexts(II->isReserved(Lang))) {
     // list from:
     // - https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html
     // - https://docs.microsoft.com/en-us/cpp/c-runtime-library/security-features-in-the-crt?view=msvc-160
     // - man 7 feature_test_macros
     // The list must be sorted for correct binary search.
Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
 
   // If we've reached the max allowed include depth, it is usually due to an
   // include cycle. Don't enter already processed files again as it can lead to
   // reaching the max allowed include depth again.
   if (Action == Enter && HasReachedMaxIncludeDepth && File &&
       alreadyIncluded(*File))
     Action = IncludeLimitReached;
 
-  bool MaybeTranslateInclude = Action == Enter && File && SuggestedModule &&
-                               !isForModuleBuilding(SuggestedModule.getModule(),
-                                                    getLangOpts().CurrentModule,
-                                                    getLangOpts().ModuleName);
-
   // FIXME: We do not have a good way to disambiguate C++ clang modules from
   // C++ standard modules (other than use/non-use of Header Units).
   Module *SM = SuggestedModule.getModule();
+
+  bool MaybeTranslateInclude =
+      Action == Enter && File && SM && !SM->isForBuilding(getLangOpts());
+
   // Maybe a usable Header Unit
   bool UsableHeaderUnit = false;
   if (getLangOpts().CPlusPlusModules && SM && SM->isHeaderUnit()) {
     if (TrackGMFState.inGMF() || IsImportDecl)
       UsableHeaderUnit = true;
     else if (!IsImportDecl) {
       // This is a Header Unit that we do not include-translate
       SuggestedModule = ModuleMap::KnownHeader();
if (SM && !SM->isHeaderUnit()) {
     // When building a pch, -fmodule-name tells the compiler to textually
     // include headers in the specified module. We are not building the
     // specified module.
     //
     // FIXME: This is the wrong way to handle this. We should produce a PCH
     // that behaves the same as the header would behave in a compilation using
     // that PCH, which means we should enter the submodule. We need to teach
     // the AST serialization layer to deal with the resulting AST.
-    if (getLangOpts().CompilingPCH &&
-        isForModuleBuilding(SM, getLangOpts().CurrentModule,
-                            getLangOpts().ModuleName))
+    if (getLangOpts().CompilingPCH && SM->isForBuilding(getLangOpts()))
       return {ImportAction::None};
 
     assert(!CurLexerSubmodule && "should not have marked this as a module yet");
     CurLexerSubmodule = SM;
 
     // Let the macro handling code know that any future macros are within
     // the new submodule.
     EnterSubmodule(SM, EndLoc, /*ForPragma*/ false);

clang/lib/Sema/SemaModule.cpp

DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
   VisibleModules.setVisible(Mod, ImportLoc);
 
   checkModuleImportContext(*this, Mod, ImportLoc, CurContext);
 
   // FIXME: we should support importing a submodule within a different submodule
   // of the same top-level module. Until we do, make it an error rather than
   // silently ignoring the import.
   // FIXME: Should we warn on a redundant import of the current module?
-  if (Mod->getTopLevelModuleName() == getLangOpts().CurrentModule &&
+  if (Mod->isForBuilding(getLangOpts()) &&
       (getLangOpts().isCompilingModule() || !getLangOpts().ModulesTS)) {
     Diag(ImportLoc, getLangOpts().isCompilingModule()
                         ? diag::err_module_self_import
                         : diag::err_module_import_in_implementation)
         << Mod->getFullModuleName() << getLangOpts().CurrentModule;
   }
 
   SmallVector<SourceLocation, 2> IdentifierLocs;

clang/test/Modules/implementation-of-module-private.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW {}
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private {}
+
+//--- tu.m
+@import FW_Private; // expected-error{{@import of module 'FW_Private' in implementation of 'FW'; use #import}}
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps \
+// RUN:   -fmodule-name=FW -F %t/frameworks %t/tu.m -verify