[C++20] Diagnose invalid and reserved module names

[aaron.ballman]

Summary

[module.unit]p1 specifies that module and import are invalid components of a module name, that module names cannot contain reserved identifiers, and that std followed by zero or more digits is reserved.

The first issue (module and import pseudo-keywords) requires a diagnostic, the second issue (use of reserved identifiers) does not require a diagnostic. We diagnose both the same -- the code is ill-formed unless the module declaration is in a system header. This allows STL implementations to use the reserved module names while preventing users from stealing them out from under us.

[cor3ntin]

clang/lib/Sema/SemaModule.cpp

288

293

I think module in a standard header is still invalid. We could not do the check in a system header at all

[cor3ntin]

clang/test/Modules/reserved-names.cpp

37 ↗

(On Diff #471550)

you should add a test for __test

[erichkeane]

I'll leave it to the modules experts to decide whether they're happy with this, but I had a drive-by.

Also, I see none of the tests validate 'import', just 'export'. Based on the standard, BOTH are illegal, right :D

clang/lib/Sema/SemaModule.cpp

279

Urgh, could you make this an enum?

[cor3ntin]

In D136953#3892060, @erichkeane wrote:

I'll leave it to the modules experts to decide whether they're happy with this, but I had a drive-by.

Also, I see none of the tests validate 'import', just 'export'. Based on the standard, BOTH are illegal, right :D

export module export is perfectly fine conforming

[aaron.ballman]

In D136953#3892077, @cor3ntin wrote:

In D136953#3892060, @erichkeane wrote:

I'll leave it to the modules experts to decide whether they're happy with this, but I had a drive-by.

Also, I see none of the tests validate 'import', just 'export'. Based on the standard, BOTH are illegal, right :D

Heh, Erich is joking about my malicious reading of the standard where module-name has blanket wording for what is reserved, but that same grammar production is used with importing modules. e.g., a malicious reading of the standard could make you think import module std; is also invalid because it's using a reserved module name.

export module export is perfectly fine conforming

Nope, that's not going to compile because export is a real keyword instead of an identifier masquerading as a keyword like module and import.

clang/lib/Sema/SemaModule.cpp

279

Absolutely.

288

lol but my way is so much more complicated, so it must be more right... ;-)

293

Good catch, that's a think-o on my part!

[erichkeane]

clang/lib/Sema/SemaModule.cpp

288

Is this logic right? By my reading of the standard,

export module stdFoo.bar << fine
export module std00Foo.bar << fine

as neither 'consist of std followed by zero or more digits'.

This patch looks like it'll only allow the 1st, but not the 2nd.

clang/test/Modules/reserved-names.cpp

30 ↗

