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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3109
+        QualType PreferredType;
+        if (TypeRep)
+          PreferredType = Actions.ProduceConstructorSignatureHelp(
----------------
sammccall wrote:
> 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."
> 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).

I believe the issue exists even before this patch, it was just not caught by tests. Added one.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77037