[PATCH] D77037: [AST] Fix crashes on decltype(recovery-expr).

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3109
+        QualType PreferredType;
+        if (TypeRep)
+          PreferredType = Actions.ProduceConstructorSignatureHelp(
----------------
hokein wrote:
> sammccall wrote:
> > Add a comment for what the null case means? (When do we actually hit this?)
> yeah, the [`CompleteTest`](https://github.com/llvm/llvm-project/blob/master/clang/unittests/Sema/CodeCompleteTest.cpp#L490) hits the assertion after this patch. 
> 
> the assertion seems incorrect  -- IIUC, the assertion is for the `isInvalidType()` sanity check on Line 3090, however
> In `ActOnTypeName`,  `DeclaratorInfo` could be modified (by `GetTypeForDeclarator`) before calling `isInvalidType`.
> 
> 
> btw, I think for the CodeCompleteTest, would be nicer to make `ActOnTypeName` return `decltype(<recovery-expr>(bar))`, rather than the null type, but I'm not sure changing the `ActOnTypeName` behavior has any side effect.
> the assertion seems incorrect -- IIUC, the assertion is for the isInvalidType() sanity check on Line 3090, however

What you say makes sense, but I think it's worth probing why it's not currently hit (e.g. by `int x(auto);`, where `GetDeclSpecTypeForDeclarator` marks the decl as invalid because auto isn't allowed in a prototype).

> btw, I think for the CodeCompleteTest, would be nicer to make ActOnTypeName return decltype(<recovery-expr>(bar)), rather than the null type

Definitely. I think "invalid" on a type-concept is stronger than what we're looking for - since we're not tracking errors in decls, we'd want to use "haserrors" on type-concepts and then promote to "invalid" on decl-concepts.

Ugh, the design of "Declarator" makes this difficult, because there's no distinction between "type of this declarator is invalid" and "type of this declarator makes the declarator invalid".

I'd suggest leaving a FIXME on the changes in SemaType, saying something like "we want resulting declarations to be marked invalid, but claiming the type is invalid is too strong - e.g. it causes ActOnTypeName to return a null type."


================
Comment at: clang/lib/Sema/SemaType.cpp:1594
     }
+    if (Result->containsErrors())
+      declarator.setInvalidType(true);
----------------
are you sure you want this in the individual cases, rather than once at the end of this function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77037