[ms] Do lookup in dependent base classes also when overload resolution fails (PR23810)

[hans]

Summary

This patch does two things in order to enable compilation of the problematic code in PR23810:

In Sema::buildOverloadedCallSet, it postpones lookup for MS mode when no viable candidate is found in the overload set. Previously, lookup would only be postponed here if the overload set was empty.

Make BuildRecoveryCallExpr call Sema::DiagnoseEmptyLookup under more circumstances. There is a comment in DiagnoseTwoPhaseLookup that says "Don't diagnose names we find in classes; we get much better diagnostics for these from DiagnoseEmptyLookup." The problem was that DiagnoseEmptyLookup might not get called later, and we failed to recover.

Please take a look!

[hans]

Fix PR nbr in the test case.

[rnk]

Oh dear, this is when error recovery and ms compatibility hacks get in the way of each other. :-(

[hans]

In D10369#186935, @rnk wrote:

Oh dear, this is when error recovery and ms compatibility hacks get in the way of each other. :-(

So... do you think me piling on another hack here is ok? :-)

cfe/trunk/lib/Sema/SemaOverload.cpp

 /// correctly implement two-stage name lookup.
 ///
 /// Returns true if a viable candidate was found and a diagnostic was issued.
 static bool
 DiagnoseTwoPhaseLookup(Sema &SemaRef, SourceLocation FnLoc,
                        const CXXScopeSpec &SS, LookupResult &R,
                        OverloadCandidateSet::CandidateSetKind CSK,
                        TemplateArgumentListInfo *ExplicitTemplateArgs,
-                       ArrayRef<Expr *> Args) {
+                       ArrayRef<Expr *> Args,
+                       bool *DoDiagnoseEmptyLookup = nullptr) {
   if (SemaRef.ActiveTemplateInstantiations.empty() || !SS.isEmpty())
     return false;
 
   for (DeclContext *DC = SemaRef.CurContext; DC; DC = DC->getParent()) {
     if (DC->isTransparentContext())
       continue;
 
     SemaRef.LookupQualifiedName(R, DC);
 
     if (!R.empty()) {
       R.suppressDiagnostics();
 
       if (isa<CXXRecordDecl>(DC)) {
         // Don't diagnose names we find in classes; we get much better
         // diagnostics for these from DiagnoseEmptyLookup.
         R.clear();
+        if (DoDiagnoseEmptyLookup)
+          *DoDiagnoseEmptyLookup = true;
         return false;
       }
 
       OverloadCandidateSet Candidates(FnLoc, CSK);
       for (LookupResult::iterator I = R.begin(), E = R.end(); I != E; ++I)
         AddOverloadedCallCandidate(SemaRef, I.getPair(),
                                    ExplicitTemplateArgs, Args,
                                    Candidates, false, /*KnownValid*/ false);
BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
   TemplateArgumentListInfo *ExplicitTemplateArgs = nullptr;
   if (ULE->hasExplicitTemplateArgs()) {
     ULE->copyTemplateArgumentsInto(TABuffer);
     ExplicitTemplateArgs = &TABuffer;
   }
 
   LookupResult R(SemaRef, ULE->getName(), ULE->getNameLoc(),
                  Sema::LookupOrdinaryName);
+  bool DoDiagnoseEmptyLookup = EmptyLookup;
   if (!DiagnoseTwoPhaseLookup(SemaRef, Fn->getExprLoc(), SS, R,
                               OverloadCandidateSet::CSK_Normal,
-                              ExplicitTemplateArgs, Args) &&
-      (!EmptyLookup ||
-       SemaRef.DiagnoseEmptyLookup(
-           S, SS, R,
-           MakeValidator(SemaRef, dyn_cast<MemberExpr>(Fn), Args.size(),
-                         ExplicitTemplateArgs != nullptr, AllowTypoCorrection),
-           ExplicitTemplateArgs, Args)))
+                              ExplicitTemplateArgs, Args,
+                              &DoDiagnoseEmptyLookup) &&
+    (!DoDiagnoseEmptyLookup || SemaRef.DiagnoseEmptyLookup(
     return ExprError();
 
   assert(!R.empty() && "lookup results empty despite recovery");
 
   // Build an implicit member call if appropriate.  Just drop the
   // casts and such from the call, we don't really care.
   ExprResult NewFn = ExprError();
   if ((*R.begin())->isCXXClassMember())
if (checkArgPlaceholdersForOverload(*this, Args, UnbridgedCasts)) {
     *Result = ExprError();
     return true;
   }
 
   // Add the functions denoted by the callee to the set of candidate
   // functions, including those from argument-dependent lookup.
   AddOverloadedCallCandidates(ULE, Args, *CandidateSet);
 
-  // If we found nothing, try to recover.
-  // BuildRecoveryCallExpr diagnoses the error itself, so we just bail
-  // out if it fails.
-  if (CandidateSet->empty()) {
+  if (getLangOpts().MSVCCompat && CurContext->isDependentContext() &&
+      (isa<FunctionDecl>(CurContext) || isa<CXXRecordDecl>(CurContext))) {
+
+    OverloadCandidateSet::iterator Best;
+    if (CandidateSet->empty() ||
+        CandidateSet->BestViableFunction(*this, Fn->getLocStart(), Best) ==
+            OR_No_Viable_Function) {
       // In Microsoft mode, if we are inside a template class member function then
       // create a type dependent CallExpr. The goal is to postpone name lookup
       // to instantiation time to be able to search into type dependent base
       // classes.
-    if (getLangOpts().MSVCCompat && CurContext->isDependentContext() &&
-        (isa<FunctionDecl>(CurContext) || isa<CXXRecordDecl>(CurContext))) {
-      CallExpr *CE = new (Context) CallExpr(Context, Fn, Args,
-                                            Context.DependentTy, VK_RValue,
-                                            RParenLoc);
+      CallExpr *CE = new (Context) CallExpr(
+          Context, Fn, Args, Context.DependentTy, VK_RValue, RParenLoc);
       CE->setTypeDependent(true);
       *Result = CE;
       return true;
     }
-    return false;
   }
 
+  if (CandidateSet->empty())
+    return false;
+
   UnbridgedCasts.restore();
   return false;
 }
 
 /// FinishOverloadedCallExpr - given an OverloadCandidateSet, builds and returns
 /// the completed call expression. If overload resolution fails, emits
 /// diagnostics and returns ExprError()
 static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,

cfe/trunk/test/SemaTemplate/ms-lookup-template-base-classes.cpp

 using D = C<T>;
 
 template <typename T>
 struct E : D<T>
 {
   XXX x; // expected-error {{unknown type name}}
 };
 }
+
+namespace PR23810 {
+void f(int);
+struct Base {
+  void f(); // expected-note{{must qualify identifier to find this declaration in dependent base class}}
+};
+template <typename T> struct Template : T {
+  void member() {
+    f(); // expected-warning {{found via unqualified lookup into dependent bases}}
+  }
+};
+void test() {
+  Template<Base> x;
+  x.member(); // expected-note{{requested here}}
+};
+}