Basic language server implementation #121

pa000 · 2024-06-07T18:58:46Z

Provides a basic language server with a diagnostics reporting feature (shows you errors in the code).
Available with a new framls executable.

The server intercepts error reports made by the typechecker so the result should be the same as running the file with dbl.

The code structure makes it easy to add new capabilities (but they will require changes in the dbl code).

Provides a dblls binary The only implemented feature is diagnostic information

ilikeheaps · 2024-11-11T17:32:37Z

I've tested it shortly on my setup, seems to be working. Does highlight errors with description.

Steps I've taken to setup it:

I have updated the branch with master locally, will put up a branch in a moment so we're sure we work on the same version
Added yojson library and findlib to my development environment (Nix)
Made lsp client configuration for Emacs with lsp-mode, will post it somewhere

ilikeheaps · 2024-11-11T17:52:03Z

And for now I'll drop the Emacs configuration here:
lsp.el:

;; eval this file to setup framls for lsp-mode
(add-to-list 'lsp-language-id-configuration '(".*\\.fram$" . "fram"))

;; Emacs must be ran within environment with 'framls' available; either:
;; - install it (TODO 'dune install ???' -- I don't use it)
;; - or start Emacs with 'dune exec emacs'
(lsp-register-client (make-lsp-client
                      :new-connection (lsp-stdio-connection "framls")
                      :activation-fn (lsp-activate-on "fram")
                      :server-id 'framls))

Update with upstream

Everything needs to be perfectly aligned

ilikeheaps

Checked some files, so far so good (left couple ideas to improve type safety, could be dismissed or done later).

ilikeheaps · 2024-11-19T12:02:16Z

src/InterpLib/Error.ml

@@ -56,6 +56,12 @@ let report ?pos ~cls msg =
  | None, _ ->
    Printf.eprintf "%s: %s\n" name msg

+let report_impl = ref report_to_stderr


I'm not a fan of mutable state. Do we need to dynamically change this? Perhaps I'll answer this myself as I read further.

I don't think anyone here is. However, without any mutable state we would have to pass a report argument to every type-checker function which seems impractical. Regular DBL code has no reason to change this variable and the language server only does it before running the type-checker, so I think there is little risk with using a ref here.

ilikeheaps · 2024-11-19T12:07:33Z

src/Lsp/Connection.ml

+  ; content_type: string option
+  }
+
+let parse_header headers line =


This sounds like something we could use a third party library for unless it's pretty custom. (could refactor later)

I'm not a fan of dependencies and the whole file is like 60 lines (and does exactly what's needed), so I'm not sure how much a library would help.

ilikeheaps · 2024-11-19T12:43:34Z

src/Lsp/Uri.ml

+let to_path uri =
+  String.sub uri (String.length "file://")
+    (String.length uri - String.length "file://")


Perhaps it should check that the prefix is indeed "file://"?

I'll provide an update which uses the uri library to manage URIs (the current version doesn't work with percent-encoded strings), so this file will be gone. That said, I'm not sure what to do if someone provides a URI with a scheme different than file (e.g http). I don't think it's worth handling such cases.

I think it would be nice to fail gracefully in that case rather than doing random nonsense but if that sounds like too much hassle then I'm okay with it.

It seems that the only place where the path to the file is actually needed is when setting the directory to look for modules, so in the worst case it will just not find other modules. I think I'll leave it like that.

* Add comments * Use `uri` library to handle URIs - +1 dependency :( * Change naming convention in Message.ml to make it more consistent - json_of_type -> from_type * Add Message.mli - +200 lines :) * Redesign JsonRpc main loop - Use result type instead of exceptions to handle errors

pa000 · 2024-12-01T21:33:52Z

I believe I'm finished with the final clean-ups (that seem to have made the PR 500 lines bigger oops) and the code is now ready to be thoroughly reviewed.

ilikeheaps

Will probably need one more pass to finish the review. Nothing major in this batch of comments, mostly "I think this could improve readability although I'm not sure and it's not a big deal".

ilikeheaps · 2025-01-06T18:41:59Z

src/Lsp/Connection.ml

