Downgrade reserved module identifier error into a warning

[aaron.ballman]

Summary

Any project that wants to import std; potentially needs to be able to build a module that does export std;. We silenced the error diagnostic if the module identified itself as a system header, but this isn't quite good enough, what we really need is a way to identify a system module. It would be nice for that feature to be shared among the major implementations, so this downgrades the diagnostic from an error to a warning temporarily to give implementers time to determine what that mechanism will look like. We may convert this warning back into an error in a future release of Clang, but it's not guaranteed we will do so.

Fixes https://github.com/llvm/llvm-project/issues/61446

[dblaikie]

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Yeah, I'd treat them equally - while we could subset the reserved names and allow implementations to only use a subset (while leaving the rest as an error for both implementations and consumers alike) that doesn't feel in keeping with the purpose of these names - to be usable by /someone/ and so necessary to allow them to be used.

(hmm - there's some discussion in the description about the fact that this error was already suppressed in "system headers" - why was that suppression inadequate for system implementation modules? (& does that suppression for reserved names risk being over-broad, since every third party library installed on a system is generally considered a "system header", even if they aren't part of the implementation?))

[aaron.ballman]

In D146986#4225121, @dblaikie wrote:

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Yeah, I'd treat them equally - while we could subset the reserved names and allow implementations to only use a subset (while leaving the rest as an error for both implementations and consumers alike) that doesn't feel in keeping with the purpose of these names - to be usable by /someone/ and so necessary to allow them to be used.

(hmm - there's some discussion in the description about the fact that this error was already suppressed in "system headers" - why was that suppression inadequate for system implementation modules? (& does that suppression for reserved names risk being over-broad, since every third party library installed on a system is generally considered a "system header", even if they aren't part of the implementation?))

We currently use line markers to "enter" a system header and that's quite fragile. I mentioned we could use #pragma clang system_header, but @ChuanqiXu didn't think that was appropriate because these are not headers, they're modules, and we should have some separation between "system headers" and "system modules". (https://github.com/llvm/llvm-project/issues/61446#issuecomment-1473029776) As for being over-broad, it might be, but this is the approach we usually take (anything that's a "system header" is considered special and gets less diagnostics because the user isn't typically able to change the contents of the header file anyway).

[dblaikie]

In D146986#4225192, @aaron.ballman wrote:

In D146986#4225121, @dblaikie wrote:

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Yeah, I'd treat them equally - while we could subset the reserved names and allow implementations to only use a subset (while leaving the rest as an error for both implementations and consumers alike) that doesn't feel in keeping with the purpose of these names - to be usable by /someone/ and so necessary to allow them to be used.

(hmm - there's some discussion in the description about the fact that this error was already suppressed in "system headers" - why was that suppression inadequate for system implementation modules? (& does that suppression for reserved names risk being over-broad, since every third party library installed on a system is generally considered a "system header", even if they aren't part of the implementation?))

We currently use line markers to "enter" a system header and that's quite fragile. I mentioned we could use #pragma clang system_header, but @ChuanqiXu didn't think that was appropriate because these are not headers, they're modules, and we should have some separation between "system headers" and "system modules". (https://github.com/llvm/llvm-project/issues/61446#issuecomment-1473029776) As for being over-broad, it might be, but this is the approach we usually take (anything that's a "system header" is considered special and gets less diagnostics because the user isn't typically able to change the contents of the header file anyway).

Presumably adding an alias for #pragma clang system_header called system_module wouldn't be too hard? (though the pragma is also being removed from libc++ soon, I think, in favor of -isystem usage, so maybe that's a sign the pragma's not a great way to do)

Presumably the line marker issue would be less significant for a module? Since there's no complex line marking, inclusion, etc, going on - it's just where the actual .cppm file is located/how it's found? (though yeah, that might get weird for building .pcms - since you're going to name the source file directly on the command line, it's not going to be found via any isystem lookup, etc... )

[ChuanqiXu]

Presumably adding an alias for #pragma clang system_header called system_module wouldn't be too hard? (though the pragma is also being removed from libc++ soon, I think, in favor of -isystem usage, so maybe that's a sign the pragma's not a great way to do)

Yeah, it shouldn't be hard but it looks not good too. I feel the current patch is better/simpler as a workaround.

[aaron.ballman]

In D146986#4225275, @dblaikie wrote:

Presumably adding an alias for #pragma clang system_header called system_module wouldn't be too hard? (though the pragma is also being removed from libc++ soon, I think, in favor of -isystem usage, so maybe that's a sign the pragma's not a great way to do)

It wouldn't be hard to add, but the modules group wanted to see if we can find some common feature that all the implementations implement instead of relying on different extensions for different compilers, IIUC. So that could be a pragma, or it could be an attribute attached to the module name, etc.

Presumably the line marker issue would be less significant for a module? Since there's no complex line marking, inclusion, etc, going on - it's just where the actual .cppm file is located/how it's found? (though yeah, that might get weird for building .pcms - since you're going to name the source file directly on the command line, it's not going to be found via any isystem lookup, etc... )

The trouble is maintenance. Someone ran into a problem where they added code to the module and then diagnostics started showing up on the wrong lines because of the line marker.

[iains]

+1 for this as an interim solution.

[Mordante]

Sorry I was not available earlier. Thanks for working on this. LGTM but one remark.

