diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -377,6 +377,9 @@ `Issue 58673 `_. - Better diagnostics when the user has missed `auto` in a declaration. `Issue 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 ------------------------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11207,6 +11207,8 @@ "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">, diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp --- a/clang/lib/Sema/SemaModule.cpp +++ b/clang/lib/Sema/SemaModule.cpp @@ -144,6 +144,37 @@ 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, @@ -238,6 +269,32 @@ } } + // 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. diff --git a/clang/test/CodeGenCXX/cxx20-module-std-subst-1.cppm b/clang/test/CodeGenCXX/cxx20-module-std-subst-1.cppm --- a/clang/test/CodeGenCXX/cxx20-module-std-subst-1.cppm +++ b/clang/test/CodeGenCXX/cxx20-module-std-subst-1.cppm @@ -6,7 +6,9 @@ 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 class allocator { diff --git a/clang/test/Modules/pair-unambiguous-ctor.cppm b/clang/test/Modules/pair-unambiguous-ctor.cppm --- a/clang/test/Modules/pair-unambiguous-ctor.cppm +++ b/clang/test/Modules/pair-unambiguous-ctor.cppm @@ -14,7 +14,9 @@ // 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; @@ -25,15 +27,19 @@ //--- 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"))) diff --git a/clang/test/Modules/reserved-names-1.cpp b/clang/test/Modules/reserved-names-1.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Modules/reserved-names-1.cpp @@ -0,0 +1,45 @@ +// 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}} diff --git a/clang/test/Modules/reserved-names-2.cpp b/clang/test/Modules/reserved-names-2.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Modules/reserved-names-2.cpp @@ -0,0 +1,6 @@ +// 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; diff --git a/clang/test/Modules/reserved-names-3.cpp b/clang/test/Modules/reserved-names-3.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Modules/reserved-names-3.cpp @@ -0,0 +1,7 @@ +// 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; diff --git a/clang/test/Modules/reserved-names-4.cpp b/clang/test/Modules/reserved-names-4.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Modules/reserved-names-4.cpp @@ -0,0 +1,7 @@ +// 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; + diff --git a/clang/test/Modules/reserved-names-system-header-1.cpp b/clang/test/Modules/reserved-names-system-header-1.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Modules/reserved-names-system-header-1.cpp @@ -0,0 +1,7 @@ +// 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 diff --git a/clang/test/Modules/reserved-names-system-header-2.cpp b/clang/test/Modules/reserved-names-system-header-2.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Modules/reserved-names-system-header-2.cpp @@ -0,0 +1,7 @@ +// 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