[AST] Use RecoveryExpr to model a DeclRefExpr which refers to an invalid Decl.

[hokein]

Summary

Previously, we didin't build a DeclRefExpr which refers to an invalid declaration.

In this patch, we handle this case by building an empty RecoveryExpr,
which will preserve more broken code (AST parent nodes that contain the
RecoveryExpr is preserved in the AST).

[hokein]

clang/test/SemaCXX/copy-assignment.cpp

117

This is a bogus diagnostic "accidentally" caused by this change, this is an existing problem in clang where we discard ill-formed base specifier.

[sammccall]

Very nice! I thought we had considered and rejected this in the past, but I think it was rather allowing lookup to find invalid decls in cases where it doesn't today.

clang/test/SemaCXX/constructor-initializer.cpp

252

update the other expected-error to to say what it refers to, too?

[hokein]

In D120812#3354308, @sammccall wrote:

Very nice! I thought we had considered and rejected this in the past, but I think it was rather allowing lookup to find invalid decls in cases where it doesn't today.

Yeah, in the past (https://reviews.llvm.org/D76831), we tried to build the DeclRefExpr even for an invalid ref decl, it made the diagnostics much worse (as we don't know the type of the invalid decl, the fallback int type is not suitable for every cases).

We use a different approach in this approach (build RecoveryExpr instead), a sad bit is that we are not able to retrieve the ref decl from the RecoveryExpr.

[sammccall]

To preserve the decl, we could consider as a next step building a recoveryexpr wrapping a declrefexpr pointing at the invalid decl.

This was discussed in the patch you linked, but I don't think it was tried?

clang/lib/Sema/SemaExpr.cpp

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))
-    return ExprError();
+  if (CheckDeclInExpr(*this, Loc, D)) {
+    // Recovery from invalid cases (e.g. D is an invalid Decl).
+    // We use the dependent type for the RecoveryExpr to prevent bogus follow-up
+    // diagnostics, as invalid decls use int as a fallback type.
+    return CreateRecoveryExpr(NameInfo.getBeginLoc(), NameInfo.getEndLoc(), {});
+  }
 
   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();
   }
 

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

 void returnInitListFromVoid() {
   // CHECK:      FunctionDecl {{.*}} returnInitListFromVoid
   return {7,8};
   // CHECK:      ReturnStmt
   // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>'
   // CHECK-NEXT:   |-IntegerLiteral {{.*}} 'int' 7
   // CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 8
 }
+
+void RecoveryExprForInvalidDecls(Unknown InvalidDecl) {
+  InvalidDecl + 1;
+  // CHECK:      BinaryOperator {{.*}}
+  // CHECK-NEXT: |-RecoveryExpr {{.*}} '<dependent type>'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  InvalidDecl();
+  // CHECK:      CallExpr {{.*}}
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>'
+}

clang/test/Sema/ms-inline-asm.c

typedef struct {
   int a;
   int b;
 } A;
 
 void t3(void) {
   __asm { mov eax, [eax] UndeclaredId } // expected-error {{unknown token in expression}} expected-error {{use of undeclared label 'UndeclaredId'}}
 
   // FIXME: Only emit one diagnostic here.
-  // expected-error@+3 {{use of undeclared label 'A'}}
   // expected-error@+2 {{unexpected type name 'A': expected expression}}
   // expected-error@+1 {{unknown token in expression}}
   __asm { mov eax, [eax] A }
 }
 
 void t4(void) {
   // The dot in the "intel dot operator" is optional in MSVC.  MSVC also does
   // global field lookup, but we don't.

clang/test/SemaCXX/constructor-initializer.cpp

 #if __cplusplus >= 201103L // C++11 or later
   // expected-note@-3 {{candidate constructor (the implicit move constructor) not viable}}
 #endif
   // expected-note@-5 {{candidate constructor (the implicit default constructor) not viable}}
 
   class B : public A {
   public:
     B(const String& s, int e=0) // expected-error {{unknown type name}} 
-      : A(e), m_String(s) , m_ErrorStr(__null) {} // expected-error {{no matching constructor}} expected-error {{does not name}}
+      : A(e), m_String(s) , m_ErrorStr(__null) {} // expected-error {{no matching constructor}} expected-error {{does not name}} expected-error {{member initializer 'm_ErrorStr' does not name}}
     B(const B& e)
       : A(e), m_String(e.m_String), m_ErrorStr(__null) { // expected-error 2{{does not name}} \
       // expected-error {{no member named 'm_String' in 'test3::B'}}
     }
   };
 }
 
 // PR8075

clang/test/SemaCXX/copy-assignment.cpp

#endif
   i = convertibleToInt;
   i = a; // expected-error{{assigning to 'int' from incompatible type 'A'}}
 }
 
 // <rdar://problem/8315440>: Don't crash
 namespace test1 {
   template<typename T> class A : public unknown::X { // expected-error {{undeclared identifier 'unknown'}} expected-error {{expected class name}}
     A(UndeclaredType n) : X(n) {} // expected-error {{unknown type name 'UndeclaredType'}}
+    // expected-error@-1 {{member initializer 'X' does not name a non-static data member or base class}}
   };
   template<typename T> class B : public A<T>     {
     virtual void foo() {}
   };
   extern template class A<char>;
   extern template class B<char>;
 }