Project improvements

[scohen]

Hi there!
I've been an elixir user for almost a decade (my first deployed code was in 2013, and I'm an early Alchemist.el user as well), and I've been a lsp user for about a decade, it's a great project, but I think it needs some love right now. Please take the rest of this out of an abundance of admiration for this project, and a deep respect and love for the elixir language. I want this ecosystem to thrive.

To start off, this project is very old, with a lot of contributions that predate popular idioms and features of the elixir language, and I think, as a result, it's very hard to contribute fixes and improvements for it. I've been playing around with the project for a bit now, and I think I can outline some areas for improvements. I have some time, and I'd like to help if there is interest.

Here are some of the areas that I think can be made better.

1. The project is currently an umbrealla.
   I don't see much reason for it to be an umbrealla, since it's always deployed as a single executable. Navigating sub-projects (there are only three) is cumbersome, and just get in the way of a lot of the elixir tooling. We should merge the project into a single project.

2. There isn't a consistent data model
   Both elixir_sense and elixir-ls suffer from the lack of a consistent data model representing either domain concepts or data types and messages from the language server protocol. Not having a data model means that all messages are represented via maps, which forces the user to know which keys the maps have and what the values of those keys should be. I have a branch that introduces a declarative syntax for json-rpc messages, requests and types ready to go if you want it.

3. The request / message handling code is interspersed with business logic. This pattern often emerges in genservers, and means that there's a lot of duplicated code dealing with the vagaries of message handling. I propose instead to have a unified way to define providers and for the main server to call them. This means that a lot of the low-level conversions are done around the project, in multiple places and by units of code with different responsibilities. This makes it hard to extend. Defining a behaviour around this will mean that additional providers will only need to implement that behaviour and a single place where various requests are handled should make additional providers "just work". I have an additional branch that extends the data model branch that implements this.

4. There needs to be a consistent style and form to the code in the project. Elixir has a bunch of tooling that it didn't have before, and we should be using credo to ensure a consistent style. In particular, elixir_ls really needs to be more consistent about its use of pipelines. Using pipes where they shouldn't be used has a very negative impact on readability. In addition, the project makes a lot of use of try/catch and pattern match failures for known errors, where success/error tuples would be used more idiomatically.

This isn't a drive by issue. I have time to devote significant effort to this project, and have already spent a couple weeks working on the second and third issue. I have made significant progress and can give a demo if you would like. In addition to clarity, there are a number of correctness and performance fixes that will be included in my fixes.

If this sounds like a direction you'd like to pursue, give me a thumbs up and I'll continue. Otherwise, close this issue.

[scohen]

This PR contains my thoughts on the future direction of the project
You can see what an example provider would look like. I've added two for your consideration. Porting each took a couple hours.

I'd really like to get your thoughts, @lukaszsamson. Thanks.

[lukaszsamson]

@scohen Big thumbs up

1. The project is currently an umbrealla.

That's a though one. The debugger is functionally a different app with its own supervision tree and a different protocol. Some form of separation is needed. Is alias namespace sufficient to enforce this separation? Beside's that Jose Valim suggested splitting up language server into separate builder/mix driver app but I'm not completely sold to that idea.

2. There isn't a consistent data model

+1

3. The request / message handling code is interspersed with business logic

I fully agree. In my projects I usually ended with designs similar to what your PR proposes. Separating pure functional business logic model from command/event handling genserver boilerplate is a must for all but most simple stateful servers. The problem here may be that many of the providers depend on non trivial initialization (e.g. mix state, code paths, loaded modules, code compiled with a specific tracer) and testing them may be problematic.

4. There needs to be a consistent style and form to the code in the project

No strong opinions here. I'm not a big fan of credo but you're probably right that enforcing code style could be useful. try/catch is not so easy to eliminate as many elixir APIs raise and OTP APIs throw or exit (looking at you :dialyzer)

[scohen]

That's a though one. The debugger is functionally a different app with its own supervision tree and a different protocol. Some form of separation is needed. Is alias namespace sufficient to enforce this separation?

Yes, because that's all you get with umbrellas anyways. They're really for creating separate deployable applications and have poor separation of concerns (indeed, when built, they can all "see" into one another, and lead to code leakage. This is incredibly common with phoenix projects, and while it hasn't happened here yet, there's nothing technically that prevents it from happening.

Jose Valim suggested splitting up language server into separate builder/mix driver app

Interesting. I've been coming around to having something similar using the unfortunately named :slave module in erlang. The idea is that it'd start and run parts of the language server / build, but run inside the app's directory, while the language server would inject some helpers and control it over distribution with RPC. A week or two ago, I ran into some issues with protocols defined in SouceFile getting kicked out when builds run, and it took me a long time to figure out what was going. As a result, I had to eliminate the calls to Enumerable in source_file.ex. I think this might be an approach for the future, it has the following advantages:

• A near Complete separation of elixir_ls and the target application's code
• No more vendoring and library conflicts
• No more slow builds dragging down the language server
• The build is now completely isolated from the language server with respect to crashing
• It would theoretically be possible for a single language server to manage many projects (not sure if this is helpful)

The downside is that it's more complicated.

initialization (e.g. mix state, code paths, loaded modules, code compiled with a specific tracer) and testing them may be problematic.

Understood, but let's fix the testing problem and not let it dictate our architecture. I'm sure we can come up with something. As you can see from my PRs, I'm a big fan of testing.

As you can see, I have a lot on my plate. Since you're on board, I'm going to ask for a favor. I've isolated the experimental server pretty well, so can I have quicker reviews on the outstanding branches? I spent the entire day getting my source file branch merge-able, and it's moved quite a bit on subsequent branches, which has really complicated it.

In the future, I would really appreciate reviews on the experimental directory in a couple of days. We need to move quickly there in order to get feature parity and direct people to code there and not build features on the current branch. The project home page will likely need to be updated to communicate this strategy.

Thank you for listening.

[skbolton]

What is the state of the experimental server? I am starting to look at ways to contribute to the project but it feels a little confusing with how things are fragmented.

[lukaszsamson]

Frankly, I don't know. I never got to play around with it as there are dozens other issues impacting user experience. Steve moved on to develop his own thing. Some people started migrating existing or implementing new providers to the experimental server but it never got any testing. In the meantime the existing providers evolved and got bugfixes. I'm thinking of removing the experimental server completely and migrating the interesting parts back to original server. One I'm considering worthy is document synchronisation optimisations. I'm not convinced about LSP data model as the generating code is really complicated and I don't think I'm able to fix bugs there. Since it was created there is a new implementation done in elixir-tools

[scohen]

@skbolton The experimental server lives on in the lexical lsp. You might want to check it out.

@lukaszsamson Do you want me to put together a PR removing the experimental server from elixir-ls?

I'm not convinced about LSP data model

I would strongly suggest that elixir-ls adopt some kind of data model. If I had to do it again, I think I'd make ecto schemas, but knowing what messages are available and which fields are on those messages is a huge improvement for developers, especially those not familiar with the project.