[AST] dont invaliate VarDecl when the initializer contains errors.

[hokein]

Summary

This might be splitted into 2 separate patchs:

1. the initializer of a variable should play no part in decl "invalid" bit;
2. preserve the exprs in an error initializer via recovery exprs;

if we do 1) only, we will regress the diagnostics (one big regression is that
we loose the "selected 'begin' function with iterator type" diagnostic in for-range stmt;
with 2) together, we don't have regressions (the new diagnostics seems to be
improved).

[sammccall]

clang/lib/Sema/SemaDecl.cpp

11998

if the variable is an undeduced auto (and the type of the recoveryexpr is unknown), we still have to mark it invalid. Does that happen somewhere else?

clang/test/AST/ast-dump-invalid-initialized.cpp

21

what actually is the AST here? invalid() is a recoveryexpr, does it get wrapped in a second recoveryexpr by the code in this patch? Or is this testing existing behavior?

clang/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp

3

not sure we should be switching this until we flip the default.
Are we regressing the diagnostics under default flags?

(and in 2 other files)

[hokein]

address comments

[hokein]

clang/lib/Sema/SemaDecl.cpp

11998

Yeah, the auto case is handled earlier (Line 11845 of this file), so we are safe at this point.

clang/test/AST/ast-dump-invalid-initialized.cpp

21

the AST looks like:

-VarDecl 0x9b80970 <col:3, col:17> col:5 a6 'A' callinit
       `-ParenListExpr 0x9b80ac0 <col:7, col:17> 'NULL TYPE' contains-errors
         `-RecoveryExpr 0x9b80a78 <col:8, col:16> '<dependent type>' contains-errors lvalue
           `-UnresolvedLookupExpr 0x9b809d8 <col:8> '<overloaded function type>' lvalue (ADL) = 'invalid' empty

my attemption was to verify the valid-bit of VarDecl, but they can be covered by ast-dump-recovery.cpp.

clang/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp

3

yes, we have a few regressions without -frecovery-ast, the worst regression is that we stop emitting the "selected 'begin' function with iterator type 'Data *'" diagnostic note in an ill-formed range-range stmt.

As discussed in another patch, lets not change the default flag in existing tests, reverted the new flag, adjusted exiting tests, and created a new test for -frecovery-ast.

[sammccall]

Most of the new diagnostics look good, as you say.

I'm okay with the regression around foreach loops, given that:

• per rsmith, the mechanism behind this diagnostic was misdesigned (relying on initializer making things invalid)
• it's a C++-only features and we should be able to flip on recoveryexpr for C++ soon

clang/test/SemaCXX/constant-expression-cxx11.cpp

880–882

I'm not *sure* what the purpose of these test is, but my guess is they're mostly trying to test nullpointer comparisons not error recovery. maybe we should have 1 with nullB and the error, and the rest use nullB1?

[sammccall]

oops, meant to approve

clang/lib/Sema/SemaDecl.cpp

if (!VDecl->isInvalidDecl()) {
     }
     if (VDecl->isInvalidDecl())
       return;
 
     InitializationSequence InitSeq(*this, Entity, Kind, Args,
                                    /*TopLevelOfInitList=*/false,
                                    /*TreatUnavailableAsInvalid=*/false);
     ExprResult Result = InitSeq.Perform(*this, Entity, Kind, Args, &DclT);
     if (Result.isInvalid()) {
-      VDecl->setInvalidDecl();
+      // If the provied initializer fails to initialize the var decl,
+      // we attach a recovery expr for better recovery.
+      auto RecoveryExpr =
+          CreateRecoveryExpr(Init->getBeginLoc(), Init->getEndLoc(), Args);
+      if (RecoveryExpr.get())
+        VDecl->setInit(RecoveryExpr.get());
       return;
     }
 
     Init = Result.getAs<Expr>();
   }
 
   // Check for self-references within variable initializers.
   // Variables declared within a function/method body (except for references)

clang/lib/Sema/SemaStmt.cpp

if (DerefExpr.isInvalid()) {
       NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
       return StmtError();
     }
 
     // Attach  *__begin  as initializer for VD. Don't touch it if we're just
     // trying to determine whether this would be a valid range.
     if (!LoopVar->isInvalidDecl() && Kind != BFRK_Check) {
       AddInitializerToDecl(LoopVar, DerefExpr.get(), /*DirectInit=*/false);
-      if (LoopVar->isInvalidDecl())
+      if (LoopVar->isInvalidDecl() ||
+          (LoopVar->getInit() && LoopVar->getInit()->containsErrors()))
         NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
     }
   }
 
   // Don't bother to actually allocate the result if we're just trying to
   // determine whether it would be valid.
   if (Kind == BFRK_Check)
     return StmtResult();

clang/test/AST/ast-dump-invalid-initialized.cpp

 // RUN: not %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -fcxx-exceptions -std=gnu++17 -ast-dump %s | FileCheck -strict-whitespace %s
 
 struct A { A(int, int) {} };
 class ForwardDecl;
 
 void test() {
+  // Verify the valid-bit of the VarDecl.
+
   // CHECK: `-VarDecl {{.*}} a1 'A'
   A a1;
   // CHECK: `-VarDecl {{.*}} a2 'const A'
   const A a2;
   // CHECK: `-VarDecl {{.*}} a3 'A'
   A a3 = garbage();
 
 
   // CHECK: `-VarDecl {{.*}} invalid b1 'const A &'
   const A& b1;
   // CHECK: `-VarDecl {{.*}} invalid b2 'ForwardDecl'
   ForwardDecl b2;
