bergercookie / asm-lsp

Language server for NASM/GAS/GO Assembly
https://crates.io/crates/asm-lsp
BSD 2-Clause "Simplified" License
269 stars 18 forks source link

feat: label autocomplete #105

Closed WillLillis closed 4 months ago

WillLillis commented 4 months ago

Implements basic autocomplete for labels. There is a minor performance concern here, as we're running a tree-sitter query over the entire document for every autocomplete request that isn't initiated by a trigger character. If this proves to be too much of a slow down, I can look into speeding it up and/or putting this feature behind a configuration flag.

Quick demo using the example from #104:

label_autocomplete

This PR also bumps tree-sitter-asm to its current latest commit, rather than the older 0.1.0 release. There's been some nice improvements there lately and all of the project's tests still pass with the updates!

cc @ChillerDragon

Closes #104

ChillerDragon commented 4 months ago

does not work for me yet :c

I assume my dependency manager installs it from cargo not from the master branch. When is the next release planned?

WillLillis commented 4 months ago

does not work for me yet :c

I assume my dependency manager installs it from cargo not from the master branch. When is the next release planned?

Which dependency manager are you using? If you have Rust installed on your system you could clone the repo, compile it in release mode, and then create a symlink from the resulting binary to the location your editor is expecting asm-lsp to be. Happy to help with this, but if it's too much trouble I'm also ok with making another minor release. Let me know! :)

ChillerDragon commented 4 months ago

Yea compiling from source was the first thing I tried but seems like it doesn't pick it up even if it's in PATH. I am using the kickstart.nvim config and it uses lazy and mason if I am not mistaken but I just cba to debug my setup haha. I'll wait for a new release.

WillLillis commented 4 months ago

Oh! If you're using mason then it's probably expecting the executable inside ~/.local/share/nvim/mason/bin/ :)

WillLillis commented 3 months ago

0.7.4 pubished

ChillerDragon commented 3 months ago

Thanks for the release @WillLillis! The shutdown bug is fixed :partying_face: and I can also complete labels from the same file. Sadly multi file support is not there yet but that might be a tree sitter limitation because it depends on the nasm %include macro.

image

Also goto definition for labels defined in other files does not work

image

WillLillis commented 3 months ago

Thanks for the release @WillLillis! The shutdown bug is fixed :partying_face: and I can also complete labels from the same file.

Glad to hear it! 😄

Sadly multi file support is not there yet but that might be a tree sitter limitation because it depends on the nasm %include macro.

image

Also goto definition for labels defined in other files does not work

image

That is indeed a tree-sitter limitation. As far as I understand it, to provide goto definition across files we'd have to do some semantic analysis on the source code, and tree-sitter is a parsing tool that deals with things on the textual level. From what I've seen working with zls, other language servers can leverage pieces of a language's compiler to do pieces of this analysis for them. I haven't had a chance to throw some real time at the problem, but integrating an assembler as a library for our tool seems like quite the undertaking (complications due to Rust FFI, said assemblers being designed as standalone CLI tools rather than libraries, etc.). In addition, we'd have to do this for multiple assemblers. It's not something I'm closed to, but I don't see a clear path forward for it currently.

(Apologies for editing your original comment, I accidentally did that instead of quote-replying 😆)

WillLillis commented 3 months ago

Also goto definition for labels defined in other files does not work

image

Looking at this again, the server shouldn't be causing neovim to error out like that. I'll have a patch merged in a minute. Thanks for the report!

111