[Clang] Fix member lookup so that we don't ignore ambiguous lookups i…

…n some cases There are some cases during member lookup we are aggressively suppressing diagnostics when we should just be suppressing access control diagnostic. In this PR I add the ability to simply suppress access diagnostics while not suppressing ambiguous lookup diagnostics. Fixes: llvm#22413 llvm#29942 llvm#35574 llvm#27224 Differential Revision: https://reviews.llvm.org/D155387
tstellar · Jul 28, 2023 · cc1b666 · cc1b666
1 parent 63f8922
commit cc1b666
Show file tree

Hide file tree

Showing 7 changed files with 113 additions and 38 deletions.
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -121,6 +121,12 @@ Bug Fixes to C++ Support
   a Unicode character whose name contains a ``-``.
   (`Fixes #64161 <https://github.com/llvm/llvm-project/issues/64161>_`)
 
+- Fix cases where we ignore ambiguous name lookup when looking up memebers.
+  (`#22413 <https://github.com/llvm/llvm-project/issues/22413>_`),
+  (`#29942 <https://github.com/llvm/llvm-project/issues/29942>_`),
+  (`#35574 <https://github.com/llvm/llvm-project/issues/35574>_`) and
+  (`#27224 <https://github.com/llvm/llvm-project/issues/27224>_`).
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 

