clojure-lsp / lsp4clj

LSP base support for any LSP that is implemented in Clojure
MIT License
51 stars 5 forks source link

lsp4clj utils #14

Open mainej opened 2 years ago

mainej commented 2 years ago

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:

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

Cyrik commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.