[AST] Preserve the invalid initializer for auto VarDecl.

[hokein]

Summary

fixes https://github.com/clangd/clangd/issues/330

[sammccall]

Nice!

Yet another case of "do we need to stop here" though. Do you have a doc or bug or somewhere to collect the different directions we could/should extend in?

clang/lib/Sema/SemaDecl.cpp

11852

Do you have an example to back this up? (Not to include in the comment, just curious).

ISTM when there are errors in the input, you can get "lucky" and they turn into recoveryexpr and survive here, or you get "unlucky" and they turn into typoexpr and are dropped here after correction fails. And lucky/unlucky might mostly depend on details about the type of error, not how ill-formed the code is?

This suggests two possible recovery strategies:

• if CorrectDelayedTyposInExpr fails typo-correction, it could rebuild with the typos "degraded" to RecoveryExpr of some sort
• or we could just wrap Init in RecoveryExpr, TypoExprs and all. With the assumption that stuff that doesn't handle TypoExpr should either treate RecoveryExpr as opaque or should bail out at the top-level based on ContainsErrors.

The former seems more precise and more general, probably more work too.

None of this needs to be addressed in this patch except maybe a FIXME and/or a tweak to the existing comment.

[sammccall]

Oops, intended to accept this (with the comment above)

[hokein]

clang/lib/Sema/SemaDecl.cpp

11852

Do you have an example to back this up?

we have an example, see the test.

auto unresolved_typo = gned.*[] {}; the initExpr looks like below, it seems less useful. I'm not sure it is worth the effort to preserve the AST if the typo correction is not resolved. it requires some work either #1 or #2.

BinaryOperator 0x8992288 '<dependent type>' contains-errors '.*'
|-TypoExpr 0x8991a78 '<dependent type>' contains-errors lvalue
`-LambdaExpr 0x8992150 'class (lambda at /tmp/t4.cpp:11:32)'
  |-CXXRecordDecl 0x8991b30  implicit class definition
  | |-DefinitionData lambda pass_in_registers empty standard_layout trivially_copyable literal can_const_default_init
  | | |-DefaultConstructor defaulted_is_constexpr
  | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | | |-MoveConstructor exists simple trivial needs_implicit
  | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  | | |-MoveAssignment
  | | `-Destructor simple irrelevant trivial
  | |-CXXMethodDecl 0x8991c70  constexpr operator() 'auto (void) const -> void' inline
  | | `-CompoundStmt 0x8991d20
  | |-CXXConversionDecl 0x8991fe8  implicit constexpr operator void (*)() 'auto (*(void) const noexcept)(void) -> void' inline
  | |-CXXMethodDecl 0x8992098  implicit __invoke 'auto (void) -> void' static inline
  | `-CXXDestructorDecl 0x8992180  implicit referenced ~ 'void (void) noexcept' inline default trivial
  `-CompoundStmt 0x8991d20

And lucky/unlucky might mostly depend on details about the type of error, not how ill-formed the code is?

probably, but this also implies our current recovery strategy can't handle this case well.

clang/lib/Sema/SemaDecl.cpp

void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
   }
 
   // C++11 [decl.spec.auto]p6. Deduce the type which 'auto' stands in for.
   if (VDecl->getType()->isUndeducedType()) {
     // Attempt typo correction early so that the type of the init expression can
     // be deduced based on the chosen correction if the original init contains a
     // TypoExpr.
     ExprResult Res = CorrectDelayedTyposInExpr(Init, VDecl);
-    if (!Res.isUsable() || Res.get()->containsErrors()) {
+    if (!Res.isUsable()) {
+      // There are unresolved typos in Init, just drop them.
+      // FIXME: improve the recovery strategy to preserve the Init.
       RealDecl->setInvalidDecl();
       return;
     }
+    if (Res.get()->containsErrors()) {
+      // Invalidate the decl as we don't know the type for recovery-expr yet.
+      RealDecl->setInvalidDecl();
+      VDecl->setInit(Res.get());
+      return;
+    }
     Init = Res.get();
 
     if (DeduceVariableDeclarationType(VDecl, DirectInit, Init))
       return;
   }
 
   // dllimport cannot be used on variable definitions.
   if (VDecl->hasAttr<DLLImportAttr>() && !VDecl->isStaticDataMember()) {

clang/test/AST/ast-dump-expr-errors.cpp

 // CHECK:     |-VarDecl {{.*}} d
 // CHECK-NEXT:| `-CXXStaticCastExpr {{.*}} 'int' contains-errors
 // CHECK-NEXT:|   `-BinaryOperator {{.*}} '<dependent type>' contains-errors '+'
 // CHECK-NEXT:|     |-RecoveryExpr {{.*}} '<dependent type>' contains-errors
 // CHECK-NEXT:|     | `-UnresolvedLookupExpr {{.*}} 'bar'
 // CHECK-NEXT:|     `-IntegerLiteral {{.*}} 1
 int d = static_cast<int>(bar() + 1);
 
-// FIXME: store initializer even when 'auto' could not be deduced.
-// Expressions with errors currently do not keep initializers around.
-// CHECK: -VarDecl {{.*}} invalid e 'auto'
-auto e = bar();
 
 // Error type should result in an invalid decl.
 // CHECK: -VarDecl {{.*}} invalid f 'decltype(<recovery-expr>(bar))'
 decltype(bar()) f;

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

void InvalidInitalizer(int x) {
   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()};
 }
+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'
+  // CHECK-NEXT: `-CallExpr {{.*}} '<dependent type>' contains-errors
+  // CHECK-NEXT:   |-UnresolvedLookupExpr {{.*}} 'some_func'
+  // CHECK-NEXT:   `-RecoveryExpr {{.*}} '<dependent type>' contains-errors
+  // CHECK-NEXT:     `-UnresolvedLookupExpr {{.*}} 'invalid'
+  auto b = some_func(invalid());
+
+  decltype(ned);
+  // very bad initailizer: there is an unresolved typo expr internally, we just
+  // drop it.
+  // CHECK: `-VarDecl {{.*}} invalid unresolved_typo 'auto'
+  auto unresolved_typo = gned.*[] {};
+}