[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

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

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) {
     if (ExportLoc.isValid() || getEnclosingExportDecl(Import))
       getCurrentModule()->Exports.emplace_back(Mod, false);
+    else
+      getCurrentModule()->Imports.insert(Mod);
   } else if (ExportLoc.isValid()) {
     Diag(ExportLoc, diag::err_export_not_in_module_interface);
   }
 
   return Import;
 }
 
 void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {

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

 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: %clang -std=c++20 %S/Inputs/module-transtive-instantiation/Templ.cppm --precompile -o %t/Templ.pcm
 // RUN: %clang -std=c++20 %S/Inputs/module-transtive-instantiation/bar.cppm  --precompile -fprebuilt-module-path=%t -o %t/bar.pcm
 // RUN: %clang -std=c++20 -fprebuilt-module-path=%t %s -c -Xclang -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>();
 }