[AST] Preserve the DeclRefExpr when it refers to an invalid decl.

[hokein]

Summary

Previously, clang refused to build the AST nodes for them.

[sammccall]

I like the idea, but it seems to regress quite a lot of diagnostics... e.g. where we fail to deduce auto and then say it's not a pointer.

Also this is regressing things in the -fno-recovery-ast case, because of the changes to CheckDeclInExpr with the existing callsites that allow invalid decls. (I'm not sure what passing AcceptInvalid actually *did* before)

[hokein]

In D76831#1952425, @sammccall wrote:

I like the idea, but it seems to regress quite a lot of diagnostics... e.g. where we fail to deduce auto and then say it's not a pointer.

Also this is regressing things in the -fno-recovery-ast case, because of the changes to CheckDeclInExpr with the existing callsites that allow invalid decls. (I'm not sure what passing AcceptInvalid actually *did* before)

Yeah, the solution makes diagnostics worse...

The motivation of the patch is to preserve the DeclRefExpr nodes even the referenced var-decl is invalid, so that clangd features can still work, e.g. the case we care about

struct A { A(int); }
void t() {
  A a; // VarDecl a is invalid.
  a.^ // none of clangd features would work as we don't have any AST node
}

we have a few options:

1. build DeclRefExpr regardless the validity of VarDecl;
2. mark VarDecl valid even when there are errors during initialization;
3. using RecoveryExpr, e.g. build a RecoveryExpr that wraps a DeclRefExpr if the VarDecl is invalid;

as shown in this patch, 1 is not ideal and hurts about clang diagnostics. 2 looks promising, and safer (probably not regress clang diagnostic a lot).

I think VarDecls in following cases should be valid. Interestingly, clang doesn't seem to have consistent behavior -- some of them are valid already, so 2 has the greatest chance of not introducing large regressions.

struct A {
  A(int, int);
};

void test() {
  // 1) default initialization (without any initializers)
  A a1; // invalid

  // 2) direct initialization
  A a2(1); // invalid
  A a3{1}; // invalid
  A a4(invalid()); // valid
  A a5{invalid()}; // valid

  int var;
  // 3) copy initializaiton
  A a6 = 1; // invalid
  A a7 = A(1); // valid
  A a8 = A(var); // valid
  A a10 = A{1}; // valid
  A a9 = {1}; // invalid
}

[sammccall]

Yeah, so I guess the point of marking decls invalid is in large part to stop recovery using it.
It's not surprising 1 has recovery regressions. 3 should have fewer (basically because you'll get the dependent bits too), but likely still some.
2 definitely seems the way to go given that this is currently inconsistent between different invalid ways of initializing a variable. Thanks for D77395!

[rsmith]

Generally I think we should be moving towards finer-grained "invalid" / "contains errors" bits, so that we can preserve as much of the AST as possible and produce accurate diagnostics for independent errors wherever possible. To that end, I think generally the "invalid" bit on a declaration should concern *only* the "declaration" part of that declaration (how it's seen from other contexts that don't "look too closely"). So in particular:

• The initializer of a variable should play no part in its "invalid" bit. If the initializer expression is marked as "contains errors", then things that care about the initializer (in particular, constant evaluation and any diagnostics that look into the initializer) may need to bail out, but we should be able to form a DeclRefExpr referring to that variable -- even if (say) the variable is constexpr. Exception: if the variable has a deduced type and the type can't be deduced due to an invalid initializer, then the declaration should be marked invalid.
• The body of a function should play no part in its "invalid" bit. (This came up in a different code review recently.)

We shouldn't build a DeclRefExpr to a declaration whose "invalid" bit is set, because (for example) we don't necessarily know what type that expression has. Building a RecoveryExpr in that case seems reasonable.

[sammccall]

@hokein I think this patch is obsolete, right?

Richard's last comment is basically what's currently implemented: if lookup doesn't yield any valid results, we create a recoveryexpr for the failed lookup. It doesn't have a pointer to the invalid decl (which would be neat) but it doesn't seem like we're planning to fix that.

clang/lib/AST/ComputeDependence.cpp

for (auto *Arg = FirstArg, *End = FirstArg + NumArgs; Arg < End; ++Arg)
       Deps |= toExprDependence(Arg->getArgument().getDependence());
   }
 
   auto *Decl = E->getDecl();
   auto Type = E->getType();
 
   if (Decl->isParameterPack())
     Deps |= ExprDependence::UnexpandedPack;
