[PATCH] D66759: [clangd] Surface errors from command-line parsing

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D66759#1646599 <https://reviews.llvm.org/D66759#1646599>, @ilya-biryukov wrote:

> How common is this? Do we have any particular examples?


Well, I don't have any data to prove, but my experience has been so far; most of the people don't touch Makefiles apart from adding their source files to the
CMakeLists within their project directory, and only some of them are comfortable with providing additional flags. For example having a new warning flag, or
a flag that has been recently renamed. Then your compiler won't complain during builds, but you will get those bogus errors in clangd.

> My worry comes from the fact that I don't think we can guarantee that clangd is working for the users after we encountered those errors.
>  In particular, after D66731 <https://reviews.llvm.org/D66731>, we will be getting a compiler invocation and building the AST in many more cases. However, the AST might be completely bogus, e.g. because we did not recognize actually important flags.
>  Surfacing this error to the user is useful, because that would explain why clangd is rusty, gives weird results, etc.
> 
> In turn, the users will report bugs and we'll be able to collect data on whether this is noisy or everyone is doing compile_commands.json wrong and we should do something about it or whether it's actually very rare in practice and turns out to be a useful indicator for the users.
>  Either result seems plausible.
>  I'm all in for hiding annoying errors, but I don't know what the failure modes are in that particular case and I'd be tempted to first see whether surfacing the errors would **actually** end up producing those annoying errors and what the errors are.

OK, I was leaning on "not showing up if we could build an AST" side because I thought that in those cases even if we have errors while building `CompilerInvocation`
these would most of the time be some small things that would not affect how AST was generated in a sense that users would care. Therefore we would end up annoying
users most of the time and be really useful only in some small portion of the cases.

But again this isn't backed by any data, following your approach of turning it on and changing it later on if we annoy people also sounds OK to me. Also as discussed offline
I misunderstood the scope of the diags generated while building compiler invocations. Since this only surfaces errors, it should be mostly OK.



================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:465
+    Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end());
+  // Finally, add diagnostics coming from the AST.
+  {
----------------
could you preserve the previous order by inserting `CompilerInvocationDiags` here instead of the ones in `AST` ?


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:399
+  // Do not forget to emit a pending diagnostic if there is one.
+  flushLastDiag();
+
----------------
I suppose we can get rid of the one in `StoreDiags::EndSourceFile` now ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66759