Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lsp4clj utils #14

Open
mainej opened this issue Aug 3, 2022 · 4 comments
Open

lsp4clj utils #14

mainej opened this issue Aug 3, 2022 · 4 comments

Comments

@mainej
Copy link
Contributor

mainej commented Aug 3, 2022

Based on a discussion started in #13, the idea was raised of creating a suite of utils for projects that use lsp4clj. This could either be additional namespaces in lsp4clj or (my vote) a separate project.

Things we could move from lsp4clj to the utils:

  • liveness-probe, which is used to shutdown automatically when the client closes. This is a useful utility, but unrelated to the core responsibility of handling LSP JSON-RPC messages.
  • coercer, which is used primarily for conforming the server's responses. I dislike 90% of the coercer. IMO, it's primarily a relic of lsp4j. It was useful when we needed to create Java objects, but now its main utility is for converting keywords to integer enum values. That could just as easily be done by looking up the enum values in a map. To be fair, the conformers do more than handling enums; they also restructure data slightly. But again, I'd rather have helper functions that do the same thing. That said, some project might want to use the coercer. They may even want to have utils that automatically apply conformations based on the method name of the JSON-RPC message.

Things we could move from clojure-lsp to the utils:

  • The IProducer protocol, which is used to send messages to clients. clojure-lsp needs this protocol so that it can swap in a no-op version in tests, and a version that prints messages to stdout in the API. Other servers may need to do the same thing, or they may want a protocol simply to organize their code and make it easier to find usages of the protocol methods.
  • The ILogger protocol, which is used to add messages to the log. Again, clojure-lsp needs this so it can swap in no-op versions. Other servers might have similar concerns.
  • shared tools for
    • managing files
    • manipulating uris
    • other cross-platform utilities
  • Anything else pioneered by clojure-lsp that other servers find useful.
  • See also lsp4clj features to move clojure-lsp#826 and Lsp4clj left-overs clojure-lsp#818, although those are getting stale.
@mainej mainej mentioned this issue Aug 3, 2022
@Cyrik
Copy link

Cyrik commented Aug 3, 2022

Thanks for the summary. I don't have a strong preference for namespace vs a new library.

I'd like to move the stuff we have in common with clojure-lsp out of our codebase though since it's difficult to keep it synced especially now that I've built some notebook handling stuff that I'd like to also get into clojure-lsp. But your summary does contain most of the stuff I'd like to aggregate.
I also agree with the coercer ns, although it is useful to check what a response should look like. The problem with it seems to be that it's not obvious what the result of a specific call should even be coerced to, although that did get a lot easier with your move away from lsp4j.

@mainej
Copy link
Contributor Author

mainej commented Aug 4, 2022

some notebook handling stuff that I'd like to also get into clojure-lsp

Cool! I'll look forward to hearing more about that sometime.

it is useful to check what a response should look like

This is personal opinion, but when I'm trying to learn about response structure, I find the LSP spec website much more informative than coercer. For whatever reason, I find it hard to follow deeply nested s/defs and would rather navigate around the website, especially because it includes documentation.

clojure-lsp doesn't really have unit tests that would exercise the coercer, so I also don't find it to be a good tool to debug whether I've generated a correct response. (We could add this kind of test, but I'm not sure we'd see much benefit for reasons described below.)

I also mistrust the coercer's completeness. I know that the LSP spec allows many keys that aren't mentioned in the coercer. And it defines many methods that the coercer doesn't cover at all.

The coercer also lags behind as the LSP spec evolves. This was a flaw with lsp4j too... to get the latest features from the LSP spec, you had to wait for an lsp4j update. We're getting away from that by dropping lsp4j and letting servers implement whichever methods they like. But we're at risk of reintroducing the problem if we rely too heavily on the coercer. If we move the coercer to a util library we may want to take this into account by including the LSP spec version number in the library's version number.

Taking one step back... I think it's useful to consider what value spec.alpha can provide, and whether we're getting that value.

  1. If you spec input to your system, you can provide feedback when outside developers give you bad data. In clojure-lsp we have only a few input validation specs. This is for two reasons. First, we trust the editors to conform to the LSP spec. And second, the editor isn't a human, so it can't react to feedback. At best, we have to notice the feedback, and pass it on to the editor developers.
  2. If you spec output from your system, you can give your own developers feedback when they generate invalid data. This might be useful while you're first developing a server, so that you can confirm you're sending data to the editors that conforms to the LSP spec. But this has limited long-term value. I've been working on clojure-lsp for 8 months and have yet to see a log line about outputting invalid data. The reason is that at this point, most of the work in clojure-lsp is about adding more refactorings and fixing bugs, not about responding to new LSP methods. Even when we do implement new methods, it's rare to correctly s/def those methods but then fail to output data that conforms to the s/defs.
  3. If you expect your data input to be messy or complicated, conformation can be a nice way to put it into a more understandable structure. The new ::json-rpc.input spec, which classifies JSON-RPC message as requests, notifications, or responses is a decent example of this, IMO. Otherwise, since we don't spec many inputs, we don't get much benefit here.
  4. If you want the data you return to look different from what your consumers ultimately need, conformation can be helpful. Helper functions which do this transformation are another option.
  5. If you want to do generative testing, specs are nice. clojure-lsp doesn't use generative tests.

So in summary, I don't think we're getting much value and don't see much prospect in the future.

it's not obvious what the result of a specific call should even be coerced to

I agree 100%. To enumerate some details about why it's hard to know what a spec applies to:

  1. Does ::code-lenses spec an entire top-level thing? Or is it a key inside another thing? Is it supposed to spec the input to a function, the output, or both?
  2. Do generic words like ::code or ::message really mean the same thing in all contexts?
  3. What is our convention for auto-resolved namespaced keys, like ::message or ::command? The first is a keyword that is used in several different places. The second is a collection of keys (in Clojure land a map and in LSP spec land, an interface).

Many projects that use clojure.spec.alpha suffer from these same problems. I have opinions about how to address some of them (separate specs into input, domain, and output namespaces, use namespaced keys in both your code and your specs, don't use auto-resolved keywords in s/keys specs). But that's a separate discussion and I'd rather not spend time overhauling the coercer.

Anyway, I should stop thinking about how I don't like the coercer and be content with working toward using it less in clojure-lsp. :)

@snoe
Copy link

snoe commented Aug 4, 2022

For background, the coercer was created 100% to make building trees of java objects easier. Without that requirement, I imagine it's more confusing/brittle/hard to maintain than useful.

It was never meant to be a spec against the protocol, just a way to build objects for the parts we used.

@Cyrik
Copy link

Cyrik commented Aug 4, 2022

I mostly agree with you on the problems of the coercer. My first PR to clojure-lsp was a bug in the coercer that took forever to find 😢 It did catch a few mistakes I had in when first implementing new message types though, but it's not helpful anymore once you have a basic version of that message.

So yeah, unless someone is super eager to put in the work to overhaul it/keep it up to date, I'm happy with removing it. Just having a few very basic tests that show the usage of the various methods might be more helpful as documentation. It might just be me, but the LSP specification often feels a little cumbersome to read, but you're correct that the spread-out sdef isn't much better. I usually cheat by looking at the clojure-lsp implementation to see a rough shape before building our own version of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants