[modules] Rationalize the behavior of Decl::declarationReplaces, and in

particular don't assume that two declarations of the same kind in the same
context are declaring the same entity. That's not true when the same name is
declared multiple times as internal-linkage symbols within a module.
(getCanonicalDecl is cheap now, so we can just use it here.)

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

diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index 098298f0c596702a32a716511e2ad53427f0943b..7485b12614d3a192a427f88ec47acd1dca3ec282 100644 (file)
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -1451,32 +1451,6 @@ void NamedDecl::getNameForDiagnostic(raw_ostream &OS,
     printName(OS);
 }
 
-static bool isKindReplaceableBy(Decl::Kind OldK, Decl::Kind NewK) {
-  // For method declarations, we never replace.
-  if (ObjCMethodDecl::classofKind(NewK))
-    return false;
-
-  if (OldK == NewK)
-    return true;
-
-  // A compatibility alias for a class can be replaced by an interface.
-  if (ObjCCompatibleAliasDecl::classofKind(OldK) &&
-      ObjCInterfaceDecl::classofKind(NewK))
-    return true;
-
-  // A typedef-declaration, alias-declaration, or Objective-C class declaration
-  // can replace another declaration of the same type. Semantic analysis checks
-  // that we have matching types.
-  if ((TypedefNameDecl::classofKind(OldK) ||
-       ObjCInterfaceDecl::classofKind(OldK)) &&
-      (TypedefNameDecl::classofKind(NewK) ||
-       ObjCInterfaceDecl::classofKind(NewK)))
-    return true;
-
-  // Otherwise, a kind mismatch implies that the declaration is not replaced.
-  return false;
-}
-
 template<typename T> static bool isRedeclarableImpl(Redeclarable<T> *) {
   return true;
 }
@@ -1500,9 +1474,19 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const {
   if (OldD->isFromASTFile() && isFromASTFile())
     return false;
 
-  if (!isKindReplaceableBy(OldD->getKind(), getKind()))
+  // A kind mismatch implies that the declaration is not replaced.
+  if (OldD->getKind() != getKind())
     return false;
 
+  // For method declarations, we never replace. (Why?)
+  if (isa<ObjCMethodDecl>(this))
+    return false;
+
+  // For parameters, pick the newer one. This is either an error or (in
+  // Objective-C) permitted as an extension.
+  if (isa<ParmVarDecl>(this))
+    return true;
+
   // Inline namespaces can give us two declarations with the same
   // name and kind in the same scope but different contexts; we should
   // keep both declarations in this case.
@@ -1510,28 +1494,8 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const {
           OldD->getDeclContext()->getRedeclContext()))
     return false;
 
