[Modules TS] Make imports from an interface unit visible to its implementation units

[hamzasood]

Summary

This provides an implementation for the changes outlined in section 4.1 of P0731r0, which clarifies the intended behaviour regarding implementation units being able to see imports made within their corresponding interface unit.

[boris]

LGTM. Maybe it makes sense to also test that an unrelated translation unit that imports module 'test' sees neither 'a' nor 'b'.

[hamzasood]

Good idea, I've added that to the test.
I'll give Richard some time to look over this before committing.

[hamzasood]

Ping

[hamzasood]

I've been investigating an issue regarding visibility during code synthesis, and I noticed that this patch half fixes it. I've added the rest of the fix since it's related to what's going on here, and I guess this is now a "correctly handle imports from an interface unit" patch.

I believe this fixes this bug posted on the LLVM bug tracker.

@boris if this is still okay with you after seeing the changes I've made, would you mind accepting the revision so I can commit this without Phabricator shouting at me?

[boris]

I don't think it will be wise for me to accept this revision since I can't claim to have good understanding of Clang's internal modules model. I think it will be wise to have Richard take a look.

lib/Basic/Module.cpp

349

This comment (not sure about the code) sounds wrong to me: names from the interface unit before the module purview (I assume this is what "global module fragment" refers to) should not be visible in the implementation units.

[hamzasood]

lib/Basic/Module.cpp

349

Yes, the documentation comment for this function (in the header) notes that things visible to the interface unit aren’t necessarily visible to the implementation units.

There’re a few reasons why I chose for the function to count modules that’re only visible from the interface unit:

1. The Module::Kind for a C++ module is ModuleInterfaceUnit. Since the module identifies itself as an interface unit, I figured it’d make sense for queries such as this to return what’s relevant for the interface unit.
2. I think it’s just more useful that way. I can’t think of any situation where a module would need to know what’s visible to implementation units of other modules (and I couldn’t find any such examples in Clang). However there’re plenty of reasons why one would want to know about visibility to an interface unit (synthesising default members, instantiating templates etc.)

[rsmith]

lib/Sema/SemaDecl.cpp

16135

For completeness, you should also call getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, Loc); here. (It turns out to not actually matter right now because ModulesTS implies LocalSubmoduleVisibility, under which makeModuleVisible happens to be a no-op.)

16185–16194

This is not appropriate; generally whether we serialize to an AST file should be treated as orthogonal to whether we're in / importing a module.

The right check here is probably getLangOpts().getCompilingModule() == CMK_ModuleInterface.

[hamzasood]

Addressed Richard's comments.

[hamzasood]

lib/Sema/SemaDecl.cpp

16185–16194

Thanks! I completely missed that lang opt.

include/clang/Basic/Module.h

public:
   /// \brief Find the submodule with the given name.
   ///
   /// \returns The submodule if found, or NULL otherwise.
   Module *findSubmodule(StringRef Name) const;
 
   /// \brief Determine whether the specified module would be visible to
   /// a lookup at the end of this module.
   ///
+  /// Note that if this is a C++ module interface unit, then modules visible
+  /// here aren't necessarily visible from the implementation units.
   /// FIXME: This may return incorrect results for (submodules of) the
   /// module currently being built, if it's queried before we see all
   /// of its imports.
   bool isModuleVisible(const Module *M) const {
     if (VisibleModulesCache.empty())
       buildVisibleModulesCache();
     return VisibleModulesCache.count(M);
   }

