[AST][RecoveryExpr] Fix the value category for recovery expr.

[hokein]

Summary

RecoveryExpr was always lvalue, but it is wrong if we use it to model
broken function calls, function call expression has more compliated rules:

• a call to a function whose return type is an lvalue reference yields an lvalue;
• a call to a function whose return type is an rvalue reference yields an xvalue;
• a call to a function whose return type is nonreference type yields a prvalue;

This patch makes the recovery-expr align with the function call if it is
modeled a broken call.

[sammccall]

clang/lib/Sema/SemaOverload.cpp

12945

here we're splitting the type (e.g. int&&) into a type + VK, and passing both to createrecoveryexpr.

Why not do that on recoveryexpr side? e.g. if we request a recoveryexpr of type int&, return an LValue recoveryexpr of type int?

[hokein]

address comment.

clang/lib/Sema/SemaOverload.cpp

12945

right, good idea, this is simpler. I was somehow taking CallExpr as a reference when writing this code.

[sammccall]

clang/lib/AST/ExprClassification.cpp

147

there's a block of cases with a similar implementation (near OpaqueValueKind), maybe move there

clang/lib/Sema/SemaOverload.cpp

12945

Hmm, this does seem simpler to me but it also seems that a few places deliberately make this mapping between two related concepts explicit.
Maybe we should at least have a comment on createrecoveryexpr that the value category will be inferred from the (reference) type.

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

66

I liked the comment explaining the purpose of the test (no crash for wrong value category)

[hokein]

address comments.

clang/lib/Sema/SemaOverload.cpp

12945

Hmm, this does seem simpler to me but it also seems that a few places deliberately make this mapping between two related concepts explicit.

yeah, that's true for at least CallExpr. Expr provides a setValueKind method, ideally (if we enable the type propagation), we could call it to set the value category here. I think it is fine to leave it at recoveryexpr side -- our case is really simple.

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

66

oops, the comment gets lost during my rebase :(.

clang/lib/AST/Expr.cpp

else {
       OriginalTy = OriginalTy->castAsArrayTypeUnsafe()->getElementType();
     }
   }
   return OriginalTy;
 }
 
 RecoveryExpr::RecoveryExpr(ASTContext &Ctx, QualType T, SourceLocation BeginLoc,
                            SourceLocation EndLoc, ArrayRef<Expr *> SubExprs)
