clangd: Set a diagnostic on a code action resulting from a tweak

[ckandeler]

Summary

... if there is a match.
This is needed to that clients can can make a connection between a
diagnostic and an associated quickfix-tweak.
Ideally, quickfix-kind tweak code actions would be provided inline along
with the non-tweak fixes, but this doesn't seem easily achievable.

[sammccall]

This is assuming a semantic connection that we don't know exists.
Without any more specific reason to draw this connection, this seems like a heuristic that could equally be applied by the client.

Is there a particular action/diagnostic pair you want this for?

[ckandeler]

This is assuming a semantic connection that we don't know exists.
Without any more specific reason to draw this connection, this seems like a heuristic that could equally be applied by the client.

The difference being that the client would have to poke around in the technically opaque Command structure to find the range.
Seems possible, but presumably there are no guarantees about the properties?

Is there a particular action/diagnostic pair you want this for?

Yes, -Wswitch/PopulateSwitch (which is the only quickfix kind of tweak at the moment).

[sammccall]

TL;DR: I think this makes sense to add, but we should make it slightly more restrictive.

In D118976#3296182, @ckandeler wrote:

This is assuming a semantic connection that we don't know exists.
Without any more specific reason to draw this connection, this seems like a heuristic that could equally be applied by the client.

The difference being that the client would have to poke around in the technically opaque Command structure to find the range.
Seems possible, but presumably there are no guarantees about the properties?

FWIW, I don't think poking in command is required: the test you're adding here is that:

• tweak kind is quickfix: tweak kind is exposed as CodeAction.kind
• diag range == params.range: both diag and params are supplied by the client

However the protocol is complicated enough here as it is without adding client-side heuristics, we should probably have the server signal them explicitly if we think they're safe.

Is there a particular action/diagnostic pair you want this for?

Yes, -Wswitch/PopulateSwitch (which is the only quickfix kind of tweak at the moment).

Yes, makes sense. I think we added the "preferred" logic as a hack.
FWIW, VSCode accepts this as associating the diagnostic with the fix, even without CodeAction.Diagnostics set.

But doing this with CodeAction.diagnostics makes at least as much sense, so let's support that.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1002

This loop is closely related to what you're doing (and the one above isn't).

Maybe we can refactor/combine as something like:

Optional<CodeAction *> OnlyFix; // nullptr means multiple fixes
// ... loop over actions and set OnlyFix ...
if (OnlyFix && *OnlyFix) {
  (*OnlyFix)->isPreferred = true;
  if (Diags.size() == 1 && Diags.front().range == Selection)
    (*OnlyFix)->diagnostics = {Diags.front()];
}

Note this adds the conditions:

• should be only one diagnostic in context
• should be only one quickfix action

I think these reduce the chance of false positives.

[ckandeler]

Formatting & logic.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1002

Done. Though it seems the code would work just as well with OnlyFix staying a normal pointer. Did I misunderstand something?

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1002

Oops, I missed the break in the loop.
Yes a normal pointer would be better, thanks!

[ckandeler]

Simplified code.

[ckandeler]

Fixed Lint complaints

[ckandeler]

It'd be great if you could merge this, as I don't have the necessary privileges.

clang-tools-extra/clangd/ClangdLSPServer.cpp

for (const Diagnostic &D : Params.context.diagnostics) {
         FixIts.push_back(toCodeAction(F, Params.textDocument.uri));
         FixIts.back().diagnostics = {D};
       }
     }
   }
 
   // Now enumerate the semantic code actions.
   auto ConsumeActions =
-      [Reply = std::move(Reply), File, Selection = Params.range,
-       FixIts = std::move(FixIts), this](
-          llvm::Expected<std::vector<ClangdServer::TweakRef>> Tweaks) mutable {
+      [Diags = Params.context.diagnostics, Reply = std::move(Reply), File,
+       Selection = Params.range, FixIts = std::move(FixIts), this](
         if (!Tweaks)
           return Reply(Tweaks.takeError());
 
         std::vector<CodeAction> Actions = std::move(FixIts);
         Actions.reserve(Actions.size() + Tweaks->size());
         for (const auto &T : *Tweaks)
           Actions.push_back(toCodeAction(T, File, Selection));
 
         // If there's exactly one quick-fix, call it "preferred".
         // We never consider refactorings etc as preferred.
-        CodeAction *OnlyFix = nullptr;
+        Optional<CodeAction *> OnlyFix; // nullptr means multiple fixes
         for (auto &Action : Actions) {
           if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) {
             if (OnlyFix) {
-              OnlyFix->isPreferred = false;
+              *OnlyFix = nullptr;
               break;
             }
-            Action.isPreferred = true;
             OnlyFix = &Action;
           }
         }
+        if (OnlyFix && *OnlyFix) {
+          (*OnlyFix)->isPreferred = true;
+          if (Diags.size() == 1 && Diags.front().range == Selection)
+            (*OnlyFix)->diagnostics = {Diags.front()};
+        }
 
         if (SupportsCodeAction)
           return Reply(llvm::json::Array(Actions));
         std::vector<Command> Commands;
         for (const auto &Action : Actions) {
           if (auto Command = asCommand(Action))
             Commands.push_back(std::move(*Command));
         }