PR44978: Accept as an extension some cases where destructor name lookup

is ambiguous, but only one of the possible lookup results could possibly
be right.

Clang recently started diagnosing ambiguity in more cases, and this
broke the build of Firefox. GCC, ICC, MSVC, and previous versions of
Clang all accept some forms of ambiguity here (albeit different ones in
each case); this patch mostly accepts anything any of those compilers
accept.

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -1941,6 +1941,9 @@ def ext_dtor_name_missing_template_arguments : Extension<
 def ext_qualified_dtor_named_in_lexical_scope : ExtWarn<
   "qualified destructor name only found in lexical scope; omit the qualifier "
   "to find this type name by unqualified lookup">, InGroup<DtorName>;
+def ext_dtor_name_ambiguous : Extension<
+  "ISO C++ considers this destructor name lookup to be ambiguous">,
+  InGroup<DtorName>;
 
 def err_destroy_attr_on_non_static_var : Error<
   "%select{no_destroy|always_destroy}0 attribute can only be applied to a"

clang/lib/Sema/SemaExprCXX.cpp

@@ -181,13 +181,23 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc,
       ObjectTypePtr ? GetTypeFromParser(ObjectTypePtr) : QualType();
 
   auto CheckLookupResult = [&](LookupResult &Found) -> ParsedType {
-    // FIXME: Should we be suppressing ambiguities here?
-    if (Found.isAmbiguous()) {
-      Failed = true;
-      return nullptr;
-    }
+    auto IsAcceptableResult = [&](NamedDecl *D) -> bool {
+      auto *Type = dyn_cast<TypeDecl>(D->getUnderlyingDecl());
+      if (!Type)
+        return false;
 
+      if (SearchType.isNull() || SearchType->isDependentType())
+        return true;
+
+      QualType T = Context.getTypeDeclType(Type);
+      return Context.hasSameUnqualifiedType(T, SearchType);
+    };
+
+    unsigned NumAcceptableResults = 0;
     for (NamedDecl *D : Found) {
+      if (IsAcceptableResult(D))
+        ++NumAcceptableResults;
+
       // Don't list a class twice in the lookup failure diagnostic if it's
       // found by both its injected-class-name and by the name in the enclosing
       // scope.
@@ -199,10 +209,34 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc,
         FoundDecls.push_back(D);
     }
 
+    // As an extension, attempt to "fix" an ambiguity by erasing all non-type
+    // results, and all non-matching results if we have a search type. It's not
+    // clear what the right behavior is if destructor lookup hits an ambiguity,
+    // but other compilers do generally accept at least some kinds of
+    // ambiguity.
+    if (Found.isAmbiguous() && NumAcceptableResults == 1) {
+      Diag(NameLoc, diag::ext_dtor_name_ambiguous);
+      LookupResult::Filter F = Found.makeFilter();
+      while (F.hasNext()) {
+        NamedDecl *D = F.next();
+        if (auto *TD = dyn_cast<TypeDecl>(D->getUnderlyingDecl()))
+          Diag(D->getLocation(), diag::note_destructor_type_here)
+              << Context.getTypeDeclType(TD);
+        else
+          Diag(D->getLocation(), diag::note_destructor_nontype_here);
+
+        if (!IsAcceptableResult(D))
+          F.erase();
+      }
+      F.done();
+    }
+
+    if (Found.isAmbiguous())
+      Failed = true;
+
     if (TypeDecl *Type = Found.getAsSingle<TypeDecl>()) {
-      QualType T = Context.getTypeDeclType(Type);
-      if (SearchType.isNull() || SearchType->isDependentType() ||
-          Context.hasSameUnqualifiedType(T, SearchType)) {
+      if (IsAcceptableResult(Type)) {
+        QualType T = Context.getTypeDeclType(Type);
         MarkAnyDeclReferenced(Type->getLocation(), Type, /*OdrUse=*/false);
         return CreateParsedType(T,
                                 Context.getTrivialTypeSourceInfo(T, NameLoc));

clang/test/SemaCXX/destructor.cpp

@@ -510,4 +510,44 @@ namespace DtorTypedef {
   N::C::~C() {}
 #pragma clang diagnostic pop
 }
+
+// Ignore ambiguity errors in destructor name lookup. This matches the observed
+// behavior of ICC, and is compatible with the observed behavior of GCC (which
+// appears to ignore lookups that result in ambiguity) and MSVC (which appears
+// to perform the lookups in the opposite order from Clang).
+namespace PR44978 {
+  // All compilers accept this despite it being clearly ill-formed per the
+  // current wording.
+  namespace n {
+    class Foo {}; // expected-note {{found}}
+  }
+  class Foo {}; // expected-note {{found}}
+  using namespace n;
+  static void func(n::Foo *p) { p->~Foo(); } // expected-warning {{ambiguous}}
+
+  // GCC rejects this case, ICC accepts, despite the class member lookup being
+  // ambiguous.
+  struct Z;
+  struct X { using T = Z; }; // expected-note {{found}}
+  struct Y { using T = int; }; // expected-note {{found}}
+  struct Z : X, Y {};
+  void f(Z *p) { p->~T(); } // expected-warning {{ambiguous}}
+
+  // GCC accepts this and ignores the ambiguous class member lookup.
+  //
+  // FIXME: We should warn on the ambiguity here too, but that requires us to
+  // keep doing lookups after we've already found the type we want.
+  using T = Z;
+  void g(Z *p) { p->~T(); }
+
+  // ICC accepts this and ignores the ambiguous unqualified lookup.
+  struct Q {};
+  namespace { using U = Q; } // expected-note {{candidate}} expected-note {{found}}
+  using U = int; // expected-note {{candidate}} expected-note {{found}}
+  void f(Q *p) { p->~U(); } // expected-warning {{ambiguous}}
+
+  // We still diagnose if the unqualified lookup is dependent, though.
+  template<typename T> void f(T *p) { p->~U(); } // expected-error {{ambiguous}}
+}
+
 #endif // BE_THE_HEADER