+    header1: value1\r\n
+    header2: value2\r\n
+    \r\n
+    [JSON-RPC message]


(suggestion might be badly formatted)

Suggested change

[JSON-RPC message]

[JSON-RPC message]

The same header may occur multiple times, in which case the last value will be taken.

It's an implementation detail (not stated in the specification) and the top comment wasn't meant for that.

Fair enough.

src/Lsp/Connection.ml

ilikeheaps · 2025-01-06T18:58:16Z

src/Lsp/README.md

+`framls` is a language server for Fram. This document is meant to be
+an entry point for someone wanting to contribute to `framls`.
+If you want to configure `framls` for use with your code editor please visit
+[fram-lang.org](https://fram-lang.org).


Is there some PR for that doc page?

No because we don't have any editor extensions yet.

ilikeheaps · 2025-01-06T19:00:59Z

src/Lsp/README.md

+Base
+----
+
+Base is a simple HTTP-like protocol. It consists of a header and content part,
+separated by "\r\n":
+```
+header1: value1\r\n
+header2: value2\r\n
+\r\n
+[JSON-RPC message]
+```
+
+It is implemented in the `Connection` module.


Do you think we'll manage to keep this description in sync with Connection.ml or would we rather just link the file? (I suppose the same question applies to other modules)

I think we will because it probably won't change. I think it's useful to have it both in README (to get an overview) and in each file on its own (so you don't have to jump between files just to read the doc comment).

src/Lsp/JsonRpc.ml

ilikeheaps · 2025-01-07T12:35:27Z

src/Lsp/JsonRpc.ml

+  let rec loop state =
+    let string = receive_string (State.in_channel state) in
+    match message_of_string string with
+    (* There was an error parsing the message *)


Is it definitely parsing error or could it be some connection error too? If it's the former we could incorporate the comment as self-explanatory custom type: type ParsingResult 'a = ParsingError ... | ....

It's definitely a parsing error (the string is already received). I don't think a custom type is a good idea because the error is produced by the callee and we're just passing it over, so we don't care what kind of error it is.

src/Lsp/State.ml

ilikeheaps · 2025-01-07T13:00:48Z

src/Lsp/framls.ml

+  set_module_dirs real_path;
+  type_check temp_path;


Why do we typecheck the temporary file instead of the real one? Does temporary one include changes from the editor that are not saved yet?

Yes, exactly. I will add that to the doc comment of State.get_document_path. Do you think it should be explained anywhere else? It's important to know the difference between the temp and real files.

I was thinking it could be sufficient to document that in type document definition but now I realized this code avoids using document type. Huh.

I don't think any other place produced the same question though.

Co-authored-by: ilikeheaps <[email protected]>

pa000 · 2025-01-16T19:35:12Z

src/Lsp/State.ml

+
+module UriMap = Map.Make(Uri)
+
+(** We keep the latest version of each document in a temp file *)


It's indeed explained in State.mli (or at least I tried). If someone wants to edit the .ml file they should know what latest means. Because of that I also removed this comment.

src/Lsp/State.ml

pa000 · 2025-01-16T19:38:30Z

src/Lsp/State.ml

+
+let close_document state uri =
+  match UriMap.find_opt uri state.documents with
+  | None -> state


This case shouldn't happen. If it does that probably means an error happened on our side and so we should fail, you're right.

pa000 · 2025-01-16T19:42:36Z

src/Lsp/State.mli

+
+(** Server state *)
+
+(** The LSP client sends a notification when the user opens, updates


It's up to the client but most probably 1 (anything else would likely make the lsp feel laggy).

pa000 · 2025-01-16T19:43:29Z

src/Lsp/Message.mli

+type range = {
+  start : position;
+  end_  : position (* json key: end *);
+}


[start, end)
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range

pa000 · 2025-01-16T19:44:11Z

src/Lsp/Message.mli

+
+type position = {
+  line      : int;
+  character : int;


pa000 · 2025-01-16T19:48:13Z

src/Lsp/Message.mli

I'd link the specification for each type but the url is crazy long (e.g. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range). If someone wants to implement an LSP feature they should probably have the specification open regardless.

ilikeheaps · 2025-01-14T12:04:25Z

src/Lsp/framls.ml

+  Yojson.Safe.from_string
+    {|
+      {
+        "capabilities": {
+          "textDocumentSync": {
+            "openClose": true,
+            "change": 1
+          }
+        },
+        "serverInfo": {
+          "name": "framls"
+        }
+      }
+    |}


Could this use JSON constructors from Message.ml?

No (but it could be added). This message is a bit different because it depends on the capabilities that the server supports. It'd be best to create this message in a way that avoids inconsistencies (e.g. someone extends the server with some capability but forgets to update it here). I added a comment explaining it. It's fine for now.

ilikeheaps

Okay, I've seen it all. I left some more minor questions/suggestions but it looks good enough to me.

ilikeheaps · 2025-01-20T16:01:51Z

Whoop, seems like I can't mark conversations as resolved, only single comments of my own.

pa000 · 2025-01-23T12:06:26Z

That's all from me. We can finally merge it.

pa000 added 4 commits June 7, 2024 19:07

Basic language server implementation

c57d579

Provides a dblls binary The only implemented feature is diagnostic information

Remove trailing whitespace

99a00f1

Change formatting slightly

8b46f4e

The language is named fram

62e8077

pa000 marked this pull request as draft October 29, 2024 14:18

Merge remote-tracking branch 'origin/master' into lsp-updated

d4a8edb

This comment was marked as resolved.

Sign in to view

pa000 added 2 commits November 14, 2024 16:18

Merge pull request #1 from ilikeheaps/lsp

22d80fb

Update with upstream

Format Message.ml

43f6c0d

Everything needs to be perfectly aligned

ilikeheaps reviewed Nov 19, 2024

View reviewed changes

pa000 added 5 commits November 22, 2024 19:00

Update CI

63563c9

Add README.md

3c3d1ab

Update README.md

01f2147

Format README

2b22275

pa000 marked this pull request as ready for review December 1, 2024 21:27

ilikeheaps reviewed Jan 7, 2025

View reviewed changes

pa000 and others added 3 commits January 9, 2025 22:56

Make JsonRpc.ml more dry

7486c8e

Co-authored-by: ilikeheaps <[email protected]>

Update src/Lsp/State.ml

36d7767

Co-authored-by: ilikeheaps <[email protected]>

Code review changes

f6841d0

ilikeheaps reviewed Jan 13, 2025

View reviewed changes

src/Lsp/State.ml Outdated Show resolved Hide resolved

ilikeheaps reviewed Jan 13, 2025

View reviewed changes

ilikeheaps reviewed Jan 14, 2025

View reviewed changes

src/Lsp/Message.mli

type position = {

line : int;

character : int;

This comment was marked as resolved.

Sign in to view

Copy link

Author

pa000 Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

ilikeheaps reviewed Jan 14, 2025

View reviewed changes

ilikeheaps approved these changes Jan 14, 2025

View reviewed changes

pa000 added 4 commits January 16, 2025 20:15

Update State.ml{,i} documentation

a884b5b

Change argument order in State functions

f68b29c

Link to spec in Message.ml

40d8d0e

Update framls.ml

952bdff

	[JSON-RPC message]
	[JSON-RPC message]
	The same header may occur multiple times, in which case the last value will be taken.


		module UriMap = Map.Make(Uri)

		(** We keep the latest version of each document in a temp file *)


		(** Server state *)

		(** The LSP client sends a notification when the user opens, updates

Basic language server implementation #121

Are you sure you want to change the base?

Basic language server implementation #121

Conversation

pa000 commented Jun 7, 2024

ilikeheaps commented Nov 11, 2024

This comment was marked as resolved.

ilikeheaps commented Nov 11, 2024

This comment was marked as resolved.

ilikeheaps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pa000 commented Dec 1, 2024

ilikeheaps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

pa000 Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilikeheaps left a comment

Choose a reason for hiding this comment

ilikeheaps commented Jan 20, 2025

pa000 commented Jan 23, 2025

pa000 Jan 16, 2025 •

edited

Loading