[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50462#1214120, @Dmitry.Kozhevnikov wrote:

> When lookin through the list of the fatal errors, I think there are different categories:
>
> 1. "Scary" ones - "module was relocated", "invalid vfs overlay file", we probably shouldn't try to recover from it
> 2. "User" errors (include not found, too many errors) - we definitely shouldn't treat them as fatal in IDE
> 3. (subclass of the previous one): "something is too deep" (instantiations, brackets) - I'm afraid treating them as non-fatal right now would lead to a possibility of them happening again and again as we recover and proceed. So, in the future, the recovery might be more clever (i.e. going all the way up the instantiation/brackets stack, and then proceeding normally), however, while it's not done, I'm a bit uneasy demoting them from fatal.
>
>   So I'm preparing an alternative patch to demote "file not found" in include directive from the fatal error in .td file, and then immediately promote it back by default (but not in clangd).  WDYT?


It feels we're uncertain whether continuing on fatal errors would work, so we choose to not do it.
E.g., about the scary ones:

- on "invalid vfs overlay file" we wouldn't even run to the parsing, as it fires when parsing compile args and noone recovers from that.
- on "module relocated". Haven't seen this one, so speculating here. If this happens at import directives, can we just pretend the import was unresolved and continue?

Having a mechanism to recover on specific errors seems like a more fine-grained approach that should work, but the current version of the patch looks very simple and if it turns out it works.
It would be a shame to have the added complexity if the simple solution would also work.

And there is `-Wfatal-errors` that turns every error into fatal, how would promote/demote approach work in that case?
Maybe we can first add an option, experiment with it and see if it works? And if not, it would give us enough data to classify the failure modes and fix them?

I wonder what others think about it.

> In general, I find the whole concept of "Fatal error occurred/Uncompilable error occurred/Too many errors occurred/etc" too coarse-grained for tooling/IDEs. Parse/PP/Sema should probably try harder to recover, and not report such global state change. I'm not ready to approach it, though :)

Totally agree, I can't imagine a fatal error that makes it absolutely impossible for parser to recover (after the parser starts, i.e. the compilation args are correct). Clang can (and will) produce too much noise when recovering, but that's up to IDEs/tools to fix this.

PS we definitely want recovery from missing includes in clangd.
We have this interesting problem when building the preamble:

  #include "resolved.h" // imagine this defines a struct named 'Foo'
  #include "unresolved.h"
  
  int main() {
    Foo f;
    f. // <-- complete after dot here
  }

In that case, completion would work even when the second header is unresolved.
However, if we swap the includes, completion stops working because the fatal error (unresolved include) happens **before** the definition of the struct, so preamble does not contain the struct definition.


Repository:
  rC Clang

https://reviews.llvm.org/D50462