[clangd] Escape only necessary characters in JSON output

[malaperle-ericsson]

Summary

Currently, the strings being output by Clangd are escaled with llvm::yaml::escape. But this is not entirely correct because it escapes a lot of additional characters things. For example, a 2 byte character "é" will be encoded with a \x which is not valid and this error is thrown by the VSCode client:

SyntaxError: Unexpected token x in JSON at position 291
    at StreamMessageReader.onData
    at Socket.<anonymous>
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)
    at Pipe.onread (net.js:543:20)

This change introduces an escaping function that does follow the JSON format.

[krasimir]

Seems that we're starting to hit some YAML/JSON mismatches, or is it that your YAML string support is lacking?

[malaperle-ericsson]

In D31992#725866, @krasimir wrote:

Seems that we're starting to hit some YAML/JSON mismatches, or is it that your YAML string support is lacking?

I don't think so. It seems like JSON and YAML are not completely aligned on escaped characters. See http://yaml.org/spec/1.2/spec.html#id2776092 which specifies unicode 8, 16, and 32 bits are escaped with \x, \u, and \U whereas http://www.json.org/string.gif specifies that unicode 16 bit characters can be encoded with \u but \x and \U are not supported. This leads me to believe that a YAML parser can read JSON but a JSON parser will not necessarily be able to read YAML. I thought about using json cpp but that's a much bigger change

[joerg]

I'm strongly against this patch. Can you give an actual test case for the problematic behavior?

[malaperle-ericsson]

In D31992#725913, @joerg wrote:

I'm strongly against this patch. Can you give an actual test case for the problematic behavior?

Sure I can add a test. If you meant more real work scenario, you can juste type "é" in VS Code and it will throw an exception trying to parse \x

[klimek]

clangd/Protocol.cpp

27–51

For json we only need the first 2 though, right?

[klimek]

In D31992#725912, @malaperle-ericsson wrote:

In D31992#725866, @krasimir wrote:

Seems that we're starting to hit some YAML/JSON mismatches, or is it that your YAML string support is lacking?

I don't think so. It seems like JSON and YAML are not completely aligned on escaped characters. See http://yaml.org/spec/1.2/spec.html#id2776092 which specifies unicode 8, 16, and 32 bits are escaped with \x, \u, and \U whereas http://www.json.org/string.gif specifies that unicode 16 bit characters can be encoded with \u but \x and \U are not supported. This leads me to believe that a YAML parser can read JSON but a JSON parser will not necessarily be able to read YAML. I thought about using json cpp but that's a much bigger change

Here we only talk about what we escape, which should be the minimum required.
If I understand correctly, that's " and \ for JSON and ", \ and all non-printable characters (which unfortunately requires understanding of unicode to solve this fully correctly) in YAML.
Am I missing something?

[joerg]

In D31992#725963, @klimek wrote:

If I understand correctly, that's " and \ for JSON and ", \ and all non-printable characters (which unfortunately requires understanding of unicode to solve this fully correctly) in YAML.

I'd modify that slightly: let's escape those two and all the known ASCII control characters, but just pass the rest through. I.e. reduce the current \x etc handling to known problematic input and not second guess the rest.

[malaperle-ericsson]

In D31992#726447, @joerg wrote:

let's escape ... all the known ASCII control characters,

Do you mean encode all of them with \u or keep the two characters representation for those that exist? I think \n is nicer than \u000A and is probably common enough to keep the short version. Same for \r, \t.

[malaperle-ericsson]

Handle other control characters and add test

[joerg]

The short version is perfectly fine as long as works for both JSON and YAML. Less output is always good :)

[malaperle-ericsson]

Once the use of "two characters representation" is clarified, I will update the patch again.

[joerg]

Just to avoid any confusion: this should be the generic YAML escape routine in llvm/lib/Support, i.e. IMO we don't want to have separate YAML and JSON escape routines.
Effective, change YAMLParser.cpp line 697 to use \u and drop the whole UTF-8 handling part.

[malaperle-ericsson]

In D31992#728036, @joerg wrote:

Just to avoid any confusion: this should be the generic YAML escape routine in llvm/lib/Support, i.e. IMO we don't want to have separate YAML and JSON escape routines.
Effective, change YAMLParser.cpp line 697 to use \u and drop the whole UTF-8 handling part.

