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

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

should be fixed in 
https://github.com/llvm/llvm-project/commit/4294619b4cdfed49502dcebc7efd5f044e587267.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D148783#4298398 , @DavidSpickett 
wrote:

> Hi, these tests are failing on our (Linaro's) Windows on Arm buildbot. First 
> appearance here https://lab.llvm.org/buildbot/#/builders/65/builds/9346
>
> Still failing as of a few hours ago 
> https://lab.llvm.org/buildbot/#/builders/65/builds/9360 (maybe you have fixed 
> this already, I am checking in infrequently over the next few days).
>
> If you need any help debugging it let me know. It appears there is a drive 
> letter on Windows:
>
>   # CHECK-NEXT:  "uri": "file:///clangd-test/foo.c",
>  ^
>   :224:30: note: scanning from here
>   "textDocument": {
>^
>   :225:15: note: possible intended match here
> "uri": "file:///C:/clangd-test/foo.c",
> ^

sorry for the breakage, looking now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Hi, these tests are failing on our (Linaro's) Windows on Arm buildbot. First 
appearance here https://lab.llvm.org/buildbot/#/builders/65/builds/9346

Still failing as of a few hours ago 
https://lab.llvm.org/buildbot/#/builders/65/builds/9360 (maybe you have fixed 
this already, I am checking in infrequently over the next few days).

If you need any help debugging it let me know. It appears there is a drive 
letter on Windows:

  # CHECK-NEXT:  "uri": "file:///clangd-test/foo.c",
 ^
  :224:30: note: scanning from here
  "textDocument": {
   ^
  :225:15: note: possible intended match here
"uri": "file:///C:/clangd-test/foo.c",
^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG67e02b282b70: [clangd] Add support TextDocumentEdit. 
(authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
  clang-tools-extra/clangd/test/fixits-command-documentchanges.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test

Index: clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
===
--- clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -48,6 +48,7 @@
 # CHECK-NEXT:"source": "clang"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  {
+# CHECK-NEXT:"codeActions": [],
 # CHECK-NEXT:"message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: clang-tools-extra/clangd/test/fixits-command-documentchanges.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/fixits-command-documentchanges.test
@@ -0,0 +1,194 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"workspaceEdit":{"documentChanges":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"int main(int i, char **a) { if (i = 2) {}}"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"code": "-Wparentheses",
+# CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 2,
+# CHECK-NEXT:"source": "clang"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c",
+# CHECK-NEXT:"version": 0
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
+#  CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "arguments": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "documentChanges": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "edits": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "newText": "(",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:{
+# CHECK-NEXT:  "newText": ")",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  "textDocument": {
+# CHECK-NEXT:"uri": "file:///clangd-test/foo.c",
+# CHECK-NEXT:"version": 0
+# CHECK-NEXT:   

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 516734.
hokein marked an inline comment as done.
hokein added a comment.

update and rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
  clang-tools-extra/clangd/test/fixits-command-documentchanges.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test

Index: clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
===
--- clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -48,6 +48,7 @@
 # CHECK-NEXT:"source": "clang"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  {
+# CHECK-NEXT:"codeActions": [],
 # CHECK-NEXT:"message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: clang-tools-extra/clangd/test/fixits-command-documentchanges.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/fixits-command-documentchanges.test
@@ -0,0 +1,194 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"workspaceEdit":{"documentChanges":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"int main(int i, char **a) { if (i = 2) {}}"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"code": "-Wparentheses",
+# CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 2,
+# CHECK-NEXT:"source": "clang"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c",
+# CHECK-NEXT:"version": 0
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
+#  CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "arguments": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "documentChanges": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "edits": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "newText": "(",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:{
+# CHECK-NEXT:  "newText": ")",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  "textDocument": {
+# CHECK-NEXT:"uri": "file:///clangd-test/foo.c",
+# CHECK-NEXT:"version": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}
+# 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks!




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1694
 
-std::vector ClangdLSPServer::getFixes(llvm::StringRef File,
-   const clangd::Diagnostic ) {
-  std::lock_guard Lock(FixItsMutex);
-  auto DiagToFixItsIter = FixItsMap.find(File);
-  if (DiagToFixItsIter == FixItsMap.end())
-return {};
+std::vector ClangdLSPServer::getFixes(StringRef File,
+  const clangd::Diagnostic ) 
{

nit: let's keep `llvm::StringRef`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1746
+
+ if (DiagOpts.EmbedFixesInDiagnostics && !CodeActions.empty()) 
{
+   Diag.codeActions.emplace(CodeActions);

kadircet wrote:
> we didn't have the empty check before, let's not introduce it now (i.e. we'll 
> still reply with an empty set of code actions rather than "none" when there 
> are no fixes known to clangd)
yeah, I added this empty check to keep `fixits-embed-in-diagnostic.test` 
unchanged (we don't emit code-action for diagnostic notes), but I think it is 
fair enough to change it.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1751-1753
  auto  = LocalFixIts[Diag];
- llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
+ llvm::move(std::move(CodeActions),
+std::back_inserter(FixItsForDiagnostic));

kadircet wrote:
> i am not sure why this logic was appending to the vector rather than just 
> overwriting. but we shouldn't receive the same diagnostics from the callback 
> here, am i missing something? so what about just:
> ```
> LocalFixIts[Diag] = std::move(CodeActions);
> ```
Agreed, changed to overwriting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 516702.
hokein added a comment.

address review comments, added one more test for command code-actions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
  clang-tools-extra/clangd/test/fixits-command-documentchanges.test
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test

Index: clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
===
--- clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -48,6 +48,7 @@
 # CHECK-NEXT:"source": "clang"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  {
+# CHECK-NEXT:"codeActions": [],
 # CHECK-NEXT:"message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: clang-tools-extra/clangd/test/fixits-command-documentchanges.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/fixits-command-documentchanges.test
@@ -0,0 +1,194 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"workspaceEdit":{"documentChanges":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"int main(int i, char **a) { if (i = 2) {}}"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"code": "-Wparentheses",
+# CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 2,
+# CHECK-NEXT:"source": "clang"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c",
+# CHECK-NEXT:"version": 0
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
+#  CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "arguments": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "documentChanges": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "edits": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "newText": "(",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:{
+# CHECK-NEXT:  "newText": ")",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  "textDocument": {
+# CHECK-NEXT:"uri": "file:///clangd-test/foo.c",
+# CHECK-NEXT:"version": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D148783#4286652 , @hokein wrote:

>> 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.
>
> I'm happy to add a test for that, but I'm not sure the deserialization issue 
> you mentioned in the comment, is the one to use `mapOptional`?

yes it was for `mapOptional`, which turns out not to be an issue as there's a 
specialization for `optional`.

but still having that test to verify deserialization logic wouldn't hurt.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1746
+
+ if (DiagOpts.EmbedFixesInDiagnostics && !CodeActions.empty()) 
{
+   Diag.codeActions.emplace(CodeActions);

we didn't have the empty check before, let's not introduce it now (i.e. we'll 
still reply with an empty set of code actions rather than "none" when there are 
no fixes known to clangd)



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1751-1753
  auto  = LocalFixIts[Diag];
- llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
+ llvm::move(std::move(CodeActions),
+std::back_inserter(FixItsForDiagnostic));

i am not sure why this logic was appending to the vector rather than just 
overwriting. but we shouldn't receive the same diagnostics from the callback 
here, am i missing something? so what about just:
```
LocalFixIts[Diag] = std::move(CodeActions);
```



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);
 }

hokein wrote:
> kadircet wrote:
> > we actually want `O.mapOptional` for both "changes" and "documentChanges".
> I think `map` is a better fit here, it has a specific version of 
> `std::optional`, see 
> https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/JSON.h#L852.
> 
> `mapOptional`  doesn't set the field when missing the key in json object,
yeah you're right, i missed the specialization of `map` for `optional`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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.

I'm happy to add a test for that, but I'm not sure the deserialization issue 
you mentioned in the comment, is the one to use `mapOptional`?




Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:201
   void bindMethods(LSPBinder &, const ClientCapabilities );
-  std::vector getFixes(StringRef File, const clangd::Diagnostic );
+  std::pair, std::vector>
+  getFixes(StringRef File, const clangd::Diagnostic );

kadircet wrote:
> instead of a pair maybe a:
> ```
> struct VersionedFixes {
>   std::optional DocumentVersion;
>   std::vector Fixes;
> };
> ```
we don't need this struct now, as we switch to store the `CodeAction`.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:236
   std::mutex FixItsMutex;
   typedef std::map, LSPDiagnosticCompare>
   DiagnosticToReplacementMap;

kadircet wrote:
> can we instead have a:
> ```
> std::map, 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`
good idea! 



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);
 }

kadircet wrote:
> we actually want `O.mapOptional` for both "changes" and "documentChanges".
I think `map` is a better fit here, it has a specific version of 
`std::optional`, see 
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/JSON.h#L852.

`mapOptional`  doesn't set the field when missing the key in json object,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 515671.
hokein marked 2 inline comments as done.
hokein added a comment.

address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test

Index: clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
@@ -0,0 +1,145 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}},"workspace":{"workspaceEdit":{"documentChanges":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"code": "-Wparentheses",
+# CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 2,
+# CHECK-NEXT:"source": "clang"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c",
+# CHECK-NEXT:"version": 1
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)", "code": "-Wparentheses", "source": "clang"}]}}}
+#  CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "diagnostics": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "code": "-Wparentheses",
+# CHECK-NEXT:  "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "severity": 2,
+# CHECK-NEXT:  "source": "clang"
+# CHECK-NEXT:}
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  "edit": {
+# CHECK-NEXT:"documentChanges": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"edits": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"newText": "(",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  {
+# CHECK-NEXT:"newText": ")",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT:  "uri": "file:///clangd-test/foo.c",
+# CHECK-NEXT:  "version": 1
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-NEXT:  },
+# CHECK-NEXT:  

[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 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 );
-  std::vector getFixes(StringRef File, const clangd::Diagnostic );
+  std::pair, std::vector>
+  getFixes(StringRef File, const clangd::Diagnostic );

instead of a pair maybe a:
```
struct VersionedFixes {
  std::optional DocumentVersion;
  std::vector Fixes;
};
```



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:236
   std::mutex FixItsMutex;
   typedef std::map, LSPDiagnosticCompare>
   DiagnosticToReplacementMap;

can we instead have a:
```
std::map, 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 , const URIForFile ) {
+CodeAction toCodeAction(const Fix , const URIForFile ,
+const std::optional ,

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 515230.
hokein added a comment.

add missing test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test

Index: clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
@@ -0,0 +1,145 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}},"workspace":{"workspaceEdit":{"documentChanges":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"code": "-Wparentheses",
+# CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 2,
+# CHECK-NEXT:"source": "clang"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c",
+# CHECK-NEXT:"version": 1
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)", "code": "-Wparentheses", "source": "clang"}]}}}
+#  CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "diagnostics": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "code": "-Wparentheses",
+# CHECK-NEXT:  "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 37,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 32,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "severity": 2,
+# CHECK-NEXT:  "source": "clang"
+# CHECK-NEXT:}
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  "edit": {
+# CHECK-NEXT:"documentChanges": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"edits": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"newText": "(",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 32,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  {
+# CHECK-NEXT:"newText": ")",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 37,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT:  "uri": "file:///clangd-test/foo.c",
+# CHECK-NEXT:  "version": 1
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "kind": "quickfix",
+# CHECK-NEXT:  "title": 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1722
[&](clangd::Diagnostic Diag, llvm::ArrayRef Fixes) {
+ if (DiagOpts.EmbedFixesInDiagnostics && !Fixes.empty()) {
+   Diag.codeActions.emplace();

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: kadircet.
Herald added a subscriber: arphaman.
Herald added a project: All.
hokein requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This is an initial patch to add TextDocumentEdit (versioned edits) support in
clangd, the scope is minimal:

- add and extend the corresponding protocol structures
- propagate the documentChanges for diagnostic codeactions (our motivated case)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148783

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h

Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -254,6 +254,17 @@
 llvm::json::Value toJSON(const TextEdit &);
 llvm::raw_ostream <<(llvm::raw_ostream &, const TextEdit &);
 
+struct TextDocumentEdit {
+  /// The text document to change.
+  VersionedTextDocumentIdentifier textDocument;
+
+	/// The edits to be applied.
+  /// FIXME: support the AnnotatedTextEdit variant.
+  std::vector edits;
+};
+bool fromJSON(const llvm::json::Value &, TextDocumentEdit &, llvm::json::Path);
+llvm::json::Value toJSON(const TextDocumentEdit &);
+
 struct TextDocumentItem {
   /// The text document's URI.
   URIForFile uri;
@@ -517,6 +528,9 @@
   /// server to the client.
   bool SemanticTokenRefreshSupport = false;
 
+  /// The client supports versioned document changes for WorkspaceEdit.
+  bool DocumentChanges = false;
+
   /// Whether the client supports the textDocument/inactiveRegions
   /// notification. This is a clangd extension.
   bool InactiveRegions = false;
@@ -970,12 +984,18 @@
 };
 bool fromJSON(const llvm::json::Value &, CodeActionParams &, llvm::json::Path);
 
+/// The edit should either provide changes or documentChanges. If the client
+/// can handle versioned document edits and if documentChanges are present,
+/// the latter are preferred over changes.
 struct WorkspaceEdit {
   /// Holds changes to existing resources.
-  std::map> changes;
-
-  /// Note: "documentChanges" is not currently used because currently there is
-  /// no support for versioned edits.
+  std::optional>> changes;
+  /// Versioned document edits.
+  ///
+  /// If a client neither supports `documentChanges` nor
+	/// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s
+	/// using the `changes` property are supported.
+  std::optional> documentChanges;
 };
 bool fromJSON(const llvm::json::Value &, WorkspaceEdit &, llvm::json::Path);
 llvm::json::Value toJSON(const WorkspaceEdit );
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -197,6 +197,17 @@
   };
 }
 
+bool fromJSON(const llvm::json::Value , TextDocumentEdit ,
+  llvm::json::Path P) {
+  llvm::json::ObjectMapper O(Params, P);
+  return O && O.map("textDocument", R.textDocument) && O.map("edits", R.edits);
+}
+llvm::json::Value toJSON(const TextDocumentEdit ) {
+  llvm::json::Object Result{{"textDocument", P.textDocument},
+{"edits", P.edits}};
+  return Result;
+}
+
 llvm::raw_ostream <<(llvm::raw_ostream , const TextEdit ) {
   OS << TE.range << " => \"";
   llvm::printEscapedString(TE.newText, OS);
@@ -444,6 +455,10 @@
   if (auto RefreshSupport = SemanticTokens->getBoolean("refreshSupport"))
 R.SemanticTokenRefreshSupport = *RefreshSupport;
 }
+if (auto *WorkspaceEdit = Workspace->getObject("workspaceEdit")) {
+  if (auto DocumentChanges = WorkspaceEdit->getBoolean("documentChanges"))
+R.DocumentChanges = *DocumentChanges;
+}
   }
   if (auto *Window = O->getObject("window")) {
 if (auto WorkDoneProgress = Window->getBoolean("workDoneProgress"))
@@ -717,7 +732,7 @@
 bool fromJSON(const llvm::json::Value , WorkspaceEdit ,
   llvm::json::Path P) {
   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);
 }
 
 bool fromJSON(const llvm::json::Value , ExecuteCommandParams ,
@@ -863,10 +878,16 @@
 }
 
 llvm::json::Value toJSON(const WorkspaceEdit ) {
-  llvm::json::Object FileChanges;
-  for (auto  : WE.changes)
-FileChanges[Change.first] = llvm::json::Array(Change.second);
-  return llvm::json::Object{{"changes", std::move(FileChanges)}};
+  llvm::json::Object Result;
+  if (WE.changes) {
+llvm::json::Object FileChanges;
+for (auto  : *WE.changes)
+  FileChanges[Change.first] = llvm::json::Array(Change.second);