-  if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(this))
-    // For function declarations, we keep track of redeclarations.
-    // FIXME: This returns false for functions that should in fact be replaced.
-    // Instead, perform some kind of type check?
-    if (FD->getPreviousDecl() != OldD)
-      return false;
-
-  // For function templates, the underlying function declarations are linked.
-  if (const FunctionTemplateDecl *FunctionTemplate =
-          dyn_cast<FunctionTemplateDecl>(this))
-    return FunctionTemplate->getTemplatedDecl()->declarationReplaces(
-        cast<FunctionTemplateDecl>(OldD)->getTemplatedDecl());
-
-  // Using shadow declarations can be overloaded on their target declarations
-  // if they introduce functions.
-  // FIXME: If our target replaces the old target, can we replace the old
-  //        shadow declaration?
-  if (auto *USD = dyn_cast<UsingShadowDecl>(this))
-    if (USD->getTargetDecl() != cast<UsingShadowDecl>(OldD)->getTargetDecl())
-      return false;
-
-  // Using declarations can be overloaded if they introduce functions.
+  // Using declarations can be replaced if they import the same name from the
+  // same context.
   if (auto *UD = dyn_cast<UsingDecl>(this)) {
     ASTContext &Context = getASTContext();
     return Context.getCanonicalNestedNameSpecifier(UD->getQualifier()) ==
@@ -1546,13 +1510,20 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const {
   }
 
   // UsingDirectiveDecl's are not really NamedDecl's, and all have same name.
-  // We want to keep it, unless it nominates same namespace.
+  // They can be replaced if they nominate the same namespace.
+  // FIXME: Is this true even if they have different module visibility?
   if (auto *UD = dyn_cast<UsingDirectiveDecl>(this))
     return UD->getNominatedNamespace()->getOriginalNamespace() ==
            cast<UsingDirectiveDecl>(OldD)->getNominatedNamespace()
                ->getOriginalNamespace();
 
-  if (!IsKnownNewer && isRedeclarable(getKind())) {
+  if (isRedeclarable(getKind())) {
+    if (getCanonicalDecl() != OldD->getCanonicalDecl())
+      return false;
+
+    if (IsKnownNewer)
+      return true;
+
     // Check whether this is actually newer than OldD. We want to keep the
     // newer declaration. This loop will usually only iterate once, because
     // OldD is usually the previous declaration.
@@ -1567,11 +1538,16 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const {
       if (D->isCanonicalDecl())
         return false;
     }
+
+    // It's a newer declaration of the same kind of declaration in the same
+    // scope: we want this decl instead of the existing one.
+    return true;
   }
 
-  // It's a newer declaration of the same kind of declaration in the same scope,
-  // and not an overload: we want this decl instead of the existing one.
-  return true;
+  // In all other cases, we need to keep both declarations in case they have
+  // different visibility. Any attempt to use the name will result in an
+  // ambiguity if more than one is visible.
+  return false;
 }
 
 bool NamedDecl::hasLinkage() const {

diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 622ff1ebbce5550346dfd49cdeddb94835aff53d..e188094f90abf720cd1da0efce27f1a7a7454e88 100644 (file)
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -3352,7 +3352,7 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
       !TemplateParameterListsAreEqual(NewTemplate->getTemplateParameters(),
                                       OldTemplate->getTemplateParameters(),
                                       /*Complain=*/true, TPL_TemplateMatch))
-    return;
+    return New->setInvalidDecl();
 
   // C++ [class.mem]p1:
   //   A member shall not be declared twice in the member-specification [...]
@@ -8220,7 +8220,7 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
     // there's no more work to do here; we'll just add the new
     // function to the scope.
     if (!AllowOverloadingOfFunction(Previous, Context)) {
-      NamedDecl *Candidate = Previous.getFoundDecl();
+      NamedDecl *Candidate = Previous.getRepresentativeDecl();
       if (shouldLinkPossiblyHiddenDecl(Candidate, NewFD)) {
         Redeclaration = true;
         OldDecl = Candidate;

diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
index 6fbe0da8c901baaa1414342df5a8bdf1316a2b85..0ba34ea8ef2511918d672f8fbd5f4c4ec5093994 100644 (file)
--- a/lib/Sema/SemaLookup.cpp
+++ b/lib/Sema/SemaLookup.cpp
@@ -474,7 +474,7 @@ void LookupResult::resolveKind() {
     D = cast<NamedDecl>(D->getCanonicalDecl());
 
     // Ignore an invalid declaration unless it's the only one left.
-    if (D->isInvalidDecl() && I < N-1) {
+    if (D->isInvalidDecl() && !(I == 0 && N == 1)) {
       Decls[I] = Decls[--N];
       continue;
     }

diff --git a/test/Modules/Inputs/internal-constants/a.h b/test/Modules/Inputs/internal-constants/a.h
new file mode 100644 (file)
index 0000000..d288138
--- /dev/null
+++ b/test/Modules/Inputs/internal-constants/a.h
@@ -0,0 +1,3 @@
+#pragma once
+#include "const.h"
+inline int f() { return N::k; }

diff --git a/test/Modules/Inputs/internal-constants/b.h b/test/Modules/Inputs/internal-constants/b.h
new file mode 100644 (file)
index 0000000..679603a
--- /dev/null
+++ b/test/Modules/Inputs/internal-constants/b.h
@@ -0,0 +1,3 @@
+#pragma once
+#include "const.h"
+inline int g() { return N::k; }

diff --git a/test/Modules/Inputs/internal-constants/c.h b/test/Modules/Inputs/internal-constants/c.h
new file mode 100644 (file)
index 0000000..43a37f8
--- /dev/null
+++ b/test/Modules/Inputs/internal-constants/c.h
@@ -0,0 +1,3 @@
+#pragma once
+#include "a.h"
+inline int h() { return N::k; }

diff --git a/test/Modules/Inputs/internal-constants/const.h b/test/Modules/Inputs/internal-constants/const.h
new file mode 100644 (file)
index 0000000..e2dc8e1
--- /dev/null
+++ b/test/Modules/Inputs/internal-constants/const.h
@@ -0,0 +1,3 @@
+namespace N {
+  const int k = 5;
+}

diff --git a/test/Modules/Inputs/internal-constants/module.modulemap b/test/Modules/Inputs/internal-constants/module.modulemap
new file mode 100644 (file)
index 0000000..6d471f5
--- /dev/null
+++ b/test/Modules/Inputs/internal-constants/module.modulemap
@@ -0,0 +1,6 @@
+module X {
+  textual header "const.h"
+  module A { header "a.h" export * }
+  module B { header "b.h" export * }
+  module C { header "c.h" export * }
+}

diff --git a/test/Modules/internal-constants.cpp b/test/Modules/internal-constants.cpp
new file mode 100644 (file)
index 0000000..f95e95c
--- /dev/null
+++ b/test/Modules/internal-constants.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodules-local-submodule-visibility -I%S/Inputs/internal-constants %s -verify
+
+// expected-no-diagnostics
+#include "c.h"
+
+int q = h();
+int r = N::k;
+
+#include "b.h"
+
+int s = N::k; // FIXME: This should be ambiguous if we really want internal linkage declarations to not collide.

diff --git a/test/SemaCXX/cxx1y-variable-templates_top_level.cpp b/test/SemaCXX/cxx1y-variable-templates_top_level.cpp
index 145bc49fff1fab4baa298b6b0f87b86008c595db..787868fae1764e8e75a98c799daa18adaa7cab59 100644 (file)
--- a/test/SemaCXX/cxx1y-variable-templates_top_level.cpp
+++ b/test/SemaCXX/cxx1y-variable-templates_top_level.cpp
@@ -90,8 +90,8 @@ namespace odr_tmpl {
   }
 
   namespace pvt_diff_params {
-    template<typename T, typename> T v;   // expected-note {{previous template declaration is here}}
-    template<typename T> T v;   // expected-error {{too few template parameters in template redeclaration}} expected-note {{previous template declaration is here}}
+    template<typename T, typename> T v;   // expected-note 2{{previous template declaration is here}}
+    template<typename T> T v;   // expected-error {{too few template parameters in template redeclaration}}
     template<typename T, typename, typename> T v; // expected-error {{too many template parameters in template redeclaration}}
   }
 

diff --git a/test/SemaCXX/enable_if.cpp b/test/SemaCXX/enable_if.cpp
index b04d9b4c9dfca6336c90913c6e6ec08cacfb8031..b32bcd01f4206013ff325b55b32cf0bf5e23224d 100644 (file)
--- a/test/SemaCXX/enable_if.cpp
+++ b/test/SemaCXX/enable_if.cpp
@@ -11,7 +11,7 @@ struct X {
   X(bool b) __attribute__((enable_if(b, "chosen when 'b' is true")));  // expected-note{{candidate disabled: chosen when 'b' is true}}
 
   void f(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero")));
-  void f(int n) __attribute__((enable_if(n == 1, "chosen when 'n' is one")));  // expected-note{{member declaration nearly matches}} expected-note{{candidate disabled: chosen when 'n' is one}}
+  void f(int n) __attribute__((enable_if(n == 1, "chosen when 'n' is one")));  // expected-note{{member declaration nearly matches}} expected-note 2{{candidate disabled: chosen when 'n' is one}}
 
   void g(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero")));  // expected-note{{candidate disabled: chosen when 'n' is zero}}
 
@@ -31,11 +31,11 @@ struct X {
   operator fp() __attribute__((enable_if(false, "never enabled"))) { return surrogate; }  // expected-note{{conversion candidate of type 'int (*)(int)'}}  // FIXME: the message is not displayed
 };
 
-void X::f(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero")))  // expected-note{{member declaration nearly matches}} expected-note{{candidate disabled: chosen when 'n' is zero}}
+void X::f(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero")))  // expected-note{{member declaration nearly matches}} expected-note 2{{candidate disabled: chosen when 'n' is zero}}
 {
 }
 
-void X::f(int n) __attribute__((enable_if(n == 2, "chosen when 'n' is two")))  // expected-error{{out-of-line definition of 'f' does not match any declaration in 'X'}} expected-note{{candidate disabled: chosen when 'n' is two}}
+void X::f(int n) __attribute__((enable_if(n == 2, "chosen when 'n' is two")))  // expected-error{{out-of-line definition of 'f' does not match any declaration in 'X'}}
 {
 }
 
@@ -73,7 +73,7 @@ void test() {
   X x;
   x.f(0);
   x.f(1);
-  x.f(2);  // no error, suppressed by erroneous out-of-line definition
+  x.f(2);  // expected-error{{no matching member function for call to 'f'}}
   x.f(3);  // expected-error{{no matching member function for call to 'f'}}
 
   x.g(0);