[C++20][Modules] Implement P2615R1 revised export diagnostics.

[iains]

Summary

It has been reported that the current clang errors for, specifically,
static_assert in export contexts are a serious blocker to adoption of
modules in some cases.

There is also implementation divergence with GCC and MSVC allowing the
constructs mentioned below where clang currently rejects them with an
error.

The category of errors [for declarations in an exported context] is:
(unnamed, static_assert, empty and asm decls). These are now permitted
after P2615R1 which was approved by WG21 as a DR (and thus should be
applied to C++20 as well).

This patch removes these diagnostics and amends the testsuite accordingly.

[iains]

thanks to Daniela Engert for reporting this and helping identify the current WG21 status on the topic.

[ChuanqiXu]

LGTM with comments.

clang/lib/Sema/SemaModule.cpp

822–825

865–878

nit: we prefer shorter indentation.

872

A namespace decl may not be a enum decl.

[h-vetinari]

Mark P2615 as implemented in https://github.com/llvm/llvm-project/blame/main/clang/www/cxx_status.html?

[iains]

In D152946#4431974, @h-vetinari wrote:

Mark P2615 as implemented in https://github.com/llvm/llvm-project/blame/main/clang/www/cxx_status.html?

In D152946#4431974, @h-vetinari wrote:

Mark P2615 as implemented in https://github.com/llvm/llvm-project/blame/main/clang/www/cxx_status.html?

I did not do that yet since this patch only implements the removal of unnamed export diagnostics. There will be a follow-on patch shortly that implements the addition of the export specialisation diagnostics and then we can claim P2615R1.

[shafik]

clang/lib/Sema/SemaModule.cpp

835

Can you please add the quoted text for clarity. This is especially helpful if the text changes in the future and we are left trying to read the tea leaves to determine motivation.

[iains]

rebased, addressed review comments.

[iains]

not sure why the debian CI is reported clang-format errors; I am not seeing them here.

clang/lib/Sema/SemaModule.cpp

822–825

(actually I just moved this code to place it closer to the use-point).

you change is more efficient, indeed, but it alters the behaviour of the diagnostics such that only the first occurrence is reported (which regresses module.interface/p5.cpp).

It seems to me to be more user-friendly to report all the errors at the same time rather than one by one.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_module_decl_not_at_start : Error<
   "module declaration must occur at the start of the translation unit">;
 def note_global_module_introducer_missing : Note<
   "add 'module;' to the start of the file to introduce a "
   "global module fragment">;
 def err_export_within_anonymous_namespace : Error<
   "export declaration appears within anonymous namespace">;
 def note_anonymous_namespace : Note<"anonymous namespace begins here">;
-def ext_export_no_name_block : ExtWarn<
-  "ISO C++20 does not permit %select{an empty|a static_assert}0 declaration "
-  "to appear in an export block">, InGroup<ExportUnnamed>;
-def ext_export_no_names : ExtWarn<
-  "ISO C++20 does not permit a declaration that does not introduce any names "
-  "to be exported">, InGroup<ExportUnnamed>;
-def introduces_no_names : Error<
-  "declaration does not introduce any names to be exported">;
 def note_export : Note<"export block begins here">;
-def err_export_no_name : Error<
-  "%select{empty|static_assert|asm}0 declaration cannot be exported">;
-def ext_export_using_directive : ExtWarn<
-  "ISO C++20 does not permit using directive to be exported">,
-  InGroup<DiagGroup<"export-using-directive">>;
 def err_export_within_export : Error<
   "export declaration appears within another export declaration">;
+def err_export_anon_ns_internal : Error<
+  "anonymous namespaces cannot be exported">;
 def err_export_internal : Error<
   "declaration of %0 with internal linkage cannot be exported">;
 def err_export_using_internal : Error<
   "using declaration referring to %1 with %select{internal|module|unknown}0 "
   "linkage cannot be exported">;
 def err_export_not_in_module_interface : Error<
   "export declaration can only be used within a module purview">;
 def err_export_inline_not_defined : Error<

clang/lib/Sema/SemaModule.cpp

if (auto *ED = getEnclosingExportDecl(D)) {
     D->setInvalidDecl();
     return D;
   }
 
   D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::VisibleWhenImported);
   return D;
 }
 
