[AST][RecoveryExpr] Preserve the invalid "undef_var" initializer.

[hokein]

Summary

And don't invalidate the VarDecl if the type is known.

[hokein]

clang/include/clang/Sema/Sema.h

3900

not quite happy with the naming, open for suggestions.

[sammccall]

Very nice! Just want to give some thought to what these callsites look like.

clang/include/clang/Sema/Sema.h

3899

This definitely seems like something that makes sense to call in more places. Wonder if the API can be smoother.

Both of the examples are of the form: CorrectDelayedTypos, if invalid, rebuildTypos.
Are *all* likely cases of this form? (Could make it an optional part of CorrectDelayedTypos)
Are there any places where you want CorrectDelayedTypos and don't want rebuild? (Could make it an eventually-mandatory part of CorrectDelayedTypos)

3900

given current API, I'd suggest RecoverFromUncorrectedTypos or so

[hokein]

clang/include/clang/Sema/Sema.h

3899

Are *all* likely cases of this form? (Could make it an optional part of CorrectDelayedTypos)

I think so (at least for the moment).

Are there any places where you want CorrectDelayedTypos and don't want rebuild? (Could make it an eventually-mandatory part of CorrectDelayedTypos)

yeah, I think the eventual goal is to make the rebuild as the default fallback when typo correction fails, but it is still too far away to do it at the moment -- many testcases are broken (some are crashes, some are diagnostics improvements) when turning on recovery-ast by default.

I suppose we'd need to fix that case by case until it is ready, given this situation, adding an option parameter (recoveryUncorrectedTypos) to CorrectDelayedTyposInExpr API seems a feasible solution.

[hokein]

Make recovery fallback as part of the CorrectDelayedTyposInExpr.

[sammccall]

LG once the overload set is fixed

clang/include/clang/Sema/Sema.h

3873

I think it's too confusing to have multiple overloads with different subsets of the parameters possible and also in different orders :-(

If there's nothing better, you could replace this with two overloads

CorrectDelayedTyposInExpr(ER, Filter)
CorrectDelayedTyposInExpr(ER, bool, Filter)

clang/lib/Sema/SemaDecl.cpp

12014

could consider splitting this change out of the refactoring (or vice versa), up to you

clang/lib/Sema/SemaExprCXX.cpp

8310

/*RecoverUncorrectedTypos=*/

[hokein]

clang/include/clang/Sema/Sema.h

3873

This method doesn't have too many usages, I'd just remove it, which leaves us two overloads (one for Expr, the other one for ExprResult).

clang/lib/Sema/SemaDecl.cpp

12014

split the refactoring change in https://reviews.llvm.org/D82047

[sammccall]

(this looks reasonable now but it'd be nice to have a rebased diff!)

[hokein]

rebase to head.

clang/include/clang/Sema/Sema.h

CorrectDelayedTyposInExpr(ExprResult ER, VarDecl *InitDecl = nullptr,
                                 [](Expr *E) -> ExprResult { return E; }) {
     return ER.isInvalid() ? ER : CorrectDelayedTyposInExpr(ER.get(), Filter);
   }
 
   ExprResult
   CorrectDelayedTyposInExpr(ExprResult ER,
                             llvm::function_ref<ExprResult(Expr *)> Filter) {
     return CorrectDelayedTyposInExpr(ER, nullptr, Filter);
   }
 
   void diagnoseTypo(const TypoCorrection &Correction,
                     const PartialDiagnostic &TypoDiag,
                     bool ErrorRecovery = true);
 
   void diagnoseTypo(const TypoCorrection &Correction,
                     const PartialDiagnostic &TypoDiag,
                     const PartialDiagnostic &PrevNote,
public:
   void FilterLookupForScope(LookupResult &R, DeclContext *Ctx, Scope *S,
                             bool ConsiderLinkage, bool AllowInlineNamespace);
 
   bool CheckRedeclarationModuleOwnership(NamedDecl *New, NamedDecl *Old);
 
   void DiagnoseAmbiguousLookup(LookupResult &Result);
   //@}
 
+  /// Rebuild the given Expr with the TypoExpr degraded to RecoveryExpr.
+  ExprResult rebuildTypoExprs(Expr *TypoExpr);
   /// Attempts to produce a RecoveryExpr after some AST node cannot be created.
   ExprResult CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
                                 ArrayRef<Expr *> SubExprs,
                                 QualType T = QualType());
 
   ObjCInterfaceDecl *getObjCInterfaceDecl(IdentifierInfo *&Id,
                                           SourceLocation IdLoc,
                                           bool TypoCorrection = false);

clang/lib/Sema/SemaDecl.cpp

if (!VDecl->isInvalidDecl()) {
     MultiExprArg Args = Init;
     if (CXXDirectInit)
       Args = MultiExprArg(CXXDirectInit->getExprs(),
                           CXXDirectInit->getNumExprs());
 
     // Try to correct any TypoExprs in the initialization arguments.
     for (size_t Idx = 0; Idx < Args.size(); ++Idx) {
       ExprResult Res = CorrectDelayedTyposInExpr(
           Args[Idx], VDecl, [this, Entity, Kind](Expr *E) {
             InitializationSequence Init(*this, Entity, Kind, MultiExprArg(E));
             return Init.Failed() ? ExprError() : E;
           });
+      if (Res.isInvalid())
+        Res = rebuildTypoExprs(Args[Idx]);
+
       if (Res.isInvalid()) {
         VDecl->setInvalidDecl();
       } else if (Res.get() != Args[Idx]) {
         Args[Idx] = Res.get();
       }
     }
     if (VDecl->isInvalidDecl())
       return;

clang/lib/Sema/SemaExpr.cpp

if (getCurFunctionOrMethodDecl())
     getEnclosingFunction()->HasPotentialAvailabilityViolations = true;
   else if (getCurBlock() || getCurLambda())
     getCurFunction()->HasPotentialAvailabilityViolations = true;
 
   return new (Context)
       ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy);
 }
 
