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

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

Differential Revision: https://reviews.llvm.org/D140927

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 7fbd8ef..3e3fb2b 100644 (file)
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3146,12 +3146,15 @@ public:
   /// 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:

diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp
index 104836a..6db3dc3 100644 (file)
--- a/clang/lib/Parse/Parser.cpp
+++ b/clang/lib/Parse/Parser.cpp
@@ -750,6 +750,10 @@ bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
     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;
 }
@@ -2427,7 +2431,9 @@ Parser::ParseModuleDecl(Sema::ModuleImportState &ImportState) {
     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);
   }
 
@@ -2544,23 +2550,28 @@ Decl *Parser::ParseModuleImport(SourceLocation AtLoc,
       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);

diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp
index 823a4e9..f52c024 100644 (file)
--- a/clang/lib/Sema/SemaModule.cpp
+++ b/clang/lib/Sema/SemaModule.cpp
@@ -591,9 +591,6 @@ DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
              (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.

diff --git a/clang/test/Modules/cxx20-import-diagnostics-a.cpp b/clang/test/Modules/cxx20-import-diagnostics-a.cpp
index d940722..c5d2280 100644 (file)
--- a/clang/test/Modules/cxx20-import-diagnostics-a.cpp
+++ b/clang/test/Modules/cxx20-import-diagnostics-a.cpp
@@ -95,14 +95,13 @@ import C; // expected-error {{imports must immediately follow the module declara
 //--- 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
 
@@ -112,7 +111,7 @@ 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
 

diff --git a/clang/test/Modules/cxx20-import-diagnostics-b.cpp b/clang/test/Modules/cxx20-import-diagnostics-b.cpp
new file mode 100644 (file)
index 0000000..f252190
--- /dev/null
+++ b/clang/test/Modules/cxx20-import-diagnostics-b.cpp
@@ -0,0 +1,61 @@
+// 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}}