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

LSP Mk I #3213

Merged
merged 42 commits into from Sep 12, 2022
Merged

LSP Mk I #3213

merged 42 commits into from Sep 12, 2022

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Jul 8, 2022

asciicast

Overview

LSP's probably don't need much introduction;

This adds the foundation of an LSP server into UCM!

This iteration supports:

  • Show type on hover (this currently does a naive name lookup, but I'll swap it to use TDNR names later)
  • Inline type and parser error messages for almost every error.
  • NO autocomplete in this release, I have a really poor implementation that I want to scrap and do correctly.

Other interesting notes:

  • The LSP listens for changes from the UCM it's linked to, so name resolution is dependent on your current UCM path.
  • Currently it type-checks as-fast-as-possible when changes are detected, typechecking is really fast so this shouldn't be a huge burden on small scratch files with fast machines, but we may wish to add a debounce option in the configuration.
  • Supports multiple scratch files at once
  • Auto-updates if new terms are added or updated in UCM
  • Currently UCM doesn't react to the LSP at all, I think it might get annoying if the LSP spams UCM with error messages or w/e. In the future we could provide a "Show in UCM" Code action or something if we really wanted.

Installation and setup

Currently the only supported configuration is to connect to the LSP via a specified port.

The default is 127.0.0.1:5757, but you can change the port using UNISON_LSP_PORT=1234.

I've got this configuration in my CocConfig for nvim, your setup may vary.

  "languageserver": {
    "unison": {
      "filetypes": ["unison"],
      "host": "127.0.0.1",
      "port": 5757
    }

Note that you'll need to start UCM before you try connecting to it in your editor or your editor might give up.

For VSCode I believe we'll have to publish an npm package to make an extension.

Implementation notes

While the lsp package does a ton of heavy lifting, this was still a lot of work.

  • Adds a new Lsp monad
    • has a built-in virtual file system to track editor files
    • tracks cached names and ppe based on current UCM state
  • Two background workers
    • UCM worker which listens for codebase updates or path changes and re-computes names accordingly
    • Typechecker worker which listens for file-changes, then re-checks changed files and emits diagnostic info.
  • Current "Hover" implementation is very naive and just queries using the Backend helpers, I'll make this smarter in a future release
  • Converting errors into diagnostics was a ton of work, but should be easy to maintain from now on 👍🏼

Interesting/controversial decisions

Currently the LSP is built into UCM itself, and you need to have a UCM session running to use it (though it also works in headless mode)

Test coverage

LSP is tough to test, so everyone will just have to dog-food it 😄

Transcripts should detect if I accidentally altered any existing Type errors or anything.

In the Future

  • It would be nice to have separate, terser error message for the LSP. UCM shows relevant info near the bottom, and inlines the source-code, which is unnecessary in LSP.
  • Hover
    • show docs
  • Upcoming features: Smart Auto-complete, code-hints for watch expressions, code actions to add/update, code actions for errors like 'ambiguous names', etc, and so many more!

This change is Reviewable

@ChrisPenner ChrisPenner marked this pull request as ready for review July 8, 2022 21:05
@ChrisPenner ChrisPenner self-assigned this Jul 8, 2022
@ceedubs
Copy link
Contributor

ceedubs commented Jul 9, 2022

It probably helps a lot that I also use Coc.nvim, but I just copy/pasted your config and opened a scratch file and hover and error diagnostics Just Worked™️! Awesome stuff, @ChrisPenner. 🎉

@ChrisPenner
Copy link
Contributor Author

Hrmm, it looks like the cli-integration tests are failing to exit on windows for some reason, I wonder if it's waiting for the LSP server to shut down or something, weird it's windows-only though.

lspPort <- getLspPort
TCP.serve (TCP.Host "127.0.0.1") lspPort $ \(sock, _sockaddr) -> do
sockHandle <- socketToHandle sock ReadWriteMode
Ki.scoped \scope -> do
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit of an anti-pattern to pass a scope down into a function; could this scope be created closer to where it's used, or not really?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it can/I don't think we want to, the goal of this scope is for every background thread and request which is related to a given LSP connection should be tied to the scope for that connection, so that if the client ever hangs up, all the background work terminates as well. Since this is where the thread will "hang" waiting for a hang-up, I think this is the right place to put it for that behaviour.

I understand why it's generally an anti-pattern, but here I legitimately do want the nested threads to be forked on this scope, and I can't create new scopes at a lower level and get the same behaviour.

@ChrisPenner ChrisPenner merged commit 19a2526 into trunk Sep 12, 2022
@ChrisPenner ChrisPenner deleted the cp/lsp branch September 12, 2022 19:45
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

Successfully merging this pull request may close these issues.

None yet

3 participants