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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> 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.

I can see the desire to fix such issues at a higher level, and not wanting to play whack-a-mole in various places. But that's the reason why this patch specifically increases robustness in a piece of code that already has some logic to deal with invalid code. Rather than core clang pieces, which has different assumptions about invariants of the AST (e.g. not adding the null-check to every getType call in FunctionDecl methods).
If we're not willing to land such fixes that only add relevant checks to pieces of clang that's suppose to be more robust against invalid code, I don't see how we can have any stable tooling in the short term. Most of the feature work that introduces handling for new language constructs introduces regressions on invalid code as it's not really a concern and never verified. Hence we fix those afterwards as we notice them, fixing the implementation whenever it's feasible or increasing robustness in rest of the pieces when fixing the implementation is infeasible (or implementation is just right and the application was relying on some assurances that were incidentally there before). As we happen to hit some increasing resistance towards these robustness improvement patches, it'd be nice to understand what should be the way to fix them going forward. e.g. treat them as usual crashes, find the offending patch and just revert it with the reproducer?

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

This is resulting in crashes in clangd whenever the users trigger code completion in such code patterns (which is ~100s times a day in our production setup, so not rare in practice). 
So it isn't linked to any bigger task, but rather general robustness/QoL improvements for invalid code handling in clang(d) which are quite important especially on big projects. As a crash could cost minutes of warm-up times because we need to rebuild all of the caches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149733