[Modules] Fix ModuleDeclState transition when module is used as a regular identifier

[MaskRay]

ModuleDeclState is incorrectly changed to NamedModuleImplementation
for struct module {}; void foo(module a);. This is mostly benign but
leads to a spurious warning after #69555.

A real world example is:

// pybind11.h
class module_ { ... };
using module = module_;

// tensorflow
void DefineMetricsModule(pybind11::module main_module);
// `module main_module);` incorrectly changes `ModuleDeclState` to `NamedModuleImplementation`

#include <algorithm> // spurious warning

[llvmbot]

@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)

Changes

ModuleDeclState is incorrectly changed to NamedModuleImplementation
for struct module {}; void foo(module a);. This is mostly benign but
leads to a spurious warning after #69555.

A real world example is:

// pybind11.h
class module_ { ... };
using module = module_;

// tensorflow
void DefineMetricsModule(pybind11::module main_module);
// `module main_module);` incorrectly changes `ModuleDeclState` to `NamedModuleImplementation`

---

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

2 Files Affected:

• (modified) clang/lib/Lex/Preprocessor.cpp (+19-18)
• (modified) clang/test/Preprocessor/include-in-module-purview.cppm (+8)

diff --git a/clang/lib/Lex/Preprocessor.cpp b/clang/lib/Lex/Preprocessor.cpp
index ede4c51487ffbe7..482d73f87df07b7 100644
--- a/clang/lib/Lex/Preprocessor.cpp
+++ b/clang/lib/Lex/Preprocessor.cpp
@@ -957,26 +957,27 @@ void Preprocessor::Lex(Token &Result) {
       ModuleDeclState.handlePeriod();
       break;
     case tok::identifier:
-      if (Result.getIdentifierInfo()->isModulesImport()) {
-        TrackGMFState.handleImport(StdCXXImportSeqState.afterTopLevelSeq());
-        StdCXXImportSeqState.handleImport();
-        if (StdCXXImportSeqState.afterImportSeq()) {
-          ModuleImportLoc = Result.getLocation();
-          NamedModuleImportPath.clear();
-          IsAtImport = false;
-          ModuleImportExpectsIdentifier = true;
-          CurLexerKind = CLK_LexAfterModuleImport;
-        }
-        break;
-      } else if (Result.getIdentifierInfo() == getIdentifierInfo("module")) {
-        TrackGMFState.handleModule(StdCXXImportSeqState.afterTopLevelSeq());
-        ModuleDeclState.handleModule();
-        break;
-      } else {
-        ModuleDeclState.handleIdentifier(Result.getIdentifierInfo());
-        if (ModuleDeclState.isModuleCandidate())
+      if (StdCXXImportSeqState.atTopLevel()) {
+        if (Result.getIdentifierInfo()->isModulesImport()) {
+          TrackGMFState.handleImport(StdCXXImportSeqState.afterTopLevelSeq());
+          StdCXXImportSeqState.handleImport();
+          if (StdCXXImportSeqState.afterImportSeq()) {
+            ModuleImportLoc = Result.getLocation();
+            NamedModuleImportPath.clear();
+            IsAtImport = false;
+            ModuleImportExpectsIdentifier = true;
+            CurLexerKind = CLK_LexAfterModuleImport;
+          }
           break;
+        } else if (Result.getIdentifierInfo() == getIdentifierInfo("module")) {
+          TrackGMFState.handleModule(StdCXXImportSeqState.afterTopLevelSeq());
+          ModuleDeclState.handleModule();
+          break;
+        }
       }
+      ModuleDeclState.handleIdentifier(Result.getIdentifierInfo());
+      if (ModuleDeclState.isModuleCandidate())
+        break;
       [[fallthrough]];
     default:
       TrackGMFState.handleMisc();
diff --git a/clang/test/Preprocessor/include-in-module-purview.cppm b/clang/test/Preprocessor/include-in-module-purview.cppm
index 0a080112b43277c..4a7883653ac45dd 100644
--- a/clang/test/Preprocessor/include-in-module-purview.cppm
+++ b/clang/test/Preprocessor/include-in-module-purview.cppm
@@ -5,6 +5,7 @@
 // RUN: %clang_cc1 -std=c++20 %t/a.cppm -E -P -I%t -o %t/tmp 2>&1 | FileCheck %t/a.cppm
 // RUN: %clang_cc1 -std=c++20 %t/a.cppm -E -P -I%t -o - 2>&1 \
 // RUN:     -Wno-include-angled-in-module-purview | FileCheck %t/a.cppm --check-prefix=CHECK-NO-WARN
+// RUN: %clang_cc1 -std=c++20 %t/b.cpp -E -P -I%t -o - 2>&1 | FileCheck %t/a.cppm --check-prefix=CHECK-NO-WARN
 
 //--- a.h
 // left empty
@@ -58,3 +59,10 @@ module :private;
 // CHECK: 10 warnings generated.
 
 // CHECK-NO-WARN-NOT: warning
+
+//--- b.cpp
+/// Don't recognize `module m);` as a module purview or report a spurious
+/// warning for <stddef.h>.
+struct module {};
+void foo(module m);
+#include <stddef.h>

[ChuanqiXu9]

Thanks for looking into this. LGTM.

[ChuanqiXu9]

I spent some time to understand the rationale here. How about add a comment like: "Otherwise we're in the unclosed brackets, then the modules related identifiers are not meaningful."