[clangd] Append "(fix available)" to diagnostic message when fixes are present.

[ioeric]

Summary

This would make diagnostic fixits more discoverable, especially for
plugins like YCM.

[kadircet]

Dropping by: Maybe add (fix available) as a prefix rather than suffix. Since long texts might end up getting truncated(especially in vim)

[sammccall]

In D57509#1378696, @kadircet wrote:

Dropping by: Maybe add (fix available) as a prefix rather than suffix. Since long texts might end up getting truncated(especially in vim)

Yeah truncation is definitely a problem, and on the other hand putting it at the start could get in the way and reads kind of backwards.
Eric: up to you - if you move it to the front probably shorten to "[Fix]" or so.

Another alternative is to use a prefix symbol like a bullet like we did with include insertion. As there, it's less explicit but also less intrusive.

clangd/Diagnostics.cpp

164 ↗

(On Diff #184488)

I'd consider passing the number here, and distinguishing between "Fix" and "Fixes" in the message.
Reasoning: when multiple fixes are available the user will have to make a choice. This may use a distinct UI (YCM) or not (VSCode), but foreshadowing the need for a choice will make the flow more predictable.

I'd suggest *not* printing the actual number though, seems too noisy.

[ioeric]

• address comments

[ioeric]

In D57509#1378707, @sammccall wrote:

In D57509#1378696, @kadircet wrote:

Dropping by: Maybe add (fix available) as a prefix rather than suffix. Since long texts might end up getting truncated(especially in vim)

Yeah truncation is definitely a problem, and on the other hand putting it at the start could get in the way and reads kind of backwards.
Eric: up to you - if you move it to the front probably shorten to "[Fix]" or so.

Another alternative is to use a prefix symbol like a bullet like we did with include insertion. As there, it's less explicit but also less intrusive.

Yeah, this could be a concern for plugins that truncate messages (YCM+Vim). There are other editors that do not truncate e.g. vscode. So I'd suggest we start with something less intrusive like suffix and try it out. If it turned out to be a problem too often, we can explore options with prefix.

clangd/Diagnostics.cpp

164 ↗

(On Diff #184488)

Sounds good. Done.

[ilya-biryukov]

Could this be made optional for VSCode?
As mentioned in the discussion before, I would personally prefer to turn it off, even though I do acknowledge the need for this for some clients, e.g. vim.

[ioeric]

In D57509#1378928, @ilya-biryukov wrote:

Could this be made optional for VSCode?
As mentioned in the discussion before, I would personally prefer to turn it off, even though I do acknowledge the need for this for some clients, e.g. vim.

Yes, but a new option seems a bit of an overkill here. The text is appended and doesn't seem to affect the readability of original diagnostic message much (IMO). Could you elaborate why this is undesirable in vscode for you?

[ilya-biryukov]

In D57509#1378943, @ioeric wrote:

Yes, but a new option seems a bit of an overkill here. The text is appended and doesn't seem to affect the readability of original diagnostic message much (IMO). Could you elaborate why this is undesirable in vscode for you?

It's basically just noise, as VSCode shows the bulb icons whenever you hover over the diagnostics with errors.
Having a standard suffix in half of the diagnostics seems undesirable.

Adding a suffix like this should be left to the client, i.e. it's clear this is desirable in vim which does not have other UI indicators and adds noise in VSCode, which uses bulb icons for those purposes.
I do agree that plumbing the parameters is annoying, but this does look like a thing where different clients might diverge in how they present results and it's totally looks fine.
Note that command-line argument is not the only way to do this, e.g. we could also have a special capability as an extension that would turn these messages of (clangd.addFixAvailableMessage) and send it from VSCode. That's totally doable, since we control the extension.

[ioeric]

In D57509#1378978, @ilya-biryukov wrote:

In D57509#1378943, @ioeric wrote:

Yes, but a new option seems a bit of an overkill here. The text is appended and doesn't seem to affect the readability of original diagnostic message much (IMO). Could you elaborate why this is undesirable in vscode for you?

It's basically just noise, as VSCode shows the bulb icons whenever you hover over the diagnostics with errors.
Having a standard suffix in half of the diagnostics seems undesirable.

I don't think it's pure noise. Vscode displays diagnostics in the "PROBLEMS" tab. A suffix allows you to tell whether fixes are available without hovering on the errors.

That said, I'm not opposed to a config option. Would love to hear others' thoughts tho. @sammccall @hokein @kadircet

[sammccall]

FWIW, I don't think this is worth adding an option or protocol extensions for at this point.

There are pros and cons to showing this in VSCode (pros are visibility without hovering, particularly in problems view and cross-editor consistency, cons are certainly noise for main-editor hover).
But this seems pretty minor either way.

If it turns out to be a big problem, a protocol extension and updating the editor extensions where relevant seems like a reasonable way to go, but it's certainly more complicated.

[ilya-biryukov]

I don't think it's pure noise. Vscode displays diagnostics in the "PROBLEMS" tab. A suffix allows you to tell whether fixes are available without hovering on the errors.

And it shows bulb icons when you hover over diagnostics in this dialog too. To be clear, it's my personal preference to not have it, that's the reason I call it 'noise', others might like the message and it feel more useful to them.

In D57509#1379003, @sammccall wrote:

But this seems pretty minor either way.

Sure :(

clang-tools-extra/trunk/clangd/Diagnostics.cpp

 ///     main.cpp:3:5: note: candidate function not viable: requires 2 arguments
 ///
 ///     dir1/dir2/dir3/../../dir4/header.h:12:23
 ///     note: candidate function not viable: requires 3 arguments
 std::string mainMessage(const Diag &D) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << D.Message;
+  if (!D.Fixes.empty())
+    OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)";
   for (auto &Note : D.Notes) {
     OS << "\n\n";
     printDiag(OS, Note);
   }
   OS.flush();
   return capitalize(std::move(Result));
 }
 

clang-tools-extra/trunk/test/clangd/diagnostic-category.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"categorySupport":true}}},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
 #      CHECK:    "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:    "params": {
 # CHECK-NEXT:     "diagnostics": [
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "category": "Semantic Issue",
-# CHECK-NEXT:        "message": "Use of 'Point' with tag type that does not match previous declaration\n\nfoo.c:1:8: note: previous use is here",
+# CHECK-NEXT:        "message": "Use of 'Point' with tag type that does not match previous declaration (fix available)\n\nfoo.c:1:8: note: previous use is here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 22,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 17,
 # CHECK-NEXT:            "line": 0

