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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: clangd/ClangdLSPServer.cpp:338
+  Command Cmd;
+  if (Action.command && Action.edit)
+    return llvm::None;
----------------
sammccall wrote:
> 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)
Oh I see, nvm then.


================
Comment at: clangd/ClangdLSPServer.cpp:350
+  if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND)
+    Cmd.title = "Apply fix: " + Cmd.title;
+  return Cmd;
----------------
sammccall wrote:
> 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...)
OK, that makes sense.


================
Comment at: clangd/Protocol.h:390
+  /// Flattened from codeAction.codeActionLiteralSupport.
+  // FIXME: flatten other properties in this way.
+  bool codeActionLiteralSupport = false;
----------------
sammccall wrote:
> 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).
> 
Ok, I thought we were also going to throw away the "valueset"s and just keep whether the client has the capability(and therefore graceful handling) or not.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53213