[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

This looks good to me. I think we can further refine the handling of duplicate diagnostics but I don't think that needs to be done as part of this bugfix.



================
Comment at: clang/lib/Sema/SemaOverload.cpp:953
     S.LookupQualifiedName(Members, RHSRec->getDecl());
-    Members.suppressDiagnostics();
+    Members.suppressAccessDiagnostics();
     for (NamedDecl *Op : Members)
----------------
Can we get duplicate diagnostics here if this lookup fails due to ambiguity? I think we'll perform the same lookup again when looking for candidates if the original operator was `!=`.

(I wonder if we can fix that by avoiding the duplicate lookup, rather than by suppressing ambiguity diagnostics.)


================
Comment at: clang/lib/Sema/SemaOverload.cpp:964
                S.getScopeForContext(EqFD->getEnclosingNamespaceContext()));
-  NonMembers.suppressDiagnostics();
+  NonMembers.suppressAccessDiagnostics();
   for (NamedDecl *Op : NonMembers) {
----------------
There can't be any access diagnostics here because this is a namespace-scope lookup. You can just drop this call entirly.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:15083
   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(
----------------
I think we should also check whether the ambiguous viable functions were all first declared in the same base class here. If so, then it still makes sense to `NoteCandidates`, because the ambiguous lookup had nothing to do with the ambiguous overload resolution.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:15293
   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(
----------------
(Likewise here.)


================
Comment at: clang/lib/Sema/SemaOverload.cpp:15191
   OverloadCandidateSet::iterator Best;
   switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) {
   case OR_Success:
----------------
shafik wrote:
> rsmith wrote:
> > shafik wrote:
> > > @rsmith if `R.isAmbiguous()` should we even check the overload candidates? 
> > > 
> > > Right now in `clang/test/SemaCXX/arrow-operator.cpp` we are getting both an ambiguous name lookup and overload diagnostic. 
> > Hm, I think perhaps the ideal thing to do is: if `BestViableFunction` returns `OR_Ambiguous`, the lookup was ambiguous, and we found viable functions in multiple different base classes, then don't produce any further diagnostics. That way, we still get good error recovery in the case where the overloading is unambiguous despite the lookup being ambiguous, or when the overloading is ambiguous for some reason unrelated to the ambiguous lookup.
> > 
> > (We'd want to do this in all the overloaded operator lookups in this file, not just for `operator->`.)
> I only found one other location that matched this pattern if you meant a larger change let me know. 
Ideally, we should make `AddMemberOperatorCandidates` (and `LookupOverloadedBinOp`) inform its three callers whether its lookup was ambiguous, so that they can also suppress the duplicate ambiguity diagnostics, for unary operators, binary operators, and subscripting.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155387/new/

https://reviews.llvm.org/D155387