[AST] Better recovery on an expression refers to an invalid decl.

[hokein]

Summary

Prior to the patch, we didn't build a DeclRefExpr if the Decl being
referred to is invalid, because many clang downstream AST consumers
assume it, violating it will cause many diagnostic regressions.

With this patch, we build a DeclRefExpr enven for an invalid decl (when the
AcceptInvalidDecl is true), and wrap it with a dependent-type
RecoveryExpr (to prevent follow-up semantic analysis, and diagnostic
regressions).

This is a revised version of https://reviews.llvm.org/D76831

[sammccall]

Diagnostic changes look great!

clang/lib/Sema/SemaDecl.cpp

1250

wonder if if the results of setting AcceptInvalidDecl here would be good/bad?
(happy with in this patch/separate one/not at all, just curious)

Also possible candidates are the calls in:

• Sema::ActOnIdExpression
• Sema::BuildQualifiedDeclarationNameExpr
• Sema::BuildPossibleImplicitMemberExpr
• BuildRecoveryCallExpr in SemaOverload

clang/lib/Sema/SemaExpr.cpp

3189

if AcceptInvalid is true here, why do we skip all the rest of the checks?

Seems like in that case there's not much point calling the function at all...

My suspicion is that if we resolve to e.g. an invalid typedef then the diagnostic is good.

3502

Maybe drop the "many", which kind of implies they're incorrect to do so?

3507

This means we're changing the AST returned for existing AcceptInvalidDecl=true callers, right?

I think this is just attemptRecovery() in SemaCXX.cpp, which is part of typo correction. Previously we might transforming typos into DeclRefExpr to invalid, but now we're transforming them to RecoveryExpr wrapping DeclExpr to invalid.

This seems sensible, but I'm a little concerned there are no test changes for it. Is it possible to construct one?

I kind of expected this to work:

m *foo;
void z() {
  goo;
}

but in fact we produce an opaque RecoveryExpr (with no DeclRefExpr) inside today.

[hokein]

rebase and address comments.

[hokein]

clang/lib/Sema/SemaDecl.cpp

1250

yeah, this is interesting. we need to try it and see how well it goes. I will follow-up after this patch.

clang/lib/Sema/SemaExpr.cpp

3507

you're right. Added one testcase for the typo correction case.

