[Modules TS] Diagnose exported internal linkage declarations

[hamzasood]

Summary

Diagnose attempts to export declarations with internal linkage, as mentioned in [dcl.module.interface]p2 in the Modules TS.

I would've liked to add a FixIt hint to remove the static keyword if present, but I couldn't work out how to get the SourceRange that covers it (it's accessible from the Declarator, but seemingly not after the actual node has been created).
I've left a fixme comment in there in case anyone else can figure it out.

[hamzasood]

I've removed the isExported fix for namespaces as it's somewhat unrelated to this patch.
I'll do a separate patch for fixing namespaces (which will include the stuff removed from here and a bit more)

[rsmith]

include/clang/AST/DeclBase.h

2072–2092

Neat :)

lib/Sema/SemaDecl.cpp

17121

This will prematurely stop traversal if we reach a LinkageSpecDecl. I think we should instead stop if we reach a declaration for which !DC->getRedeclContext()->isFileContext() (that is, we should check namespace-scope declarations only).

[hamzasood]

• Fixed the premature loop termination issue pointed out by @rsmith (and added a test case for it).
• Fixed a few small issues regarding diagnosing exported enum/struct/union declarations without a name (especially anonymous unions at namespace-scope).
• Factored out the loop body into a separate function to keep things tidy.

include/clang/AST/DeclBase.h

 #define LLVM_CLANG_AST_DECLBASE_H
 
 #include "clang/AST/AttrIterator.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/GraphTraits.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/VersionTuple.h"

 
 template<class FromTy>
 struct cast_convert_val< const ::clang::DeclContext, FromTy*, FromTy*> {
   static const ::clang::DeclContext *doit(const FromTy *Val) {
     return FromTy::castToDeclContext(Val);
   }
 };
 
+template<>
+struct GraphTraits<const ::clang::DeclContext *> {
+  using NodeRef = const ::clang::Decl *;
+  using ChildIteratorType = ::clang::DeclContext::decl_iterator;
+
+  static NodeRef getEntryNode(const ::clang::DeclContext *DC) {
+    return cast<::clang::Decl>(DC);
+  }
+
+  static ChildIteratorType child_begin(NodeRef D) {
+    if (auto *DC = dyn_cast<::clang::DeclContext>(D))
+      return DC->decls_begin();
+    return ChildIteratorType();
+  }
+
+  static ChildIteratorType child_end(NodeRef D) {
+    if (auto *DC = dyn_cast<::clang::DeclContext>(D))
+      return DC->decls_end();
+    return ChildIteratorType();
+  }
+};
+
 } // namespace llvm
 
 #endif // LLVM_CLANG_AST_DECLBASE_H

include/clang/Basic/DiagnosticSemaKinds.td

 
 // C++ Modules TS
 def err_export_within_export : Error<
   "export declaration appears within another export declaration">;
 def err_export_not_in_module_interface : Error<
   "export declaration can only be used within a module interface unit after "
   "the module declaration">;
 
+def err_export_anonymous : Error<
+  "%select{unnamed namespaces|anonymous unions at namespace or global scope}0"
+  " cannot be exported">;
+def err_export_internal_name : Error<
+  "%0 cannot be exported since it has internal linkage">;
+def note_internal_from_unnamed_namespace : Note<
+  "internal linkage is due to inclusion in this unnamed namespace">;
+
 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">;
 
 def note_redefinition_modules_same_file : Note<
   "'%0' included multiple times, additional include site in header from module '%1'">;

lib/Sema/SemaDecl.cpp

 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
+#include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Triple.h"
 #include <algorithm>
 #include <cstring>
 #include <functional>
 
 using namespace clang;
 using namespace sema;
void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation Loc,
   TU->addDecl(ImportD);
   Consumer.HandleImplicitImportDecl(ImportD);
 
   // Make the module visible.
   getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, Loc);
   VisibleModules.setVisible(Mod, Loc);
 }
 
