[PATCH] D44142: [clangd] Revamp handling of diagnostics.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Behavior looks good. I think we can do a bit better with (most) fixits - your call on whether it makes sense to do here.

As discussed i'd simplify the diagnostic containers until we know there's a strong need.



================
Comment at: clangd/ClangdLSPServer.cpp:329
+          {"title",
+           llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
           {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
----------------
nit: while here, can we add a colon after FixIt (or if appropriate in practice, just drop the prefix altogether?). My recollection is this is a bit hard to parse.


================
Comment at: clangd/Diagnostics.cpp:85
+
+bool isInsdieMainFile(const clang::Diagnostic &D) {
+  if (!D.hasSourceManager())
----------------
inside


================
Comment at: clangd/Diagnostics.cpp:125
+
+void printDiag(llvm::raw_string_ostream &OS, const PersistentDiag &D) {
+  if (D.InsideMainFile) {
----------------
mind pasting a small example as a function comment?
Logic looks good, but I had to reconstruct the message in my head.


================
Comment at: clangd/Diagnostics.cpp:147
+
+std::string presentMainMessage(const DiagWithNotes &D) {
+  std::string Result;
----------------
or just mainMessage (this is another verbs-for-pure-functions case where the style recommendation seems to hurt readability to me, but up to you)


================
Comment at: clangd/Diagnostics.cpp:192
+                llvm::function_ref<void(DiagWithFixIts)> OutFn) {
+  auto PreBuild = [](const PersistentDiag &D) {
+    DiagWithFixIts Res;
----------------
PreBuild could be FillBasicFields or something? found this name confusing.


================
Comment at: clangd/Diagnostics.cpp:200
+
+  DiagWithFixIts Main = PreBuild(D.main());
+  Main.Diag.message = presentMainMessage(D);
----------------
(nit: if the goal with the callback function is to avoid allocations, shouldn't we reuse the DiagWithFixIts?)


================
Comment at: clangd/Diagnostics.cpp:322
+      addToOutput(D);
+  }
+  LastDiagAndNotes.clear();
----------------
do you need else log here? (and just log the main diag)


================
Comment at: clangd/Diagnostics.h:28
+/// DiagList.
+struct PersistentDiag {
+  llvm::StringRef Message;
----------------
ilya-biryukov wrote:
> This could probably use a better name
I found it a bit confusing that this represents both "top-level" diagnostics and notes. It obscures the nature of the hierarchy a bit: there *are* a fixed number of levels with different "kinds", but the kinds are similar enough to share a type.

I'd actually consider using inheritance here to model the relationships more clearly. (Yes, gross, I know)
Like:

  struct DiagBase { Message, File, Range, InMainFile }
  struct Diag : DiagBase { FixIts, Notes, Severity }
  struct Note : DiagBase {} // or leave this one out until needed

(I think we came to the conclusion that different severity of notes isn't interesting and potentially just confusing)


================
Comment at: clangd/Diagnostics.h:35
+  DiagnosticsEngine::Level Severity;
+  llvm::SmallVector<TextEdit, 1> FixIts;
+  // Since File is only descriptive, we store a separate flag to distinguish
----------------
As discussed offline - I think fixits should probably be a different struct hanging off the main diagnostic, with a name. Following clang's example seems... less than clear here.

They could also be modeled as notes with an optional TextEdit - this seems sligthly less clear but more faithful to clang internals*.
Either way, it should be clear that they're only allowed in *one* of these locations - having notes and main diags be distinct types would help.

I think this probably affects how we should expose them through LSP - they should be named code actions attached to the original diagnostic.
Maybe they should also be separate diagnostics, but only if they contribute a unique opportunity to the user e.g. they have a non-empty range that isn't contained within the diagnostic's range.
This patch seems like a reasonable place to do that, but also OK if you want to defer.


================
Comment at: clangd/Diagnostics.h:35
+  DiagnosticsEngine::Level Severity;
+  llvm::SmallVector<TextEdit, 1> FixIts;
+  // Since File is only descriptive, we store a separate flag to distinguish
----------------
sammccall wrote:
> As discussed offline - I think fixits should probably be a different struct hanging off the main diagnostic, with a name. Following clang's example seems... less than clear here.
> 
> They could also be modeled as notes with an optional TextEdit - this seems sligthly less clear but more faithful to clang internals*.
> Either way, it should be clear that they're only allowed in *one* of these locations - having notes and main diags be distinct types would help.
> 
> I think this probably affects how we should expose them through LSP - they should be named code actions attached to the original diagnostic.
> Maybe they should also be separate diagnostics, but only if they contribute a unique opportunity to the user e.g. they have a non-empty range that isn't contained within the diagnostic's range.
> This patch seems like a reasonable place to do that, but also OK if you want to defer.
Nit: can we call these "Fix"es, which is a noun (at least in common usage?)

Clang calls them FixItHint, but I think Fix is no less clear (and shorter)


================
Comment at: clangd/Diagnostics.h:58
+/// by fields of PersistentDiag.
+class DiagList {
+public:
----------------
This is cool :) but really looks like premature optimization.
Particularly, if the diagnostics were a self-contained value type, this could just be vector<diag> I think. And if we care about copying strings (I'm not sure we do), being lifetime-scoped to the ASTcontext is probably OK.

(rawDiags() has potential uses but having callers explicitly traverse is going to be clearer as your comment suggests)


================
Comment at: clangd/Diagnostics.h:113
+/// An LSP diagnostic with FixIts.
+struct DiagWithFixIts {
+  clangd::Diagnostic Diag;
----------------
Can we try removing this struct in favor of two callback params? It seems like a small source of confusion, considering how important the type used to be.


================
Comment at: clangd/Diagnostics.h:123
+/// still be included as part of their main diagnostic's message.
+void toLSPDiags(const DiagList &Diags,
+                llvm::function_ref<void(DiagWithFixIts)> OutFn);
----------------
I don't think this overload pays for itself - making callers write the outer loop would be a little clearer, I think.

(I'd consider having that fn return a smallvector instead of taking a callback, but don't feel strongly)


================
Comment at: clangd/Diagnostics.h:128
+
+class StoreDiagsConsumer : public DiagnosticConsumer {
+public:
----------------
brief doc.
In particular: what filtering is applied


================
Comment at: clangd/Diagnostics.h:128
+
+class StoreDiagsConsumer : public DiagnosticConsumer {
+public:
----------------
sammccall wrote:
> brief doc.
> In particular: what filtering is applied
Maybe just StoreDiags?


================
Comment at: clangd/Diagnostics.h:130
+public:
+  StoreDiagsConsumer(DiagList &Output);
+
----------------
If there's no strong reason otherwise, `DiagList take()` is slightly easier/clearer for tests. compare

  DiagList Diags;
  StoreDiagsConsumer Cons(Diags);
  ...
  EXPECT_THAT(Diags, ...)

vs

  StoreDiagsConsumer Diags;
  ...
  EXPECT_THAT(Diags.take(), ...)


================
Comment at: clangd/SourceCode.cpp:53
+  // Clang is 1-based, LSP uses 0-based indexes.
+  Position Begin;
+  Begin.line = static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1;
----------------
isn't this sourceLocToPosition?


================
Comment at: test/clangd/diagnostics.test:23
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "change return type to 'int'",
+# CHECK-NEXT:        "message": "change return type to 'int'\n\nfoo.c:1:1: warning: return type of 'main' is not 'int'",
 # CHECK-NEXT:        "range": {
----------------
I think ideally this wouldn't show up at all, "change return type to int" would be the name of the code action returned for the first diag. Note the range is the same, so we're not giving the user an extra place to apply the fix.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44142