This is an archive of the discontinued LLVM Phabricator instance.

[clangd] less boilerplate in RPC dispatch
ClosedPublic

Authored by sammccall on Oct 2 2017, 9:39 AM.

Details

Summary

Make the ProtocolHandlers glue between JSONRPCDispatcher and
ClangdLSPServer generic.
Eliminate small differences between methods, de-emphasize the unimportant
distinction between notifications and methods.

ClangdLSPServer is no longer responsible for producing a complete
JSON-RPC response, just the JSON of the result object. (In future, we
should move that JSON serialization out, too).
Handler methods now take a context object that we may hang more
functionality off in the future.

Added documentation to ProtocolHandlers.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Oct 2 2017, 9:39 AM
sammccall updated this revision to Diff 117374.Oct 2 2017, 9:41 AM
  • clang-format
ilya-biryukov added inline comments.Oct 4 2017, 2:39 AM
clangd/ClangdLSPServer.h
47 ↗(On Diff #117374)

Maybe there's a way to have a typed return value instead of Ctx?
This would give an interface that's harder to misuse: one can't forget to call reply or call it twice.

Here's on design that comes to mind.
Notification handlers could have void return, normal requests can return Optional<Result> or Optional<std::string> (with result json).
Optional is be used to indicate whether error occurred while processing the request.

48 ↗(On Diff #117374)

Maybe there's a way to get rid of NoParams?
E.g. by adding a overload to HandlerRegisterer?

clangd/Protocol.cpp
317 ↗(On Diff #117374)

Why do we need these empty ; statements?

clangd/ProtocolHandlers.cpp
14 ↗(On Diff #117374)

Maybe move this into a separate commit that would also replace using namespace in all other files?

25 ↗(On Diff #117374)

Code style: parameter name should be Method

ilya-biryukov added inline comments.Oct 5 2017, 5:50 AM
clangd/ClangdLSPServer.h
47 ↗(On Diff #117374)

After putting more thought into it, Ctx-based API seems to have an advantage: it will allow us to easily implement async responses.
E.g., we can run code completion on a background thread and continue processing other requests. When completion is ready, we will simply call Ctx.reply to return results to the language client.

To make that possible, can we allow moving RequestContext and pass it by-value instead of by-ref?

clangd/JSONRPCDispatcher.h
55 ↗(On Diff #117374)

It seems there are no any of log. Maybe remove it from this class?

sammccall updated this revision to Diff 117835.Oct 5 2017, 9:04 AM
sammccall marked 5 inline comments as done.
  • ClangLSPServer.h should refer to ShutdownParams, not NoParams
  • Address review comments

Thanks! All addressed (except removing ShutdownParams, which I'm not sure is worthwhile).

clangd/ClangdLSPServer.h
47 ↗(On Diff #117374)

Yeah I thought about returning a value... it certainly reads more nicely, but I don't think we're ready to do a good job in this patch:

  • return value should be an object ready to be serialized (rather than a JSON string) - I don't want to bring that in scope here, but it might affect the details of the API
  • there's several cases we know about (return object, no reply, error reply) and some we're not sure about (async as you mention - any multiple responses)? I think this needs some design, and I don't yet know the project well enough to drive it.

I've switched to passing Ctx by value as you suggest (though it's certainly cheap enough to copy, too).

48 ↗(On Diff #117374)

Even if there was, I think I prefer the regularity (changed this to ShutdownParams - oops!).

Otherwise the signature's dependent on some combination of {current LSP, whether we actually implement the options, whether we've defined any extensions}. It's harder to remember, means changing lots of places when these factors change, and complicates the generic code.

Maybe I've just spent too long around Stubby, though - WDYT?

ilya-biryukov accepted this revision.Oct 6 2017, 5:30 AM

LGTM.
Note there's a new LSP method handler added upstream (textDocument/signatureHelp), we should add it to this change before submitting.

clangd/ClangdLSPServer.h
47 ↗(On Diff #117374)

Yeah, copy is also fine there performance-wise.

I do think move-only interface fits slightly better. We can check a whole bunch of invariants if Ctx is move-only (i.e., that request wasn't dropped without response or reply was not called twice).

48 ↗(On Diff #117374)

All those are good points. We also have only one method that does not have params currently, so it's probably not even worth additional complexity in the implementation.

This revision is now accepted and ready to land.Oct 6 2017, 5:30 AM
This revision was automatically updated to reflect the committed changes.