+static const NamespaceDecl *findNearestEnclosingAnonymousNamespace(const Decl *D) {
+  for (auto *DC = D->getDeclContext(); DC; DC = DC->getParent()) {
+    if (auto *Namespace = dyn_cast<NamespaceDecl>(DC)) {
+      if (Namespace->isAnonymousNamespace())
+        return Namespace;
+    }
+  }
+  return nullptr;
+}
+
+/// Check whether it's okay for the given namespace-scope decl to be exported.
+/// An appropriate diagnostic is emitted in the erroneous cases.
+static bool validateExportedDecl(Sema &SemaRef, const Decl *D) {
+  assert(D);
+  assert(D->getDeclContext()->getRedeclContext()->isFileContext() &&
+         "Declaration isn't at namespace-scope");
+
+  // Anonymous namespaces have internal linkage; they can never be exported.
+  if (auto *Namespace = dyn_cast<NamespaceDecl>(D)) {
+    if (Namespace->isAnonymousNamespace()) {
+      SemaRef.Diag(Namespace->getLocStart(), diag::err_export_anonymous)
+          << 0/* namespace */;
+      return false;
+    }
+  }
+
+  // Namespace-scope anonymous unions must be declared as static, and so they
+  // can't be exported. This is special-cased because the variable name will be
+  // empty, which makes for a confusing diagnostic.
+  if (auto *Var = dyn_cast<VarDecl>(D)) {
+    QualType Ty = Var->getType();
+    auto *Record = Ty.isNull()
+                       ? nullptr
+                       : dyn_cast_or_null<RecordDecl>(Ty->getAsTagDecl());
+    if (Record && Record->isAnonymousStructOrUnion()) {
+      // Anonymous structs are also treated as erroneous but they're left
+      // undiagnosed; they aren't valid at namespace-scope anyway so an
+      // additional diagnostic would be pointless.
+      if (Record->isUnion()) {
+        SemaRef.Diag(Record->getLocStart(), diag::err_export_anonymous)
+            << 1/* union */;
+      }
+      return false;
+    }
+  }
+
+  // Ignore the injected fields from an anonymous union; we've already diagnosed
+  // the union variable itself.
+  if (isa<IndirectFieldDecl>(D))
+    return false;
+
+  // Diagnose any other named declaration with internal linkage.
+  // Note that UsingDirectiveDecl inherits from NamedDecl even though they
+  // aren't actually named, so ignore those.
+  if (auto *ND = dyn_cast<NamedDecl>(D)) {
+    if (!isa<UsingDirectiveDecl>(ND) &&
+        (!isa<TagDecl>(ND) || cast<TagDecl>(ND)->hasNameForLinkage()) &&
+        ND->getFormalLinkage() == InternalLinkage) {
+      SemaRef.Diag(ND->getLocation(),
+                   diag::err_export_internal_name) << ND;
+
+      const bool IsExplicitlyStatic = [ND] {
+        if (auto *Var = dyn_cast<VarDecl>(ND))
+          return Var->getStorageClass() == SC_Static;
+        if (auto *Func = dyn_cast<FunctionDecl>(ND))
+          return Func->getStorageClass() == SC_Static;
+        return false;
+      }();
+
+      if (IsExplicitlyStatic) {
+        // FIXME: Add a fixit hint to remove the static keyword.
+      } else if (auto *AnonNS = findNearestEnclosingAnonymousNamespace(ND)) {
+        SemaRef.Diag(AnonNS->getLocStart(),
+                     diag::note_internal_from_unnamed_namespace);
+      }
+
+      return false;
+    }
+  }
+
+  return true;
+}
+
 /// We have parsed the start of an export declaration, including the '{'
 /// (if present).
 Decl *Sema::ActOnStartExportDecl(Scope *S, SourceLocation ExportLoc,
                                  SourceLocation LBraceLoc) {
   ExportDecl *D = ExportDecl::Create(Context, CurContext, ExportLoc);
 
   // C++ Modules TS draft:
   //   An export-declaration shall appear in the purview of a module other than

 }
 
 /// 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);
 
