[C++20] [Modules] Mark imported module as imported if not exported

[ChuanqiXu]

Summary

The intention of this patch is described in the title. I met the problem in the original test. The original behavior of the test is clearly incorrect.

Then I found this patch could solve the problem. The module Templ imported in bar is not recorded as Imported. And the original behavior looks odd. It only maintains the Exports field but not Imports field. So I am not sure if the behavior was intended or it is just an oversight. I did simple test but I am not sure if this change is good.

[ChuanqiXu]

Rebased

[urnathan]

as all exports are also imports, is there a reason we don;t also add the exports into the import table? is Exports really 'ExportedImports' and 'Imports' really 'NotExportedImports'? If that's the case, this patch is fine.

clang/lib/Sema/SemaModule.cpp

385–386

This comment will need adjusting.

[ChuanqiXu]

In D116098#3207812, @urnathan wrote:

as all exports are also imports, is there a reason we don;t also add the exports into the import table?

I am not sure if it has an intentional reason or it is just an oversight. From my practice, the Imports field in C++20 modules is empty all the time.

is Exports really 'ExportedImports' and 'Imports' really 'NotExportedImports'? If that's the case, this patch is fine.

Yes. From the codes, only export import X would add X to the exported list. And if we write import X, the Imports field is empty too.

[ChuanqiXu]

Address comments.

[urnathan]

lgtm!

clang/lib/Sema/SemaModule.cpp

DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
   ImportDecl *Import = ImportDecl::Create(Context, CurContext, StartLoc,
                                           Mod, IdentifierLocs);
   CurContext->addDecl(Import);
 
   // Sequence initialization of the imported module before that of the current
   // module, if any.
   if (!ModuleScopes.empty())
     Context.addModuleInitializer(ModuleScopes.back().Module, Import);
 
-  // Re-export the module if needed.
   if (!ModuleScopes.empty() && ModuleScopes.back().ModuleInterface) {
+    // Re-export the module if the imported module is exported.
+    // Note that we don't need to add re-exported module to Imports field
+    // since `Exports` implies the module is imported already.
     if (ExportLoc.isValid() || getEnclosingExportDecl(Import))
       getCurrentModule()->Exports.emplace_back(Mod, false);
+    else
+      getCurrentModule()->Imports.insert(Mod);
   } else if (ExportLoc.isValid()) {
+    // [module.interface]p1:
+    // An export-declaration shall inhabit a namespace scope and appear in the
+    // purview of a module interface unit.
     Diag(ExportLoc, diag::err_export_not_in_module_interface);
   }
 
   return Import;
 }
 
 void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
   checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);

clang/test/Modules/module-transtive-instantiation.cpp

 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: %clang_cc1 -std=c++20 %S/Inputs/module-transtive-instantiation/Templ.cppm -emit-module-interface -o %t/Templ.pcm
 // RUN: %clang_cc1 -std=c++20 %S/Inputs/module-transtive-instantiation/bar.cppm  -emit-module-interface -fprebuilt-module-path=%t -o %t/bar.pcm
 // RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %s -fsyntax-only -verify
+// expected-no-diagnostics
 
 import bar;
 int foo() {
-    // FIXME: It shouldn't be an error. Since the `G` is already imported in bar.
-    return bar<int>(); // expected-error@Inputs/module-transtive-instantiation/bar.cppm:5 {{definition of 'G' must be imported from module 'Templ' before it is required}}
-                       // expected-note@-1 {{in instantiation of function template specialization 'bar<int>' requested here}}
-                       // expected-note@Inputs/module-transtive-instantiation/Templ.cppm:3 {{definition here is not reachable}}
+  return bar<int>();
 }