+  // CHECK: `-VarDecl {{.*}} invalid b3 'auto'
+  auto b3 = garbage();
+  // CHECK: `-VarDecl {{.*}} invalid b4 'auto'
+  auto b4 = A(1);
+  // CHECK: `-VarDecl {{.*}} invalid b5 'auto'
+  auto b5 = A{1};
 }
 No newline at end of file

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

void test(int x) {
   foo.abc;
   foo->func(x);
 }
 
 // CHECK:     |-AlignedAttr {{.*}} alignas
 // CHECK-NEXT:| `-RecoveryExpr {{.*}} contains-errors
 // CHECK-NEXT:|   `-UnresolvedLookupExpr {{.*}} 'invalid'
 struct alignas(invalid()) Aligned {};
+
+void InvalidInitalizer(int x) {
+  struct Bar { Bar(); };
+  // CHECK:     `-VarDecl {{.*}} a1 'Bar'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:  `-IntegerLiteral {{.*}} 'int' 1
+  Bar a1(1);
+  // CHECK:     `-VarDecl {{.*}} a2 'Bar'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:  `-DeclRefExpr {{.*}} 'x'
+  Bar a2(x);
+  // CHECK:     `-VarDecl {{.*}} a3 'Bar'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:  `-InitListExpr
+  // CHECK-NEDT:   `-DeclRefExpr {{.*}} 'x'
+  Bar a3{x};
+  // CHECK:     `-VarDecl {{.*}} a4 'Bar'
+  // CHECK-NEXT: `-ParenListExpr {{.*}} 'NULL TYPE' contains-errors
+  // CHECK-NEXT:  `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:   `-UnresolvedLookupExpr {{.*}} 'invalid'
+  Bar a4(invalid());
+  // CHECK:     `-VarDecl {{.*}} a5 'Bar'
+  // CHECK-NEXT: `-InitListExpr {{.*}} contains-errors
+  // CHECK-NEXT:  `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:   `-UnresolvedLookupExpr {{.*}} 'invalid'
+  Bar a5{invalid()};
+
+  // CHECK:     `-VarDecl {{.*}} b1 'Bar'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:  `-IntegerLiteral {{.*}} 'int' 1
+  Bar b1 = 1;
+  // CHECK:     `-VarDecl {{.*}} b2 'Bar'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:  `-InitListExpr
+  Bar b2 = {1};
+  // FIXME: preserve the invalid initializer.
+  // CHECK: `-VarDecl {{.*}} b3 'Bar'
+  Bar b3 = Bar(x);
+  // FIXME: preserve the invalid initializer.
+  // CHECK: `-VarDecl {{.*}} b4 'Bar'
+  Bar b4 = Bar{x};
+  // CHECK:     `-VarDecl {{.*}} b5 'Bar'
+  // CHECK-NEXT: `-CXXUnresolvedConstructExpr {{.*}} 'Bar' contains-errors 'Bar'
+  // CHECK-NEXT:   `-RecoveryExpr {{.*}} contains-errors
+  // 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()};
+}

