[C++20][Modules] Fix named module import diagnostics.

[iains]

Summary

We have been incorrectly disallowing imports of named modules in the
global and private module fragments.

This addresses: https://github.com/llvm/llvm-project/issues/59688

[ChuanqiXu]

[module.import]p1 says:

In a module unit, all module-import-declarations and export-declarations exporting module-import-declarations shall appear before all other declarations in the declaration-seq of the translation-unit and of the private-module-fragment (if any).

So the following case is invalid:

//--- b.hpp
void func() {}
import a;

//--- c.cpp
module;
#include "b.hpp"
export module c;

I feel we'd better to address such cases in the test.

[iains]

In D140927#4024648, @ChuanqiXu wrote:

[module.import]p1 says:

In a module unit, all module-import-declarations and export-declarations exporting module-import-declarations shall appear before all other declarations in the declaration-seq of the translation-unit and of the private-module-fragment (if any).

So the following case is invalid:

//--- b.hpp
void func() {}
import a;

//--- c.cpp
module;
#include "b.hpp"
export module c;

I feel we'd better to address such cases in the test.

I am not sure about this - what you are suggesting above would seem to make modules useless when there is a GMF (since there is almost no point to having a GMF that has no decls).

https://eel.is/c++draft/basic.link#1

Make the declaration-seq of a module unit specifically follow the module keyword.

https://eel.is/c++draft/module#import-1 (which you quoted from above) adds to this the PMF - but I do not see that it includes the GMF (since that is neither the declaration-seq of the module unit nor is it the PMF.

(this + 15.5 named Header Unit import is why the original bug is there .. )

[ChuanqiXu]

From the discussion in https://github.com/llvm/llvm-project/issues/59688, I feel you are correct. But let's wait for we get a clarification.

[ChuanqiXu]

LGTM. Thanks.

[iains]

rebased

clang/include/clang/Sema/Sema.h

enum class ModuleDeclKind {
     PartitionInterface,      ///< 'export module X:Y;'
     PartitionImplementation, ///< 'module X:Y;'
   };
 
   /// An enumeration to represent the transition of states in parsing module
   /// fragments and imports.  If we are not parsing a C++20 TU, or we find
   /// an error in state transition, the state is set to NotACXX20Module.
   enum class ModuleImportState {
-    FirstDecl,       ///< Parsing the first decl in a TU.
-    GlobalFragment,  ///< after 'module;' but before 'module X;'
-    ImportAllowed,   ///< after 'module X;' but before any non-import decl.
-    ImportFinished,  ///< after any non-import decl.
-    PrivateFragment, ///< after 'module :private;'.
-    NotACXX20Module  ///< Not a C++20 TU, or an invalid state was found.
+    FirstDecl,      ///< Parsing the first decl in a TU.
+    GlobalFragment, ///< after 'module;' but before 'module X;'
+    ImportAllowed,  ///< after 'module X;' but before any non-import decl.
+    ImportFinished, ///< after any non-import decl.
+    PrivateFragmentImportAllowed,  ///< after 'module :private;' but before any
+                                   ///< non-import decl.
+    PrivateFragmentImportFinished, ///< after 'module :private;' but a
+                                   ///< non-import decl has already been seen.
+    NotACXX20Module ///< Not a C++20 TU, or an invalid state was found.
   };
 
 private:
   /// The parser has begun a translation unit to be compiled as a C++20
   /// Header Unit, helper for ActOnStartOfTranslationUnit() only.
   void HandleStartOfHeaderUnit();
 
 public:

clang/lib/Parse/Parser.cpp

bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
   // it.
   if (Result) {
     if (ImportState == Sema::ModuleImportState::FirstDecl)
       // First decl was not modular.
       ImportState = Sema::ModuleImportState::NotACXX20Module;
     else if (ImportState == Sema::ModuleImportState::ImportAllowed)
       // Non-imports disallow further imports.
       ImportState = Sema::ModuleImportState::ImportFinished;
+    else if (ImportState ==
+             Sema::ModuleImportState::PrivateFragmentImportAllowed)
+      // Non-imports disallow further imports.
+      ImportState = Sema::ModuleImportState::PrivateFragmentImportFinished;
   }
   return false;
 }
 
 /// ParseExternalDeclaration:
 ///
 /// The `Attrs` that are passed in are C++11 attributes and appertain to the
 /// declaration.
if (getLangOpts().CPlusPlusModules && Tok.is(tok::colon) &&
     if (MDK == Sema::ModuleDeclKind::Interface) {
       Diag(StartLoc, diag::err_module_fragment_exported)
         << /*private*/1 << FixItHint::CreateRemoval(StartLoc);
     }
     ConsumeToken();
     SourceLocation PrivateLoc = ConsumeToken();
     DiagnoseAndSkipCXX11Attributes();
     ExpectAndConsumeSemi(diag::err_private_module_fragment_expected_semi);
-    ImportState = Sema::ModuleImportState::PrivateFragment;
+    ImportState = ImportState == Sema::ModuleImportState::ImportAllowed
+                      ? Sema::ModuleImportState::PrivateFragmentImportAllowed
+                      : Sema::ModuleImportState::PrivateFragmentImportFinished;
     return Actions.ActOnPrivateModuleFragmentDecl(ModuleLoc, PrivateLoc);
   }
 
   SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> Path;
   if (ParseModuleName(ModuleLoc, Path, /*IsImport*/ false))
     return nullptr;
 
   // Parse the optional module-partition.
