[clang][sema] Allow unnamed decls in C++20 module export{} blocks

[tbaeder]

Summary

As part of P1766R1, allow unnamed decls in export{} blocks.

I've adjusted the module-ts tests as well, but I am (generally) not sure whether code for C++20 modules should affect the modules-ts cases. What's the policy here?

[tbaeder]

Reading this again, it sounds like this is not expected to happen for C++20; I was using https://clang.llvm.org/cxx_status.html#cxx20 as guidance here.

[tbaeder]

Limited the change to c++2b

[ChuanqiXu]

clang/lib/Sema/SemaModule.cpp

651–653

Should we add a cite to P1766R1 here?

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

12–16

This one should be deleted unintentionally.

[tbaeder]

clang/lib/Sema/SemaModule.cpp

651–653

I wanted to do that, but I'm not 100% sure how. P1766R1 doesn't seem to change any wording for this change. What exactly do I cite?

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

12–16

Right, I forgot about that. Added it back, thanks.

[ChuanqiXu]

clang/lib/Sema/SemaModule.cpp

651–653

I am not sure about the standard method. But I guess it might not be bad to write something like:

// P1766R1 removed the rule that a declaration in an export block introduces at least one name. The rule for the single-declaration form of export is retained.
// FIXME: Cite for labels when it shows in the draft.

BTW, you could send a pull request or an issue at: https://github.com/cplusplus/draft to do the change. Since P1766R1 is adopted, so this change should be editorial.

[tbaeder]

clang/lib/Sema/SemaModule.cpp

651–653

Thanks, I added a comment and opened https://github.com/cplusplus/draft/issues/5237

[ChuanqiXu]

Oh, it looks like we couldn't go on since the part of PR1766R1 is not adopted: https://github.com/cplusplus/draft/issues/5237

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 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">,

clang/lib/Sema/SemaModule.cpp

static llvm::Optional<UnnamedDeclKind> getUnnamedDeclKind(Decl *D) {
   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 llvm::None;
 }
 
-unsigned getUnnamedDeclDiag(UnnamedDeclKind UDK, bool InBlock) {
+static unsigned getUnnamedDeclDiag(UnnamedDeclKind UDK) {
   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;
+    return diag::err_export_no_name;
 
   case UnnamedDeclKind::UsingDirective:
     // Allow exporting using-directives as an extension.
     return diag::ext_export_using_directive;
 
   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 void diagExportedUnnamedDecl(Sema &S, UnnamedDeclKind UDK, Decl *D,
                                     SourceLocation BlockStart) {
-  S.Diag(D->getLocation(), getUnnamedDeclDiag(UDK, BlockStart.isValid()))
-      << (unsigned)UDK;
+  S.Diag(D->getLocation(), getUnnamedDeclDiag(UDK)) << (unsigned)UDK;
   if (BlockStart.isValid())
     S.Diag(BlockStart, diag::note_export);
 }
 
 /// 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 (!BlockStart.isValid()) {
     if (auto UDK = getUnnamedDeclKind(D))
       diagExportedUnnamedDecl(S, *UDK, D, BlockStart);
+  }
 
   //   [...] shall not declare a name with internal linkage.
   if (auto *ND = dyn_cast<NamedDecl>(D)) {
     // Don't diagnose anonymous union objects; we'll diagnose their members
     // instead.
     if (ND->getDeclName() && ND->getFormalLinkage() == InternalLinkage) {
       S.Diag(ND->getLocation(), diag::err_export_internal) << ND;
       if (BlockStart.isValid())

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

 // RUN: %clang_cc1 -std=c++2a %s -verify -pedantic-errors
 
 export module p3;
 
 namespace A { int ns_mem; }
 
 // 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 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}}

clang/test/Modules/cxx20-export.cpp

+
+// RUN: %clang_cc1 -std=c++20 -fmodules -verify %s
+export module testmodule;
+
+export static_assert(true); // expected-error {{static_assert declaration cannot be exported}}
+
+namespace A {};
+
+export {
+#define STR(x) constexpr char x[] = #x;
+  STR(foo);
+#undef STR
+
+  static_assert(true);
+
+  template <typename T> struct X { T t; };
+  template <typename T> X(T) -> X<T>;
+
+  ; // Empty declaration
+
+  using namespace A;
+}

clang/test/SemaCXX/modules-ts.cppm

 #elif TEST == 3
 int use_a = a; // expected-error {{declaration of 'a' must be imported from module 'foo' before it is required}}
 // expected-note@-14 {{declaration here is not visible}}
 
 #undef foo
 import foo;
 
 export {} // expected-error {{export declaration cannot be empty}}
-export {  // expected-note {{begins here}}
-  ;       // expected-warning {{ISO C++20 does not permit an empty declaration to appear in an export block}}
-}
-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);
 }
 
 int use_b = b;
 int use_n = n; // FIXME: this should not be visible, because it is not exported
 
 extern int n;
 static_assert(&n != p);
 #endif