[PATCH] D118976: clangd: Set a diagnostic on a code action resulting from a tweak

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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

In D118976#3296182 <https://reviews.llvm.org/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.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1011
         // We never consider refactorings etc as preferred.
         CodeAction *OnlyFix = nullptr;
         for (auto &Action : Actions) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118976