-static bool checkExportedDeclContext(Sema &S, DeclContext *DC,
-                                     SourceLocation BlockStart);
-
-namespace {
-enum class UnnamedDeclKind {
-  Empty,
-  StaticAssert,
-  Asm,
-  UsingDirective,
-  Namespace,
-  Context
-};
-}
-
-static std::optional<UnnamedDeclKind> getUnnamedDeclKind(Decl *D) {
-  if (isa<EmptyDecl>(D))
-    return UnnamedDeclKind::Empty;
-  if (isa<StaticAssertDecl>(D))
-    return UnnamedDeclKind::StaticAssert;
-  if (isa<FileScopeAsmDecl>(D))
-    return UnnamedDeclKind::Asm;
-  if (isa<UsingDirectiveDecl>(D))
-    return UnnamedDeclKind::UsingDirective;
-  // Everything else either introduces one or more names or is ill-formed.
-  return std::nullopt;
-}
-
-unsigned getUnnamedDeclDiag(UnnamedDeclKind UDK, bool InBlock) {
-  switch (UDK) {
-  case UnnamedDeclKind::Empty:
-  case UnnamedDeclKind::StaticAssert:
-    // Allow empty-declarations and static_asserts in an export block as an
-    // extension.
-    return InBlock ? diag::ext_export_no_name_block : diag::err_export_no_name;
-
-  case UnnamedDeclKind::UsingDirective:
-    // Allow exporting using-directives as an extension.
-    return diag::ext_export_using_directive;
-
-  case UnnamedDeclKind::Namespace:
-    // Anonymous namespace with no content.
-    return diag::introduces_no_names;
-
-  case UnnamedDeclKind::Context:
-    // Allow exporting DeclContexts that transitively contain no declarations
-    // as an extension.
-    return diag::ext_export_no_names;
-
-  case UnnamedDeclKind::Asm:
-    return diag::err_export_no_name;
-  }
-  llvm_unreachable("unknown kind");
-}
+static bool checkExportedDecl(Sema &, Decl *, SourceLocation);
 
