[clang] Move the diagnostic of unexpected token after module name from phase 4 to phase 7 by yronglin · Pull Request #187846 · llvm/llvm-project

yronglin · 2026-03-21T08:23:12Z

The PR #107168 implement CWG2947. And clang checks next pp-token after module name in phase 4.

This PR move the check to phase 7, there are three main motivations:

We already convert the module name pp-tokens sequence into a annot_module_name token, so for the following case, macro expansion will be disabled.
In the original implementation, we need to look ahead a token even if we already hit an eod token, but in phase7, we don't need to look ahead a token.
The original implementation checked and emit a diagnosis in phase 4, and then skipped these unexpected tokens in phase 7. Now we've merged them into one step and are able to get a diagnosis approximate as before.

// #define m(x) x
export module m
     (foo);                 // Before: unexpected preprocessing token '(' after module name .....
                              // After: unexpected '(' after module name ....

But in the following will be breaking current behavior:

// #define m(x) x
// #define LPAREN (
export module m
    LPAREN foo);     // Before: unexpected preprocessing token 'LPAREN' after module name .....
                              // After: unexpected '(' after module name ....

yronglin · 2026-03-21T08:23:33Z

I'm not sure it's the correct approach, feel free to comments!

llvmbot · 2026-03-21T08:23:43Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: None (yronglin)

Changes

The PR #107168 implement CWG2947. And clang checks next pp-token after module name in phase 4.

This PR move the check to phase 7, there are three main motivations:

We already convert the module name pp-tokens sequence into a annot_module_name token, so for the following case, macro expansion will be disabled.
In the original implementation, we need to look ahead a token even if we already hit eod, but in phase7, we don't need to look ahead a token.
The original implementation checked and emit a diagnosis in phase 4, and then skipped these unexpected tokens in phase 7. Now we've merged them into one step and are able to get a more approximate diagnosis.

// #define m(x) x
export module m
     (foo);                 // Before: unexpected preprocessing token '(' after module name .....
                              // After: unexpected '(' after module name ....

But in the following will be breaking current behavior:

// #define m(x) x
// #define LPAREN (
export module m
    LPAREN foo);     // Before: unexpected preprocessing token 'LPAREN' after module name .....
                              // After: unexpected '(' after module name ....

Full diff: https://github.com/llvm/llvm-project/pull/187846.diff

9 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/include/clang/Basic/DiagnosticLexKinds.td (-3)
(modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+3)
(modified) clang/lib/Lex/PPDirectives.cpp (-15)
(modified) clang/lib/Parse/Parser.cpp (+4-2)
(modified) clang/test/CXX/basic/basic.link/p3.cpp (+1-1)
(modified) clang/test/CXX/drs/cwg2947.cpp (+2-2)
(modified) clang/test/CXX/module/basic/basic.link/module-declaration.cpp (+1-1)
(modified) clang/test/CXX/module/cpp.pre/p1.cpp (+3-3)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b13171f7ecafc..9e2531e2c5645 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -339,6 +339,7 @@ Bug Fixes in This Version
 - Fixed a crash when normalizing constraints involving concept template parameters whose index coincided with non-concept template parameters in the same parameter mapping.
 - Fixed a crash caused by accessing dependent diagnostics of a non-dependent context.
 - Fixed a crash when substituting into a non-type template parameter that has a type containing an undeduced placeholder type.
