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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Diagnostic changes look great!



================
Comment at: clang/lib/Sema/SemaDecl.cpp:1230
   LookupResult Result(*this, Name, NameLoc, LookupOrdinaryName);
   return BuildDeclarationNameExpr(SS, Result, /*ADL=*/true);
 }
----------------
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



================
Comment at: clang/lib/Sema/SemaExpr.cpp:3156
   if (D->isInvalidDecl())
-    return true;
+    return !AcceptInvalid;
 
----------------
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3473
+                       /*FIXME: TemplateKWLoc*/ SourceLocation(), TemplateArgs);
+  // Many clang AST consumers assume a DeclRefExpr refers to a valid decl. We
+  // wrap a DeclRefExpr referring to an invalid decl with a dependent-type
----------------
Maybe drop the "many", which kind of implies they're incorrect to do so?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3478
+  if (VD->isInvalidDecl() && E)
+    return CreateRecoveryExpr(E->getBeginLoc(), E->getEndLoc(), {E});
+  return E;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121599/new/

https://reviews.llvm.org/D121599