-    : Expr(RecoveryExprClass, T, VK_LValue, OK_Ordinary), BeginLoc(BeginLoc),
-      EndLoc(EndLoc), NumExprs(SubExprs.size()) {
+    : Expr(RecoveryExprClass, T.getNonReferenceType(),
+           T->isDependentType() ? VK_LValue : getValueKindForType(T),
+           OK_Ordinary),
+      BeginLoc(BeginLoc), EndLoc(EndLoc), NumExprs(SubExprs.size()) {
   assert(!T.isNull());
   assert(llvm::all_of(SubExprs, [](Expr* E) { return E != nullptr; }));
 
   llvm::copy(SubExprs, getTrailingObjects<Expr *>());
   setDependence(computeDependence(this));
 }
 
 RecoveryExpr *RecoveryExpr::Create(ASTContext &Ctx, QualType T,

clang/lib/AST/ExprClassification.cpp

case Expr::ObjCPropertyRefExprClass:
     // C++ [expr.typeid]p1: The result of a typeid expression is an lvalue of...
   case Expr::CXXTypeidExprClass:
   case Expr::CXXUuidofExprClass:
     // Unresolved lookups and uncorrected typos get classified as lvalues.
     // FIXME: Is this wise? Should they get their own kind?
   case Expr::UnresolvedLookupExprClass:
   case Expr::UnresolvedMemberExprClass:
   case Expr::TypoExprClass:
-  case Expr::RecoveryExprClass:
   case Expr::DependentCoawaitExprClass:
   case Expr::CXXDependentScopeMemberExprClass:
   case Expr::DependentScopeDeclRefExprClass:
     // ObjC instance variables are lvalues
     // FIXME: ObjC++0x might have different rules
   case Expr::ObjCIvarRefExprClass:
   case Expr::FunctionParmPackExprClass:
   case Expr::MSPropertyRefExprClass:
   case Expr::MSPropertySubscriptExprClass:
   case Expr::OMPArraySectionExprClass:
   case Expr::OMPArrayShapingExprClass:
   case Expr::OMPIteratorExprClass:
     return Cl::CL_LValue;
 
     // C99 6.5.2.5p5 says that compound literals are lvalues.
     // In C++, they're prvalue temporaries, except for file-scope arrays.
   case Expr::CompoundLiteralExprClass:
     return !E->isLValue() ? ClassifyTemporary(E->getType()) : Cl::CL_LValue;
 
     // Expressions that are prvalues.
   case Expr::CXXBoolLiteralExprClass:
   case Expr::CXXPseudoDestructorExprClass:
   case Expr::UnaryExprOrTypeTraitExprClass:
case Expr::UnaryOperatorClass:
     case UO_PreInc:
     case UO_PreDec:
       return Lang.CPlusPlus ? Cl::CL_LValue : Cl::CL_PRValue;
 
     default:
       return Cl::CL_PRValue;
     }
 
+  case Expr::RecoveryExprClass:
   case Expr::OpaqueValueExprClass:
     return ClassifyExprValueKind(Lang, E, E->getValueKind());
 
     // Pseudo-object expressions can produce l-values with reference magic.
   case Expr::PseudoObjectExprClass:
     return ClassifyExprValueKind(Lang, E,
                                  cast<PseudoObjectExpr>(E)->getValueKind());
 

clang/lib/Sema/SemaOverload.cpp

 // 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();
+    QualType T = Candidate.Function->getReturnType();
     if (T.isNull())
       return;
     if (!Result)
       Result = T;
     else if (Result != T)
       Result = QualType();
   };
 
static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
 
   // 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;
       I->FailureKind = ovl_fail_addr_not_available;
     }
   }

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 -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: `-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);
 void test_invalid_call(int s) {
   // CHECK:      CallExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} 'some_func'
void test_invalid_call(int s) {
   // FIXME: preserve the broken call.
   int var = some_func(undef1);
 }
 
 int ambig_func(double);
 int ambig_func(float);
 
 // CHECK:     VarDecl {{.*}} ambig_call
-// 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 {{.*}} '<dependent type>' contains-errors

 // CHECK-NEXT:   `-InitListExpr
 // CHECK-NEXT:     `-DesignatedInitExpr {{.*}} 'void'
 // CHECK-NEXT:       `-CXXNullPtrLiteralExpr {{.*}} 'nullptr_t'
 struct {
   int& abc;
 } NoCrashOnInvalidInitList = {
   .abc = nullptr,
 };
+
+// Verify the value category of recovery expression.
+int prvalue(int);
+int &lvalue(int);
+int &&xvalue(int);
+void ValueCategory() {
+  // CHECK:  RecoveryExpr {{.*}} 'int' contains-errors
+  prvalue(); // call to a function (nonreference return type) yields a prvalue (not print by default)
+  // CHECK:  RecoveryExpr {{.*}} 'int' contains-errors lvalue
+  lvalue(); // call to a function (lvalue reference return type) yields an lvalue.
+  // CHECK:  RecoveryExpr {{.*}} 'int' contains-errors xvalue
+  xvalue(); // call to a function (rvalue reference return type) yields an xvalue.
+}

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

return AA<T>::getB(); // expected-error{{no matching function for call to 'getB'}} \
                           // expected-note {{subexpression not valid in a constant expression}}
   }
 };
 // FIXME: should we suppress the "be initialized by a constant expression" diagnostic?
 constexpr auto x2 = AA<int>::foo2(); // expected-error {{be initialized by a constant expression}} \
                                      // expected-note {{in instantiation of member function}} \
                                      // expected-note {{in call to}}
 }
+
+// verify no assertion failure on violating value category.
+namespace test4 {
+int &&f(int);  // expected-note {{candidate function not viable}}
+int &&k = f(); // expected-error {{no matching function for call}}
+}