+  if (Decl->isInvalidDecl())
+    Deps |= ExprDependence::Error;
   Deps |= toExprDependence(Type->getDependence()) & ExprDependence::Error;
 
   // (TD) C++ [temp.dep.expr]p3:
   //   An id-expression is type-dependent if it contains:
   //
   // and
   //
   // (VD) C++ [temp.dep.constexpr]p2:

clang/lib/Sema/SemaDecl.cpp

if (auto *Ivar = dyn_cast<ObjCIvarDecl>(Found->getUnderlyingDecl()))
       return BuildIvarRefExpr(S, NameLoc, Ivar);
 
   // Reconstruct the lookup result.
   LookupResult Result(*this, Found->getDeclName(), NameLoc, LookupOrdinaryName);
   Result.addDecl(Found);
   Result.resolveKind();
 
   bool ADL = UseArgumentDependentLookup(SS, Result, NextToken.is(tok::l_paren));
-  return BuildDeclarationNameExpr(SS, Result, ADL);
+  return BuildDeclarationNameExpr(SS, Result, ADL,
+                                  Context.getLangOpts().RecoveryAST);
 }
 
 Sema::TemplateNameKindForDiagnostics
 Sema::getTemplateNameKindForDiagnostics(TemplateName Name) {
   auto *TD = Name.getAsTemplateDecl();
   if (!TD)
     return TemplateNameKindForDiagnostics::DependentTemplate;
   if (isa<ClassTemplateDecl>(TD))

clang/lib/Sema/SemaExpr.cpp

bool Sema::UseArgumentDependentLookup(const CXXScopeSpec &SS,
   return true;
 }
 
 
 /// Diagnoses obvious problems with the use of the given declaration
 /// as an expression.  This is only actually called for lookups that
 /// were not overloaded, and it doesn't promise that the declaration
 /// will in fact be used.
-static bool CheckDeclInExpr(Sema &S, SourceLocation Loc, NamedDecl *D) {
-  if (D->isInvalidDecl())
+static bool CheckDeclInExpr(Sema &S, SourceLocation Loc, NamedDecl *D,
+                            bool AcceptInvalidDecl) {
+  if (!AcceptInvalidDecl && D->isInvalidDecl())
     return true;
 
   if (isa<TypedefNameDecl>(D)) {
     S.Diag(Loc, diag::err_unexpected_typedef) << D->getDeclName();
     return true;
   }
 
   if (isa<ObjCInterfaceDecl>(D)) {
if (!NeedsADL && R.isSingleResult() &&
     return BuildDeclarationNameExpr(SS, R.getLookupNameInfo(), R.getFoundDecl(),
                                     R.getRepresentativeDecl(), nullptr,
                                     AcceptInvalidDecl);
 
   // We only need to check the declaration if there's exactly one
   // result, because in the overloaded case the results can only be
   // functions and function templates.
   if (R.isSingleResult() && !ShouldLookupResultBeMultiVersionOverload(R) &&
-      CheckDeclInExpr(*this, R.getNameLoc(), R.getFoundDecl()))
+      CheckDeclInExpr(*this, R.getNameLoc(), R.getFoundDecl(),
+                      AcceptInvalidDecl))
     return ExprError();
 
   // Otherwise, just build an unresolved lookup expression.  Suppress
   // any lookup-related diagnostics; we'll hash these out later, when
   // we've picked a target.
   R.suppressDiagnostics();
 
   UnresolvedLookupExpr *ULE
ExprResult Sema::BuildDeclarationNameExpr(
     const CXXScopeSpec &SS, const DeclarationNameInfo &NameInfo, NamedDecl *D,
     NamedDecl *FoundD, const TemplateArgumentListInfo *TemplateArgs,
     bool AcceptInvalidDecl) {
   assert(D && "Cannot refer to a NULL declaration");
   assert(!isa<FunctionTemplateDecl>(D) &&
          "Cannot refer unambiguously to a function template");
 
   SourceLocation Loc = NameInfo.getLoc();
-  if (CheckDeclInExpr(*this, Loc, D))
+  if (CheckDeclInExpr(*this, Loc, D, AcceptInvalidDecl))
     return ExprError();
 
   if (TemplateDecl *Template = dyn_cast<TemplateDecl>(D)) {
     // Specifically diagnose references to class templates that are missing
     // a template argument list.
     diagnoseMissingTemplateArguments(TemplateName(Template), Loc);
     return ExprError();
   }
#include "clang/AST/DeclNodes.inc"
     // pointer-to-member expressions; we just call them l-values for
     // internal consistency, because this subexpression doesn't really
     // exist in the high-level semantics.
     case Decl::Field:
     case Decl::IndirectField:
     case Decl::ObjCIvar:
       assert(getLangOpts().CPlusPlus &&
              "building reference to field in C?");
-
       // These can't have reference type in well-formed programs, but
       // for internal consistency we do this anyway.
       type = type.getNonReferenceType();
       valueKind = VK_LValue;
       break;
 
     // Non-type template parameters are either l-values or r-values
     // depending on the type.

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

 // 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'
+// CHECK:     |-VarDecl {{.*}} invalid e 'auto'
 auto e = bar();
+
+// CHECK:      FunctionDecl {{.*}}
+// CHECK-NEXT: `-CompoundStmt {{.*}}
+// CHECK-NEXT: |-DeclStmt {{.*}}
+// CHECK-NEXT: | `-VarDecl {{.*}} invalid foo 'Foo'
+// CHECK-NEXT: `-DeclRefExpr {{.*}} 'Foo' contains-errors
+struct Foo { Foo(int); };
+void invalidDeclRefExpr() {
+  Foo foo;
+  foo;
+}

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p6.cpp

struct X {
     int method(int); // expected-note{{possible target for call}}
     int method(float); // expected-note{{possible target for call}}
   };
 
   template<typename T> T g(T);
 
   void f(X *x) {
     auto value = x->method; // expected-error {{reference to non-static member function must be called}}
-    if (value) { }
+    if (value) { } // expected-error {{value of type 'auto' is not contextually convertible to 'bool'}}
 
     auto funcptr = &g<int>;
     int (*funcptr2)(int) = funcptr;
   }
 }
 
 // if the initializer is a braced-init-list, deduce auto as std::initializer_list<T>:
 // see SemaCXX/cxx0x-initializer-stdinitializerlist.cpp

clang/test/CXX/drs/dr3xx.cpp

void f(A *a) {
       (void) reinterpret_cast<B3*>(a); // expected-error {{casts away qualifiers}}
       (void) reinterpret_cast<B4*>(a);
     }
   }
 }
 
 namespace dr331 { // dr331: yes
   struct A {
-    A(volatile A&); // expected-note {{candidate}}
-  } const a, b(a); // expected-error {{no matching constructor}}
+    A(volatile A&); // expected-note 2{{candidate}}
+  } const a, b(a); // expected-error 2{{no matching constructor}}
 }
 
 namespace dr332 { // dr332: dup 577
   void f(volatile void); // expected-error {{'void' as parameter must not have type qualifiers}}
   void g(const void); // expected-error {{'void' as parameter must not have type qualifiers}}
   void h(int n, volatile void); // expected-error {{'void' must be the first and only parameter}}
 }
 

clang/test/CodeCompletion/error-covery.cpp

+struct Foo { Foo(int); int abc; };
+void invalidDeclRefExpr() {
+  Foo foo;
+  foo.;
+  // RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:5:7 %s -o - | FileCheck %s
+  // CHECK: COMPLETION: abc
+}

clang/test/Modules/submodules-merge-defs.cpp

 extern class A pre_a2;
 int pre_use_a = use_a(pre_a2); // expected-error 2{{'A' must be imported}} expected-error {{'use_a' must be imported}}
 // expected-note@defs.h:2 +{{here}}
 
 B::Inner2 pre_bi; // expected-error +{{must be imported}}
 // expected-note@defs.h:4 +{{here}}
 // expected-note@defs.h:17 +{{here}}
 void pre_bfi(B b) { // expected-error +{{must be imported}}
-  b.f<int>();
+  b.f<int>(); // expected-error {{member reference base type}} \
+                 expected-error {{expected '(' for function-style}} \
+                 expected-error {{expected expression}}
 }
 
 C_Base<1> pre_cb1; // expected-error +{{must be imported}}
 // expected-note@defs.h:23 +{{here}}
 C1 pre_c1; // expected-error +{{must be imported}}
 // expected-note@defs.h:25 +{{here}}
 C2 pre_c2; // expected-error +{{must be imported}}
 // expected-note@defs.h:26 +{{here}}

clang/test/OpenMP/openmp_check.cpp

 
 #pragma omp parallel
     {
 #pragma omp target
       ++a;
     }
   };
   F();