diff --git a/clang/include/clang/Sema/Lookup.h b/clang/include/clang/Sema/Lookup.h
@@ -148,20 +148,22 @@ class LookupResult {
       : SemaPtr(&SemaRef), NameInfo(NameInfo), LookupKind(LookupKind),
         Redecl(Redecl != Sema::NotForRedeclaration),
         ExternalRedecl(Redecl == Sema::ForExternalRedeclaration),
-        Diagnose(Redecl == Sema::NotForRedeclaration) {
+        DiagnoseAccess(Redecl == Sema::NotForRedeclaration),
+        DiagnoseAmbiguous(Redecl == Sema::NotForRedeclaration) {
     configure();
   }
 
   // TODO: consider whether this constructor should be restricted to take
   // as input a const IdentifierInfo* (instead of Name),
   // forcing other cases towards the constructor taking a DNInfo.
-  LookupResult(Sema &SemaRef, DeclarationName Name,
-               SourceLocation NameLoc, Sema::LookupNameKind LookupKind,
+  LookupResult(Sema &SemaRef, DeclarationName Name, SourceLocation NameLoc,
+               Sema::LookupNameKind LookupKind,
                Sema::RedeclarationKind Redecl = Sema::NotForRedeclaration)
       : SemaPtr(&SemaRef), NameInfo(Name, NameLoc), LookupKind(LookupKind),
         Redecl(Redecl != Sema::NotForRedeclaration),
         ExternalRedecl(Redecl == Sema::ForExternalRedeclaration),
-        Diagnose(Redecl == Sema::NotForRedeclaration) {
+        DiagnoseAccess(Redecl == Sema::NotForRedeclaration),
+        DiagnoseAmbiguous(Redecl == Sema::NotForRedeclaration) {
     configure();
   }
 
@@ -192,12 +194,14 @@ class LookupResult {
         Redecl(std::move(Other.Redecl)),
         ExternalRedecl(std::move(Other.ExternalRedecl)),
         HideTags(std::move(Other.HideTags)),
-        Diagnose(std::move(Other.Diagnose)),
+        DiagnoseAccess(std::move(Other.DiagnoseAccess)),
+        DiagnoseAmbiguous(std::move(Other.DiagnoseAmbiguous)),
         AllowHidden(std::move(Other.AllowHidden)),
         Shadowed(std::move(Other.Shadowed)),
         TemplateNameLookup(std::move(Other.TemplateNameLookup)) {
     Other.Paths = nullptr;
-    Other.Diagnose = false;
+    Other.DiagnoseAccess = false;
+    Other.DiagnoseAmbiguous = false;
   }
 
   LookupResult &operator=(LookupResult &&Other) {
@@ -215,17 +219,22 @@ class LookupResult {
     Redecl = std::move(Other.Redecl);
     ExternalRedecl = std::move(Other.ExternalRedecl);
     HideTags = std::move(Other.HideTags);
-    Diagnose = std::move(Other.Diagnose);
+    DiagnoseAccess = std::move(Other.DiagnoseAccess);
+    DiagnoseAmbiguous = std::move(Other.DiagnoseAmbiguous);
     AllowHidden = std::move(Other.AllowHidden);
     Shadowed = std::move(Other.Shadowed);
     TemplateNameLookup = std::move(Other.TemplateNameLookup);
     Other.Paths = nullptr;
-    Other.Diagnose = false;
+    Other.DiagnoseAccess = false;
+    Other.DiagnoseAmbiguous = false;
     return *this;
   }
 
   ~LookupResult() {
-    if (Diagnose) diagnose();
+    if (DiagnoseAccess)
+      diagnoseAccess();
+    if (DiagnoseAmbiguous)
+      diagnoseAmbiguous();
     if (Paths) deletePaths(Paths);
   }
 
@@ -607,13 +616,20 @@ class LookupResult {
   /// Suppress the diagnostics that would normally fire because of this
   /// lookup.  This happens during (e.g.) redeclaration lookups.
   void suppressDiagnostics() {
-    Diagnose = false;
+    DiagnoseAccess = false;
+    DiagnoseAmbiguous = false;
   }
 
-  /// Determines whether this lookup is suppressing diagnostics.
-  bool isSuppressingDiagnostics() const {
-    return !Diagnose;
-  }
+  /// Suppress the diagnostics that would normally fire because of this
+  /// lookup due to access control violations.
+  void suppressAccessDiagnostics() { DiagnoseAccess = false; }
+
+  /// Determines whether this lookup is suppressing access control diagnostics.
+  bool isSuppressingAccessDiagnostics() const { return !DiagnoseAccess; }
+
+  /// Determines whether this lookup is suppressing ambiguous lookup
+  /// diagnostics.
+  bool isSuppressingAmbiguousDiagnostics() const { return !DiagnoseAmbiguous; }
 
   /// Sets a 'context' source range.
   void setContextRange(SourceRange SR) {
@@ -726,11 +742,14 @@ class LookupResult {
   }
 
 private:
-  void diagnose() {
+  void diagnoseAccess() {
+    if (isClassLookup() && getSema().getLangOpts().AccessControl)
+      getSema().CheckLookupAccess(*this);
+  }
+
+  void diagnoseAmbiguous() {
     if (isAmbiguous())
       getSema().DiagnoseAmbiguousLookup(*this);
-    else if (isClassLookup() && getSema().getLangOpts().AccessControl)
-      getSema().CheckLookupAccess(*this);
   }
 
   void setAmbiguous(AmbiguityKind AK) {
@@ -776,7 +795,8 @@ class LookupResult {
   ///   are present
   bool HideTags = true;
 
-  bool Diagnose = false;
+  bool DiagnoseAccess = false;
+  bool DiagnoseAmbiguous = false;
 
   /// True if we should allow hidden declarations to be 'visible'.
   bool AllowHidden = false;

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
@@ -950,7 +950,7 @@ static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
     LookupResult Members(S, NotEqOp, OpLoc,
                          Sema::LookupNameKind::LookupMemberName);
     S.LookupQualifiedName(Members, RHSRec->getDecl());
-    Members.suppressDiagnostics();
+    Members.suppressAccessDiagnostics();
     for (NamedDecl *Op : Members)
       if (FunctionsCorrespond(S.Context, EqFD, Op->getAsFunction()))
         return false;
@@ -961,7 +961,7 @@ static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
                           Sema::LookupNameKind::LookupOperatorName);
   S.LookupName(NonMembers,
                S.getScopeForContext(EqFD->getEnclosingNamespaceContext()));
-  NonMembers.suppressDiagnostics();
+  NonMembers.suppressAccessDiagnostics();
   for (NamedDecl *Op : NonMembers) {
     auto *FD = Op->getAsFunction();
     if(auto* UD = dyn_cast<UsingShadowDecl>(Op))
@@ -7987,7 +7987,7 @@ void Sema::AddMemberOperatorCandidates(OverloadedOperatorKind Op,
 
     LookupResult Operators(*this, OpName, OpLoc, LookupOrdinaryName);
     LookupQualifiedName(Operators, T1Rec->getDecl());
-    Operators.suppressDiagnostics();
+    Operators.suppressAccessDiagnostics();
 
     for (LookupResult::iterator Oper = Operators.begin(),
                                 OperEnd = Operators.end();
@@ -14983,7 +14983,7 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj,
   const auto *Record = Object.get()->getType()->castAs<RecordType>();
   LookupResult R(*this, OpName, LParenLoc, LookupOrdinaryName);
   LookupQualifiedName(R, Record->getDecl());
-  R.suppressDiagnostics();
+  R.suppressAccessDiagnostics();
 
   for (LookupResult::iterator Oper = R.begin(), OperEnd = R.end();
        Oper != OperEnd; ++Oper) {
@@ -15080,12 +15080,13 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj,
     break;
   }
   case OR_Ambiguous:
-    CandidateSet.NoteCandidates(
-        PartialDiagnosticAt(Object.get()->getBeginLoc(),
-                            PDiag(diag::err_ovl_ambiguous_object_call)
-                                << Object.get()->getType()
-                                << Object.get()->getSourceRange()),
-        *this, OCD_AmbiguousCandidates, Args);
+    if (!R.isAmbiguous())
+      CandidateSet.NoteCandidates(
+          PartialDiagnosticAt(Object.get()->getBeginLoc(),
+                              PDiag(diag::err_ovl_ambiguous_object_call)
+                                  << Object.get()->getType()
+                                  << Object.get()->getSourceRange()),
+          *this, OCD_AmbiguousCandidates, Args);
     break;
 
   case OR_Deleted:
@@ -15248,7 +15249,7 @@ Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc,
 
   LookupResult R(*this, OpName, OpLoc, LookupOrdinaryName);
   LookupQualifiedName(R, Base->getType()->castAs<RecordType>()->getDecl());
-  R.suppressDiagnostics();
+  R.suppressAccessDiagnostics();
 
   for (LookupResult::iterator Oper = R.begin(), OperEnd = R.end();
        Oper != OperEnd; ++Oper) {
@@ -15289,11 +15290,12 @@ Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc,
     return ExprError();
   }
   case OR_Ambiguous:
-    CandidateSet.NoteCandidates(
-        PartialDiagnosticAt(OpLoc, PDiag(diag::err_ovl_ambiguous_oper_unary)
-                                       << "->" << Base->getType()
-                                       << Base->getSourceRange()),
-        *this, OCD_AmbiguousCandidates, Base);
+    if (!R.isAmbiguous())
+      CandidateSet.NoteCandidates(
+          PartialDiagnosticAt(OpLoc, PDiag(diag::err_ovl_ambiguous_oper_unary)
+                                         << "->" << Base->getType()
+                                         << Base->getSourceRange()),
+          *this, OCD_AmbiguousCandidates, Base);
     return ExprError();
 
   case OR_Deleted:

diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
@@ -593,7 +593,7 @@ bool Sema::LookupTemplateName(LookupResult &Found,
       //     postfix-expression and does not name a class template, the name
       //     found in the class of the object expression is used, otherwise
       FoundOuter.clear();
-    } else if (!Found.isSuppressingDiagnostics()) {
+    } else if (!Found.isSuppressingAmbiguousDiagnostics()) {
       //   - if the name found is a class template, it must refer to the same
       //     entity as the one found in the class of the object expression,
       //     otherwise the program is ill-formed.

diff --git a/clang/test/CXX/class.derived/class.member.lookup/gh22413.cpp b/clang/test/CXX/class.derived/class.member.lookup/gh22413.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct A {
+  void operator()(int); // expected-note {{member found by ambiguous name lookup}}
+  void f(int); // expected-note {{member found by ambiguous name lookup}}
+};
+struct B {
+  void operator()(); // expected-note {{member found by ambiguous name lookup}}
+  void f() {} // expected-note {{member found by ambiguous name lookup}}
+};
+
+struct C : A, B {};
+
+int f() {
+    C c;
+    c(); // expected-error {{member 'operator()' found in multiple base classes of different types}}
+    c.f(10); //expected-error {{member 'f' found in multiple base classes of different types}}
+    return 0;
+}
diff --git a/clang/test/CXX/class.derived/class.member.lookup/p11.cpp b/clang/test/CXX/class.derived/class.member.lookup/p11.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct B1 {
+  void f();
+  static void f(int);
+  int i; // expected-note 2{{member found by ambiguous name lookup}}
+};
+struct B2 {
+  void f(double);
+};
+struct I1: B1 { };
+struct I2: B1 { };
+
+struct D: I1, I2, B2 {
+  using B1::f;
+  using B2::f;
+  void g() {
+    f(); // expected-error {{ambiguous conversion from derived class 'D' to base class 'B1'}}
+    f(0); // ok
+    f(0.0); // ok
+    // FIXME next line should be well-formed
+    int B1::* mpB1 = &D::i; // expected-error {{non-static member 'i' found in multiple base-class subobjects of type 'B1'}}
+    int D::* mpD = &D::i; // expected-error {{non-static member 'i' found in multiple base-class subobjects of type 'B1'}}
+  }
+};
diff --git a/clang/test/SemaCXX/arrow-operator.cpp b/clang/test/SemaCXX/arrow-operator.cpp
@@ -4,11 +4,13 @@ struct T {
 };
 
 struct A {
-  T* operator->(); // expected-note{{candidate function}}
+  T* operator->();
+                   // expected-note@-1 {{member found by ambiguous name lookup}}
 };
 
 struct B {
-  T* operator->(); // expected-note{{candidate function}}
+  T* operator->();
+                   // expected-note@-1 {{member found by ambiguous name lookup}}
 };
 
 struct C : A, B {
@@ -19,7 +21,8 @@ struct D : A { };
 struct E; // expected-note {{forward declaration of 'E'}}
 
 void f(C &c, D& d, E& e) {
-  c->f(); // expected-error{{use of overloaded operator '->' is ambiguous}}
+  c->f();
+          // expected-error@-1 {{member 'operator->' found in multiple base classes of different types}}
   d->f();
   e->f(); // expected-error{{incomplete definition of type}}
 }