I'm not sure it's possible or desirable to reuse yaml::escape as they are two formats and it's expected that a yaml output might not be able to be read by a json parser. If I look at the yaml::escape routine, there are several escape that are incompatible with JSON like \0, \v, \e, and 32-bit handling via \U.
I think ideally this patch would be a stop-gap solution until jsoncpp is introduced so that one can build json output in a structured way (as opposed to manually appending strings now) and the output will also be escape via the library.

[joerg]

We already have a couple of case that expect the encoding to be compatible. I'm not very attached to the additional special cases from YAML, but having either a common escape function OR a JSON escape in LLVM/Support is quite important.

clangd/ASTManager.cpp

if (!D->getLocation().isValid() ||
       continue;
     Position P;
     P.line = D->getLocation().getSpellingLineNumber() - 1;
     P.character = D->getLocation().getSpellingColumnNumber();
     Range R = {P, P};
     Diagnostics +=
         R"({"range":)" + Range::unparse(R) +
         R"(,"severity":)" + std::to_string(getSeverity(D->getLevel())) +
-        R"(,"message":")" + llvm::yaml::escape(D->getMessage()) +
+        R"(,"message":")" + jsonEscape(D->getMessage()) +
         R"("},)";
 
     // We convert to Replacements to become independent of the SourceManager.
     clangd::Diagnostic Diag = {R, getSeverity(D->getLevel()), D->getMessage()};
     auto &FixItsForDiagnostic = LocalFixIts[Diag];
     for (const FixItHint &Fix : D->getFixIts()) {
       FixItsForDiagnostic.push_back(clang::tooling::Replacement(
           Unit->getSourceManager(), Fix.RemoveRange, Fix.CodeToInsert));

clangd/Protocol.h

 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/YAMLParser.h"
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
+std::string jsonEscape(llvm::StringRef Input);
+
 struct URI {
   std::string uri;
   std::string file;
 
   static URI fromUri(llvm::StringRef uri);
   static URI fromFile(llvm::StringRef file);
 
   static URI parse(llvm::yaml::ScalarNode *Param);

clangd/Protocol.cpp

 // This file contains the serialization code for the LSP structs.
 // FIXME: This is extremely repetetive and ugly. Is there a better way?
 //
 //===----------------------------------------------------------------------===//
 
 #include "Protocol.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallString.h"
+#include <llvm/ADT/StringExtras.h>
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Path.h"
 using namespace clang::clangd;
 
 
+std::string clang::clangd::jsonEscape(llvm::StringRef Input) {
+  std::string EscapedInput;
+  for (llvm::StringRef::iterator i = Input.begin(), e = Input.end(); i != e; ++i) {
+    if (*i == '\\')
+      EscapedInput += "\\\\";
+    else if (*i == '"')
+      EscapedInput += "\\\"";
+    // bell
+    else if (*i == 0x07)
+      EscapedInput += "\\a";
+    // backspace
+    else if (*i == 0x08)
+      EscapedInput += "\\b";
+    // hoz tab
+    else if (*i == 0x09)
+      EscapedInput += "\\t";
+    // new line
+    else if (*i == 0x0A)
+      EscapedInput += "\\n";
+    // form feed
+    else if (*i == 0x0C)
+      EscapedInput += "\\f";
+    // carr return
+    else if (*i == 0x0D)
+      EscapedInput += "\\r";
+    else if ((unsigned char)*i < 0x20) { // Control characters not handled above.
+      std::string HexStr = llvm::utohexstr(*i);
+      EscapedInput += "\\u" + std::string(4 - HexStr.size(), '0') + HexStr;
+    }
+    else
+      EscapedInput.push_back(*i);
+  }
+  return EscapedInput;
+}
+
 URI URI::fromUri(llvm::StringRef uri) {
   URI Result;
   Result.uri = uri;
   uri.consume_front("file://");
   // For Windows paths e.g. /X:
   if (uri.size() > 2 && uri[0] == '/' && uri[2] == ':')
     uri.consume_front("/");
   // Make sure that file paths are in native separators
llvm::Optional<TextEdit> TextEdit::parse(llvm::yaml::MappingNode *Params) {
   }
   return Result;
 }
 
 std::string TextEdit::unparse(const TextEdit &P) {
   std::string Result;
   llvm::raw_string_ostream(Result) << llvm::format(
       R"({"range": %s, "newText": "%s"})", Range::unparse(P.range).c_str(),
-      llvm::yaml::escape(P.newText).c_str());
+      jsonEscape(P.newText).c_str());
   return Result;
 }
 
 llvm::Optional<DidOpenTextDocumentParams>
 DidOpenTextDocumentParams::parse(llvm::yaml::MappingNode *Params) {
   DidOpenTextDocumentParams Result;
   for (auto &NextKeyValue : *Params) {
     auto *KeyString = dyn_cast<llvm::yaml::ScalarNode>(NextKeyValue.getKey());
TextDocumentPositionParams::parse(llvm::yaml::MappingNode *Params) {
   }
   return Result;
 }
 
 std::string CompletionItem::unparse(const CompletionItem &CI) {
   std::string Result = "{";
   llvm::raw_string_ostream Os(Result);
   assert(!CI.label.empty() && "completion item label is required");
-  Os << R"("label":")" << llvm::yaml::escape(CI.label) << R"(",)";
+  Os << R"("label":")" << jsonEscape(CI.label) << R"(",)";
   if (CI.kind != CompletionItemKind::Missing)
     Os << R"("kind":)" << static_cast<int>(CI.kind) << R"(,)";
   if (!CI.detail.empty())
-    Os << R"("detail":")" << llvm::yaml::escape(CI.detail) << R"(",)";
+    Os << R"("detail":")" << jsonEscape(CI.detail) << R"(",)";
   if (!CI.documentation.empty())
-    Os << R"("documentation":")" << llvm::yaml::escape(CI.documentation)
+    Os << R"("documentation":")" << jsonEscape(CI.documentation)
        << R"(",)";
   if (!CI.sortText.empty())
