[PATCH] D149733: [clang][USR] Prevent crashes on incomplete FunctionDecls

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D149733#4321610 <https://reviews.llvm.org/D149733#4321610>, @kadircet wrote:

>> However, I'm not keen on us playing whack-a-mole with the kinds of checks from this review. For starters, that's going to have a long-tail that makes it hard to know if we've ever gotten to the bottom of the issue. But also, each one of these checks is basically useless for the primary way in which Clang is consumed (as a compiler), so this increases compile times for limited benefit to "most" users.
>
> I completely agree, that's the reason why I've stayed away from adding those checks to various FunctionDecl helpers (isVariadic, params, etc.).
>
>> In this particular case, we may be fine as this is limited to libclang and so it shouldn't add overhead for the common path, but I suspect we're going to find cases that need fixing in more common areas of the compiler that could be more troublesome.
>
> Agreed, and I am also pretty sure this is not the only place that can be affected from incomplete decls/types. But this is the only one showing up quite frequently ever since changes to lambda parsing.
> I think there's some strategy decision to be made about clang's invariants:
>
> - whether to accept declarations/types can be inspected in the middle of parsing as a new constraint, declare all the existing violations as bugs and fix them as we go (without introducing new ones, which is quite hard) and give people the means to construct ast nodes "safely".
> - claim that variants are WAI and it's on use cases that perform such inspections to figure out how to deal with consequences (e.g. in code-completion consumers).
>
> But in either case, I don't think this review is the right medium to make that decision. Surely it contains a lot of useful pointers, and I am happy to move them to a discourse thread (LMK if I should do it, or you'd like to kick it off @aaron.ballman) to raise awareness, but in the end this review is just trying to fix an issue by adding extra checks to only the applications that can violate contracts of clang parsing. So unless we've specific concerns about the issue being addressed in this patch, I'd like to move forward.

I think we probably should have a broader discussion before moving forward here. It's not that this isn't incremental progress fixing an issue, but it's more that this same justification works to add the workaround 200 more times without ever addressing the underlying architectural concerns.

That said, is this issue blocking further work so you need to land this in order to make forward progress elsewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149733