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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry, last comments were concurrent with the latest snapshot.

Except where noted, the old comments still apply I think.



================
Comment at: clangd/ClangdLSPServer.cpp:317
   for (Diagnostic &D : Params.context.diagnostics) {
-    auto Edits = getFixIts(Params.textDocument.uri.file(), D);
-    if (!Edits.empty()) {
+    auto Fixes = getFixIts(Params.textDocument.uri.file(), D);
+    for (auto &F : Fixes) {
----------------
inline this into the for loop?


================
Comment at: clangd/ClangdLSPServer.h:76
 
-  std::vector<TextEdit> getFixIts(StringRef File, const clangd::Diagnostic &D);
+  std::vector<Fix> getFixIts(StringRef File, const clangd::Diagnostic &D);
 
----------------
getFixes?


================
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:
> 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)
This is done, and looks good to me.


================
Comment at: clangd/Diagnostics.h:40
+
+struct PersistentDiag {
+  /// Main diagnostic.
----------------
sammccall wrote:
> I don't understand what "persistent" means in this context.
> This does seem to be an is-a relationship rather than has-a, so I'd find DiagBase/Diag + inheritance clearer.
> 
> But with composition, this one seems rike it should be "Diag" and the type of Main could be DiagDetails or so?
(this happened already, yay!)


================
Comment at: clangd/Diagnostics.h:47
+
+/// Represents a note for the diagnostic. Severity of this Diagnostic can only
+/// be 'note' or 'remark'.
----------------
nit: severity of notes can...


================
Comment at: clangd/Diagnostics.h:51
+
+/// A non-note diagnostic with Notes and fix-its.
+struct Diag : DiagBase {
----------------
Seems a bit confusing to use "diagnostic" to mean something importantly different than "diag", and to define top-level diagnostic in terms of notes.

Maybe just "A top-level diagnostic that may have Notes and Fixes"?


================
Comment at: clangd/Diagnostics.h:54
+  std::vector<Note> Notes;
+  std::vector<Fix> Fixes;
+};
----------------
I think these deserve some comments:
 - fixes are *alternative* fixes for this diagnostic, one should be chosen
 - notes elaborate on the problem, usually pointing to a related piece of code


================
Comment at: clangd/Diagnostics.h:57
+
+/// Conventional conversion to LSP diagnostics. Formats the error message of
+/// each diagnostic to include all its notes. Notes inside main file are also
----------------
I'm not sure what "conventional" means here?


================
Comment at: unittests/clangd/ClangdUnitTests.cpp:24
+
+void PrintTo(const DiagBase &D, std::ostream *O) {
+  llvm::raw_os_ostream OS(*O);
----------------
can you make these operator<< and move to the main code? This seems a fine general-purpose debug representation (could be tweaked, but also later).

That avoids the ODR issue, and makes casual logging easier.


================
Comment at: unittests/clangd/ClangdUnitTests.cpp:70
 
+class WithFixesMatcher : public testing::MatcherInterface<const Diag &> {
+public:
----------------
Ah, and now that you've written this... ;-)

For the specific case of "field X matches matcher M" you can use ::testing::Field (you'd keep WithNote, but make it call Field)

Messages are good as long as you pass the optional field_name param.


================
Comment at: unittests/clangd/ClangdUnitTests.cpp:149
+/// Matches diagnostic that has exactly one fix with the same range and message
+/// as the diagnostic itself.
+testing::Matcher<const clangd::Diag &>
----------------
this "same range and message as the diagnostic itself" is an important property of the code being tested, and it's hidden here in a comment of a helper function.
I'd probably prefer avoiding this helper altogether (spelling the string twice in the test) so the test behavior/change is obvious when changing the code.
Failing that, DiagWithEqualFix or something?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44142