(On Diff #471550)

SHOULD this fail?

This is NOT a module name beginning with an identifier-consisting-of-std followed by 0 or more digits.

The 'containing' part applies to just the reserved identifiers.

[cor3ntin]

clang/lib/Sema/SemaModule.cpp

288

I've been wondering the same, I think the standard is a bit confusing in that regard.
std00Foo is "std followed by 0 or more digit".

[erichkeane]

clang/lib/Sema/SemaModule.cpp

288

But it isn't 'consisting' of that, it is consisting of std, 0 or more digits, then 'Foo'.

[aaron.ballman]

Updated based on review feedback and reflection.

I think the standards wording here is a bit unclear as to what's intended and I had misunderstood the wording around which part of the path needs to be checked. Specifically:

All module-names either beginning with an identifier consisting of std followed by zero or more digits or containing a reserved identifier ([lex.name]) are reserved and shall not be specified in a module-declaration; no diagnostic is required.

I took this to mean all identifier components of a module name should check for std followed by zero or more digits, but I now believe it means only the first component in the path and it intends to only catch use of std[0-9]+ and not .*std[0-9]+.*. e.g., std.foo is reserved while foo.std and std12Three are not. I've changed the patch accordingly while addressing the other review feedback.

[cor3ntin]

clang/lib/Sema/SemaModule.cpp

272–279

Did you consider making this a function?
I think you could just check Path[0] for reserved stdxxx syntax, and then all parts for reserved/import/module. Might simplify the code a bit.

clang/test/Modules/reserved-names-2.cpp

7

should_fail? Did you mean the opposite?

[aaron.ballman]

Updating based on review feedback.

clang/lib/Sema/SemaModule.cpp

272–279

I'm not opposed.

clang/test/Modules/reserved-names-2.cpp

7

Indeed I do, good catch!

[tschuett]

Thx.

clang/test/Modules/reserved-names-1.cpp

43

import?

[cor3ntin]

Modulo the import typo. I'm happy with this.

[tschuett]

Are malformed imports an issue or are they already covered? There is limit test coverage for import. Am I missing something?

import import;
import module;

[tschuett]

clang/lib/Sema/SemaModule.cpp

148

Returns

[ChuanqiXu]

In D136953#3892578, @aaron.ballman wrote:

I think the standards wording here is a bit unclear as to what's intended and I had misunderstood the wording around which part of the path needs to be checked. Specifically:

All module-names either beginning with an identifier consisting of std followed by zero or more digits or containing a reserved identifier ([lex.name]) are reserved and shall not be specified in a module-declaration; no diagnostic is required.

I took this to mean all identifier components of a module name should check for std followed by zero or more digits, but I now believe it means only the first component in the path and it intends to only catch use of std[0-9]+ and not .*std[0-9]+.*. e.g., std.foo is reserved while foo.std and std12Three are not. I've changed the patch accordingly while addressing the other review feedback.

Oh, yeah, the wording here looks confusing. And I agree with your understanding.

clang/lib/Sema/SemaModule.cpp

282

std modules should be irreverent with system headers; The intuition of the wording should be that the users can't declare modules like std or std.compat to avoid possible conflicting. The approach I imaged may be add a new compilation flags (call it -fstd-modules) now. And if the compiler found a std module declaration without -fstd-modules, emit an error.

For now, I think we can skip the check for -fstd-modules and add it back when we starts to support std modules actually.

clang/test/Modules/reserved-names-1.cpp

24–25

or diagnose about export module std.foo.

[cor3ntin]

clang/lib/Sema/SemaModule.cpp

282

The idea is that standard modules are built from system directories... it seems a better heuristic than adding a flag for the purpose of 1 diagnostics ( maybe some other system library could in theory export std with no warning, but I'm not super worried about that being a concern in practice)

[erichkeane]

clang/docs/ReleaseNotes.rst

376

So question: ARE we diagnosing all 'use' of invalid/reserved module names? So the idea is you cannot import:
std.concepts? Or are we diagnosing export only?

[ChuanqiXu]

clang/lib/Sema/SemaModule.cpp

282

The idea is that standard modules are built from system directories...

This is not true. For example, if someday libc++ supports std modules, then we need to build the std modules in our working directory, which is not system directories. And ideally, we would distribute and install module file in the system directories. But it is irreverent with the path of the source file.

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

then we need to build the std modules in our working directory, which is not system directories.

-isystem, pragmas, and linemarkers are all ways around that -- I don't think we need a feature flag for this, unless I'm misunderstanding something.

[aaron.ballman]

In D136953#3893039, @tschuett wrote:

Are malformed imports an issue or are they already covered? There is limit test coverage for import. Am I missing something?

import import;
import module;

It's unclear to me whether the constraints also apply to module imports or not. The wording for the constraints is in the section about module units and the section about module imports doesn't say anything further. Certainly we want to allow import std; instead of telling the user about the use of a reserved identifier, but it's less clear to me about import module; -- if the user writes that, they'll either get an error about not being able to find a module by that name, or they'd import the module (but how did they produce that module in the first place?).

clang/docs/ReleaseNotes.rst

376

Export only for the moment. I'll update the release note accordingly.

clang/lib/Sema/SemaModule.cpp

148

Good catch!

[aaron.ballman]

Updated based on review feedback:

• Fixed a typo
• Clarified the release note
• Added a test case for std.foo being reserved

[ChuanqiXu]

clang/lib/Sema/SemaModule.cpp

282

Although it may be a little bit nit picker, the module unit which declares the std modules won't be header. It is a module unit. So it requires we implement std modules by wrapping linemarkers around the std modules declaration, which looks a little bit overkill.

And another point is that maybe we need to introduce another feature flags to implement std modules. Although I tried to implement std modules within the current features, I can't prove we can implement std modules in that way in the end of the day.

Let me add some more words. The standards require us to implement std modules without deprecating the system headers. This strategy leads the direction to "implement the components in the original headers and control their visibility in the std module unit".
It may look like:

//--- std.cppm
module;
#include <algorithm>
...
export module std;

Then how can control the visibility? In my original experiments, I use the style:

//--- std.cppm
module;
#include <algorithm>
...
export module std;
export namespace std {
    using std::sort;
}

but this doesn't always work. For example, we can't using a in-class friend definition. And there are more reasons, the unreachable declarations in the global module fragment (the section from module; to export module [module_name]) can be discarded to reduce the size of the module file. And the reachable rules are complex. But the simple story is that it is highly possible the a lot of necessary declarations in global module fragment in the above snippet would be discarded so that the user can't use std modules correctly. I mean, we may need a special feature flag for it. And the method with system headers looks not good and semantics are not so consistency.

[ChuanqiXu]

clang/test/Modules/reserved-names-1.cpp

26

It looks like the diagnostic message should be 'std.foo is a ...'

[aaron.ballman]

clang/test/Modules/reserved-names-1.cpp

26

We tell the user which part of the path name is reserved instead of giving them the whole path name. That helps for cases like: export module aaron.wrote.this.awesome.module; If there's confusion from this, we could probably change the diagnostic to be 'this' is a reserved identifier within module name 'aaron.wrote.this.awesome.module', but because you can only declare one module at a time, that seemed like overkill.

[ChuanqiXu]

clang/test/Modules/reserved-names-1.cpp

26

Got it. Makes sense.

[erichkeane]

clang/lib/Sema/SemaModule.cpp

282

IMO, any such additional flag (say -isystem-module) should ALSO use the isInSystemHeader infrastructure. I suspect nearly every place we use isInSystemHeader we also mean to exclude a system-module as well.

I think that any such flag can/should be added later as you figure out how it should be specified/work. That said, when you do so, it should either also feed isInSystemHeader, or basically every use of isInSystemHeader should ALSO changed to use the new flag as well

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

I think that any such flag can/should be added later as you figure out how it should be specified/work. That said, when you do so, it should either also feed isInSystemHeader, or basically every use of isInSystemHeader should ALSO changed to use the new flag as well

+1, that's my thinking as well.

[ChuanqiXu]

clang/lib/Sema/SemaModule.cpp

282

The main confusion part to me is that why we connect std modules with system paths? I know implementors can work around the check like the tests did. But what's the point? I know every header of libcxx contains:

#ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
#  pragma GCC system_header
#endif

but it is for the compatibility with GCC. And it looks not so meaningful to force the implementation of modules to keep such contraints.

clang/test/Modules/reserved-names-1.cpp

34

We lack a test for foo.std;

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

The main confusion part to me is that why we connect std modules with system paths?

We consider the system paths to be "special" in that they can do things "user" paths cannot do. I think we want to keep that model for modules because of how successful it has been for includes. (e.g., don't suggest fixits in a system module but do suggest them for user modules).

clang/test/Modules/reserved-names-1.cpp

34

reserved-named-2.cpp has that test (it uses std0 instead of std). Is that sufficient?

[ChuanqiXu]

LGTM if we add a test for foo.std.

clang/docs/ReleaseNotes.rst

378

It reads odd about system header. But I can't get better name now (I guess we don't have one now). I think we need to update the StandardCPlusCplusModules.rst documents in a new section. I'll take it.

clang/lib/Sema/SemaModule.cpp

282

OK, I got it and it won't be a problem we can't workaround.

clang/test/Modules/reserved-names-1.cpp

34

I feel better with std

[philnik]

clang/lib/Sema/SemaModule.cpp

282

IIUC this would prevent the library from handling the std module the same as a user module, right? AFAIK the actual use of _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is to enable warnings in the headers for development, which would not work with the modules with this patch, or am I misunderstanding something? Is there a reason this isn't a warning that's an error by default? That would allow the library to disable it and still serve the same purpose.

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

AFAIK the actual use of _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is to enable warnings in the headers for development, which would not work with the modules with this patch, or am I misunderstanding something?

Why would the library want a diagnostic telling them they're using a reserved identifier as a module name?

Is there a reason this isn't a warning that's an error by default? That would allow the library to disable it and still serve the same purpose.

It also allows users to produce modules with reserved identifiers. It's an error that can't be downgraded specifically because I don't think we want our implementation to give arbitrary users that ability.

[aaron.ballman]

Updated based on review feedback.

Added an additional test case, reworked the comments in one of the tests as well.

[aaron.ballman]

clang/test/Modules/reserved-names-1.cpp

34

Okay, I added an additional test.

[philnik]

clang/lib/Sema/SemaModule.cpp

282

AFAIK the actual use of _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is to enable warnings in the headers for development, which would not work with the modules with this patch, or am I misunderstanding something?

Why would the library want a diagnostic telling them they're using a reserved identifier as a module name?

I don't mean specifically this error, I mean more generally that other warnings should be generated from std modules. Treating the headers as system headers disables most warnings, which is the reason libc++ treat them as normal headers in the tests.

Is there a reason this isn't a warning that's an error by default? That would allow the library to disable it and still serve the same purpose.

It also allows users to produce modules with reserved identifiers. It's an error that can't be downgraded specifically because I don't think we want our implementation to give arbitrary users that ability.

I think there should be some way to enable normal warnings in the special modules, since it makes the life of library developers a lot easier. I don't care whether that's through disabling a warning or some special sauce to enable warnings from the std module, but there should be some way.

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

I don't mean specifically this error, I mean more generally that other warnings should be generated from std modules. Treating the headers as system headers disables most warnings, which is the reason libc++ treat them as normal headers in the tests.

Ahhh, thank you, that makes a lot more sense to me. :-D

I think there should be some way to enable normal warnings in the special modules, since it makes the life of library developers a lot easier. I don't care whether that's through disabling a warning or some special sauce to enable warnings from the std module, but there should be some way.

Just like we have -Wsystem-headers, I would expect we'd have something similar for modules (or reuse it, perhaps with a different name, for both headers and modules).

[philnik]

clang/lib/Sema/SemaModule.cpp

282

-Wsystem-headers doesn't work because that enables warnings in all system headers, but we only want the warnings from the system library that we write, i.e. libc++. Or can you somehow control in which system headers warnings are emitted?

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

-Wsystem-headers doesn't work because that enables warnings in all system headers, but we only want the warnings from the system library that we write, i.e. libc++

You're correct that it enabled warnings in all system headers if used from the command line, but you can use pragmas to control which warnings are or are not enabled on a file by file basis.

However, maybe I'm not understanding your concerns properly, so let me back up a step. With this patch, trying to create a module with a reserved name fails with an error unless the source file with the module declaration is in a file considered to be a "system header" (whether through -isystem, a pragma, linemarkers, etc). For libc++, I was thinking this should not be an issue because libc++ has _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER to control whether the file is considered a system header or not, so when exporting modules you have to ensure that macro is not defined. If you want to test the behavior as though the user was the one declaring that exported module... well, they get an error because they're not allowed to define that module, so I'm not certain what value there is in testing that specific situation. Am I misunderstanding something?

[philnik]

clang/lib/Sema/SemaModule.cpp

282

My understanding right now is that libc++ would have to build the module as a system header. You want this, so users have to try really hard to produce a module with a reserved identifier, which makes sense. Because of that, warnings generated inside the module would be suppressed though.
Essentially, my concern is that we don't get all the warnings we want when testing anything inside the std module. To get warnings from the std module we could use -Wsystem-headers. The problem there is that it would also produce warnings in headers we have no control over, which means we can't use it.

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

Ah, thank you for clarifying! I think we're considering the libc++ implementation strategy from different angles. One way to do it is as you said -- build the module as a system header. But I was thinking libc++ would do something a bit more novel (and maybe that's a bad idea!):

# __LINE__ __FILE__ 1 3 // Enter system header
export module std;
# __LINE__ __FILE__ 2 3 // Leave system header

and leave the rest of the module code "outside" of the faked up system header block of linemarker directives.

[philnik]

clang/lib/Sema/SemaModule.cpp

282

I guess it's # <line> <file> <mode?> <???>? That seems to me quite hacky. Maybe it would be nicer if we add an attribute like [[clang::system_module]] that would allow reserved identifiers, and maybe even ensure that it's actually one. Though that might be overkill, and would again allow people to more easily create modules with reserved identifiers. It would make the code a lot nicer to read though.

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

282

I guess it's # <line> <file> <mode?> <???>? That seems to me quite hacky.

Yeah, this is a pretty arcane feature but it's been around for ages. GCC documents this feature (Clang does not and we absolutely should): https://gcc.gnu.org/onlinedocs/gcc-12.2.0/cpp/Preprocessor-Output.html#Preprocessor-Output

It would make the code a lot nicer to read though.

It would, but we have to balance protecting this reserved space from the compiler while we can against readability of (already generally unreadable, IMHO) STL headers. But then again, linemarkers allow the user to do this themselves in the same manner as an attribute would. Between the two, I think linemarkers are far less likely to be used by users just trying to get their code to compile without worrying about the details of what they're doing.

[philnik]

clang/lib/Sema/SemaModule.cpp

282

I guess it's # <line> <file> <mode?> <???>? That seems to me quite hacky.

Yeah, this is a pretty arcane feature but it's been around for ages. GCC documents this feature (Clang does not and we absolutely should): https://gcc.gnu.org/onlinedocs/gcc-12.2.0/cpp/Preprocessor-Output.html#Preprocessor-Output

Thanks for the link!

It would make the code a lot nicer to read though.

It would, but we have to balance protecting this reserved space from the compiler while we can against readability of (already generally unreadable, IMHO) STL headers. But then again, linemarkers allow the user to do this themselves in the same manner as an attribute would. Between the two, I think linemarkers are far less likely to be used by users just trying to get their code to compile without worrying about the details of what they're doing.

Yeah, I also thought that it might be a bit too easy to add an attribute.
It would also be possible to just have an internal header like

__export_module_std

#pragma GCC system_header
export module std;

right? That wouldn't be great, but it would be a lot easier to understand than linemarkers.
As a side note: Anything that we only have to make available through modules could be a lot more readable, since the preprocessor wouldn't be a problem anymore. That won't happen soon, but I can dream.

Anyways, since we can enable warnings inside modules I don't have any concerns here anymore. Thanks for being so patient with me!

[aaron.ballman]

I believe we now have consensus on the patch. I'll land it in a few days in case any reviewers want to weigh in now that the discussion has stabilized.

clang/lib/Sema/SemaModule.cpp

282

It would also be possible to just have an internal header like ... right?

Yes, that should work I believe. And yes, that would be more direct than using linemarkers.

Anyways, since we can enable warnings inside modules I don't have any concerns here anymore. Thanks for being so patient with me!

Thank you for the great discussion! If we find we need an attribute/something more here in the future. we can certainly revisit once we have a concrete problem to solve.

clang/docs/ReleaseNotes.rst

- Clang now correctly points to the problematic parameter for the ``-Wnonnull``
   `Issue 58273 <https://github.com/llvm/llvm-project/issues/58273>`_.
 - Introduced ``-Wcast-function-type-strict`` to warn about function type mismatches
   in casts that may result in runtime indirect call `Control-Flow Integrity (CFI)
   <https://clang.llvm.org/docs/ControlFlowIntegrity.html>`_ failures. This diagnostic
   is grouped under ``-Wcast-function-type`` as it identifies a more strict set of
   potentially problematic function type casts.
 - Clang will now disambiguate NTTP types when printing diagnostic that contain NTTP types.
   Fixes `Issue 57562 <https://github.com/llvm/llvm-project/issues/57562>`_.
 - Better error recovery for pack expansion of expressions.
   `Issue 58673 <https://github.com/llvm/llvm-project/issues/58673>`_.
 - Better diagnostics when the user has missed `auto` in a declaration.
   `Issue 49129 <https://github.com/llvm/llvm-project/issues/49129>`_.
+- Clang now diagnoses use of invalid or reserved module names in a module
+  export declaration. Both are diagnosed as an error, but the diagnostic is
+  suppressed for use of reserved names in a system header.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
 - When using header modules, inclusion of a private header and violations of
   the `use-declaration rules

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_private_module_fragment_not_module : Error<
   "private module fragment declaration with no preceding module declaration">;
 def err_private_module_fragment_redefined : Error<
   "private module fragment redefined">;
 def err_private_module_fragment_not_module_interface : Error<
   "private module fragment in module implementation unit">;
 def note_not_module_interface_add_export : Note<
   "add 'export' here if this is intended to be a module interface unit">;
+def err_invalid_module_name : Error<
+  "%0 is %select{an invalid|a reserved}1 name for a module">;
 
 def ext_equivalent_internal_linkage_decl_in_modules : ExtWarn<
   "ambiguous use of internal linkage declaration %0 defined in multiple modules">,
   InGroup<DiagGroup<"modules-ambiguous-internal-linkage">>;
 def note_equivalent_internal_linkage_decl : Note<
   "declared here%select{ in module '%1'|}0">;
 
 def note_redefinition_modules_same_file : Note<

clang/lib/Sema/SemaModule.cpp

void Sema::HandleStartOfHeaderUnit() {
 
   // From now on, we have an owning module for all declarations we see.
   // All of these are implicitly exported.
   auto *TU = Context.getTranslationUnitDecl();
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::Visible);
   TU->setLocalOwningModule(Mod);
 }
 
+/// Tests whether the given identifier is reserved as a module name and
+/// diagnoses if it is. Returns true if a diagnostic is emitted and false
+/// otherwise.
+static bool DiagReservedModuleName(Sema &S, const IdentifierInfo *II,
+                                   SourceLocation Loc) {
+  enum {
+    Valid = -1,
+    Invalid = 0,
+    Reserved = 1,
+  } Reason = Valid;
+
+  StringRef PartName = II->getName();
+  if (II->isStr("module") || II->isStr("import"))
+    Reason = Invalid;
+  else if (II->isReserved(S.getLangOpts()) !=
+           ReservedIdentifierStatus::NotReserved)
+    Reason = Reserved;
+
+  // If the identifier is reserved (not invalid) but is in a system header,
+  // we do not diagnose (because we expect system headers to use reserved
+  // identifiers).
+  if (Reason == Reserved && S.getSourceManager().isInSystemHeader(Loc))
+    Reason = Valid;
+
+  if (Reason != Valid) {
+    S.Diag(Loc, diag::err_invalid_module_name) << II << (int)Reason;
+    return true;
+  }
+  return false;
+}
+
 Sema::DeclGroupPtrTy
 Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc,
                       ModuleDeclKind MDK, ModuleIdPath Path,
                       ModuleIdPath Partition, ModuleImportState &ImportState) {
   assert((getLangOpts().ModulesTS || getLangOpts().CPlusPlusModules) &&
          "should only have module decl in Modules TS or C++20");
 
   bool IsFirstDecl = ImportState == ModuleImportState::FirstDecl;
SourceLocation BeginLoc =
             ? SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID())
             : ModuleScopes.back().BeginLoc;
     if (BeginLoc.isValid()) {
       Diag(BeginLoc, diag::note_global_module_introducer_missing)
           << FixItHint::CreateInsertion(BeginLoc, "module;\n");
     }
   }
 
+  // C++2b [module.unit]p1: ... The identifiers module and import shall not
+  // appear as identifiers in a module-name or module-partition. All
+  // module-names either beginning with an identifier consisting of std
+  // followed by zero or more digits or containing a reserved identifier
+  // ([lex.name]) are reserved and shall not be specified in a
+  // module-declaration; no diagnostic is required.
+
+  // Test the first part of the path to see if it's std[0-9]+ but allow the
+  // name in a system header.
+  StringRef FirstComponentName = Path[0].first->getName();
+  if (!getSourceManager().isInSystemHeader(Path[0].second) &&
+      (FirstComponentName == "std" ||
+       (FirstComponentName.startswith("std") &&
+        llvm::all_of(FirstComponentName.drop_front(3), &llvm::isDigit)))) {
+    Diag(Path[0].second, diag::err_invalid_module_name)
+        << Path[0].first << /*reserved*/ 1;
+    return nullptr;
+  }
+
+  // Then test all of the components in the path to see if any of them are
+  // using another kind of reserved or invalid identifier.
+  for (auto Part : Path) {
+    if (DiagReservedModuleName(*this, Part.first, Part.second))
+      return nullptr;
+  }
+
   // Flatten the dots in a module name. Unlike Clang's hierarchical module map
   // modules, the dots here are just another character that can appear in a
   // module name.
   std::string ModuleName = stringFromPath(Path);
   if (IsPartition) {
     ModuleName += ":";
     ModuleName += stringFromPath(Partition);
   }

clang/test/CodeGenCXX/cxx20-module-std-subst-1.cppm

 // RUN: %clang_cc1 -std=c++20 %s -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %s
 
 module;
 # 5 __FILE__ 1
 class Pooh;
 class Piglet;
 # 8 "" 2
 
+# 8 "" 1 3
 export module std; // might happen, you can't say it won't!
+# 9 "" 2 3
 
 namespace std {
 export template<typename T> class allocator {
 // just for testing, not real!
 void M (T *);
 template <typename U> U *N (T *);
 };
 

clang/test/Modules/pair-unambiguous-ctor.cppm

 // Test case reduced from an experimental std modules implementation.
 // Tests that the compiler don't emit confusing error about the ambiguous ctor
 // about std::pair.
 //
 // RUN: rm -fr %t
 // RUN: mkdir %t
 // RUN: split-file %s %t
 //
 // RUN: %clang_cc1 -std=c++20 %t/string.cppm -I%t -emit-module-interface -o %t/std-string.pcm
 // RUN: %clang_cc1 -std=c++20 %t/algorithm.cppm -I%t -emit-module-interface -o %t/std-algorithm.pcm
 // RUN: %clang_cc1 -std=c++20 %t/Use.cppm -I%t -fprebuilt-module-path=%t -emit-module-interface -verify -o %t/Use.pcm
 
 //--- Use.cppm
 // expected-no-diagnostics
 module;
 #include "config.h"
+# 3 "pair-unambiguous-ctor.cppm" 1 3
 export module std:M;
+# 3 "pair-unambiguous-ctor.cppm" 2 3
 import :string;
 import :algorithm;
 
 auto check() {
     return std::string();
 }
 
 //--- string.cppm
 module;
 #include "string.h"
+# 28 "pair-unambiguous-ctor.cppm" 1 3
 export module std:string;
 export namespace std {
     using std::string;
 }
+# 28 "pair-unambiguous-ctor.cppm" 2 3
 
 //--- algorithm.cppm
 module;
 #include "algorithm.h"
+# 38 "pair-unambiguous-ctor.cppm" 1 3
 export module std:algorithm;
+# 38 "pair-unambiguous-ctor.cppm" 2 3
 
 //--- pair.h
 namespace std __attribute__ ((__visibility__ ("default")))
 { 
   typedef long unsigned int size_t;
   typedef long int ptrdiff_t;
 
   typedef decltype(nullptr) nullptr_t;

clang/test/Modules/reserved-names-1.cpp

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+
+// expected-note@1 15{{add 'module;' to the start of the file to introduce a global module fragment}}
+
+module std;    // expected-error {{'std' is a reserved name for a module}}
+module _Test;  // expected-error {{'_Test' is a reserved name for a module}} \
+                  expected-error {{module declaration must occur at the start of the translation unit}}
+module module; // expected-error {{'module' is an invalid name for a module}} \
+                  expected-error {{module declaration must occur at the start of the translation unit}}
+module std0;   // expected-error {{'std0' is a reserved name for a module}} \
+                  expected-error {{module declaration must occur at the start of the translation unit}}
+
+export module module; // expected-error {{'module' is an invalid name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module import; // expected-error {{'import' is an invalid name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module _Test;  // expected-error {{'_Test' is a reserved name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module __test; // expected-error {{'__test' is a reserved name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module te__st; // expected-error {{'te__st' is a reserved name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module std;    // expected-error {{'std' is a reserved name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module std.foo;// expected-error {{'std' is a reserved name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module std0;   // expected-error {{'std0' is a reserved name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module std1000000; // expected-error {{'std1000000' is a reserved name for a module}} \
+                         expected-error {{module declaration must occur at the start of the translation unit}}
+export module should_fail._Test; // expected-error {{'_Test' is a reserved name for a module}} \
+                                    expected-error {{module declaration must occur at the start of the translation unit}}
+
+// Show that being in a system header doesn't save you from diagnostics about
+// use of an invalid module-name identifier.
+# 34 "reserved-names-1.cpp" 1 3
+export module module;       // expected-error {{'module' is an invalid name for a module}} \
+                               expected-error {{module declaration must occur at the start of the translation unit}}
+
+export module _Test.import; // expected-error {{'import' is an invalid name for a module}} \
+                               expected-error {{module declaration must occur at the start of the translation unit}}
+# 39 "reserved-names-1.cpp" 2 3
+
+// We can still use a reserved name on imoport.
+import std; // expected-error {{module 'std' not found}}

clang/test/Modules/reserved-names-2.cpp

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Demonstrate that we don't consider use of 'std' followed by digits to be a
+// reserved identifier if it is not the first part of the path.
+export module should_succeed.std0;

clang/test/Modules/reserved-names-3.cpp

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Demonstrate that we don't consider use of 'std' (potentially followed by
+// zero or more digits) to be a reserved identifier if it is not the only part
+// of the path.
+export module std12Three;

clang/test/Modules/reserved-names-4.cpp

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Demonstrate that we don't consider use of 'std' a reserved identifier if it
+// is not the first part of the path.
+export module should_succeed.std;
+

clang/test/Modules/reserved-names-system-header-1.cpp

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Show that we suppress the reserved identifier diagnostic in a system header.
+# 100 "file.cpp" 1 3  // Enter a system header
+export module std;
+# 100 "file.cpp" 2 3  // Leave the system header

clang/test/Modules/reserved-names-system-header-2.cpp

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Show that we suppress the reserved identifier diagnostic in a system header.
+# 100 "file.cpp" 1 3  // Enter a system header
+export module __test;
+# 100 "file.cpp" 2 3  // Leave the system header