Decl *Parser::ParseModuleImport(SourceLocation AtLoc,
   case Sema::ModuleImportState::NotACXX20Module:
     // We can only import a partition within a module purview.
     if (IsPartition)
       Diag(ImportLoc, diag::err_partition_import_outside_module);
     else
       SeenError = false;
     break;
   case Sema::ModuleImportState::GlobalFragment:
-    // We can only have pre-processor directives in the global module
-    // fragment.  We cannot import a named modules here, however we have a
-    // header unit import.
-    if (!HeaderUnit || HeaderUnit->Kind != Module::ModuleKind::ModuleHeaderUnit)
-      Diag(ImportLoc, diag::err_import_in_wrong_fragment) << IsPartition << 0;
+  case Sema::ModuleImportState::PrivateFragmentImportAllowed:
+    // We can only have pre-processor directives in the global module fragment
+    // which allows pp-import, but not of a partition (since the global module
+    // does not have partitions).
+    // We cannot import a partition into a private module fragment, since
+    // [module.private.frag]/1 disallows private module fragments in a multi-
+    // TU module.
+    if (IsPartition || (HeaderUnit && HeaderUnit->Kind !=
+                                          Module::ModuleKind::ModuleHeaderUnit))
+      Diag(ImportLoc, diag::err_import_in_wrong_fragment)
+          << IsPartition
+          << (ImportState == Sema::ModuleImportState::GlobalFragment ? 0 : 1);
     else
       SeenError = false;
     break;
   case Sema::ModuleImportState::ImportFinished:
+  case Sema::ModuleImportState::PrivateFragmentImportFinished:
     if (getLangOpts().CPlusPlusModules)
       Diag(ImportLoc, diag::err_import_not_allowed_here);
     else
       SeenError = false;
     break;
-  case Sema::ModuleImportState::PrivateFragment:
-    Diag(ImportLoc, diag::err_import_in_wrong_fragment) << IsPartition << 1;
-    break;
   }
   if (SeenError) {
     ExpectAndConsumeSemi(diag::err_module_expected_semi);
     return nullptr;
   }
 
   DeclResult Import;
   if (HeaderUnit)

clang/lib/Sema/SemaModule.cpp

DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
   if (getLangOpts().CPlusPlusModules && ExportLoc.isValid() &&
       Mod->Kind == Module::ModuleKind::ModulePartitionImplementation) {
     Diag(ExportLoc, diag::err_export_partition_impl)
         << SourceRange(ExportLoc, Path.back().second);
   } else if (!ModuleScopes.empty() &&
              (ModuleScopes.back().ModuleInterface ||
               (getLangOpts().CPlusPlusModules &&
                ModuleScopes.back().Module->isGlobalModule()))) {
-    assert((!ModuleScopes.back().Module->isGlobalModule() ||
-            Mod->Kind == Module::ModuleKind::ModuleHeaderUnit) &&
-           "should only be importing a header unit into the GMF");
     // 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()) {

clang/test/Modules/cxx20-import-diagnostics-a.cpp

 
 int foo () { return 10; }
 
 import C; // expected-error {{imports must immediately follow the module declaration}}
 
 //--- import-diags-tu7.cpp
 
 module;
-// We can only have preprocessor directives here, which permits an include-
-// translated header unit.  However those are identified specifically by the
-// preprocessor; non-preprocessed user code should not contain an 'import' here.
-import B; // expected-error {{module imports cannot be in the global module fragment}}
-
+// We can only have preprocessor directives here, which permits
+// header units (include-translated or not) and named modules.
+import B;
 export module D;
 
 int delta ();
+// expected-no-diagnostics
 
 //--- import-diags-tu8.cpp
 
 export module D;
 
 int delta ();
 
 module :private;
 
-import B; // expected-error {{module imports cannot be in the private module fragment}}
+import B; // expected-error {{imports must immediately follow the module declaration}}
 
 //--- import-diags-tu9.cpp
 
 module B;
 
 import B; // expected-error {{import of module 'B' appears within its own implementation}}
 
 //--- import-diags-tu10.cpp

clang/test/Modules/cxx20-import-diagnostics-b.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface  %t/a.cpp -o %t/a.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface  %t/c.cpp \
+// RUN: -fmodule-file=%t/a.pcm -o %t/c.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface  %t/d.cpp \
+// RUN: -fmodule-file=%t/a.pcm -o %t/d.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface  %t/e.cpp \
+// RUN: -fmodule-file=%t/a.pcm -o %t/e.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface  %t/a-part.cpp \
+// RUN: -o %t/a-part.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface  %t/f.cpp \
+// RUN: -fmodule-file=%t/a.pcm -o %t/f.pcm -verify
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface  %t/g.cpp \
+// RUN: -fmodule-file=%t/a.pcm -o %t/g.pcm -verify
+
+//--- a.cpp
+export module a;
+
+//--- b.hpp
+import a;
+
+//--- c.cpp
+module;
+#include "b.hpp"
+export module c;
+
+//--- d.cpp
+module;
+import a;
+
+export module d;
+
+//--- e.cpp
+export module e;
+
+module :private;
+import a;
+
+//--- a-part.cpp
+export module a:part;
+
+//--- f.cpp
+module;
+import :part ; // expected-error {{module partition imports cannot be in the global module fragment}}
+
+export module f;
+
+//--- g.cpp
+
+export module g;
+module :private;
+import :part; // expected-error {{module partition imports cannot be in the private module fragment}}