-  // FIXME: Diagnose export of internal-linkage declaration (including
-  // anonymous namespace).
+  // Traverse the subtree of exported nodes to ensure that they all have external
+  // linkage, and emit an appropriate diagnostic if that isn't the case.
+  auto It  = llvm::df_iterator<const DeclContext *>::begin(ED),
+       End = llvm::df_iterator<const DeclContext *>::end(ED);
+  ++It; // Skip the ExportDecl
+  for (bool VisitChildren; It != End; VisitChildren ? ++It : It.skipChildren()) {
+    const Decl *D = *It;
+    if (validateExportedDecl(*this, D)) {
+      // We only need to check namespace-scope declarations.
+      VisitChildren = isa<DeclContext>(D) &&
+                      cast<DeclContext>(D)->getRedeclContext()->isFileContext();
+    } else {
+      // Skip the children of erroneously exported nodes to avoid "diagnostic
+      // bombing" when a namespace with internal linkage is exported.
+      VisitChildren = false;
+    }
+  }
 
   PopDeclContext();
   return D;
 }
 
 void Sema::ActOnPragmaRedefineExtname(IdentifierInfo* Name,
                                       IdentifierInfo* AliasName,
                                       SourceLocation PragmaLoc,

test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp

 // RUN: %clang_cc1 -fmodules-ts %S/module.cppm -triple %itanium_abi_triple -emit-module-interface -o %t
 // RUN: %clang_cc1 -fmodules-ts %s -triple %itanium_abi_triple -fmodule-file=%t -emit-llvm -o - | FileCheck %s --implicit-check-not=unused --implicit-check-not=global_module
 
 // CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global
 // CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = available_externally {{(dso_local )?}}global i32 0,
 // CHECK-DAG: @const_var_exported = available_externally {{(dso_local )?}}constant i32 3,
 //
 // CHECK-DAG: @_ZW6ModuleE25extern_var_module_linkage = external {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6ModuleE25inline_var_module_linkage = linkonce_odr {{(dso_local )?}}global
-// CHECK-DAG: @_ZW6ModuleE25static_var_module_linkage = available_externally {{(dso_local )?}}global i32 0,
 // CHECK-DAG: @_ZW6ModuleE24const_var_module_linkage = available_externally {{(dso_local )?}}constant i32 3,
 
 module Module;
 
 void use() {
   // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv
   used_inline_exported();
   // CHECK: declare {{.*}}@_Z18noninline_exportedv
   noninline_exported();
 
   (void)&extern_var_exported;
   (void)&inline_var_exported;
-  (void)&static_var_exported; // FIXME: Should not be exported.
   (void)&const_var_exported;
 
   // FIXME: This symbol should not be visible here.
   // CHECK: declare {{.*}}@_ZW6ModuleE26used_static_module_linkagev
   used_static_module_linkage();
 
   // CHECK: define linkonce_odr {{.*}}@_ZW6ModuleE26used_inline_module_linkagev
   used_inline_module_linkage();
 
   // CHECK: declare {{.*}}@_ZW6ModuleE24noninline_module_linkagev
   noninline_module_linkage();
 
   (void)&extern_var_module_linkage;
   (void)&inline_var_module_linkage;
-  (void)&static_var_module_linkage; // FIXME: Should not be visible here.
   (void)&const_var_module_linkage;
 }

test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm

 // RUN: %clang_cc1 -fmodules-ts %s -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %s --implicit-check-not unused_inline --implicit-check-not unused_stastic_global_module
 
 // CHECK-DAG: @extern_var_global_module = external {{(dso_local )?}}global
 // CHECK-DAG: @inline_var_global_module = linkonce_odr {{(dso_local )?}}global
 // CHECK-DAG: @_ZL24static_var_global_module = internal global
 // CHECK-DAG: @_ZL23const_var_global_module = internal constant
 //
 // For ABI compatibility, these symbols do not include the module name.
 // CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global
 // FIXME: Should this be 'weak_odr global'? Presumably it must be, since we
 // can discard this global and its initializer (if any), and other TUs are not
 // permitted to run the initializer for this variable.
 // CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = {{(dso_local )?}}global
 // CHECK-DAG: @const_var_exported = {{(dso_local )?}}constant
 //
 // CHECK-DAG: @_ZW6ModuleE25extern_var_module_linkage = external {{(dso_local )?}}global
 // FIXME: Should this be 'weak_odr global'? Presumably it must be, since we
 // can discard this global and its initializer (if any), and other TUs are not
 // permitted to run the initializer for this variable.
 // CHECK-DAG: @_ZW6ModuleE25inline_var_module_linkage = linkonce_odr {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6ModuleE25static_var_module_linkage = {{(dso_local )?}}global
void noninline_global_module() {
   (void)&inline_var_global_module;
   (void)&static_var_global_module;
   (void)&const_var_global_module;
 }
 
 export module Module;
 
 export {
-  // FIXME: These should be ill-formed: you can't export an internal linkage
-  // symbol, per [dcl.module.interface]p2.
-  // CHECK: define {{(dso_local )?}}void {{.*}}@_ZW6ModuleE22unused_static_exportedv
-  static void unused_static_exported() {}
-  // CHECK: define {{(dso_local )?}}void {{.*}}@_ZW6ModuleE20used_static_exportedv
-  static void used_static_exported() {}
-
   inline void unused_inline_exported() {}
   inline void used_inline_exported() {}
 
   extern int extern_var_exported;
   inline int inline_var_exported;
-  // FIXME: This should be ill-formed: you can't export an internal linkage
-  // symbol.
-  static int static_var_exported;
   const int const_var_exported = 3;
 
   // CHECK: define {{(dso_local )?}}void {{.*}}@_Z18noninline_exportedv
   void noninline_exported() {
-    used_static_exported();
     // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv
     used_inline_exported();
 
     (void)&extern_var_exported;
     (void)&inline_var_exported;
-    (void)&static_var_exported;
     (void)&const_var_exported;
   }
 }
 
 // FIXME: Ideally we wouldn't emit this as its name is not visible outside this
 // TU, but this module interface might contain a template that can use this
 // function so we conservatively emit it for now.
 // CHECK: define {{(dso_local )?}}void {{.*}}@_ZW6ModuleE28unused_static_module_linkagev

test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp

 // RUN: %clang_cc1 -fmodules-ts %S/module.cppm -triple %itanium_abi_triple -emit-module-interface -o %t
 // RUN: %clang_cc1 -fmodules-ts %s -triple %itanium_abi_triple -fmodule-file=%t -emit-llvm -o - | FileCheck %s --implicit-check-not=unused --implicit-check-not=global_module
 
 // CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global
 // CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = available_externally {{(dso_local )?}}global i32 0
 // CHECK-DAG: @const_var_exported = available_externally {{(dso_local )?}}constant i32 3
 
 import Module;
 
 void use() {
   // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv
   used_inline_exported();
   // CHECK: declare {{.*}}@_Z18noninline_exportedv
   noninline_exported();
 
   (void)&extern_var_exported;
   (void)&inline_var_exported;
-  (void)&static_var_exported;
   (void)&const_var_exported;
 
   // Module-linkage declarations are not visible here.
 }

test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp

+// RUN: %clang_cc1 -fmodules-ts %s -emit-module-interface -verify -o /dev/null
+
+export module A;
+
+export namespace { } // expected-error {{unnamed namespace}}
+
+export static int n = 5; // expected-error {{internal linkage}}
+
+namespace { // expected-note 5 {{in this}}
+  export {
+    int a = 1;    // expected-error {{internal linkage}}
+    void f() { }  // expected-error {{internal linkage}}
+    class B { };  // expected-error {{internal linkage}}
+    struct { } x; // expected-error {{internal linkage}}
+
+    extern "C++" void g() { } // expected-error {{internal linkage}}
+  }
+}
+
+export namespace a {
+  namespace b {
+    namespace c {
+      static int i = 3; // expected-error {{internal linkage}}
+    }
+  }
+}

test/SemaCXX/anonymous-union-export.cpp

 // RUN: %clang_cc1 -std=c++17 -fmodules-ts -emit-obj -verify -o %t.pcm %s
 
 export module M;
+
 export {
-    union { bool a; }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}}
+    union { bool a; }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}} \
+                       // expected-error{{anonymous unions at namespace or global scope cannot be exported}}
+
+    static union { bool a; }; // expected-error{{anonymous unions at namespace or global scope cannot be exported}}
+}
+
+namespace {
+    export union { bool a; }; // expected-error{{anonymous unions at namespace or global scope cannot be exported}}
 }