lib/Basic/Module.cpp

 }
 
 void Module::buildVisibleModulesCache() const {
   assert(VisibleModulesCache.empty() && "cache does not need building");
 
   // This module is visible to itself.
   VisibleModulesCache.insert(this);
 
+  // Everything visible to the interface unit's global module fragment is
+  // visible to the interface unit.
+  if (Kind == ModuleKind::ModuleInterfaceUnit) {
+    assert(!SubModules.empty() &&
+           SubModules.front()->Kind == ModuleKind::GlobalModuleFragment);
+    const Module *Global = SubModules.front();
+    if (Global->VisibleModulesCache.empty())
+      Global->buildVisibleModulesCache();
+    VisibleModulesCache.insert(Global->VisibleModulesCache.begin(),
+                               Global->VisibleModulesCache.end());
+  }
+
   // Every imported module is visible.
   SmallVector<Module *, 16> Stack(Imports.begin(), Imports.end());
   while (!Stack.empty()) {
     Module *CurrModule = Stack.pop_back_val();
 
     // Every module transitively exported by an imported module is visible.
     if (VisibleModulesCache.insert(CurrModule).second)
       CurrModule->getExportedModules(Stack);

lib/Sema/SemaDecl.cpp

Sema::DeclGroupPtrTy Sema::ActOnModuleDecl(SourceLocation StartLoc,
 
   // From now on, we have an owning module for all declarations we see.
   // However, those declarations are module-private unless explicitly
   // exported.
   auto *TU = Context.getTranslationUnitDecl();
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   TU->setLocalOwningModule(Mod);
 
+  // Modules TS + p0731r0 [dcl.module.interface]p1:
+  //   Every name of an entity with linkage other than internal linkage made
+  //   visible in the purview of the module interface unit of a module M is
+  //   visible in the purview of all module implementation units of M.
+  if (MDK == ModuleDeclKind::Implementation) {
+    for (Module *ImportedModule : Mod->Imports)
+      VisibleModules.setVisible(ImportedModule, ModuleLoc);
+  }
+
   // FIXME: Create a ModuleDecl.
   return nullptr;
 }
 
 DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
                                    SourceLocation ImportLoc,
                                    ModuleIdPath Path) {
   Module *Mod =
DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
   }
 
   ImportDecl *Import = ImportDecl::Create(Context, CurContext, StartLoc,
                                           Mod, IdentifierLocs);
   if (!ModuleScopes.empty())
     Context.addModuleInitializer(ModuleScopes.back().Module, Import);
   CurContext->addDecl(Import);
 
+  if (getLangOpts().ModulesTS) {
+    Module *CurrentModule = getCurrentModule();
+    assert(CurrentModule && "Expected to be in a module scope");
+
+    // If the current module has been loaded from disk, then this is an
+    // implementation unit and hence we shouldn't modify the module.
+    // FIXME: Is that a hacky assumption? We can't just check
+    // ModuleScope::ModuleInterface because we also need to know about imports
+    // that occur in the global module fragment of an interface unit.
+    if (!CurrentModule->getASTFile()) {
+      // Record imports that occur in a module interface unit; other units may
+      // need to know what's visible in the interface unit (e.g. for code
+      // synthesis purposes).
+      CurrentModule->Imports.insert(Mod);
+
       // Re-export the module if needed.
-  if (Import->isExported() &&
-      !ModuleScopes.empty() && ModuleScopes.back().ModuleInterface)
-    getCurrentModule()->Exports.emplace_back(Mod, false);
+      if (Import->isExported())
+        CurrentModule->Exports.emplace_back(Mod, false);
+    }
+  }
 
   return Import;
 }
 
 void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
   checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);
   BuildModuleInclude(DirectiveLoc, Mod);
 }

test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp

 // RUN: rm -rf %t
 // RUN: mkdir %t
 //
 // Some of the following tests intentionally have no -verify in their RUN
 // lines; we are testing that those cases do not produce errors.
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %S/global-vs-module.cpp -emit-module-interface -o %t/M.pcm -DNO_GLOBAL -DEXPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -DMODULE_INTERFACE -verify
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -DMODULE_INTERFACE -DNO_IMPORT
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N-no-M.pcm -DMODULE_INTERFACE -DNO_ERRORS -DNO_IMPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -verify
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -DNO_IMPORT
 
 #ifdef MODULE_INTERFACE
 export
 #endif

test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp

-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif

test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DTEST_INTERFACE
+//
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DTEST_IMPLEMENTATION
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DOTHER_TU
+
+
+#ifdef TEST_INTERFACE
+import a;
+export module test;
+import b;
+#endif
+
+#ifdef TEST_IMPLEMENTATION
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+     // [e-mail]:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+#endif
+
+
+#ifdef OTHER_TU
+import test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+     // [e-mail]:1 {{here}}
+
+B b; // expected-error {{must be imported from module 'b'}}
+     // [e-mail]:1 {{here}}
+#endif