[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D146971#4224540 <https://reviews.llvm.org/D146971#4224540>, @erichkeane wrote:

> Ah, hrmph. I guess I was just hoping for some assert (perhaps even in `GetFullTypeForDeclarator`?) to assert in order to give future folks a hint as to why their change crashes?  Basically, whenever the `FunctionType`/`FunctionTypeLoc` are 'finalized' as a check perhaps?  Not really looking for a refactor, so much as an assert to prevent us from getting here again.

Yeah, that makes a lot of sense. We need to fix the `FIXMEs` for that to work, I'm eager to try this in follow-ups. But probably not in this particular change as it seems like a lot of work and this is already moving in the right direction.

We probably need asserts in two places: `GetFullTypeForDeclarator` and `getTrivialTypeSourceInfo`.
I'm less worried about the former, but fixing `getTrivialTypeSourceInfo` might be harder as it has a lot of call sites and I suspect some of them actually pass `FunctionType` as inputs. The assert would basically prohibit creating `FunctionTypeLoc` without parameter declarations.
I don't think it's unreasonable, the code that produces `FunctionTypeLoc` without having parameters should either find the corresponding parameters or be rewritten to use `QualType`  instead of `TypeLoc`, which does not need parameters.
But I expect this to be a lot of work to actually update the callsites, and this will probably take quite some time.



================
Comment at: clang/lib/Sema/SemaType.cpp:5949
   assert(!T.isNull() && "T must not be null at the end of this function");
-  if (D.isInvalidType())
+  if (!AreDeclaratorChunksValid)
     return Context.getTrivialTypeSourceInfo(T);
----------------
erichkeane wrote:
> ilya-biryukov wrote:
> > erichkeane wrote:
> > > Shouldn't the `D.setInvalidType(true)` in each of the branches work here?  So this variable is unnecessary?  Else this is a good change IMO. 
> > (Let me know if I'm misreading the suggestion, I was not sure if my understanding is correct).
> > If we call `setInvalidType` more, we would actually get more crashes as the problematic branch is the one that calls `getTrivialTypeSourceInfo`.
> > To avoid the extra variable, we could instead ensure that the type always gets replaced with something trivial `T = Context.IntTy`. But I didn't want to go this path because this causes worse error recovery (going from something like `void(<recovered-to:int>)` to `<recovered-to:int>`) and possibly correctness issues (e.g. will we start getting function-decls or lambdas that do not have function types and other assertions may fire).
> My suggestion was that `setInvalidType` is called everywhere that `AreDeclatorChunksValid` above (sans 1 perhaps?). So my question was whether this introduced variable was necessary.
Ah, thanks for clarifying the question.
`setInvalidType` is called in much more places and the point of this patch is to capture a subcategory of invalid types that still follow the declarator structure.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146971