+#if __cplusplus <= 199711L
+  // expected-error@-2 {{called object type 'auto' is not a function}}
+#endif
   return a;
 }

clang/test/SemaCXX/block-call.cpp

int main() {
 
   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 = 1; // expected-error {{invalid block pointer conversion assigning}} 
   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/conversion-function.cpp

struct A {
     operator enum E { e } (); // expected-error {{'PR18234::A::E' cannot be defined in a type specifier}}
     operator struct S { int n; } (); // expected-error {{'PR18234::A::S' cannot be defined in a type specifier}}
     // expected-note@-1 {{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'struct A' to 'const PR18234::A::S &' for 1st argument}}
 #if __cplusplus >= 201103L
   // expected-note@-3 {{candidate constructor (the implicit move constructor) not viable: no known conversion from 'struct A' to 'PR18234::A::S &&' for 1st argument}}
 #endif
   } a;
   A::S s = a; // expected-error {{no viable conversion from 'struct A' to 'A::S'}}
-  A::E e = a;
+  A::E e = a; // expected-note {{'e' declared here}}
   bool k1 = e == A::e; // expected-error {{no member named 'e'}}
   bool k2 = e.n == 0;
 }
 
 namespace PR30595 {
 struct S {
   const operator int(); // expected-error {{cannot specify any part of a return type in the declaration of a conversion function; put the complete type after 'operator'}}
   const operator int() const; // expected-error {{cannot specify any part of a return type}}
   volatile const operator int(); // expected-error {{cannot specify any part of a return type}}
 
   operator const int() const;
 };
 }

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) {
       // We used to attempt to evaluate the initializer of this variable,
       // and crash because it has an undeduced type.
-      const int &n(x);
+      const int &n(x); // expected-error {{reference to type 'const int' could not bind}}
     }
   }
 }
 
 namespace incorrect_auto_type_deduction_for_typo {
 struct S {
   template <typename T> S(T t) {
     (void)sizeof(t);

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-examples.cpp

namespace test4 {
   }
 }
 
 namespace test5 {
   // Test error-recovery.
   void f() {
     for (auto x : undeclared_identifier) // expected-error {{undeclared identifier}}
       for (auto y : x->foo)
-        y->bar();
+        y->bar(); // expected-error {{member reference type 'auto' is not a pointer}}
     for (auto x : 123) // expected-error {{no viable 'begin'}}
-      x->foo();
+      x->foo(); // expected-error {{member reference type 'auto' is not a pointer}}
   }
 }
 
 namespace test6 {
   void foo(int arr[]) {  // expected-note {{declared here}}
     for (auto i : arr) { }
       // expected-error@-1 {{cannot build range expression with array function parameter 'arr' since parameter with array type 'int []' is treated as pointer type 'int *'}}
   }