[C++20] [Modules] Exit early if export decl is invalid

[ChuanqiXu]

Summary

See https://godbolt.org/z/Ka9foTov4, the following simple example would make the compiler crash:

export struct Unit {
    bool operator<(const Unit&);
};

The problem here is that we would set module ownership for the export decl even if we know it is invalid. And for decls who owns a module ownership, the Sema would assume there is a module for it. So here is the crash.

Another straight forward fix would be https://reviews.llvm.org/D117094. But I prefer the current revision. Since it didn't add new branch and it is more easy to read. I guess people reading D117094 without a background might be confusing. "Why is here a branch?". But this patch is more simple. We meet an error so we exit directly. So I prefer this one.

[aaron.ballman]

clang/lib/Sema/SemaModule.cpp

530–531

Am I understanding properly that this moved up here so that the for loop on line 553 can traverse the new context?

If so, can it be moved down to immediately before the for loop?

539–550

It seems a bit suspicious to me that we call D->getInvalidDecl() below within the for loop, but all the other places we leave it as a valid declaration despite it causing error diagnostics.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/lib/Sema/SemaModule.cpp

530–531

The intention to move these up is to make sure D could be put in the context even error detected. It wouldn't affect the traverse on line 556 since an ExportDecl wouldn't be a NamespaceDecl.

539–550

Yeah, it is intentional to call D->setInvalidDecl() this loop. It would suppress more diagnostic messages. But I feel it is good to call D->setInvalidDecl() in other places.

[aaron.ballman]

LGTM!

clang/lib/Sema/SemaModule.cpp

530–531

Ah yes, that makes far more sense. Thanks!

539–550

Thanks, I agree that we want to mark the decl invalid rather than leave it seeming valid.

clang/lib/Sema/SemaModule.cpp

 /// (if present).
 Decl *Sema::ActOnStartExportDecl(Scope *S, SourceLocation ExportLoc,
                                  SourceLocation LBraceLoc) {
   ExportDecl *D = ExportDecl::Create(Context, CurContext, ExportLoc);
 
   // Set this temporarily so we know the export-declaration was braced.
   D->setRBraceLoc(LBraceLoc);
 
+  CurContext->addDecl(D);
+  PushDeclContext(S, D);
+
   // C++2a [module.interface]p1:
   //   An export-declaration shall appear only [...] in the purview of a module
   //   interface unit. An export-declaration shall not appear directly or
   //   indirectly within [...] a private-module-fragment.
   if (ModuleScopes.empty() || !ModuleScopes.back().Module->isModulePurview()) {
     Diag(ExportLoc, diag::err_export_not_in_module_interface) << 0;
+    D->setInvalidDecl();
+    return D;
   } else if (!ModuleScopes.back().ModuleInterface) {
     Diag(ExportLoc, diag::err_export_not_in_module_interface) << 1;
     Diag(ModuleScopes.back().BeginLoc,
          diag::note_not_module_interface_add_export)
         << FixItHint::CreateInsertion(ModuleScopes.back().BeginLoc, "export ");
+    D->setInvalidDecl();
+    return D;
   } else if (ModuleScopes.back().Module->Kind ==
              Module::PrivateModuleFragment) {
     Diag(ExportLoc, diag::err_export_in_private_module_fragment);
     Diag(ModuleScopes.back().BeginLoc, diag::note_private_module_fragment);
+    D->setInvalidDecl();
+    return D;
   }
 
   for (const DeclContext *DC = CurContext; DC; DC = DC->getLexicalParent()) {
     if (const auto *ND = dyn_cast<NamespaceDecl>(DC)) {
       //   An export-declaration shall not appear directly or indirectly within
       //   an unnamed namespace [...]
       if (ND->isAnonymousNamespace()) {
         Diag(ExportLoc, diag::err_export_within_anonymous_namespace);
         Diag(ND->getLocation(), diag::note_anonymous_namespace);
         // Don't diagnose internal-linkage declarations in this region.
         D->setInvalidDecl();
-        break;
+        return D;
       }
 
       //   A declaration is exported if it is [...] a namespace-definition
       //   that contains an exported declaration.
       //
       // Defer exporting the namespace until after we leave it, in order to
       // avoid marking all subsequent declarations in the namespace as exported.
       if (!DeferredExportedNamespaces.insert(ND).second)
         break;
     }
   }
 
   //   [...] its declaration or declaration-seq shall not contain an
   //   export-declaration.
   if (auto *ED = getEnclosingExportDecl(D)) {
     Diag(ExportLoc, diag::err_export_within_export);
     if (ED->hasBraces())
       Diag(ED->getLocation(), diag::note_export);
+    D->setInvalidDecl();
+    return D;
   }
 
-  CurContext->addDecl(D);
-  PushDeclContext(S, D);
   D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::VisibleWhenImported);
   return D;
 }
 
 static bool checkExportedDeclContext(Sema &S, DeclContext *DC,
                                      SourceLocation BlockStart);
 
 namespace {

clang/test/Modules/export-in-non-modules.cpp

+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+export struct Unit { // expected-error {{export declaration can only be used within a module interface unit after the module declaration}}
+  bool operator<(const Unit &);
+};