clang/test/CXX/special/class.copy/p11.0x.move.cpp

 
 // Note, move constructor is not a candidate because it is deleted.
 template<typename T> struct DeletedNTVariant3 { // expected-note 2{{default}} expected-note 2{{copy}}
   union {
     T NT;
   };
 };
 extern DeletedNTVariant3<NonTrivial> dntv3a(0); // expected-error {{no matching}}
-extern DeletedNTVariant3<DeletedCopy> dntv3a(0); // expected-error {{no matching}}
+extern DeletedNTVariant3<DeletedCopy> dntv3b(0); // expected-error {{no matching}}
 
 // -- a non-static data member of class type M (or array thereof) that cannot be
 //    copied because overload resolution results in an ambiguity or a function
 //    that is deleted or inaccessible
 struct NoAccess {
   NoAccess() = default;
 private:
   NoAccess(NoAccess&&);

clang/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -std=c++14 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
 
 struct pr12960 {
   int begin;
   void foo(int x) {
     for (int& it : x) { // expected-error {{invalid range expression of type 'int'; no viable 'begin' function available}}
     }
   }
 };
 
 struct null_t {
   operator int*();
 };
 
 namespace X {
   template<typename T>
     auto begin(T &&t) -> decltype(t.begin()) { return t.begin(); } // expected-note 2{{ignored: substitution failure}}
   template<typename T>
     auto end(T &&t) -> decltype(t.end()) { return t.end(); } // expected-note {{candidate template ignored: substitution failure [with T = }}
 
   template<typename T>
-    auto begin(T &&t) -> decltype(t.alt_begin()) { return t.alt_begin(); } // expected-note {{selected 'begin' template [with T = }} \
-                                                                              expected-note 2{{candidate template ignored: substitution failure [with T = }}
+    auto begin(T &&t) -> decltype(t.alt_begin()) { return t.alt_begin(); } // expected-note 2{{candidate template ignored: substitution failure [with T = }}
   template<typename T>
     auto end(T &&t) -> decltype(t.alt_end()) { return t.alt_end(); } // expected-note {{candidate template ignored: substitution failure [with T = }}
 
   namespace inner {
     // These should never be considered.
     int begin(int);
     int end(int);
   }
 
   using namespace inner;
 
   struct A { // expected-note 2 {{candidate constructor}}
     A();
-    int *begin(); // expected-note 3{{selected 'begin' function with iterator type 'int *'}} expected-note {{'begin' declared here}}
+    int *begin(); // expected-note {{selected 'begin' function with iterator type 'int *'}} expected-note {{'begin' declared here}}
     int *end();
   };
 
   struct B {
     B();
     int *alt_begin();
     int *alt_end();
   };

clang/test/OpenMP/task_messages.cpp

 template <class T>
 int foo() {
   T a;
   T &b = a;
   int r;
   S1 s1;
 // expected-error@+1 2 {{call to deleted constructor of 'S1'}}
 #pragma omp task
-// expected-note@+1 2 {{predetermined as a firstprivate in a task construct here}}
   ++s1;
 #pragma omp task default(none) // expected-note 2 {{explicit data sharing attribute requested here}}
 #pragma omp task default(shared)
   ++a; // expected-error 2 {{variable 'a' must have explicitly specified data sharing attributes}}
 #pragma omp task default(none) // expected-note 2 {{explicit data sharing attribute requested here}}
 #pragma omp task
   // expected-error@+1 {{calling a private constructor of class 'S'}}
   ++a; // expected-error 2 {{variable 'a' must have explicitly specified data sharing attributes}}

clang/test/SemaCXX/block-call.cpp

 // RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s -fblocks
 
 int (*FP)();
 int (^IFP) ();
 int (^II) (int);
 int main() {
   int (*FPL) (int) = FP; // expected-error {{cannot initialize a variable of type 'int (*)(int)' with an lvalue of type 'int (*)()'}} 
 
   // For Blocks, the ASTContext::typesAreBlockCompatible() makes sure this is an error.
   int (^PFR) (int) = IFP; // expected-error {{cannot initialize a variable of type 'int (^)(int)' with an lvalue of type 'int (^)()'}}
   PFR = II;       // OK
 
-  int (^IFP) () = PFR; // OK
-
 
   const int (^CIC) () = IFP; // OK -  initializing 'const int (^)()' with an expression of type 'int (^)()'}}
 
   const int (^CICC) () = CIC;
 
 
   int * const (^IPCC) () = 0;
 
   int * const (^IPCC1) () = IPCC;
 
   int * (^IPCC2) () = IPCC;       // expected-error  {{cannot initialize a variable of type 'int *(^)()' with an lvalue of type 'int *const (^)()'}}
 
   int (^IPCC3) (const int) = PFR;
 
   int (^IPCC4) (int, char (^CArg) (double));
 
   int (^IPCC5) (int, char (^CArg) (double)) = IPCC4;
 
   int (^IPCC6) (int, char (^CArg) (float))  = IPCC4; // expected-error {{cannot initialize a variable of type 'int (^)(int, char (^)(float))' with an lvalue of type}}
 
-  IPCC2 = 0;
-  IPCC2 = 1; 
+  IPCC2 = 0; // OK - assign a nullptr to a pointer.
+  IPCC2 = 1; // expected-error {{invalid block pointer conversion assigning to 'int *(^)()' from 'int'}}
   int (^x)() = 0;
   int (^y)() = 3;   // expected-error {{cannot initialize a variable of type 'int (^)()' with an rvalue of type 'int'}}
   int a = 1;
   int (^z)() = a+4;   // expected-error {{cannot initialize a variable of type 'int (^)()' with an rvalue of type 'int'}}
 }
 
 int blah() {
   int (^IFP) (float);
   char (^PCP)(double, double, char);
 
   IFP(1.0);
   IFP (1.0, 2.0); // expected-error {{too many arguments to block call}}
 
   char ch = PCP(1.0, 2.0, 'a');
   return PCP(1.0, 2.0);   // expected-error {{too few arguments to block}}
 }

clang/test/SemaCXX/constant-expression-cxx11.cpp

 static_assert(&ok2 == pok2, "");
 static_assert((Base2*)(Derived*)(Base*)pb1 == pok2, "");
 static_assert((Derived*)(Base*)pb1 == (Derived*)pok2, "");
 
 // Core issue 903: we do not perform constant evaluation when checking for a
 // null pointer in C++11. Just check for an integer literal with value 0.
 constexpr Base *nullB = 42 - 6 * 7; // expected-error {{cannot initialize a variable of type 'Class::Base *const' with an rvalue of type 'int'}}
 constexpr Base *nullB1 = 0;
-static_assert((Bottom*)nullB == 0, "");
-static_assert((Derived*)nullB == 0, "");
-static_assert((void*)(Bottom*)nullB == (void*)(Derived*)nullB, "");
+static_assert((Bottom*)nullB == 0, ""); // expected-error {{static_assert expression is not an integral constant expression}}
+static_assert((Derived*)nullB1 == 0, "");
+static_assert((void*)(Bottom*)nullB1 == (void*)(Derived*)nullB1, "");
 Base *nullB2 = '\0'; // expected-error {{cannot initialize a variable of type 'Class::Base *' with an rvalue of type 'char'}}
 Base *nullB3 = (0);
 Base *nullB4 = false; // expected-error {{cannot initialize a variable of type 'Class::Base *' with an rvalue of type 'bool'}}
 Base *nullB5 = ((0ULL));
 Base *nullB6 = 0.; // expected-error {{cannot initialize a variable of type 'Class::Base *' with an rvalue of type 'double'}}
 enum Null { kNull };
 Base *nullB7 = kNull; // expected-error {{cannot initialize a variable of type 'Class::Base *' with an rvalue of type 'Class::Null'}}
 static_assert(nullB1 == (1 - 1), ""); // expected-error {{comparison between pointer and integer}}

clang/test/SemaCXX/cxx11-crashes.cpp

 }
 
 }
 
 namespace b6981007 {
   struct S {}; // expected-note 3{{candidate}}
   void f() {
     S s(1, 2, 3); // expected-error {{no matching}}
-    for (auto x : s) {
+    for (auto x : s) { // expected-error {{invalid range expression of}}
       // We used to attempt to evaluate the initializer of this variable,
       // and crash because it has an undeduced type.
       const int &n(x);
     }
   }
 }
 
 namespace incorrect_auto_type_deduction_for_typo {

clang/test/SemaCXX/cxx2a-explicit-bool.cpp

 A<true> a1( 0);
 A<true> && a2 = 0;// expected-error {{could not bind}}
 A<true> && a3( 0);// expected-error {{could not bind}}
 A<true> a4{ 0};
 A<true> && a5 = { 0};// expected-error {{chosen constructor is explicit}}
 A<true> && a6{ 0};
 A<true> a7 = { 0}; // expected-error {{chosen constructor is explicit in copy-initialization}}
 
-a0 = 0;
+a0 = 0; // expected-error {{no viable overloaded '='}}
 a1 = { 0}; // expected-error {{no viable overloaded '='}}
 a2 = A<true>( 0);
 a3 = A<true>{ 0};
 
 A<false> c0 =  ((short)0);
 A<false> c1( ((short)0));
 A<false> && c2 =  ((short)0);
 A<false> && c3( ((short)0));

clang/test/SemaCXX/for-range-dereference.cpp

expected-note {{when looking up 'end' function for range expression of type 'DeletedEnd'}}
 
   OverloadedStar OS;
   for (auto i : *OS) { }
 
   for (auto i : OS) { } // expected-error {{invalid range expression of type 'OverloadedStar'; did you mean to dereference it with '*'?}}
 
   for (Data *p : pt) { } // expected-error {{invalid range expression of type 'T *'; did you mean to dereference it with '*'?}}
   // expected-error@-1 {{no viable conversion from 'Data' to 'Data *'}}
-  // expected-note@4 {{selected 'begin' function with iterator type 'Data *'}}
 }

clang/test/SemaCXX/member-init.cpp

struct TypedefInit {
   typedef int A = 0; // expected-error {{illegal initializer}}
 };
 
 // PR10578 / <rdar://problem/9877267>
 namespace PR10578 {
   template<typename T>
   struct X { 
     X() {
-      T* x = 1; // expected-error{{cannot initialize a variable of type 'int *' with an rvalue of type 'int'}}
+      T* x = 1; // expected-error{{cannot initialize a variable of type 'int *' with an rvalue of type 'int'}} expected-warning {{unused variable}}
     }
   };
 
   struct Y : X<int> {
     Y();
   };
 
   Y::Y() try { // expected-note{{in instantiation of member function 'PR10578::X<int>::X' requested here}}

clang/test/SemaCXX/recovery-initializer.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -frecovery-ast -verify %s
+
+// NOTE: these tests can be merged to existing tests after -frecovery-ast is
+// turned on by default.
+void test1() {
+  struct Data {};
+  struct T {
+    Data *begin();
+    Data *end();
+  };
+  T *pt;
+  for (Data *p : T()) {} // expected-error {{no viable conversion from 'Data' to 'Data *'}}
+                         // expected-note@-5 {{selected 'begin' function with iterator type}}
+}
+
+void test2() {
+  struct Bottom {
+    constexpr Bottom() {}
+  };
+  struct Base : Bottom {
+    constexpr Base(int a = 42, const char *b = "test") : a(a), b(b) {}
+    int a;
+    const char *b;
+  };
+  constexpr Base *nullB = 12; // expected-error {{cannot initialize a variable of type}}
+  // verify that the "static_assert expression is not an integral constant expr"
+  // diagnostic is suppressed.
+  static_assert((Bottom*)nullB == 0, "");
+}

clang/test/SemaObjCXX/parameterized_classes_arc.mm

 // RUN: %clang_cc1 -fblocks -fobjc-arc -fobjc-runtime-has-weak %s -verify
 
 // rdar://21612439
 
 __attribute__((objc_root_class))
 @interface NSObject
 @end
   
 @class Forward;
 @class Forward2;
 
 // Tests for generic arguments.
 
 @interface PC1<T> : NSObject
 - (T) get;
-- (void) set: (T) v;
+- (void) set: (T) v; // expected-note 4{{passing argument to}}
 @end
 
 void test1a(PC1<__weak id> *obj) { // expected-error {{type argument '__weak id' cannot be qualified with '__weak'}}
   id x = [obj get];
   [obj set: x];
 }
 
 void test1b(PC1<__strong id> *obj) { // expected-error {{type argument '__strong id' cannot be qualified with '__strong'}}
   id x = [obj get];
   [obj set: x];
 }
 
 void test1c(PC1<id> *obj) {
   id x = [obj get];
   [obj set: x];
 }
 
 // Test that this doesn't completely kill downstream type-checking.
 void test1d(PC1<__weak Forward*> *obj) { // expected-error {{type argument 'Forward *__weak' cannot be qualified with '__weak'}}
   Forward2 *x = [obj get]; // expected-error {{cannot initialize}}
-  [obj set: x];
+  [obj set: x]; // expected-error {{cannot initialize a parameter of type 'Forward *' with an lvalue of type 'Forward2 *__strong'}}
 }
 
 void test1e(PC1<__strong Forward*> *obj) { // expected-error {{type argument 'Forward *__strong' cannot be qualified with '__strong'}}
   Forward2 *x = [obj get]; // expected-error {{cannot initialize}}
-  [obj set: x];
+  [obj set: x]; // expected-error {{cannot initialize a parameter of type 'Forward *'}}
 }
 
 void test1f(PC1<Forward*> *obj) {
   Forward2 *x = [obj get]; // expected-error {{cannot initialize}}
-  [obj set: x];
+  [obj set: x]; // expected-error {{cannot initialize a parameter of type 'Forward *'}}
 }
 
 // Typedefs are fine, just silently ignore them.
 typedef __strong id StrongID;
 void test1g(PC1<StrongID> *obj) {
   Forward2 *x = [obj get];
   [obj set: x];
 }
 
 typedef __strong Forward *StrongForward;
 void test1h(PC1<StrongForward> *obj) {
   Forward2 *x = [obj get]; // expected-error {{cannot initialize}}
-  [obj set: x];
+  [obj set: x]; // expected-error {{cannot initialize a parameter of type 'Forward *'}}
 }
 
 // These aren't really ARC-specific, but they're the same basic idea.
 void test1i(PC1<const id> *obj) { // expected-error {{type argument 'const id' cannot be qualified with 'const'}}
   id x = [obj get];
   [obj set: x];
 }