-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add minimal Scala support - tree-sitter, Metals #7933
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this, it's great to see support for Scala! 🙂 I have left a few suggestions for a more complete and rounded integration, but I don't know if those would be required immediately or could be worked on later as a followup. 👍
crates/zed/src/languages/scala.rs
Outdated
false | ||
} | ||
|
||
async fn installation_test_binary( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could execute metals -v
to test if the binary is properly installed (can be found on path) and runs. It should return exit code 0 and print version to the standard output (which perhaps could be used in the future to compare the installed and requested releases?). 🙂
crates/zed/src/languages/scala.rs
Outdated
"metals" | ||
} | ||
|
||
async fn fetch_latest_server_version( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could fetch the latest version from the official source: https://scalameta.org/metals/latests.json
. It's a JSON file that contains latest release and snapshot versions. It's automatically updated and used by the Metals webpage/documentation (https://scalameta.org/metals/docs/) and plugins (Neovim, VS Code). See an example in the nvim-metals: https://github.com/scalameta/nvim-metals/blob/main/lua/metals/install.lua#L138. 🙂
crates/zed/src/languages/scala.rs
Outdated
Ok(Box::new(())) | ||
} | ||
|
||
async fn fetch_server_binary( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could do what all the other editor integrations are doing (Neovim, Helix, etc.) and require the Coursier to be installed first by the user (probably would be good to describe it in the language documentation under docs/src/languages
). Then it could be used to download any desired Metals version (for example to some Zed cache directory, to not conflict with anything else). Here's a reference from nvim-metals, but it basically boils down to somethng like this:
cs bootstrap --java-opt -Xss4m --java-opt -Xms100m org.scalameta:metals_2.13:1.2.1+9-e65409f2-SNAPSHOT -r sonatype:snapshots -o ~/path/metals -f
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can copy my coursier based installation from my WIP branch https://github.com/bishabosha/zed/blob/feature-add-scala/crates/zed/src/languages/scala.rs
@@ -0,0 +1,27 @@ | |||
(class_definition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original PR by @hmemcpy (#6982) contained overrides.scm
as well and the WIP branch from @bishabosha (https://github.com/bishabosha/zed/tree/feature-add-scala) had locals.scm
. Would it make sense to integrate these too (sorry, I'm not really familiar with how Zed's treesitter configuration works)? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://github.com/mprihoda/zed/blob/aeb9e97138d332643d1d5b74584a222a709212a0/crates/language/src/language.rs#L761 it seems that locals.scm would not be used. Overrides should, but I'm not sure what to use it for, I have to figure it out yet.
The queries in current state seem to be working quite fine, though I'm sure there is a lot to be improved upon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially followed the comment in the code about the steps regarding adding a new language support, it mentioned copying the SCM files from the language's tree-sitter repo. The Scala one had those, so I copied them as-is, but haven't checked if they were actually used by Zed.
Thank you for the review, all the comments make very much sense and I considered merging @bishabosha's changes, but if at all possible, I would prefer to merge this and follow up with improvements in next PRs, as it would delay the initial Scala support without much additional value. I know the current state works, as I'm using it to write production Scala code daily, but I can't myself vouch for the rest and I would be hard pressed for time to test it. If we merge it, @bishabosha and @hmemcpy can the chime in to improve upon it and we can start to collaborate. There are more languages with this basic LSP support already in Zed, so it does not seem to be impossible. Of course, if you deem it not fit for merge yet, I will try to update it as soon as possible. |
I'm fine either way, and good to just get some initial support ASAP. Just without those changes it's on the user to update metals rather than automatically get the latest version. Also users are expected to have Coursier on the path following the standard download instructions, but metals is something you have to manually download |
I don't have strong opinions here, but I really like @bishabosha'approach of automatically downloading metals from github. Seems consistent with several other LSP implementations in zed, and the API is built for that. |
I agree, installation is definitely good to have and Zed has the necessary functions prepared. But I also assume that any user trying Scala with Zed right now will probably have already working Metals setup with another editor and I wanted to get this out as soon as possible. I'm fine either way, just let me know. If the installation or query tuning is necessary before initial merge, I'll be able to get to it within a few days. |
aeb9e97
to
d5b9d63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well I tested this locally and im having a good experience on a scala-cli project, BSP is being connected to (so go-to definition of external library dependencies is working).
up to the Zed people now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea for the future, to improve the overall out-of-the-box experience: #8188 has introduced a "native" way for LSP integrations to discover (and use) an already present server binary in case user has one installed. 👍
d5b9d63
to
985c48f
Compare
I'll investigate the changes. They work with the environment, which I'm interested in anyway because of direnv support (#4977) - it would be very useful to be able to run the LSP server with properly configured PATHs from the project dir. |
probably blocked until #7096 |
I'm not sure if this is blocked on the ongoing extensions work:
@maxbrunsfeld Is there anything we can do to help push this through? Also, pinging @SomeoneToIgnore since he's been involved in reviewing the Java integration efforts, which should be fairly similar in nature to Scala (JVM family tree). 🙂 |
Adds - Scala syntax highlighting using `tree-sitter-scala` - basic Metals LSP support, without automatic installation Most of the tree-sitter queries have been picked up and repurposed from [tree-sitter-scala](https://github.com/tree-sitter/tree-sitter-scala) and [nvim-treesitter](https://github.com/nvim-treesitter/nvim-treesitter/tree/master/queries/scala) projects.
We have an initial version of providing language servers via extensions that is ready for use, so I think that Scala support can move to an extension. We have a couple extensions in the repo that can serve as a reference: https://github.com/zed-industries/zed/tree/eaa803298ee078c38af4e2ccf753ed98b43a9859/extensions I'm also happy to answer any questions for someone who wants to work on a Scala extension. |
Update: the extension is now available in the preview version (0.129.1), should hopefully land in stable next week! Thanks again for everyone's contributions! |
Adds
tree-sitter-scala
Most of the tree-sitter queries have been picked up and repurposed from tree-sitter-scala and nvim-treesitter projects.
Release Notes: