[AST] Preserve the type in RecoveryExprs for broken function calls.

[hokein]

Summary

RecoveryExprs are modeled as dependent type to prevent bogus diagnostics
and crashes in clang.

This patch allows to preseve the type for broken calls when the
RecoveryEprs have a known type, e.g. a broken non-overloaded call, a
overloaded call when the all candidates have the same return type, so
that more features (code completion still work on "take2args(x).^") still
work.

However, adding the type is risky, which may result in more clang code being
affected leading to new crashes and hurt diagnostic, and it requires large
effort to minimize the affect (update all sites in clang to handle errorDepend
case), so we add a new flag (off by default) to allow us to develop/test
them incrementally.

This patch also has some trivial fixes to suppress diagnostics (to prevent regressions).

Tested:

• all existing tests are passed (when both "-frecovery-ast", "-frecovery-ast-type" flags are flipped on);

[hokein]

update

[hokein]

clang/lib/Sema/SemaOverload.cpp

12779

I steal this function from D61722.

clang/test/SemaCXX/enable_if.cpp

417 ↗

(On Diff #261157)

this diagnostic is changed slightly (without -frecovery-ast-type). I think this is acceptable.

before this patch:

/tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
  return templated<N>();  // expected-error {{no matching function for call to 'templated'}} 
         ^~~~~~~~~~~~
/tmp/t5.cpp:13:5: note: in instantiation of function template specialization 'enable_if_diags::callTemplated<0>' requested here
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^
/tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
template <int N> constexpr int templated() __attribute__((enable_if(N, ""))) {
                               ^                                    ~
/tmp/t5.cpp:13:5: error: constexpr variable 'B' must be initialized by a constant expression
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^~~~~~~~~~~~~~~~~~
2 errors generated.

vs after this patch:

/tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
  return templated<N>();  // expected-error {{no matching function for call to 'templated'}} 
         ^~~~~~~~~~~~
/tmp/t5.cpp:13:5: note: in instantiation of function template specialization 'enable_if_diags::callTemplated<0>' requested here
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^
/tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
template <int N> constexpr int templated() __attribute__((enable_if(N, ""))) {
                               ^                                    ~
/tmp/t5.cpp:12:15: error: constexpr variable 'B' must be initialized by a constant expression
constexpr int B = 10 +  // expected-error {{constexpr variable 'B' must be initialized by a constant expression}}
              ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/t5.cpp:7:10: note: subexpression not valid in a constant expression
  return templated<N>();  // expected-error {{no matching function for call to 'templated'}} 
         ^
/tmp/t5.cpp:13:5: note: in call to 'callTemplated()'
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^
2 errors generated.

clang/test/SemaTemplate/instantiate-function-params.cpp

1 ↗

(On Diff #261157)

this test is very tricky, and hard for human to understand. It was added to test clang not crashing.

Different compilers (gcc, msvc) emit different diagnostics, with this patch, clang emits 3 more errs no type named 'type'..., it seems reasonable, and gcc also emits them.

[sammccall]

clang/include/clang/AST/Expr.h

6099–6100

Comment is stale.

6099–6100

We should mention something about optional type preservation.

clang/lib/AST/ComputeDependence.cpp

494

Dropping type-dependence seems like the point here and is risky, doesn't dropping value- and instantiation-dependence introduce unneccesary extra risk?

Probably correct and maybe useful, but splitting it into a separate change and maybe delaying it might make it easier to get the principal change to stick.

clang/lib/AST/Expr.cpp

4687

Why is this in ifndef?
Actually, why is the loop in ifndef? surely the compiler can inline and remove an empty loop over an arrayref?

clang/lib/Sema/SemaDecl.cpp

16429

why is this needed/does it help?
I would have thought in the cases where we now don't consider the type dependent, the *type* wouldn't contain errors (rather the expression owning the type would).

Is this just for decltype?

clang/lib/Sema/SemaOverload.cpp

12904–12908

nit: try to recover
(or just "Overload resolution failed", what the code is doing is obvious enough here)

clang/test/CodeCompletion/member-access.cpp

276

I think there's an interesting test of the "viable" case where you have a const/non-const overload set with different return types:

class Collection {
  const char *find(int) const;
  char* find(int) const;
};
void test1(const Collection &C, Collection &NC) {
 C.find(); // missing arg, type const char*
 NC.find(); // missing arg, is type NC or is it unresolved?
}

(Not sure if it's best written as a codecompletion test, I guess AST dump is better)

clang/test/Index/getcursor-recovery.cpp

6

nit: unresolved overloaded call?

19

nit: a valid foo(int, int) call?
(just to draw the contrast)

clang/test/SemaCXX/enable_if.cpp

417 ↗

(On Diff #261157)

Yeah, this is noisier but seems OK.
Can you take a look at the blame for the split-line test and see if there was any special code to support it that can be cleaned up?

clang/test/SemaCXX/recovery-expr-type.cpp

12 ↗

(On Diff #261167)

what if we make this constexpr and try to static-assert on it, use it as the size of an array etc?

clang/test/SemaTemplate/instantiate-function-params.cpp

1 ↗

(On Diff #261157)

Ugh, I remember this test. It's partially reduced from boost IIRC.

[hokein]

address comments.

[hokein]

clang/lib/AST/ComputeDependence.cpp

494

Dropping type-dependence seems like the point here and is risky, doesn't dropping value- and instantiation-dependence introduce unneccesary extra risk?

maybe, but you are right, we should be conservative.

separating this patch seems good to me:

1. create a new patch like what we do here,
2. this patch just getting rid of the type bit (still set the value/instantiation bits)

with 1, we will get rid of the value/instantiation-depend bits (for the default Ctx.DependentType case), and we get closer to our goal.

clang/lib/AST/Expr.cpp

4687

removed.

clang/lib/Sema/SemaDecl.cpp

16429

yeah, decltype is the only case we have been discovered so far.

clang/test/CodeCompletion/member-access.cpp

276

oh, this is a good point, this patch doesn't capture the type for member functions, it is in a different code path (probably we should handle that in BuildCallToMemberFunction).

I'd prefer to handle it in a separate patch.

clang/test/SemaCXX/enable_if.cpp

417 ↗

(On Diff #261157)

git blame showed https://github.com/llvm/llvm-project/commit/c7d3e609fbf05d1a1236f99efd1e2fd344554f4b, but I didn't see any special code that we need clean up.

clang/test/SemaCXX/recovery-expr-type.cpp

12 ↗

(On Diff #261167)

added tests

[sammccall]

clang/lib/AST/ComputeDependence.cpp

488

This comment is hard to understand in isolation: it describes what the patch is doing (compared to the baseline, and to some alternative), rather than what the code is doing.

I'd suggest something like:

Mark the expression as value- and instantiation- dependent to reuse existing suppressions for dependent code, e.g. avoiding constant-evaluation.

clang/lib/AST/Expr.cpp

3310

This can move into the later patch I think - we're always instantiation-dependent where it matters.

clang/lib/Sema/SemaDecl.cpp

12815

again - can move into later patch

clang/lib/Sema/SemaDeclCXX.cpp

15887 ↗

(On Diff #261762)

can defer this too

clang/test/CodeCompletion/member-access.cpp

283

nit: CHECK-RECOVERY or so?

[hokein]

address comments.

[hokein]

rebase to master (not the D78350)

[hokein]

add a missing test file during the rebase.

[hokein]

clang/test/SemaCXX/enable_if.cpp

417 ↗

(On Diff #261157)

actually we don't need this change if we land this patch before the enable-recovery-expr patch, defer this change to D78350.

clang/test/SemaTemplate/instantiate-function-params.cpp

1 ↗

(On Diff #261157)

the same to this file, defer it to D78350.

clang/include/clang/AST/Expr.h

 /// other well-formed expressions. E.g. when type-checking of a binary operator
 /// fails, we cannot produce a BinaryOperator expression. Instead, we can choose
 /// to produce a recovery expression storing left and right operands.
 ///
 /// RecoveryExpr does not have any semantic meaning in C++, it is only useful to
 /// preserve expressions in AST that would otherwise be dropped. It captures
 /// subexpressions of some expression that we could not construct and source
 /// range covered by the expression.
 ///
-/// For now, RecoveryExpr is type-, value- and instantiation-dependent to take
-/// advantage of existing machinery to deal with dependent code in C++, e.g.
-/// RecoveryExpr is preserved in `decltype(<broken-expr>)` as part of the
+/// By default, RecoveryExpr is type-, value- and instantiation-dependent to
+/// take advantage of existing machinery to deal with dependent code in C++,
+/// e.g. RecoveryExpr is preserved in `decltype(<broken-expr>)` as part of the
 /// `DependentDecltypeType`. In addition to that, clang does not report most
 /// errors on dependent expressions, so we get rid of bogus errors for free.
 /// However, note that unlike other dependent expressions, RecoveryExpr can be
 /// produced in non-template contexts.
+/// In addition, we will preserve the type in RecoveryExpr when the type is
+/// known, e.g. preserving the return type for a broken non-overloaded function
+/// call, a overloaded call where all candidates have the same return type.
 ///
 /// One can also reliably suppress all bogus errors on expressions containing
 /// recovery expressions by examining results of Expr::containsErrors().
 class RecoveryExpr final : public Expr,
                            private llvm::TrailingObjects<RecoveryExpr, Expr *> {
 public:
-  static RecoveryExpr *Create(ASTContext &Ctx, SourceLocation BeginLoc,
-                              SourceLocation EndLoc, ArrayRef<Expr *> SubExprs);
+  static RecoveryExpr *Create(ASTContext &Ctx, QualType T,
+                              SourceLocation BeginLoc, SourceLocation EndLoc,
+                              ArrayRef<Expr *> SubExprs);
   static RecoveryExpr *CreateEmpty(ASTContext &Ctx, unsigned NumSubExprs);
 
   ArrayRef<Expr *> subExpressions() {
     auto *B = getTrailingObjects<Expr *>();
     return llvm::makeArrayRef(B, B + NumExprs);
   }
 
   ArrayRef<const Expr *> subExpressions() const {
     return const_cast<RecoveryExpr *>(this)->subExpressions();
   }
 
   child_range children() {
     Stmt **B = reinterpret_cast<Stmt **>(getTrailingObjects<Expr *>());
     return child_range(B, B + NumExprs);
   }
 
   SourceLocation getBeginLoc() const { return BeginLoc; }
   SourceLocation getEndLoc() const { return EndLoc; }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == RecoveryExprClass;
   }
 
 private:
-  RecoveryExpr(ASTContext &Ctx, SourceLocation BeginLoc, SourceLocation EndLoc,
-               ArrayRef<Expr *> SubExprs);
+  RecoveryExpr(ASTContext &Ctx, QualType T, SourceLocation BeginLoc,
+               SourceLocation EndLoc, ArrayRef<Expr *> SubExprs);
   RecoveryExpr(EmptyShell Empty, unsigned NumSubExprs)
       : Expr(RecoveryExprClass, Empty), NumExprs(NumSubExprs) {}
 
   size_t numTrailingObjects(OverloadToken<Stmt *>) const { return NumExprs; }
 
   SourceLocation BeginLoc, EndLoc;
   unsigned NumExprs;
   friend TrailingObjects;
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_AST_EXPR_H

clang/include/clang/Basic/LangOptions.def

 LANGOPT(GNUAsm            , 1, 1, "GNU-style inline assembly")
 LANGOPT(Coroutines        , 1, 0, "C++20 coroutines")
 LANGOPT(DllExportInlines  , 1, 1, "dllexported classes dllexport inline methods")
 LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template template arguments")
 
 LANGOPT(DoubleSquareBracketAttributes, 1, 0, "'[[]]' attributes extension for all language standard modes")
 
 COMPATIBLE_LANGOPT(RecoveryAST, 1, 0, "Preserve expressions in AST when encountering errors")
+COMPATIBLE_LANGOPT(RecoveryASTType, 1, 0, "Preserve the type in recovery expressions")
 
 BENIGN_LANGOPT(ThreadsafeStatics , 1, 1, "thread-safe static initializers")
 LANGOPT(POSIXThreads      , 1, 0, "POSIX thread support")
 LANGOPT(Blocks            , 1, 0, "blocks extension to C")
 BENIGN_LANGOPT(EmitAllDecls      , 1, 0, "emitting all declarations")
 LANGOPT(MathErrno         , 1, 1, "errno in math functions")
 BENIGN_LANGOPT(HeinousExtensions , 1, 0, "extensions that we really don't like and may be ripped out at any time")
 LANGOPT(Modules           , 1, 0, "modules semantics")

clang/include/clang/Driver/CC1Options.td

 def fno_concept_satisfaction_caching : Flag<["-"],
                                             "fno-concept-satisfaction-caching">,
   HelpText<"Disable satisfaction caching for C++2a Concepts.">;
 
 def frecovery_ast : Flag<["-"], "frecovery-ast">,
   HelpText<"Preserve expressions in AST rather than dropping them when "
            "encountering semantic errors">;
 def fno_recovery_ast : Flag<["-"], "fno-recovery-ast">;
+def frecovery_ast_type : Flag<["-"], "frecovery-ast-type">,
+  HelpText<"Preserve the type for recovery expressions when possible "
+          "(experimental)">;
+def fno_recovery_ast_type : Flag<["-"], "fno-recovery-ast-type">;
 
 let Group = Action_Group in {
 
 def Eonly : Flag<["-"], "Eonly">,
   HelpText<"Just run preprocessor, no output (for timings)">;
 def dump_raw_tokens : Flag<["-"], "dump-raw-tokens">,
   HelpText<"Lex file in raw mode and dump raw tokens">;
 def analyze : Flag<["-"], "analyze">,

clang/include/clang/Sema/Sema.h

public:
 
   bool CheckRedeclarationModuleOwnership(NamedDecl *New, NamedDecl *Old);
 
   void DiagnoseAmbiguousLookup(LookupResult &Result);
   //@}
 
   /// Attempts to produce a RecoveryExpr after some AST node cannot be created.
   ExprResult CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
-                                ArrayRef<Expr *> SubExprs);
+                                ArrayRef<Expr *> SubExprs,
+                                QualType T = QualType());
 
   ObjCInterfaceDecl *getObjCInterfaceDecl(IdentifierInfo *&Id,
                                           SourceLocation IdLoc,
                                           bool TypoCorrection = false);
   NamedDecl *LazilyCreateBuiltin(IdentifierInfo *II, unsigned ID,
                                  Scope *S, bool ForRedeclaration,
                                  SourceLocation Loc);
   NamedDecl *ImplicitlyDefineFunction(SourceLocation Loc, IdentifierInfo &II,

clang/lib/AST/ComputeDependence.cpp

ExprDependence clang::computeDependence(DeclRefExpr *E, const ASTContext &Ctx) {
   //      -  a member function or a static data member of the current
   //         instantiation
   if (isa<CXXMethodDecl>(Decl) && Decl->getDeclContext()->isDependentContext())
     Deps |= ExprDependence::ValueInstantiation;
   return Deps;
 }
 
 ExprDependence clang::computeDependence(RecoveryExpr *E) {
+  // Mark the expression as value- and instantiation- dependent to reuse
+  // existing suppressions for dependent code, e.g. avoiding
+  // constant-evaluation.
   // FIXME: drop type+value+instantiation once Error is sufficient to suppress
   // bogus dianostics.
-  auto D = ExprDependence::TypeValueInstantiation | ExprDependence::Error;
+  auto D = toExprDependence(E->getType()->getDependence()) |
+           ExprDependence::ValueInstantiation | ExprDependence::Error;
   for (auto *S : E->subExpressions())
     D |= S->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(PredefinedExpr *E) {
   return toExprDependence(E->getType()->getDependence()) &
          ~ExprDependence::UnexpandedPack;

clang/lib/AST/Expr.cpp

 bool Expr::HasSideEffects(const ASTContext &Ctx,
                           bool IncludePossibleEffects) const {
   // In circumstances where we care about definite side effects instead of
   // potential side effects, we want to ignore expressions that are part of a
   // macro expansion as a potential side effect.
   if (!IncludePossibleEffects && getExprLoc().isMacroID())
     return false;
 
   if (isInstantiationDependent())
     return IncludePossibleEffects;
 
   switch (getStmtClass()) {
   case NoStmtClass:
   #define ABSTRACT_STMT(Type)
   #define STMT(Type, Base) case Type##Class:
   #define EXPR(Type, Base)
   #include "clang/AST/StmtNodes.inc"
for (unsigned Cnt = 0; Cnt < ArraySectionCount; ++Cnt) {
     else {
       assert (OriginalTy->isArrayType());
       OriginalTy = OriginalTy->castAsArrayTypeUnsafe()->getElementType();
     }
   }
   return OriginalTy;
 }
 
-RecoveryExpr::RecoveryExpr(ASTContext &Ctx, SourceLocation BeginLoc,
+RecoveryExpr::RecoveryExpr(ASTContext &Ctx, QualType T, SourceLocation BeginLoc,
                            SourceLocation EndLoc, ArrayRef<Expr *> SubExprs)
-    : Expr(RecoveryExprClass, Ctx.DependentTy, VK_LValue, OK_Ordinary),
-      BeginLoc(BeginLoc), EndLoc(EndLoc), NumExprs(SubExprs.size()) {
-#ifndef NDEBUG
+    : Expr(RecoveryExprClass, T, VK_LValue, OK_Ordinary), BeginLoc(BeginLoc),
+      EndLoc(EndLoc), NumExprs(SubExprs.size()) {
+  assert(!T.isNull());
   for (auto *E : SubExprs)
     assert(E != nullptr);
-#endif
 
   llvm::copy(SubExprs, getTrailingObjects<Expr *>());
   setDependence(computeDependence(this));
 }
 
-RecoveryExpr *RecoveryExpr::Create(ASTContext &Ctx, SourceLocation BeginLoc,
+RecoveryExpr *RecoveryExpr::Create(ASTContext &Ctx, QualType T,
+                                   SourceLocation BeginLoc,
                                    SourceLocation EndLoc,
                                    ArrayRef<Expr *> SubExprs) {
   void *Mem = Ctx.Allocate(totalSizeToAlloc<Expr *>(SubExprs.size()),
                            alignof(RecoveryExpr));
-  return new (Mem) RecoveryExpr(Ctx, BeginLoc, EndLoc, SubExprs);
+  return new (Mem) RecoveryExpr(Ctx, T, BeginLoc, EndLoc, SubExprs);
 }
 
 RecoveryExpr *RecoveryExpr::CreateEmpty(ASTContext &Ctx, unsigned NumSubExprs) {
   void *Mem = Ctx.Allocate(totalSizeToAlloc<Expr *>(NumSubExprs),
                            alignof(RecoveryExpr));
   return new (Mem) RecoveryExpr(EmptyShell(), NumSubExprs);
 }
 

clang/lib/Frontend/CompilerInvocation.cpp

if (Opts.NewAlignOverride && !llvm::isPowerOf2_32(Opts.NewAlignOverride)) {
     Opts.NewAlignOverride = 0;
   }
   Opts.ConceptSatisfactionCaching =
       !Args.hasArg(OPT_fno_concept_satisfaction_caching);
   if (Args.hasArg(OPT_fconcepts_ts))
     Diags.Report(diag::warn_fe_concepts_ts_flag);
   Opts.RecoveryAST =
       Args.hasFlag(OPT_frecovery_ast, OPT_fno_recovery_ast, false);
+  Opts.RecoveryASTType =
++      Args.hasFlag(OPT_frecovery_ast_type, OPT_fno_recovery_ast_type, false);
   Opts.HeinousExtensions = Args.hasArg(OPT_fheinous_gnu_extensions);
   Opts.AccessControl = !Args.hasArg(OPT_fno_access_control);
   Opts.ElideConstructors = !Args.hasArg(OPT_fno_elide_constructors);
   Opts.MathErrno = !Opts.OpenCL && Args.hasArg(OPT_fmath_errno);
   Opts.InstantiationDepth =
       getLastArgIntValue(Args, OPT_ftemplate_depth, 1024, Diags);
   Opts.ArrowDepth =
       getLastArgIntValue(Args, OPT_foperator_arrow_depth, 256, Diags);

clang/lib/Sema/SemaDecl.cpp

void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
 
   if (var->hasAttr<BlocksAttr>())
     getCurFunction()->addByrefBlockVar(var);
 
   Expr *Init = var->getInit();
   bool IsGlobal = GlobalStorage && !var->isStaticLocal();
   QualType baseType = Context.getBaseElementType(type);
 
   if (Init && !Init->isValueDependent()) {
     if (var->isConstexpr()) {
       SmallVector<PartialDiagnosticAt, 8> Notes;
       if (!var->evaluateValue(Notes) || !var->isInitICE()) {
         SourceLocation DiagLoc = var->getLocation();
         // If the note doesn't add any useful information other than a source
         // location, fold it into the primary diagnostic.
         if (Notes.size() == 1 && Notes[0].second.getDiagID() ==
               diag::note_invalid_subexpr_in_const_expr) {
FieldDecl *Sema::CheckFieldDecl(DeclarationName Name, QualType T,
   // If we receive a broken type, recover by assuming 'int' and
   // marking this declaration as invalid.
   if (T.isNull()) {
     InvalidDecl = true;
     T = Context.IntTy;
   }
 
   QualType EltTy = Context.getBaseElementType(T);
-  if (!EltTy->isDependentType()) {
+  if (!EltTy->isDependentType() && !EltTy->containsErrors()) {
     if (RequireCompleteSizedType(Loc, EltTy,
                                  diag::err_field_incomplete_or_sizeless)) {
       // Fields of incomplete type force their record to be invalid.
       Record->setInvalidDecl();
       InvalidDecl = true;
     } else {
       NamedDecl *Def;
       EltTy->isIncompleteType(&Def);

clang/lib/Sema/SemaExpr.cpp

 }
 
 bool Sema::IsDependentFunctionNameExpr(Expr *E) {
   assert(E->isTypeDependent());
   return isa<UnresolvedLookupExpr>(E);
 }
 
 ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
-                                    ArrayRef<Expr *> SubExprs) {
+                                    ArrayRef<Expr *> SubExprs, QualType T) {
   // FIXME: enable it for C++, RecoveryExpr is type-dependent to suppress
   // bogus diagnostics and this trick does not work in C.
   // FIXME: use containsErrors() to suppress unwanted diags in C.
   if (!Context.getLangOpts().RecoveryAST)
     return ExprError();
 
   if (isSFINAEContext())
     return ExprError();
 
-  return RecoveryExpr::Create(Context, Begin, End, SubExprs);
+  if (T.isNull() || !Context.getLangOpts().RecoveryASTType)
+    // We don't know the concrete type, fallback to dependent type.
+    T = Context.DependentTy;
+  return RecoveryExpr::Create(Context, T, Begin, End, SubExprs);
 }

clang/lib/Sema/SemaOverload.cpp

#endif
 
   if (CandidateSet->empty())
     return false;
 
   UnbridgedCasts.restore();
   return false;
 }
 
+// Guess at what the return type for an unresolvable overload should be.
+static QualType chooseRecoveryType(OverloadCandidateSet &CS,
+                                   OverloadCandidateSet::iterator *Best) {
+  llvm::Optional<QualType> Result;
+  // Adjust Type after seeing a candidate.
+  auto ConsiderCandidate = [&](const OverloadCandidate &Candidate) {
+    if (!Candidate.Function)
+      return;
+    QualType T = Candidate.Function->getCallResultType();
+    if (T.isNull())
+      return;
+    if (!Result)
+      Result = T;
+    else if (Result != T)
+      Result = QualType();
+  };
+
+  // Look for an unambiguous type from a progressively larger subset.
+  // e.g. if types disagree, but all *viable* overloads return int, choose int.
+  //
+  // First, consider only the best candidate.
+  if (Best && *Best != CS.end())
+    ConsiderCandidate(**Best);
+  // Next, consider only viable candidates.
+  if (!Result)
+    for (const auto &C : CS)
+      if (C.Viable)
+        ConsiderCandidate(C);
+  // Finally, consider all candidates.
+  if (!Result)
+    for (const auto &C : CS)
+      ConsiderCandidate(C);
+
+  return Result.getValueOr(QualType());
+}
+
 /// 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,
                                            UnresolvedLookupExpr *ULE,
                                            SourceLocation LParenLoc,
                                            MultiExprArg Args,
                                            SourceLocation RParenLoc,
case OR_Deleted: {
     FunctionDecl *FDecl = (*Best)->Function;
     Fn = SemaRef.FixOverloadedFunctionReference(Fn, (*Best)->FoundDecl, FDecl);
     return SemaRef.BuildResolvedCallExpr(Fn, FDecl, LParenLoc, Args, RParenLoc,
                                          ExecConfig, /*IsExecConfig=*/false,
                                          (*Best)->IsADLCandidate);
   }
   }
 
-  // Overload resolution failed.
-  return ExprError();
+  // Overload resolution failed, try to recover.
+  SmallVector<Expr *, 8> SubExprs = {Fn};
+  SubExprs.append(Args.begin(), Args.end());
+  return SemaRef.CreateRecoveryExpr(Fn->getBeginLoc(), RParenLoc, SubExprs,
+                                    chooseRecoveryType(*CandidateSet, Best));
 }
 
 static void markUnaddressableCandidatesUnviable(Sema &S,
                                                 OverloadCandidateSet &CS) {
   for (auto I = CS.begin(), E = CS.end(); I != E; ++I) {
     if (I->Viable &&
         !S.checkAddressOfFunctionIsAvailable(I->Function, /*Complain=*/false)) {
       I->Viable = false;

clang/test/AST/ast-dump-recovery.cpp

-// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -fcxx-exceptions -std=gnu++17 -frecovery-ast -ast-dump %s | FileCheck -strict-whitespace %s
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -fcxx-exceptions -std=gnu++17 -frecovery-ast -frecovery-ast-type -ast-dump %s | FileCheck -strict-whitespace %s
 // RUN: not %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -fcxx-exceptions -std=gnu++17 -fno-recovery-ast -ast-dump %s | FileCheck --check-prefix=DISABLED -strict-whitespace %s
 
 int some_func(int *);
 
 // CHECK:     VarDecl {{.*}} invalid_call
-// CHECK-NEXT:`-RecoveryExpr {{.*}} contains-errors
-// CHECK-NEXT:  |-UnresolvedLookupExpr {{.*}} 'some_func'
-// CHECK-NEXT:  `-IntegerLiteral {{.*}} 123
+// CHECK-NEXT: `-ImplicitCastExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT:  `-RecoveryExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT:    |-UnresolvedLookupExpr {{.*}} 'some_func'
+// CHECK-NEXT:    `-IntegerLiteral {{.*}} 123
 // DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
 int invalid_call = some_func(123);
 
 int ambig_func(double);
 int ambig_func(float);
 
 // CHECK:     VarDecl {{.*}} ambig_call
-// CHECK-NEXT:`-RecoveryExpr {{.*}} contains-errors
-// CHECK-NEXT:  |-UnresolvedLookupExpr {{.*}} 'ambig_func'
-// CHECK-NEXT:  `-IntegerLiteral {{.*}} 123
+// CHECK-NEXT: `-ImplicitCastExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT:  `-RecoveryExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT:    |-UnresolvedLookupExpr {{.*}} 'ambig_func'
+// CHECK-NEXT:    `-IntegerLiteral {{.*}} 123
 // DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
 int ambig_call = ambig_func(123);
 
 // CHECK:     VarDecl {{.*}} unresolved_call1
-// CHECK-NEXT:`-RecoveryExpr {{.*}} contains-errors
+// CHECK-NEXT:`-RecoveryExpr {{.*}} '<dependent type>' contains-errors
 // CHECK-NEXT:  `-UnresolvedLookupExpr {{.*}} 'bar'
 // DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
 int unresolved_call1 = bar();
 
 // CHECK:     VarDecl {{.*}} unresolved_call2
 // CHECK-NEXT:`-CallExpr {{.*}} contains-errors
 // CHECK-NEXT:  |-UnresolvedLookupExpr {{.*}} 'bar'
 // CHECK-NEXT:  |-RecoveryExpr {{.*}} contains-errors

clang/test/CodeCompletion/member-access.cpp

struct S {
     int operator=(int);
   } s;
   return s.
 }
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:270:12 %s -o - | FileCheck -check-prefix=CHECK-OPER %s \
 // RUN: --implicit-check-not="[#char#]operator=("
 // CHECK-OPER: [#int#]operator=(
 
+struct S { int member; };
+S overloaded(int);
+S overloaded(double);
+void foo() {
+  // No overload matches, but we have recovery-expr with the correct type.
+  overloaded().
+}
+// RUN: not %clang_cc1 -fsyntax-only -frecovery-ast -frecovery-ast-type -code-completion-at=%s:281:16 %s -o - | FileCheck -check-prefix=CHECK-RECOVERY %s
+// CHECK-RECOVERY: [#int#]member

clang/test/Index/getcursor-recovery.cpp

 int foo(int, int);
 int foo(int, double);
 int x;
 
-void testTypedRecoveryExpr() {
-  // Inner foo() is a RecoveryExpr, outer foo() is an overloaded call.
-  foo(x, foo(x));
+void testTypedRecoveryExpr1() {
+  // Inner bar() is an unresolved overloaded call, outer foo() is an overloaded call.
+  foo(x, bar(x));
 }
-// RUN: c-index-test -cursor-at=%s:7:3 %s -Xclang -frecovery-ast | FileCheck -check-prefix=OUTER-FOO %s
+// RUN: c-index-test -cursor-at=%s:7:3 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=OUTER-FOO %s
 // OUTER-FOO: OverloadedDeclRef=foo[2:5, 1:5]
-// RUN: c-index-test -cursor-at=%s:7:7 %s -Xclang -frecovery-ast | FileCheck -check-prefix=OUTER-X %s
+// RUN: c-index-test -cursor-at=%s:7:7 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=OUTER-X %s
 // OUTER-X: DeclRefExpr=x:3:5
-// RUN: c-index-test -cursor-at=%s:7:10 %s -Xclang -frecovery-ast | FileCheck -check-prefix=INNER-FOO %s
-// INNER-FOO: OverloadedDeclRef=foo[2:5, 1:5]
-// RUN: c-index-test -cursor-at=%s:7:14 %s -Xclang -frecovery-ast | FileCheck -check-prefix=INNER-X %s
+// RUN: c-index-test -cursor-at=%s:7:10 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=INNER-FOO %s
+// INNER-FOO: OverloadedDeclRef=bar
+// RUN: c-index-test -cursor-at=%s:7:14 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=INNER-X %s
 // INNER-X: DeclRefExpr=x:3:5
+
+void testTypedRecoveryExpr2() {
+  // Inner foo() is a RecoveryExpr (with int type), outer foo() is a valid "foo(int, int)" call.
+  foo(x, foo(x));
+}
+// RUN: c-index-test -cursor-at=%s:20:3 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=TEST2-OUTER %s
+// TEST2-OUTER: DeclRefExpr=foo:1:5
+// RUN: c-index-test -cursor-at=%s:20:10 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=TEST2-INNER %s
+// TEST2-INNER: OverloadedDeclRef=foo[2:5, 1:5]