Sema: Fix PR12350 destructor name lookup, addressing (some of) DR244

[jmorse]

Summary

Hi,

This patch tries to fix a problem in clangs implementation of C++11's
[basic.lookup.qual]p6 as demonstrated in PR12350, by:

• Re-enabling looking in name-specifier prefixes for destructor names, when the penultimate name is a class
• In that case, forces the destructor name to be sought as a type-name

Which has some history. Looking in the name-specifier prefix for a
destructor name was partially disabled in r96836 for C++ < 11 because
name hiding could hide the final destructor name lookup (PR6358), and
then for all C++ in r107835 because the initial fix for D244 (r209319)
wasn't in.

However, the spec still specifies looking in the name-specifier prefix, and
without doing so bugs like PR12350 can't be fixed. This patch gets around
the original name hiding issue by forcing a type-name lookup if we're
looking in the name-specifier prefix where the penultimate is a class,
avoiding any hiding.

This is almost certainly what the spec intended, IMHO. You can read
[basic.lookup.qual]p6 as supporting this interpretation if you take the
pseudo destructors phrasing "...the type-names are looked up _as_ types
in the scope..." and assume that the class-name form being looked
up "Similarly" means that the second class-name should also be looked
up _as_ a class-name.

[jmorse]

Ping

[jmorse]

Ping

[jmorse]

Ping

[jmorse]

ping

lib/Sema/SemaExprCXX.cpp

ParsedType Sema::getDestructorName(SourceLocation TildeLoc,
   //
   // See also PR6358 and PR6359.
   // For this reason, we're currently only doing the C++03 version of this
   // code; the C++0x version has to wait until we get a proper spec.
   QualType SearchType;
   DeclContext *LookupCtx = nullptr;
   bool isDependent = false;
   bool LookInScope = false;
+  LookupNameKind LookupKind = LookupOrdinaryName;
 
   if (SS.isInvalid())
     return nullptr;
 
   // If we have an object type, it's because we are in a
   // pseudo-destructor-expression or a member access expression, and
   // we know what type we're looking for.
   if (ObjectTypePtr)
if (SS.isSet()) {
     // Here, we determine whether the code below is permitted to look at the
     // prefix of the nested-name-specifier.
     DeclContext *DC = computeDeclContext(SS, EnteringContext);
     if (DC && DC->isFileContext()) {
       AlreadySearched = true;
       LookupCtx = DC;
       isDependent = false;
     } else if (DC && isa<CXXRecordDecl>(DC)) {
-      LookAtPrefix = false;
+      // Penultimate name is a class-name, we will LookAtPrefix. However, look
+      // up class-name's in the same scope as the first.
+      LookupKind = LookupNestedNameSpecifierName;
       LookInScope = true;
     }
 
     // The second case from the C++03 rules quoted further above.
     NestedNameSpecifier *Prefix = nullptr;
     if (AlreadySearched) {
       // Nothing left to do.
     } else if (LookAtPrefix && (Prefix = NNS->getPrefix())) {
if (SS.isSet()) {
 
     LookInScope = true;
   } else {
     // Perform lookup into the current scope (only).
     LookInScope = true;
   }
 
   TypeDecl *NonMatchingTypeDecl = nullptr;
-  LookupResult Found(*this, &II, NameLoc, LookupOrdinaryName);
+  LookupResult Found(*this, &II, NameLoc, LookupKind);
   for (unsigned Step = 0; Step != 2; ++Step) {
     // Look for the name first in the computed lookup context (if we
     // have one) and, if that fails to find a match, in the scope (if
     // we're allowed to look there).
     Found.clear();
     if (Step == 0 && LookupCtx) {
       if (RequireCompleteDeclContext(SS, LookupCtx))
         return nullptr;

test/CXX/drs/dr2xx.cpp

void g(N::F f) {
     f.G::~F(); // expected-error {{expected the class name after '~' to name a destructor}}
     f.G::~G();
     // This is technically ill-formed; E is looked up in 'N::' and names the
     // class template, not the injected-class-name of the class. But that's
     // probably a bug in the standard.
     f.N::F::~E();
     // This is valid; we look up the second F in the same scope in which we
     // found the first one, that is, 'N::'.
-    f.N::F::~F(); // FIXME: expected-error {{expected the class name after '~' to name a destructor}}
+    f.N::F::~F();
     // This is technically ill-formed; G is looked up in 'N::' and is not found;
     // as above, this is probably a bug in the standard.
     f.N::F::~G();
   }
 }
 
 namespace dr245 { // dr245: yes
   struct S {

test/SemaCXX/destructor.cpp

void C1<T>::foo1() {
   C1::~C1();
 }
 
 void foo1() {
   C1<int> x;
   x.foo1();
 }
 }
+
+namespace PR12350 {
+  class basic_string {
+  public:
+    ~basic_string();
+    char *something;
+  };
+
+  namespace notstd {
+    typedef basic_string string;
+  }
+
+  void
+  foo(notstd::string *bar)
+  {
+    bar->notstd::string::~string(); // OK
+  }
+}
+
 #endif // BE_THE_HEADER