clang/docs/ReleaseNotes.rst

92

Note this is about the standard modules, which are not precompiled per se.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

92

Thanks! I fixed this up on the Clang 17 branch in 711b70d77e99dd4101b9dfa94d9ea0d4149997ab

clang/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^
 - Support for out-of-line definitions of constrained templates has been improved.
   This partially fixes `#49620 <https://github.com/llvm/llvm-project/issues/49620>`_.
 - Lambda templates with a requires clause directly after the template parameters now parse
   correctly if the requires clause consists of a variable with a dependent type.
   (`#61278 <https://github.com/llvm/llvm-project/issues/61278>`_)
 - Announced C++20 Coroutines is fully supported on all targets except Windows, which
   still has some stability and ABI issues.
+- Downgraded use of a reserved identifier in a module export declaration from
+  an error to a warning under the ``-Wreserved-module-identifier`` warning
+  group. This warning is enabled by default. This addresses `#61446
+  <https://github.com/llvm/llvm-project/issues/61446>`_ and allows easier
+  building of precompiled modules. This diagnostic may be strengthened into an
+  error again in the future once there is a less fragile way to mark a module
+  as being part of the implementation rather than a user module.
 
 C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 - Implemented `P2036R3: Change scope of lambda trailing-return-type <https://wg21.link/P2036R3>`_
   and `P2579R0 Mitigation strategies for P2036 <https://wg21.link/P2579R0>`_.
   These proposals modify how variables captured in lambdas can appear in trailing return type
   expressions and how their types are deduced therein, in all C++ language versions.

clang/include/clang/Basic/DiagnosticGroups.td

 //        not just deprecated) or not?
 def GCCWriteStrings : DiagGroup<"write-strings" , [WritableStrings]>;
 
 def CharSubscript : DiagGroup<"char-subscripts">;
 def LargeByValueCopy : DiagGroup<"large-by-value-copy">;
 def DuplicateArgDecl : DiagGroup<"duplicate-method-arg">;
 def SignedEnumBitfield : DiagGroup<"signed-enum-bitfield">;
 
+def ReservedModuleIdentifier : DiagGroup<"reserved-module-identifier">;
 def ReservedIdentifier : DiagGroup<"reserved-identifier",
-    [ReservedIdAsMacro]>;
+    [ReservedIdAsMacro, ReservedModuleIdentifier]>;
 
 // Unreachable code warning groups.
 //
 //  The goal is make -Wunreachable-code on by default, in -Wall, or at
 //  least actively used, with more noisy versions of the warning covered
 //  under separate flags.
 //
 def UnreachableCodeLoopIncrement : DiagGroup<"unreachable-code-loop-increment">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_reserved_extern_symbol: Warning<
   "identifier %0 is reserved because %select{"
   "<ERROR>|" // ReservedIdentifierStatus::NotReserved
   "it starts with '_' at global scope|"
   "it starts with '_' and has C language linkage|"
   "it starts with '__'|"
   "it starts with '_' followed by a capital letter|"
   "it contains '__'}1">,
   InGroup<ReservedIdentifier>, DefaultIgnore;
+def warn_reserved_module_name : Warning<
+  "%0 is a reserved name for a module">, InGroup<ReservedModuleIdentifier>;
 
 def warn_parameter_size: Warning<
   "%0 is a large (%1 bytes) pass-by-value argument; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;
 def warn_return_value_size: Warning<
   "return value of %0 is a large (%1 bytes) pass-by-value object; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;
 def warn_return_value_udt: Warning<

 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 err_invalid_module_name : Error<"%0 is an invalid name for a module">;
 def err_extern_def_in_header_unit : Error<
   "non-inline external definitions are not permitted in C++ header units">;
 
 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">;

clang/lib/Sema/SemaModule.cpp

else if (II->isReserved(S.getLangOpts()) !=
     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;
-  }
+  switch (Reason) {
+  case Valid:
     return false;
+  case Invalid:
+    return S.Diag(Loc, diag::err_invalid_module_name) << II;
+  case Reserved:
+    return S.Diag(Loc, diag::warn_reserved_module_name) << II;
+  }
+  llvm_unreachable("fell off a fully covered switch");
 }
 
 Sema::DeclGroupPtrTy
 Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc,
                       ModuleDeclKind MDK, ModuleIdPath Path,
                       ModuleIdPath Partition, ModuleImportState &ImportState) {
   assert(getLangOpts().CPlusPlusModules &&
          "should only have module decl in standard C++ modules");
Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc,
 
   // 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;
+    Diag(Path[0].second, diag::warn_reserved_module_name) << Path[0].first;
     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;

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

-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %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}} \
+module std;    // loud-warning {{'std' is a reserved name for a module}}
+module _Test;  // loud-warning {{'_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}} \
+module std0;   // loud-warning {{'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}} \
+export module _Test;  // loud-warning {{'_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}} \
+export module __test; // loud-warning {{'__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}} \
+export module te__st; // loud-warning {{'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}} \
+export module std;    // loud-warning {{'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}} \
+export module std.foo;// loud-warning {{'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}} \
+export module std0;   // loud-warning {{'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}} \
+export module std1000000; // loud-warning {{'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}} \
+export module should_diag._Test; // loud-warning {{'_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}}