clang-tools-extra/trunk/test/clangd/diagnostics.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "Return type of 'main' is not 'int'",
+# CHECK-NEXT:        "message": "Return type of 'main' is not 'int' (fix available)",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 4,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 0,
 # CHECK-NEXT:            "line": 0

clang-tools-extra/trunk/test/clangd/did-change-configuration-params.test

 # CHECK-NEXT:    "uri": "file://{{.*}}/bar.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": {"workingDirectory":"/clangd-test2", "compilationCommand": ["clang", "-c", "foo.c", "-Wall", "-Werror"]}}}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here",
+# CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here (fix available)",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 28,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,
 # CHECK-NEXT:            "line": 0

clang-tools-extra/trunk/test/clangd/execute-command.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0

clang-tools-extra/trunk/test/clangd/fixits-codeaction.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}}},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
 #      CHECK:    "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "diagnostics": [
 # CHECK-NEXT:        {
-# CHECK-NEXT:          "message": "Using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT:          "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:          "range": {
 # CHECK-NEXT:            "end": {
 # CHECK-NEXT:              "character": 37,
 # CHECK-NEXT:              "line": 0
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "start": {
 # CHECK-NEXT:              "character": 32,
 # CHECK-NEXT:              "line": 0

 # CHECK-NEXT:        }
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "kind": "quickfix",
 # CHECK-NEXT:      "title": "place parentheses around the assignment to silence this warning"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "diagnostics": [
 # CHECK-NEXT:        {
-# CHECK-NEXT:          "message": "Using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT:          "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:          "range": {
 # CHECK-NEXT:            "end": {
 # CHECK-NEXT:              "character": 37,
 # CHECK-NEXT:              "line": 0
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "start": {
 # CHECK-NEXT:              "character": 32,
 # CHECK-NEXT:              "line": 0

clang-tools-extra/trunk/test/clangd/fixits-command.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
 #      CHECK:    "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {
 # CHECK-NEXT:            "file://{{.*}}/foo.c": [

 # CHECK-NEXT:          }
 # CHECK-NEXT:        }
 # CHECK-NEXT:      ],
 # CHECK-NEXT:      "command": "clangd.applyFix",
 # CHECK-NEXT:      "title": "Apply fix: use '==' to turn this assignment into an equality comparison"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
-{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
+{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {

clang-tools-extra/trunk/test/clangd/fixits-embed-in-diagnostic.test

 # CHECK-NEXT:                  }
 # CHECK-NEXT:                ]
 # CHECK-NEXT:              }
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "kind": "quickfix",
 # CHECK-NEXT:            "title": "change 'union' to 'struct'"
 # CHECK-NEXT:          }
 # CHECK-NEXT:        ],
-# CHECK-NEXT:        "message": "Use of 'Point' with tag type that does not match previous declaration\n\nfoo.c:1:8: note: previous use is here",
+# CHECK-NEXT:        "message": "Use of 'Point' with tag type that does not match previous declaration (fix available)\n\nfoo.c:1:8: note: previous use is here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 22,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 17,
 # CHECK-NEXT:            "line": 0

clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp

auto MatchingLSP = [](const DiagBase &D, StringRef Message) {
     clangd::Diagnostic Res;
     Res.range = D.Range;
     Res.severity = getSeverity(D.Severity);
     Res.message = Message;
     return Res;
   };
 
   // Diagnostics should turn into these:
-  clangd::Diagnostic MainLSP = MatchingLSP(D, R"(Something terrible happened
+  clangd::Diagnostic MainLSP =
+      MatchingLSP(D, R"(Something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
 ../foo/baz/header.h:10:11:
 note: declared somewhere in the header file)");
 
   clangd::Diagnostic NoteInMainLSP =
       MatchingLSP(NoteInMain, R"(Declared somewhere in the main file