clang/lib/Sema/SemaDecl.cpp

 }
 
 ExprResult
 Sema::ActOnNameClassifiedAsUndeclaredNonType(IdentifierInfo *Name,
                                              SourceLocation NameLoc) {
   assert(getLangOpts().CPlusPlus && "ADL-only call in C?");
   CXXScopeSpec SS;
   LookupResult Result(*this, Name, NameLoc, LookupOrdinaryName);
   return BuildDeclarationNameExpr(SS, Result, /*ADL=*/true);
 }
 
 ExprResult
 Sema::ActOnNameClassifiedAsDependentNonType(const CXXScopeSpec &SS,
                                             IdentifierInfo *Name,
                                             SourceLocation NameLoc,
                                             bool IsAddressOfOperand) {
   DeclarationNameInfo NameInfo(Name, NameLoc);
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, /*AcceptInvalidDecl=*/true);
 }
 
 ExprResult Sema::ActOnNameClassifiedAsOverloadSet(Scope *S, Expr *E) {
   // For an implicit class member access, transform the result into a member
   // access expression if necessary.
   auto *ULE = cast<UnresolvedLookupExpr>(E);
   if ((*ULE->decls_begin())->isCXXClassMember()) {
     CXXScopeSpec SS;

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 AcceptInvalid) {
+  if (D->isInvalidDecl() && !AcceptInvalid)
     return true;
 
   if (isa<TypedefNameDecl>(D)) {
     S.Diag(Loc, diag::err_unexpected_typedef) << D->getDeclName();
     return true;
   }
 
   if (isa<ObjCInterfaceDecl>(D)) {
     S.Diag(Loc, diag::err_unexpected_interface) << D->getDeclName();
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)) {
     // 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
#include "clang/AST/DeclNodes.inc"
 
   case Decl::CXXConversion:
   case Decl::CXXDestructor:
   case Decl::CXXConstructor:
     valueKind = VK_PRValue;
     break;
   }
 
-  return BuildDeclRefExpr(VD, type, valueKind, NameInfo, &SS, FoundD,
-                          /*FIXME: TemplateKWLoc*/ SourceLocation(),
-                          TemplateArgs);
+  auto *E =
+      BuildDeclRefExpr(VD, type, valueKind, NameInfo, &SS, FoundD,
+                       /*FIXME: TemplateKWLoc*/ SourceLocation(), TemplateArgs);
+  // Clang AST consumers assume a DeclRefExpr refers to a valid decl. We
+  // wrap a DeclRefExpr referring to an invalid decl with a dependent-type
+  // RecoveryExpr to avoid follow-up semantic analysis (thus prevent bogus
+  // diagnostics).
+  if (VD->isInvalidDecl() && E)
+    return CreateRecoveryExpr(E->getBeginLoc(), E->getEndLoc(), {E});
+  return E;
 }
 
 static void ConvertUTF8ToWideString(unsigned CharByteWidth, StringRef Source,
                                     SmallString<32> &Target) {
   Target.resize(CharByteWidth * (Source.size() + 1));
   char *ResultPtr = &Target[0];
   const llvm::UTF8 *ErrorPtr;
   bool success =

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

void returnInitListFromVoid() {
   // 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: | | `-DeclRefExpr {{.*}} 'InvalidDecl' 'int'
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   InvalidDecl();
   // CHECK:      CallExpr {{.*}}
   // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>'
 }
+
+void RecoverToAnInvalidDecl() {
+  Unknown* foo; // invalid decl
+  goo; // the typo was correct to the invalid foo.
+  // Verify that RecoveryExpr has an inner DeclRefExpr.
+  // CHECK:      RecoveryExpr {{.*}} '<dependent type>' contains-errors lvalue
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'foo' 'int *'
+}

clang/test/SemaTemplate/constraints.cpp

namespace PR45589 {
   template<typename T> struct X { static constexpr bool value = T::value; }; // expected-error {{cannot be used prior to '::'}}
   struct False { static constexpr bool value = false; };
   struct True { static constexpr bool value = true; };
 
   template<typename T> concept C = true;
 
   template<bool B, typename T> constexpr int test = 0;
   template<bool B, typename T> requires C<T> constexpr int test<B, T> = 1;
-  template<bool B, typename T> requires (B && C<T>) || (X<T>::value && C<T>) constexpr int test<B, T> = 2; // expected-error {{non-constant expression}} expected-note {{subexpression}} expected-note {{instantiation of}} expected-note {{while substituting}}
+  template<bool B, typename T> requires (B && C<T>) || (X<T>::value && C<T>) constexpr int test<B, T> = 2; // expected-note {{instantiation of}} expected-note {{while substituting}}
   static_assert(test<true, False> == 2);
   static_assert(test<true, True> == 2);
   static_assert(test<true, char> == 2); // satisfaction of second term of || not considered
   static_assert(test<false, False> == 1);
   static_assert(test<false, True> == 2); // constraints are partially ordered
-  // FIXME: These diagnostics are excessive.
-  static_assert(test<false, char> == 1); // expected-note 2{{while}} expected-note 2{{during}}
+  static_assert(test<false, char> == 1); // expected-note {{while}} expected-note {{during}}
 }

clang/test/SemaTemplate/cxx2a-constraint-exprs.cpp

namespace unevaluated {
   template<typename T> struct M { static void foo() requires false && f<int> { }; };
   // expected-note@-1{{because}}
   int c = (M<int>::foo(), 0);
   // expected-error@-1{{invalid reference to function 'foo': constraints not satisfied}}
 }
 
 namespace constant_evaluated {
   template<typename T> requires f<int[0]> struct S {};
-  // expected-note@-1{{in instantiation of}} expected-note@-1{{while substituting}} \
-     expected-error@-1{{substitution into constraint expression resulted in a non-constant expression}} \
-     expected-note@-1{{subexpression not valid}}
+  // expected-note@-1{{in instantiation of}} expected-note@-1{{while substituting}}
   using s = S<int>;
-  // expected-note@-1 2{{while checking}}
+  // expected-note@-1 {{while checking}}
   template<typename T> void foo() requires f<int[1]> { };
   // expected-note@-1{{in instantiation}} expected-note@-1{{while substituting}} \
-     expected-note@-1{{candidate template ignored}} expected-note@-1{{subexpression not valid}} \
-     expected-error@-1{{substitution into constraint expression resulted in a non-constant expression}}
+     expected-note@-1{{candidate template ignored}}
   int a = (foo<int>(), 0);
-  // expected-note@-1 2{{while checking}} expected-error@-1{{no matching function}} \
-     expected-note@-1 2{{in instantiation}}
+  // expected-note@-1 {{while checking}} expected-error@-1{{no matching function}} \
+     expected-note@-1 {{in instantiation}}
   template<typename T> void bar() requires requires { requires f<int[2]>; } { };
-  // expected-note@-1{{in instantiation}} expected-note@-1{{subexpression not valid}} \
+  // expected-note@-1{{in instantiation}} \
      expected-note@-1{{while substituting}} \
-     expected-error@-1{{substitution into constraint expression resulted in a non-constant expression}} \
-     expected-note@-1 2{{while checking the satisfaction of nested requirement}}
+     expected-note@-1 {{while checking the satisfaction of nested requirement}}
   int b = (bar<int>(), 0);
   template<typename T> struct M { static void foo() requires f<int[3]> { }; };
-  // expected-note@-1{{in instantiation}} expected-note@-1{{subexpression not valid}} \
-     expected-note@-1{{while substituting}} \
-     expected-error@-1{{substitution into constraint expression resulted in a non-constant expression}}
+  // expected-note@-1{{in instantiation}} expected-note@-1{{while substituting}}
   int c = (M<int>::foo(), 0);
-  // expected-note@-1 2{{while checking}}
+  // expected-note@-1 {{while checking}}
 }

clang/test/SemaTemplate/instantiate-var-template.cpp

 namespace InstantiationDependent {
   int f(int);
   void f(char);
 
   template<int> constexpr int a = 1;
   template<typename T> constexpr T b = a<sizeof(sizeof(f(T())))>; // expected-error {{invalid application of 'sizeof' to an incomplete type 'void'}}
 
   static_assert(b<int> == 1, "");
-  static_assert(b<char> == 1, ""); // expected-note {{in instantiation of}} expected-error {{not an integral constant}}
+  static_assert(b<char> == 1, ""); // expected-note {{in instantiation of}}
 
   template<typename T> void f() {
     static_assert(a<sizeof(sizeof(f(T())))> == 0, ""); // expected-error {{static assertion failed due to requirement 'a<sizeof (sizeof (f(type-parameter-0-0())))> == 0'}} \
                                                        // expected-note {{evaluates to '1 == 0'}}
   }
 }
 
 namespace PR24483 {