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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:1230
   LookupResult Result(*this, Name, NameLoc, LookupOrdinaryName);
   return BuildDeclarationNameExpr(SS, Result, /*ADL=*/true);
 }
----------------
sammccall wrote:
> 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
> 
yeah, this is interesting. we need to try it and see how well it goes. I will follow-up after this patch.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3478
+  if (VD->isInvalidDecl() && E)
+    return CreateRecoveryExpr(E->getBeginLoc(), E->getEndLoc(), {E});
+  return E;
----------------
sammccall wrote:
> 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.
you're right. Added one testcase for the typo correction case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121599