[PATCH] D157868: [clang] Use RecoveryExprs for broken defaultargs, instead of OpaqueValueExprs

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
kadircet marked 3 inline comments as done.
Closed by commit rG373fcd5d73a3: [clang] Use RecoveryExprs for broken 
defaultargs, instead of OpaqueValueExprs (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D157868?vs=549939=550659#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157868

Files:
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseCXXInlineMethods.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/AST/ast-dump-default-arg-recovery.cpp
  clang/test/Index/complete-optional-params.cpp

Index: clang/test/Index/complete-optional-params.cpp
===
--- clang/test/Index/complete-optional-params.cpp
+++ clang/test/Index/complete-optional-params.cpp
@@ -79,7 +79,7 @@
 // CHECK-CC5-NEXT: Objective-C interface
 
 // RUN: c-index-test -code-completion-at=%s:17:11 %s | FileCheck -check-prefix=CHECK-CC6 %s
-// CHECK-CC6: FunctionDecl:{ResultType void}{TypedText foo_2}{LeftParen (}{Optional {Placeholder Bar1 b1 = Bar1()}{Optional {Comma , }{Placeholder Bar2 b2}}}{RightParen )} (50)
+// CHECK-CC6: FunctionDecl:{ResultType void}{TypedText foo_2}{LeftParen (}{Optional {Placeholder Bar1 b1 = Bar1()}{Optional {Comma , }{Placeholder Bar2 b2 = Bar2()}}}{RightParen )} (50)
 // CHECK-CC6: Completion contexts:
 // CHECK-CC6-NEXT: Any type
 // CHECK-CC6-NEXT: Any value
Index: clang/test/AST/ast-dump-default-arg-recovery.cpp
===
--- /dev/null
+++ clang/test/AST/ast-dump-default-arg-recovery.cpp
@@ -0,0 +1,8 @@
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -ast-dump -frecovery-ast %s | FileCheck %s
+
+void foo();
+void fun(int arg = foo());
+//  CHECK: -ParmVarDecl {{.*}}  col:14 invalid arg 'int' cinit
+// CHECK-NEXT:   -RecoveryExpr {{.*}}  'int' contains-errors
+// Make sure we also preserve structure of the errorneous expression
+//  CHECK: -DeclRefExpr {{.*}}  'void ()' {{.*}} 'foo'
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -20,6 +20,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
@@ -37,11 +38,13 @@
 #include "clang/Sema/EnterExpressionEvaluationContext.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Ownership.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
@@ -329,23 +332,16 @@
   ParmVarDecl *Param = cast(param);
   UnparsedDefaultArgLocs.erase(Param);
 
-  auto Fail = [&] {
-Param->setInvalidDecl();
-Param->setDefaultArg(new (Context) OpaqueValueExpr(
-EqualLoc, Param->getType().getNonReferenceType(), VK_PRValue));
-  };
-
   // Default arguments are only permitted in C++
   if (!getLangOpts().CPlusPlus) {
 Diag(EqualLoc, diag::err_param_default_argument)
   << DefaultArg->getSourceRange();
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
   }
 
   // Check for unexpanded parameter packs.
-  if (DiagnoseUnexpandedParameterPack(DefaultArg, UPPC_DefaultArgument)) {
-return Fail();
-  }
+  if (DiagnoseUnexpandedParameterPack(DefaultArg, UPPC_DefaultArgument))
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   // C++11 [dcl.fct.default]p3
   //   A default argument expression [...] shall not be specified for a
@@ -360,14 +356,14 @@
 
   ExprResult Result = ConvertParamDefaultArgument(Param, DefaultArg, EqualLoc);
   if (Result.isInvalid())
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   DefaultArg = Result.getAs();
 
   // Check that the default argument is well-formed
   CheckDefaultArgumentVisitor DefaultArgChecker(*this, DefaultArg);
   if (DefaultArgChecker.Visit(DefaultArg))
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   SetParamDefaultArgument(Param, DefaultArg, EqualLoc);
 }
@@ -389,16 +385,23 @@
 
 /// ActOnParamDefaultArgumentError - Parsing or semantic analysis of
 /// the default argument for the parameter param failed.