-    Os << R"("sortText":")" << llvm::yaml::escape(CI.sortText) << R"(",)";
+    Os << R"("sortText":")" << jsonEscape(CI.sortText) << R"(",)";
   if (!CI.filterText.empty())
-    Os << R"("filterText":")" << llvm::yaml::escape(CI.filterText) << R"(",)";
+    Os << R"("filterText":")" << jsonEscape(CI.filterText) << R"(",)";
   if (!CI.insertText.empty())
-    Os << R"("insertText":")" << llvm::yaml::escape(CI.insertText) << R"(",)";
+    Os << R"("insertText":")" << jsonEscape(CI.insertText) << R"(",)";
   if (CI.insertTextFormat != InsertTextFormat::Missing) {
     Os << R"("insertTextFormat":")" << static_cast<int>(CI.insertTextFormat)
        << R"(",)";
   }
   if (CI.textEdit)
     Os << R"("textEdit":)" << TextEdit::unparse(*CI.textEdit) << ',';
   if (!CI.additionalTextEdits.empty()) {
     Os << R"("additionalTextEdits":[)";

clangd/ProtocolHandlers.cpp

void CodeActionHandler::handleMethod(llvm::yaml::MappingNode *Params,
   std::string Code = AST.getStore().getDocument(CAP->textDocument.uri.file);
   std::string Commands;
   for (Diagnostic &D : CAP->context.diagnostics) {
     std::vector<clang::tooling::Replacement> Fixes = AST.getFixIts(CAP->textDocument.uri.file, D);
     std::string Edits = replacementsToEdits(Code, Fixes);
 
     if (!Edits.empty())
       Commands +=
-          R"({"title":"Apply FixIt ')" + llvm::yaml::escape(D.message) +
+          R"({"title":"Apply FixIt ')" + jsonEscape(D.message) +
           R"('", "command": "clangd.applyFix", "arguments": [")" +
-          llvm::yaml::escape(CAP->textDocument.uri.uri) +
+          jsonEscape(CAP->textDocument.uri.uri) +
           R"(", [)" + Edits +
           R"(]]},)";
   }
   if (!Commands.empty())
     Commands.pop_back();
 
   writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +

test/clangd/encoding.test

+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#
+Content-Length: 154
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"void main() {é;}"}}}
+#
+# CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///foo.c","diagnostics":[{"range":{"start": {"line": 0, "character": 1}, "end": {"line": 0, "character": 1}},"severity":2,"message":"return type of 'main' is not 'int'"},{"range":{"start": {"line": 0, "character": 1}, "end": {"line": 0, "character": 1}},"severity":3,"message":"change return type to 'int'"},{"range":{"start": {"line": 0, "character": 14}, "end": {"line": 0, "character": 14}},"severity":1,"message":"use of undeclared identifier 'é'"}]}}
+#
+#
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}