[modules] Simplify and generalize the existing rule for finding hidden

declarations in redeclaration lookup. A declaration is now visible to
lookup if:

 * It is visible (not in a module, or in an imported module), or
 * We're doing redeclaration lookup and it's externally-visible, or
 * We're doing typo correction and looking for unimported decls.

We now support multiple modules having different internal-linkage or no-linkage
definitions of the same name for all entities, not just for functions,
variables, and some typedefs. As previously, if multiple such entities are
visible, any attempt to use them will result in an ambiguity error.

This patch fixes the linkage calculation for a number of entities where we
previously didn't need to get it right (using-declarations, namespace aliases,
and so on).  It also classifies enumerators as always having no linkage, which
is a slight deviation from the C++ standard's definition, but not an observable
change outside modules (this change is being discussed on the -core reflector
currently).

This also removes the prior special case for tag lookup, which made some cases
of this work, but also led to bizarre, bogus "must use 'struct' to refer to type
'Foo' in this scope" diagnostics in C++.

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

diff --git a/include/clang/Sema/Lookup.h b/include/clang/Sema/Lookup.h
index 1162a1d65abcb8474e1317006ee7c8bdbdd9c72d..f291a8aef0597711e6fc34393dab86016b89567b 100644 (file)
--- a/include/clang/Sema/Lookup.h
+++ b/include/clang/Sema/Lookup.h
@@ -139,8 +139,7 @@ public:
       Redecl(Redecl != Sema::NotForRedeclaration),
       HideTags(true),
       Diagnose(Redecl == Sema::NotForRedeclaration),
