[clang] d031ff3 - [Sema] Fix handling of functions that hide classes

[John Brawn via cfe-commits]

Author: John Brawn
Date: 2023-07-25T12:30:41+01:00
New Revision: d031ff38779bd688c514136dbdcce3169ee82b6e

URL: https://github.com/llvm/llvm-project/commit/d031ff38779bd688c514136dbdcce3169ee82b6e
DIFF: https://github.com/llvm/llvm-project/commit/d031ff38779bd688c514136dbdcce3169ee82b6e.diff

LOG: [Sema] Fix handling of functions that hide classes

When a function is declared in the same scope as a class with the same
name then the function hides that class. Currently this is done by a
single check after the main loop in LookupResult::resolveKind, but
this can give the wrong result when we have a using declaration in
multiple namespace scopes in two different ways:

 * When the using declaration is hidden in one namespace but not the
   other we can end up considering only the hidden one when deciding
   if the result is ambiguous, causing an incorrect "not ambiguous"
   result.

 * When two classes with the same name in different namespace scopes
   are both hidden by using declarations this can result in
   incorrectly deciding the result is ambiguous. There's currently a
   comment saying this is expected, but I don't think that's correct.

Solve this by checking each Decl to see if it's hidden by some other
Decl in the same scope. This means we have to delay removing anything
from Decls until after the main loop, in case a Decl is hidden by
another that is removed due to being non-unique.

Differential Revision: https://reviews.llvm.org/D154503

Added: 
    clang/test/SemaCXX/using-hiding.cpp

