[PATCH] D148783: [clangd] Add support TextDocumentEdit.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

can you also have a version of the clang-tools-extra/clangd/test/fixits-command.test with `documentChanges` support? it's unlikely to have clients in that configuration but i believe the deserialization issue i mentioned above would be discoverable by such a test.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1722
                [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
+                 if (DiagOpts.EmbedFixesInDiagnostics && !Fixes.empty()) {
+                   Diag.codeActions.emplace();
----------------
hokein wrote:
> This part of code is moved from the `toLSPDiags`, we can keep it unchanged then we have to pass the `Version` and `SupportsDocumentChanges` to `toLSPDiags` which makes the API ugly. Open for ideas.
no i think it makes more sense in LSP layer, as it's an LSP extension. I believe we had it in the guts because i believe we were using it internally with a client that doesn't use LSPServer at some point.

it would be better to drop the extension but i see that qt-creator & sourcekit-lsp has mentions of this :/ so as I mentioned above, we can convert the fixes to CodeActions here unconditionally (it's not more expensive than the copy we perform today). afterwards we only make copying those CodeActions into the Diag conditioned on the capability.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:201
   void bindMethods(LSPBinder &, const ClientCapabilities &Caps);
-  std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D);
+  std::pair<std::optional<int64_t>, std::vector<Fix>>
+  getFixes(StringRef File, const clangd::Diagnostic &D);
----------------
instead of a pair maybe a:
```
struct VersionedFixes {
  std::optional<int64_t> DocumentVersion;
  std::vector<Fix> Fixes;
};
```


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:236
   std::mutex FixItsMutex;
   typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
       DiagnosticToReplacementMap;
----------------
can we instead have a:
```
std::map<clangd::Diagnostic, std::vector<CodeAction>, LSPDiagnosticCompare> Fixes;
```

We'll make sure we store code actions with necessary version information.
That way `FixItsMap` can stay the same, and rest of the code will look more uniform; we'll do the conversion from Fixes to CodeActions during `onDiagnosticsReady`


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:426
 
-CodeAction toCodeAction(const Fix &F, const URIForFile &File) {
+CodeAction toCodeAction(const Fix &F, const URIForFile &File,
+                        const std::optional<int64_t> &Version,
----------------
we can now move this into clangdlsperver.cpp


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:735
   llvm::json::ObjectMapper O(Params, P);
-  return O && O.map("changes", R.changes);
+  return O && O.map("changes", R.changes) && O.map("documentChanges", R.documentChanges);
 }
----------------
we actually want `O.mapOptional` for both "changes" and "documentChanges".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783