From: Richard Smith <richard-llvm@metafoo.co.uk>
Date: Wed, 12 Sep 2018 02:13:48 +0000 (+0000)
Subject: Track definition merging on the canonical declaration even when local
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=78f85fba22f07782b114526040654c5498fef78e;p=clang

Track definition merging on the canonical declaration even when local
submodule visibility is disabled.

Attempting to pick a specific declaration to make visible when the
module containing the merged declaration becomes visible is error-prone,
as we don't yet know which declaration we'll choose to be the definition
when we are informed of the merging.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342019 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
index 0ed52ffd35..b107e8bd2a 100644
--- a/lib/AST/ASTContext.cpp
+++ b/lib/AST/ASTContext.cpp
@@ -932,10 +932,7 @@ void ASTContext::mergeDefinitionIntoModule(NamedDecl *ND, Module *M,
     if (auto *Listener = getASTMutationListener())
       Listener->RedefinedHiddenDefinition(ND, M);
 
-  if (getLangOpts().ModulesLocalVisibility)
-    MergedDefModules[cast<NamedDecl>(ND->getCanonicalDecl())].push_back(M);
-  else
-    ND->setVisibleDespiteOwningModule();
+  MergedDefModules[cast<NamedDecl>(ND->getCanonicalDecl())].push_back(M);
 }
 
 void ASTContext::deduplicateMergedDefinitonsFor(NamedDecl *ND) {
diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
index 900ccb18c7..819f9ef8a4 100644
--- a/lib/Sema/SemaType.cpp
+++ b/lib/Sema/SemaType.cpp
@@ -7623,8 +7623,15 @@ bool Sema::hasVisibleDefinition(NamedDecl *D, NamedDecl **Suggested,
 
     // A visible module might have a merged definition instead.
     if (D->isModulePrivate() ? hasMergedDefinitionInCurrentModule(D)
-                             : hasVisibleMergedDefinition(D))
+                             : hasVisibleMergedDefinition(D)) {
+      if (CodeSynthesisContexts.empty() &&
+          !getLangOpts().ModulesLocalVisibility) {
+        // Cache the fact that this definition is implicitly visible because
+        // there is a visible merged definition.
+        D->setVisibleDespiteOwningModule();
+      }
       return true;
+    }
 
     return false;
   };
diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp
index b60e5f14a6..ac3b737401 100644
--- a/lib/Serialization/ASTReader.cpp
+++ b/lib/Serialization/ASTReader.cpp
@@ -3780,22 +3780,15 @@ void ASTReader::makeModuleVisible(Module *Mod,
 /// visible.
 void ASTReader::mergeDefinitionVisibility(NamedDecl *Def,
                                           NamedDecl *MergedDef) {
-  // FIXME: This doesn't correctly handle the case where MergedDef is visible
-  // in modules other than its owning module. We should instead give the
-  // ASTContext a list of merged definitions for Def.
   if (Def->isHidden()) {
     // If MergedDef is visible or becomes visible, make the definition visible.
     if (!MergedDef->isHidden())
       Def->setVisibleDespiteOwningModule();
-    else if (getContext().getLangOpts().ModulesLocalVisibility) {
+    else {
       getContext().mergeDefinitionIntoModule(
           Def, MergedDef->getImportedOwningModule(),
           /*NotifyListeners*/ false);
       PendingMergedDefinitionsToDeduplicate.insert(Def);
-    } else {
-      auto SubmoduleID = MergedDef->getOwningModuleID();
-      assert(SubmoduleID && "hidden definition in no module");
-      HiddenNamesMap[getSubmodule(SubmoduleID)].push_back(Def);
     }
   }
 }
diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp
index 00fad25eea..8295107476 100644
--- a/lib/Serialization/ASTReaderDecl.cpp
+++ b/lib/Serialization/ASTReaderDecl.cpp
@@ -4419,22 +4419,9 @@ void ASTDeclReader::UpdateDecl(Decl *D,
     case UPD_DECL_EXPORTED: {
       unsigned SubmoduleID = readSubmoduleID();
       auto *Exported = cast<NamedDecl>(D);
-      if (auto *TD = dyn_cast<TagDecl>(Exported))
-        Exported = TD->getDefinition();
       Module *Owner = SubmoduleID ? Reader.getSubmodule(SubmoduleID) : nullptr;
-      if (Reader.getContext().getLangOpts().ModulesLocalVisibility) {
-        Reader.getContext().mergeDefinitionIntoModule(cast<NamedDecl>(Exported),
-                                                      Owner);
-        Reader.PendingMergedDefinitionsToDeduplicate.insert(
-            cast<NamedDecl>(Exported));
-      } else if (Owner && Owner->NameVisibility != Module::AllVisible) {
-        // If Owner is made visible at some later point, make this declaration
-        // visible too.
-        Reader.HiddenNamesMap[Owner].push_back(Exported);
-      } else {
-        // The declaration is now visible.
-        Exported->setVisibleDespiteOwningModule();
-      }
+      Reader.getContext().mergeDefinitionIntoModule(Exported, Owner);
+      Reader.PendingMergedDefinitionsToDeduplicate.insert(Exported);
       break;
     }
 
diff --git a/test/Modules/merge-template-pattern-visibility-3.cpp b/test/Modules/merge-template-pattern-visibility-3.cpp
new file mode 100644
index 0000000000..9ac1b6699d
--- /dev/null
+++ b/test/Modules/merge-template-pattern-visibility-3.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fmodules -emit-llvm-only %s -verify
+
+#pragma clang module build A
+module A {}
+#pragma clang module contents
+#pragma clang module begin A
+template<typename T> void f(const T&) { T::error; }
+#pragma clang module end
+#pragma clang module endbuild
+
+#pragma clang module build B
+module B {}
+#pragma clang module contents
+#pragma clang module begin B
+template<typename T> void f(const T&) { T::error; }
+#pragma clang module end
+#pragma clang module endbuild
+
+#pragma clang module build C
+module C {}
+#pragma clang module contents
+#pragma clang module begin C
+#pragma clang module load B
+template<typename T> void f(const T&) { T::error; }
+#pragma clang module end
+#pragma clang module endbuild
+
+#pragma clang module load A
+inline void f() {}
+void x() { f(); }
+
+#pragma clang module import C
+// expected-error@* {{cannot be used prior to}}
+void y(int n) { f(n); } // expected-note {{instantiation of}}