This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Switch from YAMLParser to JSONExpr
ClosedPublic

Authored by sammccall on Nov 23 2017, 4:10 PM.

Details

Summary
  • Converted Protocol.h parse() functions to take JSON::Expr. These no longer detect and log unknown fields, as this is not that useful and no longer free. I haven't changed the error handling too much: fields that were treated as optional before are still optional, even when it's wrong. Exception: object properties with the wrong type are now ignored.
  • Made JSONRPCDispatcher parse using json::parse
  • The bug where 'method' must come before 'params' in the stream is fixed as a side-effect. (And the same bug in executeCommand).
  • Some parser crashers fixed as a side effect. e.g. https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3890
  • The debug stream now prettyprints the input messages with --pretty.
  • Request params are attached to traces when tracing is enabled.
  • Fixed some bugs in tests (errors tolerated by YAMLParser, and off-by-ones in Content-Length that our null-termination was masking)
  • Fixed a random double-escape bug in ClangdLSPServer (it was our last use of YAMLParser!)

Diff Detail

Event Timeline

sammccall created this revision.Nov 23 2017, 4:10 PM
sammccall edited reviewers, added: ioeric; removed: ilya-biryukov.Nov 23 2017, 4:22 PM

Oops, Eric is most likely to have this stuff in cache :-)
Sorry this is huge and ugly, -500 lines though!

I think the Protocol code could be a lot tighter (there's still a lot of repeated "is this an object", and "if this property exists parse it as type X and store it in field Y, else fail". (The kind of stuff YamlIO is good at).
I wanted to stick to a pretty literal translation first, and maybe later find a way to clean it up further. Hope that's OK!

ioeric added inline comments.Nov 24 2017, 2:49 AM
clangd/JSONRPCDispatcher.cpp
221

Use llvm::toString?

clangd/JSONRPCDispatcher.h
47

It seems that we are piggybacking a state of clangd with JSONOutput. It might make sense to store the option in clangd (or the dispatcher?) and pass it via writeMessage, but what you have is also fine if it saves trouble :)

sammccall updated this revision to Diff 124163.Nov 24 2017, 4:38 AM
sammccall marked an inline comment as done.

Use llvm::toString

sammccall added inline comments.Nov 24 2017, 4:38 AM
clangd/JSONRPCDispatcher.h
47

Yeah, this isn't great. In the short term I don't think it does much harm. Longer-term we'll revisit logging organization as part of RequestContext that Ilya's working on.

This revision is now accepted and ready to land.Nov 28 2017, 1:02 AM
This revision was automatically updated to reflect the committed changes.
test/clangd/definitions.test