+ExprResult Sema::rebuildTypoExprs(Expr *TypoExpr) {
+  struct TyposReplace : TreeTransform<TyposReplace> {
+    TyposReplace(Sema &SemaRef) : TreeTransform(SemaRef) {}
+    ExprResult TransformTypoExpr(clang::TypoExpr *E) {
+      return this->SemaRef.CreateRecoveryExpr(E->getBeginLoc(), E->getEndLoc(),
+                                              {});
+    }
+  } TT(*this);
+  return TT.TransformExpr(TypoExpr);
+}
+
 ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
                                     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();
 
   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/SemaExprCXX.cpp

if (DiscardedValue) {
     if (FullExpr.isInvalid())
       return ExprError();
 
     DiagnoseUnusedExprResult(FullExpr.get());
   }
 
   FullExpr = CorrectDelayedTyposInExpr(FullExpr.get());
   if (FullExpr.isInvalid()) {
-    // Typo-correction fails, we rebuild the broken AST with the typos degraded
-    // to RecoveryExpr.
-    struct TyposReplace : TreeTransform<TyposReplace> {
-      TyposReplace(Sema &SemaRef) : TreeTransform(SemaRef) {}
-      ExprResult TransformTypoExpr(TypoExpr *E) {
-        return this->SemaRef.CreateRecoveryExpr(E->getBeginLoc(),
-                                                E->getEndLoc(), {});
-      }
-    } TT(*this);
-
-    return TT.TransformExpr(FE);
+    return rebuildTypoExprs(FE);
   }
 
   CheckCompletedExpr(FullExpr.get(), CC, IsConstexpr);
 
   // At the end of this full expression (which could be a deeply nested
   // lambda), if there is a potential capture within the nested lambda,
   // have the outer capture-able lambda try and capture it.
   // Consider the following code:
   // void f(int, int);
   // void f(const int&, double);

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

void test_invalid_call(int s) {
   some_func(undef1, undef2+1);
 
   // CHECK:      BinaryOperator {{.*}} '<dependent type>' contains-errors '='
   // CHECK-NEXT: |-DeclRefExpr {{.*}} 's'
   // CHECK-NEXT: `-CallExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT:   |-UnresolvedLookupExpr {{.*}} 'some_func'
   // CHECK-NEXT:   `-RecoveryExpr {{.*}} contains-errors
   s = some_func(undef1);
-  // CHECK: `-VarDecl {{.*}} invalid var 'int'
-  // FIXME: preserve the broken call.
+
+  // CHECK:     VarDecl {{.*}} var 'int'
+  // CHECK-NEXT: `-CallExpr {{.*}} '<dependent type>' contains-errors
+  // CHECK-NEXT:   |-UnresolvedLookupExpr {{.*}} 'some_func'
+  // CHECK-NEXT:   `-RecoveryExpr {{.*}} contains-errors
   int var = some_func(undef1);
 }
 
 int ambig_func(double);
 int ambig_func(float);
 
 // CHECK:     VarDecl {{.*}} ambig_call
 // CHECK-NEXT: `-ImplicitCastExpr {{.*}} 'int' contains-errors
void InvalidInitalizer(int x) {
   // CHECK-NEXT:     `-UnresolvedLookupExpr {{.*}} 'invalid'
   Bar b5 = Bar(invalid());
   // CHECK:     `-VarDecl {{.*}} b6 'Bar'
   // CHECK-NEXT: `-CXXUnresolvedConstructExpr {{.*}} 'Bar' contains-errors 'Bar'
   // CHECK-NEXT:  `-InitListExpr {{.*}} contains-errors
   // CHECK-NEXT:   `-RecoveryExpr {{.*}} contains-errors
   // CHECK-NEXT:     `-UnresolvedLookupExpr {{.*}} 'invalid'
   Bar b6 = Bar{invalid()};
+
+  // CHECK:     `-VarDecl {{.*}} var1
+  // CHECK-NEXT: `-BinaryOperator {{.*}} '<dependent type>' contains-errors
+  // CHECK-NEXT:   |-RecoveryExpr {{.*}} '<dependent type>' contains-errors
+  // CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+  int var1 = undef + 1;
 }
 void InitializerForAuto() {
   // CHECK:     `-VarDecl {{.*}} invalid a 'auto'
   // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT:   `-UnresolvedLookupExpr {{.*}} 'invalid'
   auto a = invalid();
 
   // CHECK:     `-VarDecl {{.*}} invalid b 'auto'