Modified: 
    clang/lib/Sema/SemaLookup.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index d1ff688c2a21d0..c4f4edb6666cc1 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -513,21 +513,42 @@ void LookupResult::resolveKind() {
   const NamedDecl *HasNonFunction = nullptr;
 
   llvm::SmallVector<const NamedDecl *, 4> EquivalentNonFunctions;
+  llvm::BitVector RemovedDecls(N);
 
-  unsigned UniqueTagIndex = 0;
-
-  unsigned I = 0;
-  while (I < N) {
+  for (unsigned I = 0; I < N; I++) {
     const NamedDecl *D = Decls[I]->getUnderlyingDecl();
     D = cast<NamedDecl>(D->getCanonicalDecl());
 
     // Ignore an invalid declaration unless it's the only one left.
     // Also ignore HLSLBufferDecl which not have name conflict with other Decls.
-    if ((D->isInvalidDecl() || isa<HLSLBufferDecl>(D)) && !(I == 0 && N == 1)) {
-      Decls[I] = Decls[--N];
+    if ((D->isInvalidDecl() || isa<HLSLBufferDecl>(D)) &&
+        N - RemovedDecls.count() > 1) {
+      RemovedDecls.set(I);
       continue;
     }
 
+    // C++ [basic.scope.hiding]p2:
+    //   A class name or enumeration name can be hidden by the name of
+    //   an object, function, or enumerator declared in the same
+    //   scope. If a class or enumeration name and an object, function,
+    //   or enumerator are declared in the same scope (in any order)
+    //   with the same name, the class or enumeration name is hidden
+    //   wherever the object, function, or enumerator name is visible.
+    if (HideTags && isa<TagDecl>(D)) {
+      bool Hidden = false;
+      for (auto *OtherDecl : Decls) {
+        if (canHideTag(OtherDecl) &&
+            getContextForScopeMatching(OtherDecl)->Equals(
+                getContextForScopeMatching(Decls[I]))) {
+          RemovedDecls.set(I);
+          Hidden = true;
+          break;
+        }
+      }
+      if (Hidden)
+        continue;
+    }
+
     std::optional<unsigned> ExistingI;
 
     // Redeclarations of types via typedef can occur both within a scope
@@ -560,7 +581,7 @@ void LookupResult::resolveKind() {
       if (isPreferredLookupResult(getSema(), getLookupKind(), Decls[I],
                                   Decls[*ExistingI]))
         Decls[*ExistingI] = Decls[I];
-      Decls[I] = Decls[--N];
+      RemovedDecls.set(I);
       continue;
     }
 
@@ -571,7 +592,6 @@ void LookupResult::resolveKind() {
     } else if (isa<TagDecl>(D)) {
       if (HasTag)
         Ambiguous = true;
-      UniqueTagIndex = I;
       HasTag = true;
     } else if (isa<FunctionTemplateDecl>(D)) {
       HasFunction = true;
@@ -587,7 +607,7 @@ void LookupResult::resolveKind() {
         if (getSema().isEquivalentInternalLinkageDeclaration(HasNonFunction,
                                                              D)) {
           EquivalentNonFunctions.push_back(D);
-          Decls[I] = Decls[--N];
+          RemovedDecls.set(I);
           continue;
         }
 
@@ -595,28 +615,6 @@ void LookupResult::resolveKind() {
       }
       HasNonFunction = D;
     }
-    I++;
-  }
-
-  // C++ [basic.scope.hiding]p2:
-  //   A class name or enumeration name can be hidden by the name of
-  //   an object, function, or enumerator declared in the same
-  //   scope. If a class or enumeration name and an object, function,
-  //   or enumerator are declared in the same scope (in any order)
-  //   with the same name, the class or enumeration name is hidden
-  //   wherever the object, function, or enumerator name is visible.
-  // But it's still an error if there are distinct tag types found,
-  // even if they're not visible. (ref?)
-  if (N > 1 && HideTags && HasTag && !Ambiguous &&
-      (HasFunction || HasNonFunction || HasUnresolved)) {
-    const NamedDecl *OtherDecl = Decls[UniqueTagIndex ? 0 : N - 1];
-    if (isa<TagDecl>(Decls[UniqueTagIndex]->getUnderlyingDecl()) &&
-        getContextForScopeMatching(Decls[UniqueTagIndex])->Equals(
-            getContextForScopeMatching(OtherDecl)) &&
-        canHideTag(OtherDecl))
-      Decls[UniqueTagIndex] = Decls[--N];
-    else
-      Ambiguous = true;
   }
 
   // FIXME: This diagnostic should really be delayed until we're done with
@@ -625,9 +623,15 @@ void LookupResult::resolveKind() {
     getSema().diagnoseEquivalentInternalLinkageDeclarations(
         getNameLoc(), HasNonFunction, EquivalentNonFunctions);
 
+  // Remove decls by replacing them with decls from the end (which
+  // means that we need to iterate from the end) and then truncating
+  // to the new size.
+  for (int I = RemovedDecls.find_last(); I >= 0; I = RemovedDecls.find_prev(I))
+    Decls[I] = Decls[--N];
   Decls.truncate(N);
 
-  if (HasNonFunction && (HasFunction || HasUnresolved))
+  if ((HasNonFunction && (HasFunction || HasUnresolved)) ||
+      (HideTags && HasTag && (HasFunction || HasNonFunction || HasUnresolved)))
     Ambiguous = true;
 
   if (Ambiguous)

diff  --git a/clang/test/SemaCXX/using-hiding.cpp b/clang/test/SemaCXX/using-hiding.cpp
new file mode 100644
index 00000000000000..12324740549fd2
--- /dev/null
+++ b/clang/test/SemaCXX/using-hiding.cpp
@@ -0,0 +1,373 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+namespace A {
+  class X { }; // expected-note{{candidate found by name lookup is 'A::X'}}
+               // expected-note at -1{{candidate found by name lookup is 'A::X'}}
+}
+namespace B {
+  void X(int); // expected-note{{candidate found by name lookup is 'B::X'}}
+               // expected-note at -1{{candidate found by name lookup is 'B::X'}}
+}
+
+// Using directive doesn't cause A::X to be hidden, so X is ambiguous.
+namespace Test1a {
+  using namespace A;
+  using namespace B;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test1b {
+  using namespace B;
+  using namespace A;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+// The behaviour here should be the same as using namespaces A and B directly
+namespace Test2a {
+  namespace C {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test2a::C::X'}}
+  }
+  namespace D {
+    using B::X; // expected-note{{candidate found by name lookup is 'Test2a::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test2b {
+  namespace C {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test2b::C::X'}}
+  }
+  namespace D {
+    using B::X; // expected-note{{candidate found by name lookup is 'Test2b::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+// Defining a function X inside C should hide using A::X in C but not D, so the result is ambiguous.
+namespace Test3a {
+  namespace C {
+    using A::X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3a::C::X'}}
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3a::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test3b {
+  namespace C {
+    using A::X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3b::C::X'}}
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3b::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test3c {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3c::C::X'}}
+    using A::X;
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3c::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test3d {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3d::C::X'}}
+    using A::X;
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3d::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+// A::X hidden in both C and D by overloaded function, so the result is not ambiguous.
+namespace Test4a {
+  namespace C {
+    using A::X;
+    void X(int);
+  }
+  namespace D {
+    using A::X;
+    void X(int, int);
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test4b {
+  namespace C {
+    using A::X;
+    void X(int);
+  }
+  namespace D {
+    using A::X;
+    void X(int, int);
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test4c {
+  namespace C {
+    void X(int);
+    using A::X;
+  }
+  namespace D {
+    void X(int, int);
+    using A::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test4d {
+  namespace C {
+    void X(int);
+    using A::X;
+  }
+  namespace D {
+    void X(int, int);
+    using A::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+// B::X hides class X in C, so the the result is not ambiguous
+namespace Test5a {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test5b {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test5c {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test5d {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+// B::X hides class X declared in both C and D, so the result is not ambiguous.
+namespace Test6a {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    class X { };
+    using B::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test6b {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    class X { };
+    using B::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test6c {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+    class X { };
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test6d {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+    class X { };
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+// function X inside C should hide class X in C but not D.
+namespace Test7a {
+  namespace C {
+    class X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7a::C::X'}}
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7a::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test7b {
+  namespace C {
+    class X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7b::C::X'}}
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7b::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test7c {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7c::C::X'}}
+    class X;
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7c::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test7d {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7d::C::X'}}
+    class X;
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7d::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}