-void Sema::ActOnParamDefaultArgumentError(Decl 

[PATCH] D157868: [clang] Use RecoveryExprs for broken defaultargs, instead of OpaqueValueExprs

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Still LG once you're happy




Comment at: clang/include/clang/Sema/Sema.h:3026
+  void ActOnParamDefaultArgumentError(Decl *param, SourceLocation EqualLoc,
+  ExprResult DefaultArg);
   ExprResult ConvertParamDefaultArgument(ParmVarDecl *Param, Expr *DefaultArg,

kadircet wrote:
> sammccall wrote:
> > nit: Nullable `ExprResult*` since there are only two states?
> > Extra get() in some callers, but less mysterious
> i guess you meant `Expr*` ?
Yes, sorry!



Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:400
   if (DefArgResult.isInvalid()) {
-Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
+Actions.ActOnParamDefaultArgumentError(Param, EqualLoc, DefArgResult);
   } else {

kadircet wrote:
> sammccall wrote:
> > DefArgResult is never anything here. I'd probably find 
> > `/*DefaultArg=*/nullptr` more obvious
> maybe i am missing something, but "invalid" doesn't mean "unusable".
ActionResult can be pointer-and-valid, null-and-valid, or null-and-invalid.

So invalid does indeed mean unusable ("usable" means pointer-and-valid).

Its documentation strongly suggests an ActionResult can be pointer-and-invalid, 
but good luck constructing one :-)



Comment at: clang/test/AST/ast-dump-default-arg-recovery.cpp:6
+//  CHECK: -ParmVarDecl {{.*}}  col:14 invalid arg 'int' 
cinit
+// CHECK-NEXT:   -RecoveryExpr {{.*}}  'int' contains-errors

Can you also check the contents (there should be a declrefexpr at least)?

Preserving this internal structure is important for clangd, the recoveryexpr 
alone doesn't help much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157868: [clang] Use RecoveryExprs for broken defaultargs, instead of OpaqueValueExprs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3026
+  void ActOnParamDefaultArgumentError(Decl *param, SourceLocation EqualLoc,
+  ExprResult DefaultArg);
   ExprResult ConvertParamDefaultArgument(ParmVarDecl *Param, Expr *DefaultArg,

sammccall wrote:
> nit: Nullable `ExprResult*` since there are only two states?
> Extra get() in some callers, but less mysterious
i guess you meant `Expr*` ?



Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:398
 DefArgResult = ParseAssignmentExpression();
-  DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult);
+  DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult, Param);
   if (DefArgResult.isInvalid()) {

sammccall wrote:
> If this fixes the recursive copy-constructor case you mentioned, can you add 
> a test?
> 
> (Else does it do anything? Or just cleanup)
no it doesn't. unfortunately that requires change in a separate code path to 
mark any method decls with invalid parmvardecls as invalid, which i'll try to 
put together. as that's the behavior for regular functiondecls, i don't see why 
it should be different for methods (if so we should probably change the 
behavior for functions instead).



Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:400
   if (DefArgResult.isInvalid()) {
-Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
+Actions.ActOnParamDefaultArgumentError(Param, EqualLoc, DefArgResult);
   } else {

sammccall wrote:
> DefArgResult is never anything here. I'd probably find 
> `/*DefaultArg=*/nullptr` more obvious
maybe i am missing something, but "invalid" doesn't mean "unusable".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157868: [clang] Use RecoveryExprs for broken defaultargs, instead of OpaqueValueExprs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 549939.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Use Expr* instead of ExprResult
- Add dump test to demonstrate new RecoveryExpr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157868

Files:
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseCXXInlineMethods.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/AST/ast-dump-default-arg-recovery.cpp
  clang/test/Index/complete-optional-params.cpp

Index: clang/test/Index/complete-optional-params.cpp
===
--- clang/test/Index/complete-optional-params.cpp
+++ clang/test/Index/complete-optional-params.cpp
@@ -79,7 +79,7 @@
 // CHECK-CC5-NEXT: Objective-C interface
 
 // RUN: c-index-test -code-completion-at=%s:17:11 %s | FileCheck -check-prefix=CHECK-CC6 %s
-// CHECK-CC6: FunctionDecl:{ResultType void}{TypedText foo_2}{LeftParen (}{Optional {Placeholder Bar1 b1 = Bar1()}{Optional {Comma , }{Placeholder Bar2 b2}}}{RightParen )} (50)
+// CHECK-CC6: FunctionDecl:{ResultType void}{TypedText foo_2}{LeftParen (}{Optional {Placeholder Bar1 b1 = Bar1()}{Optional {Comma , }{Placeholder Bar2 b2 = Bar2()}}}{RightParen )} (50)
 // CHECK-CC6: Completion contexts:
 // CHECK-CC6-NEXT: Any type
 // CHECK-CC6-NEXT: Any value
Index: clang/test/AST/ast-dump-default-arg-recovery.cpp
===
--- /dev/null
+++ clang/test/AST/ast-dump-default-arg-recovery.cpp
@@ -0,0 +1,6 @@
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -ast-dump -frecovery-ast %s | FileCheck %s
+
+void foo();
+void fun(int arg = foo());
+//  CHECK: -ParmVarDecl {{.*}}  col:14 invalid arg 'int' cinit
+// CHECK-NEXT:   -RecoveryExpr {{.*}}  'int' contains-errors
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -20,6 +20,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
@@ -37,11 +38,13 @@
 #include "clang/Sema/EnterExpressionEvaluationContext.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Ownership.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
@@ -329,23 +332,16 @@
   ParmVarDecl *Param = cast(param);
   UnparsedDefaultArgLocs.erase(Param);
 
-  auto Fail = [&] {
-Param->setInvalidDecl();
-Param->setDefaultArg(new (Context) OpaqueValueExpr(
-EqualLoc, Param->getType().getNonReferenceType(), VK_PRValue));
-  };
-
   // Default arguments are only permitted in C++
   if (!getLangOpts().CPlusPlus) {
 Diag(EqualLoc, diag::err_param_default_argument)
   << DefaultArg->getSourceRange();
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
   }
 
   // Check for unexpanded parameter packs.
-  if (DiagnoseUnexpandedParameterPack(DefaultArg, UPPC_DefaultArgument)) {
-return Fail();
-  }
+  if (DiagnoseUnexpandedParameterPack(DefaultArg, UPPC_DefaultArgument))
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   // C++11 [dcl.fct.default]p3
   //   A default argument expression [...] shall not be specified for a
@@ -360,14 +356,14 @@
 
   ExprResult Result = ConvertParamDefaultArgument(Param, DefaultArg, EqualLoc);
   if (Result.isInvalid())
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   DefaultArg = Result.getAs();
 
   // Check that the default argument is well-formed
   CheckDefaultArgumentVisitor DefaultArgChecker(*this, DefaultArg);
   if (DefaultArgChecker.Visit(DefaultArg))
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   SetParamDefaultArgument(Param, DefaultArg, EqualLoc);
 }
@@ -389,16 +385,23 @@
 
 /// ActOnParamDefaultArgumentError - Parsing or semantic analysis of
 /// the default argument for the parameter param failed.
-void Sema::ActOnParamDefaultArgumentError(Decl *param,
-  SourceLocation EqualLoc) {
+void Sema::ActOnParamDefaultArgumentError(Decl *param, SourceLocation EqualLoc,
+  Expr *DefaultArg) {
   if (!param)
 return;
 
   ParmVarDecl *Param = cast(param);
   Param->setInvalidDecl();
   

[PATCH] D157868: [clang] Use RecoveryExprs for broken defaultargs, instead of OpaqueValueExprs

[Sam McCall via Phabricator via cfe-commits]

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

Nice, recoveryexpr is a better fit here. These changes tend to cause occasional 
new error-handling crashes on under-tested paths, but I guess it's a good time 
in the release cycle for that!

You might want a clangd or ast-dump test showing that we now preserve the 
internal structure of (some) invalid default initializes.




Comment at: clang/include/clang/Sema/Sema.h:3026
+  void ActOnParamDefaultArgumentError(Decl *param, SourceLocation EqualLoc,
+  ExprResult DefaultArg);
   ExprResult ConvertParamDefaultArgument(ParmVarDecl *Param, Expr *DefaultArg,

nit: Nullable `ExprResult*` since there are only two states?
Extra get() in some callers, but less mysterious



Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:398
 DefArgResult = ParseAssignmentExpression();
-  DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult);
+  DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult, Param);
   if (DefArgResult.isInvalid()) {

If this fixes the recursive copy-constructor case you mentioned, can you add a 
test?

(Else does it do anything? Or just cleanup)



Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:400
   if (DefArgResult.isInvalid()) {
-Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
+Actions.ActOnParamDefaultArgumentError(Param, EqualLoc, DefArgResult);
   } else {

DefArgResult is never anything here. I'd probably find `/*DefaultArg=*/nullptr` 
more obvious



Comment at: clang/lib/Parse/ParseDecl.cpp:7478
+Actions.ActOnParamDefaultArgumentError(Param, EqualLoc,
+   DefArgResult);
 SkipUntil(tok::comma, tok::r_paren, StopAtSemi | StopBeforeMatch);

Ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157868: [clang] Use RecoveryExprs for broken defaultargs, instead of OpaqueValueExprs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: sammccall, aaron.ballman.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

This makes sure we can preserve invalid-ness for consumers of this
node, it prevents crashes. It also aligns better with rest of the places that
store invalid expressions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157868

Files:
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseCXXInlineMethods.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/Index/complete-optional-params.cpp

Index: clang/test/Index/complete-optional-params.cpp
===
--- clang/test/Index/complete-optional-params.cpp
+++ clang/test/Index/complete-optional-params.cpp
@@ -79,7 +79,7 @@
 // CHECK-CC5-NEXT: Objective-C interface
 
 // RUN: c-index-test -code-completion-at=%s:17:11 %s | FileCheck -check-prefix=CHECK-CC6 %s
-// CHECK-CC6: FunctionDecl:{ResultType void}{TypedText foo_2}{LeftParen (}{Optional {Placeholder Bar1 b1 = Bar1()}{Optional {Comma , }{Placeholder Bar2 b2}}}{RightParen )} (50)
+// CHECK-CC6: FunctionDecl:{ResultType void}{TypedText foo_2}{LeftParen (}{Optional {Placeholder Bar1 b1 = Bar1()}{Optional {Comma , }{Placeholder Bar2 b2 = Bar2()}}}{RightParen )} (50)
 // CHECK-CC6: Completion contexts:
 // CHECK-CC6-NEXT: Any type
 // CHECK-CC6-NEXT: Any value
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -20,6 +20,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
@@ -37,11 +38,13 @@
 #include "clang/Sema/EnterExpressionEvaluationContext.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Ownership.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
@@ -329,23 +332,16 @@
   ParmVarDecl *Param = cast(param);
   UnparsedDefaultArgLocs.erase(Param);
 
-  auto Fail = [&] {
-Param->setInvalidDecl();
-Param->setDefaultArg(new (Context) OpaqueValueExpr(
-EqualLoc, Param->getType().getNonReferenceType(), VK_PRValue));
-  };
-
   // Default arguments are only permitted in C++
   if (!getLangOpts().CPlusPlus) {
 Diag(EqualLoc, diag::err_param_default_argument)
   << DefaultArg->getSourceRange();
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
   }
 
   // Check for unexpanded parameter packs.
-  if (DiagnoseUnexpandedParameterPack(DefaultArg, UPPC_DefaultArgument)) {
-return Fail();
-  }
+  if (DiagnoseUnexpandedParameterPack(DefaultArg, UPPC_DefaultArgument))
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   // C++11 [dcl.fct.default]p3
   //   A default argument expression [...] shall not be specified for a
@@ -360,14 +356,14 @@
 
   ExprResult Result = ConvertParamDefaultArgument(Param, DefaultArg, EqualLoc);
   if (Result.isInvalid())
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   DefaultArg = Result.getAs();
 
   // Check that the default argument is well-formed
   CheckDefaultArgumentVisitor DefaultArgChecker(*this, DefaultArg);
   if (DefaultArgChecker.Visit(DefaultArg))
-return Fail();
+return ActOnParamDefaultArgumentError(param, EqualLoc, DefaultArg);
 
   SetParamDefaultArgument(Param, DefaultArg, EqualLoc);
 }
@@ -389,16 +385,24 @@
 
 /// ActOnParamDefaultArgumentError - Parsing or semantic analysis of
 /// the default argument for the parameter param failed.
-void Sema::ActOnParamDefaultArgumentError(Decl *param,
-  SourceLocation EqualLoc) {
+void Sema::ActOnParamDefaultArgumentError(Decl *param, SourceLocation EqualLoc,
+  ExprResult DefaultArg) {
   if (!param)
 return;
 
   ParmVarDecl *Param = cast(param);
   Param->setInvalidDecl();
   UnparsedDefaultArgLocs.erase(Param);
-  Param->setDefaultArg(new (Context) OpaqueValueExpr(
-  EqualLoc, Param->getType().getNonReferenceType(), VK_PRValue));
+  ExprResult RE;
+  if (DefaultArg.isUsable()) {
+RE = CreateRecoveryExpr(EqualLoc, DefaultArg.get()->getEndLoc(),
+{DefaultArg.get()},
+