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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