-      AllowHidden(Redecl == Sema::ForRedeclaration),
-      AllowHiddenInternal(AllowHidden),
+      AllowHidden(false),
       Shadowed(false)
   {
     configure();
@@ -162,8 +161,7 @@ public:
       Redecl(Redecl != Sema::NotForRedeclaration),
       HideTags(true),
       Diagnose(Redecl == Sema::NotForRedeclaration),
-      AllowHidden(Redecl == Sema::ForRedeclaration),
-      AllowHiddenInternal(AllowHidden),
+      AllowHidden(false),
       Shadowed(false)
   {
     configure();
@@ -184,7 +182,6 @@ public:
       HideTags(Other.HideTags),
       Diagnose(false),
       AllowHidden(Other.AllowHidden),
-      AllowHiddenInternal(Other.AllowHiddenInternal),
       Shadowed(false)
   {}
 
@@ -226,27 +223,16 @@ public:
   /// \brief Specify whether hidden declarations are visible, e.g.,
   /// for recovery reasons.
   void setAllowHidden(bool AH) {
-    AllowHiddenInternal = AllowHidden = AH;
-  }
-
-  /// \brief Specify whether hidden internal declarations are visible.
-  void setAllowHiddenInternal(bool AHI) {
-    AllowHiddenInternal = AHI;
+    AllowHidden = AH;
   }
 
   /// \brief Determine whether this lookup is permitted to see hidden
   /// declarations, such as those in modules that have not yet been imported.
   bool isHiddenDeclarationVisible(NamedDecl *ND) const {
-    // If a using-shadow declaration is hidden, it's never visible, not
-    // even to redeclaration lookup.
-    // FIXME: Should this apply to typedefs and namespace aliases too?
-    if (isa<UsingShadowDecl>(ND) && LookupKind != Sema::LookupUsingDeclName)
-      return false;
-    return (AllowHidden &&
-            (AllowHiddenInternal || ND->isExternallyVisible())) ||
-           LookupKind == Sema::LookupTagName;
+    return AllowHidden ||
+           (isForRedeclaration() && ND->isExternallyVisible());
   }
-  
+
   /// Sets whether tag declarations should be hidden by non-tag
   /// declarations during resolution.  The default is true.
   void setHideTags(bool Hide) {
@@ -317,7 +303,8 @@ public:
     if (!D->isInIdentifierNamespace(IDNS))
       return nullptr;
 
-    if (isHiddenDeclarationVisible(D) || isVisible(getSema(), D))
+    if (!D->isHidden() || isHiddenDeclarationVisible(D) ||
+        isVisibleSlow(getSema(), D))
       return D;
 
     return getAcceptableDeclSlow(D);
@@ -526,7 +513,6 @@ public:
   /// \brief Change this lookup's redeclaration kind.
   void setRedeclarationKind(Sema::RedeclarationKind RK) {
     Redecl = RK;
-    AllowHiddenInternal = AllowHidden = (RK == Sema::ForRedeclaration);
     configure();
   }
 
@@ -698,9 +684,6 @@ private:
   /// \brief True if we should allow hidden declarations to be 'visible'.
   bool AllowHidden;
 
-  /// \brief True if we should allow hidden internal declarations to be visible.
-  bool AllowHiddenInternal;
-
   /// \brief True if the found declarations were shadowed by some other
   /// declaration that we skipped. This only happens when \c LookupKind
   /// is \c LookupRedeclarationWithLinkage.

diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index 0f6e8b09706a69429df7f7d64f59ca304889f160..8c50af615cd01a71613e809438881638cfe05040 100644 (file)
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -635,6 +635,8 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
   if (D->isInAnonymousNamespace()) {
     const auto *Var = dyn_cast<VarDecl>(D);
     const auto *Func = dyn_cast<FunctionDecl>(D);
+    // FIXME: In C++11 onwards, anonymous namespaces should give decls
+    // within them internal linkage, not unique external linkage.
     if ((!Var || !isFirstInExternCContext(Var)) &&
         (!Func || !isFirstInExternCContext(Func)))
       return LinkageInfo::uniqueExternal();
@@ -821,10 +823,14 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
   } else if (isa<ObjCInterfaceDecl>(D)) {
     // fallout
 
+  } else if (auto *TD = dyn_cast<TypedefNameDecl>(D)) {
+    // A typedef declaration has linkage if it gives a type a name for
+    // linkage purposes.
+    if (!TD->getAnonDeclWithTypedefName(/*AnyRedecl*/true))
+      return LinkageInfo::none();
+
   // Everything not covered here has no linkage.
   } else {
-    // FIXME: A typedef declaration has linkage if it gives a type a name for
-    // linkage purposes.
     return LinkageInfo::none();
   }
 
@@ -1226,8 +1232,32 @@ static LinkageInfo computeLVForDecl(const NamedDecl *D,
   switch (D->getKind()) {
     default:
       break;
+
+    // Per C++ [basic.link]p2, only the names of objects, references,
+    // functions, types, templates, namespaces, and values ever have linkage.
+    //
+    // Note that the name of a typedef, namespace alias, using declaration,
+    // and so on are not the name of the corresponding type, namespace, or
+    // declaration, so they do *not* have linkage.
+    case Decl::EnumConstant: // FIXME: This has linkage, but that's dumb.
+    case Decl::ImplicitParam:
+    case Decl::Label:
+    case Decl::NamespaceAlias:
     case Decl::ParmVar:
+    case Decl::Using:
+    case Decl::UsingShadow:
+    case Decl::UsingDirective:
       return LinkageInfo::none();
+
+    case Decl::Typedef:
+    case Decl::TypeAlias:
+      // A typedef declaration has linkage if it gives a type a name for
+      // linkage purposes.
+      if (!cast<TypedefNameDecl>(D)
+               ->getAnonDeclWithTypedefName(/*AnyRedecl*/true))
+        return LinkageInfo::none();
+      break;
+
     case Decl::TemplateTemplateParm: // count these as external
     case Decl::NonTypeTemplateParm:
     case Decl::ObjCAtDefsField:

diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 728697e78f5d72ec10a051254eb8762dcc3f73db..a23d9debb861e40245137619bb17ef24ce4671ec 100644 (file)
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -4819,12 +4819,6 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
   LookupResult Previous(*this, NameInfo, LookupOrdinaryName,
                         ForRedeclaration);
 
-  // If we're hiding internal-linkage symbols in modules from redeclaration
-  // lookup, let name lookup know.
-  if ((getLangOpts().Modules || getLangOpts().ModulesLocalVisibility) &&
-      D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_typedef)
-    Previous.setAllowHiddenInternal(false);
-
   // See if this is a redefinition of a variable in the same scope.
   if (!D.getCXXScopeSpec().isSet()) {
     bool IsLinkageLookup = false;

diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp
index 74bffb2a044c14ab369348885050b60e415f1e8a..910ebcf05dde9fe571900a35716ece8f712aee05 100644 (file)
--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -7206,23 +7206,13 @@ Decl *Sema::ActOnStartNamespaceDef(Scope *NamespcScope,
     //   treated as an original-namespace-name.
     //
     // Since namespace names are unique in their scope, and we don't
-    // look through using directives, just look for any ordinary names.
-    
-    const unsigned IDNS = Decl::IDNS_Ordinary | Decl::IDNS_Member | 
-    Decl::IDNS_Type | Decl::IDNS_Using | Decl::IDNS_Tag | 
-    Decl::IDNS_Namespace;
-    NamedDecl *PrevDecl = nullptr;
-    DeclContext::lookup_result R = CurContext->getRedeclContext()->lookup(II);
-    for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E;
-         ++I) {
-      if ((*I)->getIdentifierNamespace() & IDNS) {
-        PrevDecl = *I;
-        break;
-      }
-    }
-    
+    // look through using directives, just look for any ordinary names
+    // as if by qualified name lookup.
+    LookupResult R(*this, II, IdentLoc, LookupOrdinaryName, ForRedeclaration);
+    LookupQualifiedName(R, CurContext->getRedeclContext());
+    NamedDecl *PrevDecl = R.getAsSingle<NamedDecl>();
     PrevNS = dyn_cast_or_null<NamespaceDecl>(PrevDecl);
-    
+
     if (PrevNS) {
       // This is an extended namespace definition.
       if (IsInline != PrevNS->isInline())

diff --git a/test/Index/linkage.c b/test/Index/linkage.c
index ab006590b61c0992694d1b23fdb6bfb2302df6cf..b0dcb30990a02fb028831d34bdc7a8607b4d4526 100644 (file)
--- a/test/Index/linkage.c
+++ b/test/Index/linkage.c
@@ -20,7 +20,7 @@ void f16(void) {
 
 
 // CHECK: EnumDecl=Baz:3:6 (Definition)linkage=External
-// CHECK: EnumConstantDecl=Qux:3:12 (Definition)linkage=External
+// CHECK: EnumConstantDecl=Qux:3:12 (Definition)linkage=NoLinkage
 // CHECK: VarDecl=x:4:5linkage=External
 // CHECK: FunctionDecl=foo:5:6linkage=External
 // CHECK: VarDecl=w:6:12linkage=Internal

diff --git a/test/Index/usrs.m b/test/Index/usrs.m
index fc3fbc9105789f6f9dc0a680a75f4d1c11eb19f9..aa0c4a04fc7e8f1216bea25caee0cff0a94153c7 100644 (file)
--- a/test/Index/usrs.m
+++ b/test/Index/usrs.m
@@ -119,7 +119,7 @@ int test_multi_declaration(void) {
 // CHECK: usrs.m c:@SA@MyStruct Extent=[15:9 - 18:2]
 // CHECK: usrs.m c:@SA@MyStruct@FI@wa Extent=[16:3 - 16:9]
 // CHECK: usrs.m c:@SA@MyStruct@FI@moo Extent=[17:3 - 17:10]
-// CHECK: usrs.m c:usrs.m@T@MyStruct Extent=[15:1 - 18:11]
+// CHECK: usrs.m c:@T@MyStruct Extent=[15:1 - 18:11]
 // CHECK: usrs.m c:@E@Pizza Extent=[20:1 - 23:2]
 // CHECK: usrs.m c:@E@Pizza@CHEESE Extent=[21:3 - 21:9]
 // CHECK: usrs.m c:@E@Pizza@MUSHROOMS Extent=[22:3 - 22:12]

diff --git a/test/Modules/Inputs/no-linkage/decls.h b/test/Modules/Inputs/no-linkage/decls.h
new file mode 100644 (file)
index 0000000..5c29044
--- /dev/null
+++ b/test/Modules/Inputs/no-linkage/decls.h
@@ -0,0 +1,6 @@
+namespace RealNS { int UsingDecl; }
+namespace NS = RealNS;
+typedef int Typedef;
+using AliasDecl = int;
+enum Enum { Enumerator };
+using RealNS::UsingDecl;

diff --git a/test/Modules/Inputs/no-linkage/empty.h b/test/Modules/Inputs/no-linkage/empty.h
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/test/Modules/Inputs/no-linkage/module.modulemap b/test/Modules/Inputs/no-linkage/module.modulemap
new file mode 100644 (file)
index 0000000..3931b0f
--- /dev/null
+++ b/test/Modules/Inputs/no-linkage/module.modulemap
@@ -0,0 +1 @@
+module M { module E { header "empty.h" } module D { header "decls.h" } }

diff --git a/test/Modules/decldef.m b/test/Modules/decldef.m
index 420c6774ecbaadf03c0ddc735210ef6246984eb9..784743ff6e79bdfcdb6513fa48d14ac1ca6f37fc 100644 (file)
--- a/test/Modules/decldef.m
+++ b/test/Modules/decldef.m
@@ -11,7 +11,13 @@ Def *def;
 #ifdef USE_EARLY
 A *a1; // expected-error{{declaration of 'A' must be imported from module 'decldef.Def' before it is required}}
 #endif
-B *b1; // expected-error{{must use 'struct' tag to refer to type 'B'}}
+B *b1;
+#ifdef USE_EARLY
+// expected-error@-2{{must use 'struct' tag to refer to type 'B'}}
+#else
+// expected-error@-4{{declaration of 'B' must be imported from module 'decldef.Decl' before it is required}}
+// expected-note@Inputs/decl.h:2 {{previous}}
+#endif
 @import decldef.Decl;
 
 A *a2;

diff --git a/test/Modules/merge-enumerators.cpp b/test/Modules/merge-enumerators.cpp
index 5267ac658b8be1a913a2e81d860aa375cc49fd8c..10e1914bd7b847b98dede73e1b034e3535ef9bf3 100644 (file)
--- a/test/Modules/merge-enumerators.cpp
+++ b/test/Modules/merge-enumerators.cpp
@@ -16,11 +16,13 @@
 
 #ifdef MERGE_LATE
 namespace N {
-  // FIXME: Should we accept this and reject the usage below due to ambiguity instead?
-  enum { A } a; // expected-error {{redefinition of enumerator 'A'}}
-  // expected-note@a.h:1 {{here}} (from module B.b)
+  enum { A } a; // expected-note {{candidate}}
+  // expected-note@a.h:1 {{candidate}} (from module B.b)
 }
 #include "a.h"
 #endif
 
 N::E e = N::A;
+#ifdef MERGE_LATE
+// expected-error@-2 {{ambiguous}}
+#endif

diff --git a/test/Modules/module-private.cpp b/test/Modules/module-private.cpp
index 6e723c863ce3dd9902a1157a56d2f6e3ca06ef78..42ab185760bb45bcf2cb4d479ca1d2767e291933 100644 (file)
--- a/test/Modules/module-private.cpp
+++ b/test/Modules/module-private.cpp
@@ -12,11 +12,7 @@ void test() {
 }
 
 int test_broken() {
-  HiddenStruct hidden; // \
-  // expected-error{{must use 'struct' tag to refer to type 'HiddenStruct' in this scope}} \
-  // expected-error{{definition of 'HiddenStruct' must be imported}}
-  // expected-note@Inputs/module_private_left.h:3 {{previous definition is here}}
-
+  HiddenStruct hidden; // expected-error{{unknown type name 'HiddenStruct'}}
   Integer i; // expected-error{{unknown type name 'Integer'}}
 
   int *ip = 0;

diff --git a/test/Modules/no-linkage.cpp b/test/Modules/no-linkage.cpp
new file mode 100644 (file)
index 0000000..508464e
--- /dev/null
+++ b/test/Modules/no-linkage.cpp
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fmodules-local-submodule-visibility -I%S/Inputs/no-linkage -fmodule-map-file=%S/Inputs/no-linkage/module.modulemap %s -verify
+
+#include "empty.h"
+
+namespace NS { int n; } // expected-note {{candidate}}
+struct Typedef { int n; }; // expected-note {{candidate}}
+int AliasDecl; // expected-note {{candidate}}
+enum AlsoAnEnum { Enumerator }; // expected-note {{candidate}}
+int UsingDecl; // expected-note {{candidate}}
+
+// expected-note@decls.h:2 {{candidate}}
+// expected-note@decls.h:3 {{candidate}}
+// expected-note@decls.h:4 {{candidate}}
+// expected-note@decls.h:5 {{candidate}}
+// expected-note@decls.h:6 {{candidate}}
+
+void use(int);
+void use_things() {
+  use(Typedef().n);
+  use(NS::n);
+  use(AliasDecl);
+  use(Enumerator);
+  use(UsingDecl);
+}
+
+#include "decls.h"
+
+void use_things_again() {
+  use(Typedef().n); // expected-error {{ambiguous}}
+  use(NS::n); // expected-error {{ambiguous}} expected-error{{'NS' is not a class, namespace, or enumeration}}
+  use(AliasDecl); // expected-error {{ambiguous}}
+  use(Enumerator); // expected-error {{ambiguous}}
+  use(UsingDecl); // expected-error {{ambiguous}}
+}

diff --git a/test/Modules/submodule-visibility-cycles.cpp b/test/Modules/submodule-visibility-cycles.cpp
index f26f6f21eea6c6927131e8cfeb4c80a865f7f8b5..a01fe562b14f5856a10683523726cc9ca53e48d4 100644 (file)
--- a/test/Modules/submodule-visibility-cycles.cpp
+++ b/test/Modules/submodule-visibility-cycles.cpp
@@ -3,7 +3,7 @@
 
 #include "cycle1.h"
 C1 c1;
-C2 c2; // expected-error {{must be imported}} expected-error {{}}
+C2 c2; // expected-error {{must be imported}}
 // expected-note@cycle2.h:6 {{here}}
 
 #include "cycle2.h"

diff --git a/test/Modules/submodules-merge-defs.cpp b/test/Modules/submodules-merge-defs.cpp
index bb8e69367cafbd0436ed0de2895cad80e9c20411..361f05b553e540bfd1f6f525a1a173a6597aa1b0 100644 (file)
--- a/test/Modules/submodules-merge-defs.cpp
+++ b/test/Modules/submodules-merge-defs.cpp
@@ -12,7 +12,7 @@
 #include "indirect.h"
 #endif
 
-A pre_a; // expected-error {{must use 'struct'}}
+A pre_a;
 #ifdef IMPORT_USE_2
 // expected-error-re@-2 {{must be imported from one of {{.*}}stuff.use{{.*}}stuff.use-2}}
 #elif EARLY_INDIRECT_INCLUDE
@@ -21,29 +21,28 @@ A pre_a; // expected-error {{must use 'struct'}}
 // expected-error@-6 {{must be imported from module 'stuff.use'}}
 #endif
 // expected-note@defs.h:1 +{{here}}
+extern class A pre_a2;
+int pre_use_a = use_a(pre_a2); // expected-error {{'A' must be imported}} expected-error {{'use_a' must be imported}}
 // expected-note@defs.h:2 +{{here}}
-int pre_use_a = use_a(pre_a); // expected-error {{'A' must be imported}} expected-error {{'use_a' must be imported}}
 
 B::Inner2 pre_bi; // expected-error +{{must be imported}}
 // expected-note@defs.h:4 +{{here}}
 // expected-note@defs.h:17 +{{here}}
-void pre_bfi(B b) { // expected-error {{must use 'class'}} expected-error +{{must be imported}}
-  b.f<int>(); // expected-error +{{must be imported}} expected-error +{{}}
-  // expected-note@defs.h:19 +{{here}}
+void pre_bfi(B b) { // expected-error +{{must be imported}}
+  b.f<int>(); // expected-error +{{}}
 }
 
 C_Base<1> pre_cb1; // expected-error +{{must be imported}}
 // expected-note@defs.h:23 +{{here}}
-C1 pre_c1; // expected-error +{{must be imported}} expected-error {{must use 'struct'}}
+C1 pre_c1; // expected-error +{{must be imported}}
 // expected-note@defs.h:25 +{{here}}
-C2 pre_c2; // expected-error +{{must be imported}} expected-error {{must use 'struct'}}
+C2 pre_c2; // expected-error +{{must be imported}}
 // expected-note@defs.h:26 +{{here}}
 
 D::X pre_dx; // expected-error +{{must be imported}}
 // expected-note@defs.h:28 +{{here}}
 // expected-note@defs.h:29 +{{here}}
-// FIXME: We should warn that use_dx is being used without being imported.
-int pre_use_dx = use_dx(pre_dx);
+int pre_use_dx = use_dx(pre_dx); // ignored; pre_dx is invalid
 
 int pre_e = E(0); // expected-error {{must be imported}}
 // expected-note@defs.h:32 +{{here}}
@@ -69,8 +68,9 @@ J<> pre_j; // expected-error {{declaration of 'J' must be imported}}
 #endif
 // expected-note@defs.h:58 +{{here}}
 
-ScopedEnum pre_scopedenum; // expected-error {{must be imported}} expected-error {{must use 'enum'}}
-// expected-note@defs.h:106 {{here}}
+ScopedEnum pre_scopedenum; // expected-error {{must be imported}}
+// expected-note@defs.h:105 0-1{{here}}
+// expected-note@defs.h:106 0-1{{here}}
 enum ScopedEnum : int;
 ScopedEnum pre_scopedenum_declared; // ok