-static void diagExportedUnnamedDecl(Sema &S, UnnamedDeclKind UDK, Decl *D,
+/// Check that it's valid to export all the declarations in \p DC.
+static bool checkExportedDeclContext(Sema &S, DeclContext *DC,
                                      SourceLocation BlockStart) {
-  S.Diag(D->getLocation(), getUnnamedDeclDiag(UDK, BlockStart.isValid()))
-      << (unsigned)UDK;
-  if (BlockStart.isValid())
-    S.Diag(BlockStart, diag::note_export);
+  bool AllUnnamed = true;
+  for (auto *D : DC->decls())
+    AllUnnamed &= checkExportedDecl(S, D, BlockStart);
+  return AllUnnamed;
 }
 
 /// Check that it's valid to export \p D.
 static bool checkExportedDecl(Sema &S, Decl *D, SourceLocation BlockStart) {
-  // C++2a [module.interface]p3:
-  //   An exported declaration shall declare at least one name
-  if (auto UDK = getUnnamedDeclKind(D))
-    diagExportedUnnamedDecl(S, *UDK, D, BlockStart);
 
-  //   [...] shall not declare a name with internal linkage.
+  //  C++20 [module.interface]p3:
+  //   [...] it shall not declare a name with internal linkage.
   bool HasName = false;
   if (auto *ND = dyn_cast<NamedDecl>(D)) {
     // Don't diagnose anonymous union objects; we'll diagnose their members
     // instead.
     HasName = (bool)ND->getDeclName();
     if (HasName && ND->getFormalLinkage() == InternalLinkage) {
       S.Diag(ND->getLocation(), diag::err_export_internal) << ND;
       if (BlockStart.isValid())
         S.Diag(BlockStart, diag::note_export);
+      return false;
     }
   }
 
   // C++2a [module.interface]p5:
   //   all entities to which all of the using-declarators ultimately refer
   //   shall have been introduced with a name having external linkage
   if (auto *USD = dyn_cast<UsingShadowDecl>(D)) {
     NamedDecl *Target = USD->getUnderlyingDecl();
     Linkage Lk = Target->getFormalLinkage();
     if (Lk == InternalLinkage || Lk == ModuleLinkage) {
       S.Diag(USD->getLocation(), diag::err_export_using_internal)
           << (Lk == InternalLinkage ? 0 : 1) << Target;
       S.Diag(Target->getLocation(), diag::note_using_decl_target);
       if (BlockStart.isValid())
         S.Diag(BlockStart, diag::note_export);
+      return false;
     }
   }
 
   // Recurse into namespace-scope DeclContexts. (Only namespace-scope
-  // declarations are exported.).
+  // declarations are exported).
   if (auto *DC = dyn_cast<DeclContext>(D)) {
-    if (isa<NamespaceDecl>(D) && DC->decls().empty()) {
-      if (!HasName)
-        // We don't allow an empty anonymous namespace (we don't allow decls
-        // in them either, but that's handled in the recursion).
-        diagExportedUnnamedDecl(S, UnnamedDeclKind::Namespace, D, BlockStart);
-      // We allow an empty named namespace decl.
-    } else if (DC->getRedeclContext()->isFileContext() && !isa<EnumDecl>(D))
+    if (!isa<NamespaceDecl>(D))
+      return true;
+
+    if (auto *ND = dyn_cast<NamedDecl>(D)) {
+      if (!ND->getDeclName()) {
+        S.Diag(ND->getLocation(), diag::err_export_anon_ns_internal);
+        if (BlockStart.isValid())
+          S.Diag(BlockStart, diag::note_export);
+        return false;
+      } else if (!DC->decls().empty() &&
+                 DC->getRedeclContext()->isFileContext()) {
         return checkExportedDeclContext(S, DC, BlockStart);
       }
-  return false;
     }
-
-/// Check that it's valid to export all the declarations in \p DC.
-static bool checkExportedDeclContext(Sema &S, DeclContext *DC,
-                                     SourceLocation BlockStart) {
-  bool AllUnnamed = true;
-  for (auto *D : DC->decls())
-    AllUnnamed &= checkExportedDecl(S, D, BlockStart);
-  return AllUnnamed;
+  }
+  return true;
 }
 
 /// Complete the definition of an export declaration.
 Decl *Sema::ActOnFinishExportDecl(Scope *S, Decl *D, SourceLocation RBraceLoc) {
   auto *ED = cast<ExportDecl>(D);
   if (RBraceLoc.isValid())
     ED->setRBraceLoc(RBraceLoc);
 
   PopDeclContext();
 
   if (!D->isInvalidDecl()) {
     SourceLocation BlockStart =
         ED->hasBraces() ? ED->getBeginLoc() : SourceLocation();
     for (auto *Child : ED->decls()) {
-      if (checkExportedDecl(*this, Child, BlockStart)) {
-        // If a top-level child is a linkage-spec declaration, it might contain
-        // no declarations (transitively), in which case it's ill-formed.
-        diagExportedUnnamedDecl(*this, UnnamedDeclKind::Context, Child,
-                                BlockStart);
-      }
+      checkExportedDecl(*this, Child, BlockStart);
       if (auto *FD = dyn_cast<FunctionDecl>(Child)) {
         // [dcl.inline]/7
         // If an inline function or variable that is attached to a named module
         // is declared in a definition domain, it shall be defined in that
         // domain.
         // So, if the current declaration does not have a definition, we must
         // check at the end of the TU (or when the PMF starts) to see that we
         // have a definition at that point.

clang/test/CXX/module/module.interface/p3.cpp

-// RUN: %clang_cc1 -std=c++2a %s -verify -pedantic-errors
+// RUN: %clang_cc1 -std=c++20 %s -verify -pedantic-errors
 
+// As amended by P2615R1 applied as a DR against C++20.
 export module p3;
 
 namespace A { int ns_mem; } // expected-note 2{{target}}
 
 // An exported declaration shall declare at least one name.
-export; // expected-error {{empty declaration cannot be exported}}
-export static_assert(true); // expected-error {{static_assert declaration cannot be exported}}
-export using namespace A;   // expected-error {{ISO C++20 does not permit using directive to be exported}}
-
-export { // expected-note 3{{export block begins here}}
-  ; // expected-error {{ISO C++20 does not permit an empty declaration to appear in an export block}}
-  static_assert(true); // expected-error {{ISO C++20 does not permit a static_assert declaration to appear in an export block}}
-  using namespace A;   // expected-error {{ISO C++20 does not permit using directive to be exported}}
+export; // No diagnostic after P2615R1 DR
+export static_assert(true); // No diagnostic after P2615R1 DR
+export using namespace A;   // No diagnostic after P2615R1 DR
+
+export { // No diagnostic after P2615R1 DR
+  ; // No diagnostic after P2615R1 DR
+  static_assert(true); // No diagnostic after P2615R1 DR
+  using namespace A;   // No diagnostic after P2615R1 DR
 }
 
 export struct {}; // expected-error {{must be class member}} expected-error {{GNU extension}} expected-error {{does not declare anything}}
 export struct {} struct_;
 export union {}; // expected-error {{must be declared 'static'}} expected-error {{does not declare anything}}
 export union {} union_;
 export enum {}; // expected-error {{does not declare anything}}
 export enum {} enum_;
 export enum E : int;
 export typedef int; // expected-error {{typedef requires a name}}
 export static union {}; // expected-error {{does not declare anything}}
-export asm(""); // expected-error {{asm declaration cannot be exported}}
+export asm(""); // No diagnostic after P2615R1 DR
 export namespace B = A;
 export using A::ns_mem; // expected-error {{using declaration referring to 'ns_mem' with module linkage cannot be exported}}
 namespace A {
   export using A::ns_mem; // expected-error {{using declaration referring to 'ns_mem' with module linkage cannot be exported}}
 }
 export using Int = int;
-export extern "C++" {} // expected-error {{ISO C++20 does not permit a declaration that does not introduce any names to be exported}}
-export extern "C++" { extern "C" {} } // expected-error {{ISO C++20 does not permit a declaration that does not introduce any names to be exported}}
+export extern "C++" {} // No diagnostic after P2615R1 DR
+export extern "C++" { extern "C" {} } // No diagnostic after P2615R1 DR
 export extern "C++" { extern "C" int extern_c; }
-export { // expected-note {{export block}}
+export { // No diagnostic after P2615R1 DR
   extern "C++" int extern_cxx;
-  extern "C++" {} // expected-error {{ISO C++20 does not permit a declaration that does not introduce any names to be exported}}
+  extern "C++" {} // No diagnostic after P2615R1 DR
 }
-export [[]]; // FIXME (bad diagnostic text): expected-error {{empty declaration cannot be exported}}
-export [[example::attr]]; // FIXME: expected-error {{empty declaration cannot be exported}} expected-warning {{unknown attribute 'attr'}}
+export [[]]; // No diagnostic after P2615R1 DR
+export [[example::attr]]; // expected-warning {{unknown attribute 'attr'}}
 
 // [...] shall not declare a name with internal linkage
 export static int a; // expected-error {{declaration of 'a' with internal linkage cannot be exported}}
 export static int b(); // expected-error {{declaration of 'b' with internal linkage cannot be exported}}
-export namespace { int c; } // expected-error {{declaration of 'c' with internal linkage cannot be exported}}
+export namespace { } // expected-error {{anonymous namespaces cannot be exported}}
+export namespace { int c; } // expected-error {{anonymous namespaces cannot be exported}}
 namespace { // expected-note {{here}}
   export int d; // expected-error {{export declaration appears within anonymous namespace}}
 }
 export template<typename> static int e; // expected-error {{declaration of 'e' with internal linkage cannot be exported}}
 export template<typename> static int f(); // expected-error {{declaration of 'f' with internal linkage cannot be exported}}
 export const int k = 5;
 export static union { int n; }; // expected-error {{declaration of 'n' with internal linkage cannot be exported}}

clang/test/Modules/cxx20-10-2-ex1.cpp

 
 //--- std-10-2-ex1-tu1.cpp
 module;
 
 #include "std-10-2-ex1.h"
 // expected-error@std-10-2-ex1.h:* {{export declaration can only be used within a module purview}}
 
 export module M1;
-export namespace {} // expected-error {{declaration does not introduce any names to be exported}}
-export namespace {
-int a1; // expected-error {{declaration of 'a1' with internal linkage cannot be exported}}
+export namespace {} // expected-error {{anonymous namespaces cannot be exported}}
+export namespace { // expected-error {{anonymous namespaces cannot be exported}}
+int a1;
 }
 namespace {    // expected-note {{anonymous namespace begins here}}
 export int a2; // expected-error {{export declaration appears within anonymous namespace}}
 }
 export static int b; // expected-error {{declaration of 'b' with internal linkage cannot be exported}}
 export int f();      // OK
 
 export namespace N {}     // namespace N
-export using namespace N; // expected-error {{ISO C++20 does not permit using directive to be exported}}
+export using namespace N; // No diagnostic after P2615R1 DR

clang/test/Modules/cxx20-10-2-ex7.cpp

 // Based on C++20 10.2 example 6.
 
 // RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o %t
 
+// expected-no-diagnostics
+
 export module M;
 export namespace N {
 int x;                 // OK
-static_assert(1 == 1); // expected-error {{static_assert declaration cannot be exported}}
+static_assert(1 == 1); // No diagnostic after P2615R1 DR
 } // namespace N

clang/test/SemaCXX/modules.cppm

 } t;
 #elif TEST == 3
 int use_a = a; // expected-error {{use of undeclared identifier 'a'}}
 
 #undef foo
 import foo; // expected-error {{imports must immediately follow the module declaration}}
 
 export {}
-export {  // expected-note {{begins here}}
-  ;       // expected-warning {{ISO C++20 does not permit an empty declaration to appear in an export block}}
+export {
+  ;       // No diagnostic after P2615R1 DR
 }
-export {               // expected-note {{begins here}}
-  static_assert(true); // expected-warning {{ISO C++20 does not permit a static_assert declaration to appear in an export block}}
+export {
+  static_assert(true); // No diagnostic after P2615R1 DR
 }
 
 int use_b = b; // expected-error{{use of undeclared identifier 'b'}}
 int use_n = n; // FIXME: this should not be visible, because it is not exported
 
 extern int n;
 static_assert(&n != p); // expected-error{{use of undeclared identifier 'p'}}
 #endif