[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:338
+  Command Cmd;
+  if (Action.command && Action.edit)
+    return llvm::None;
----------------
kadircet wrote:
> What would you think about emitting two commands in this case? First the edit and then the command. I believe LSP doesn't specify any ordering on how the commands returned should be executed by the client, so I am OK with current state as well. Just wanted to know if there were any other concerns.
That doesn't have the right semantics. Multiple commands returned from `textDocument/codeAction` are alternatives that the user should select between, whereas having both an edit and a command means they should be performed atomically as one action.

(This never actually happens as we don't emit such actions, added a comment)


================
Comment at: clangd/ClangdLSPServer.cpp:350
+  if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND)
+    Cmd.title = "Apply fix: " + Cmd.title;
+  return Cmd;
----------------
kadircet wrote:
> It seems we only prepend title with Apply fix when we fallback, I believe it would be better to add them in CodeAction instead?
The `CodeAction` has a slot to describe the type of action: if the client wants to prepend "Quick Fix: " or so it can.
(Personally this just seems like noise to me, so I'd rather the client omit it, but...)


================
Comment at: clangd/ClangdLSPServer.cpp:355
 void ClangdLSPServer::onCodeAction(CodeActionParams &Params) {
   // We provide a code action for each diagnostic at the requested location
   // which has FixIts available.
----------------
kadircet wrote:
> I believe this comment is misleading, do we perform any location check? Maybe change that to say "requested file"?
Updated the comment.


================
Comment at: clangd/Protocol.h:390
+  /// Flattened from codeAction.codeActionLiteralSupport.
+  // FIXME: flatten other properties in this way.
+  bool codeActionLiteralSupport = false;
----------------
kadircet wrote:
> What is the reason behind this one? Is it because clients must handle unknown items on their own and fallback to a default one?
> 
> Since that default is client specific, behavior might change from client to client. I agree that clients should be up-to-date with the specs and handle all kinds of items but these might still create confusions during the transition period.
> 
> For example, ycmd decided to fallback to None instead of Text when they don't know about a symbolkind of a completion item, so users will get to see "File" for the include insertions on both folders and files but when they update to a newer clangd, they will start to see "File" for files and "None" for directory elements. Which I believe might create confusion, but we could still fallback to File for those elements(if we handled them within clangd) and user experience would neither worsen or improve.
> 
> (Currently ycmd's symbolkindcapabilities are actually up-to-date with specs, so this issue wouldn't happen. Just wanted to make my point clearer). 
Sorry, I don't really understand the question.

Are you talking about the default for `codeActionLiteralSupport`? The protocol says servers must send `Command`s unless the client indicates support for `CodeAction`s. There's no room for a different default here.

Or flattening of other properties? That will have no effect on logic, it just simplifies the code (see D53266).



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53213