+- Fixed a crash when module directive export module foo not following a semicolon and there are no rest pp-tokens in current module file. (#GH187771)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td
index 5eceeced311f2..ed448fdfb2011 100644
--- a/clang/include/clang/Basic/DiagnosticLexKinds.td
+++ b/clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -1006,9 +1006,6 @@ def err_pp_module_name_is_macro : Error<
   "%select{module|partition}0 name component %1 cannot be a object-like macro">;
 def err_pp_module_expected_ident : Error<
   "expected %select{identifier after '.' in |}0module name">;
-def err_pp_unexpected_tok_after_module_name : Error<
-  "unexpected preprocessing token '%0' after module name, "
-  "only ';' and '[' (start of attribute specifier sequence) are allowed">;
 def warn_pp_extra_tokens_at_module_directive_eol
     : Warning<"extra tokens after semicolon in '%0' directive">,
       InGroup<ExtraTokens>;
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 19edc7f7a3b23..42d2f20f8c8c3 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1782,6 +1782,9 @@ def ext_bit_int : Extension<
 } // end of Parse Issue category.
 
 let CategoryName = "Modules Issue" in {
+def err_unexpected_tok_after_module_name : Error<
+  "unexpected '%0' after module name, only ';' and '[' (start of attribute "
+  "specifier sequence) are allowed">;
 def err_unexpected_module_or_import_decl : Error<
   "%select{module|import}0 declaration can only appear at the top level">;
 def err_attribute_not_module_attr : Error<
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 09f0c3fe8c61e..2a1b2792ae548 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -4405,21 +4405,6 @@ void Preprocessor::HandleCXXModuleDirective(Token ModuleTok) {
     break;
   }
 
-  // Consume the pp-import-suffix and expand any macros in it now, if we're not
-  // at the semicolon already.
-  std::optional<Token> NextPPTok = DirToks.back();
-  if (DirToks.back().is(tok::eod)) {
-    NextPPTok = peekNextPPToken();
-    if (NextPPTok && NextPPTok->is(tok::raw_identifier))
-      LookUpIdentifierInfo(*NextPPTok);
-  }
-
-  // Only ';' and '[' are allowed after module name.
-  // We also check 'private' because the previous is not a module name.
-  if (!NextPPTok->isOneOf(tok::semi, tok::eod, tok::l_square, tok::kw_private))
-    Diag(*NextPPTok, diag::err_pp_unexpected_tok_after_module_name)
-        << getSpelling(*NextPPTok);
-
   if (!DirToks.back().isOneOf(tok::semi, tok::eod)) {
     // Consume the pp-import-suffix and expand any macros in it now. We'll add
     // it back into the token stream later.
diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp
index 5d18414b1a746..61b64133d35d2 100644
--- a/clang/lib/Parse/Parser.cpp
+++ b/clang/lib/Parse/Parser.cpp
@@ -2375,9 +2375,11 @@ Parser::ParseModuleDecl(Sema::ModuleImportState &ImportState) {
   }
 
   // This should already diagnosed in phase 4, just skip unil semicolon.
-  if (!Tok.isOneOf(tok::semi, tok::l_square))
+  if (!Tok.isOneOf(tok::semi, tok::l_square)) {
+    Diag(Tok, diag::err_unexpected_tok_after_module_name)
+        << PP.getSpelling(Tok);
     SkipUntil(tok::semi, SkipUntilFlags::StopBeforeMatch);
-
+  }
   // We don't support any module attributes yet; just parse them and diagnose.
   ParsedAttributes Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs);
diff --git a/clang/test/CXX/basic/basic.link/p3.cpp b/clang/test/CXX/basic/basic.link/p3.cpp
index bc3622c7bbd64..0784a7d879d7d 100644
--- a/clang/test/CXX/basic/basic.link/p3.cpp
+++ b/clang/test/CXX/basic/basic.link/p3.cpp
@@ -14,7 +14,7 @@ constexpr int n = 123;
 
 export module m; // #1
 module y = {}; // expected-error {{multiple module declarations}}
-// expected-error@-1 {{unexpected preprocessing token '=' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
+// expected-error@-1 {{unexpected '=' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
 // expected-note@#1 {{previous module declaration}}
 
 ::import x = {};
diff --git a/clang/test/CXX/drs/cwg2947.cpp b/clang/test/CXX/drs/cwg2947.cpp
index dd66a2cfcfc4d..b932fca4a8175 100644
--- a/clang/test/CXX/drs/cwg2947.cpp
+++ b/clang/test/CXX/drs/cwg2947.cpp
@@ -37,7 +37,7 @@
 //--- cwg2947_example1.cpp
 // #define DOT_BAR .bar
 export module foo DOT_BAR; // error: expansion of DOT_BAR; does not begin with ; or [
-// expected-error@-1 {{unexpected preprocessing token '.' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
+// expected-error@-1 {{unexpected '.' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
 
 //--- cwg2947_example2.cpp
 export module M MOD_ATTR;        // OK
@@ -46,7 +46,7 @@ export module M MOD_ATTR;        // OK
 //--- cwg2947_example3.cpp
 export module a
   .b;                         // error: preprocessing token after pp-module-name is not ; or [
-// expected-error@-1 {{unexpected preprocessing token '.' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
+// expected-error@-1 {{unexpected '.' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
 
 //--- cwg2947_example4.cpp
 export module M [[
diff --git a/clang/test/CXX/module/basic/basic.link/module-declaration.cpp b/clang/test/CXX/module/basic/basic.link/module-declaration.cpp
index 52ba1d9f82f2f..b22fc87aa296c 100644
--- a/clang/test/CXX/module/basic/basic.link/module-declaration.cpp
+++ b/clang/test/CXX/module/basic/basic.link/module-declaration.cpp
@@ -47,7 +47,7 @@ export module x;
 
 //--- invalid_module_name.cppm
 export module z elderberry;
-// expected-error@-1 {{unexpected preprocessing token 'elderberry' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
+// expected-error@-1 {{unexpected 'elderberry' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
 
 //--- empty_attribute.cppm
 // expected-no-diagnostics
diff --git a/clang/test/CXX/module/cpp.pre/p1.cpp b/clang/test/CXX/module/cpp.pre/p1.cpp
index 989915004ff57..dc5a3c36d4139 100644
--- a/clang/test/CXX/module/cpp.pre/p1.cpp
+++ b/clang/test/CXX/module/cpp.pre/p1.cpp
@@ -153,7 +153,7 @@ export module m:n;
 
 //--- unexpected_character_in_pp_module_suffix.cpp
 export module m();
-// expected-error@-1 {{unexpected preprocessing token '(' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
+// expected-error@-1 {{unexpected '(' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
 
 //--- semi_in_same_line.cpp
 export module m // OK
@@ -189,13 +189,13 @@ module x;
 //--- func_like_macro.cpp
 // #define m(x) x
 export module m
-     (foo); // expected-error {{unexpected preprocessing token '(' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
+     (foo); // expected-error {{unexpected '(' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
 
 //--- lparen.cpp
 // #define m(x) x
 // #define LPAREN (
 export module m
-    LPAREN foo); // expected-error {{unexpected preprocessing token 'LPAREN' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
+    LPAREN foo); // expected-error {{unexpected '(' after module name, only ';' and '[' (start of attribute specifier sequence) are allowed}}
 
 //--- control_line.cpp
 #if 0 // #1

erichkeane

I don't have a good feel if this is the 'right' solution, but it seems reasonable to me. Hopefully someone with a better understanding of module parsing comes along.

Bigcheese

I believe this has the correct behavior and matches CWG2947. We do the macro expansion, then check the rules. I think the only issue is that we will no longer error with -E.

I have a minor preference for handling this with just -E as I model that as "stop after phase 4". Would fixing the crash be more complicated in that case? I do think this is an ok approach if needed.

yronglin · 2026-06-11T17:42:23Z

Thanks for the review!

I'm a little bit concern about the diagnostic message:

def err_unexpected_tok_after_module_name : Error<
  "unexpected '%0' after module name, only ';' and '[' (start of attribute "
  "specifier sequence) are allowed">;

I'm not sure it's clean enough.

yronglin · 2026-06-23T13:53:37Z

Friendly ping~

yronglin · 2026-07-22T06:47:25Z

@Bigcheese Ping~

cor3ntin · 2026-07-22T10:01:36Z

Can you add -E tests?

yronglin · 2026-07-22T17:37:43Z

Can you add -E tests?

Thanks for the review!, I'll add.

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin · 2026-08-20T16:24:19Z

@Bigcheese @cor3ntin Friendly ping~

yronglin · 2026-08-21T16:48:55Z

Thanks a lot for your review!

llvm-ci · 2026-08-21T17:00:14Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-sie-ubuntu-fast running on sie-linux-worker while building clang at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/62780