[PATCH] D41627: [Modules TS] Fix namespace visibility

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: rsmith, bruno, boris.

Namespaces without an explicit `export` were being mistakenly marked as module private.
This patch fixes the visibility as per `[basic.namespace]p1` and adds a test case with previously rejected (yet valid) code.


https://reviews.llvm.org/D41627

Files:
  include/clang/AST/DeclBase.h
  lib/AST/DeclBase.cpp
  lib/Sema/SemaDeclCXX.cpp
  test/CXX/modules-ts/basic/basic.namespace/p1.cpp


Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===================================================================
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+  export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8623,6 +8623,19 @@
   // for the namespace has the declarations that showed up in that particular
   // namespace definition.
   PushDeclContext(NamespcScope, Namespc);
+
+  // A namespace with external linkage should always be visible when imported,
+  // regardless of whether or not it's explicitly exported.
+  if (getLangOpts().ModulesTS &&
+      !Namespc->isAnonymousNamespace() && !Namespc->isInAnonymousNamespace()) {
+    const Module *CurrentModule = getCurrentModule();
+    assert(CurrentModule);
+    Namespc->setModuleOwnershipKind(
+        CurrentModule->Kind == Module::ModuleKind::GlobalModuleFragment
+            ? Decl::ModuleOwnershipKind::Visible
+            : Decl::ModuleOwnershipKind::VisibleWhenImported);
+  }
+
   return Namespc;
 }
 
Index: lib/AST/DeclBase.cpp
===================================================================
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -281,6 +281,23 @@
 // Out-of-line virtual method providing a home for Decl.
 Decl::~Decl() = default;
 
+Decl::ModuleOwnershipKind Decl::getModuleOwnershipKindForChildOf(DeclContext *DC) {
+  // Ignore namespaces because they're visible by default.
+  while (DC && DC->isNamespace())
+    DC = DC->getParent();
+
+  if (auto *D = cast_or_null<Decl>(DC)) {
+    auto MOK = D->getModuleOwnershipKind();
+    if (MOK != ModuleOwnershipKind::Unowned &&
+        (!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
+      return MOK;
+    // If D is not local and we have no local module storage, then we don't
+    // need to track module ownership at all.
+  }
+
+  return ModuleOwnershipKind::Unowned;
+}
+
 void Decl::setDeclContext(DeclContext *DC) {
   DeclCtx = DC;
 }
Index: include/clang/AST/DeclBase.h
===================================================================
--- include/clang/AST/DeclBase.h
+++ include/clang/AST/DeclBase.h
@@ -349,18 +349,7 @@
 
   /// Get the module ownership kind to use for a local lexical child of \p DC,
   /// which may be either a local or (rarely) an imported declaration.
-  static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC) {
-    if (DC) {
-      auto *D = cast<Decl>(DC);
-      auto MOK = D->getModuleOwnershipKind();
-      if (MOK != ModuleOwnershipKind::Unowned &&
-          (!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
-        return MOK;
-      // If D is not local and we have no local module storage, then we don't
-      // need to track module ownership at all.
-    }
-    return ModuleOwnershipKind::Unowned;
-  }
+  static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC);
 
 protected:
   Decl(Kind DK, DeclContext *DC, SourceLocation L)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41627.128327.patch
Type: text/x-patch
Size: 3393 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171229/d94a1425/attachment.bin>