Improve Neovim Lua API

[mawkler]

Hi! This PR improves the Neovim Lua API by exposing the functions rustowl.enable(), rustowl.disable() and rustowl.toggle() so that the user can, for instance, map those to keymaps that they desire. It also automatically does the configuration for the user in a more conventional manner, so that they don't have to explicitly call require('lspconfig').setup() themselves. It also exposes options for configuring whether to auto-enable RustOwl on startup, and lets the user set the idle time. Moreover, I added more extensive documentation for how the user can automatically build rustowl from source on installation from inside Neovim.

I started working on this last week, and since then c5cd9cd got merged. Since my PR does kind of the same thing, but a little more extensively I found the easiest way was to simply revert that commit.

[cordx56]

It will take some time since this update didn't work in my environment. Please wait for the review.

[mrcjkb]

@mawkler what do you think of removing the lspconfig dependency altogether in this PR, for a more out-of-the-box experience, similar to rustaceanvim's (see #57 for details)?

It could also be done in a follow-up, but existing Neovim users might not be happy about too much churn.

[mawkler]

@mrcjkb I would speculate that 99% of Neovim users that use language servers use nvim-lspconfig (if they're not using coc.nvim), so I think it would be a very small win to remove it. That being said, I'm not against removing the dependency, but I think that should be done in a different PR.

Also, the nightly Neovim 0.11 adds vim.lsp.config() which is going to simplify language server setup, and if I'm understanding it correctly it is without nvim-lspconfig.

[mrcjkb]

I would speculate that 99% of Neovim users that use language servers use nvim-lspconfig

That's not a good reason to depend on it.

so I think it would be a very small win to remove it.

For the "wins" of removing it, see the points outlined in #57, as well as the initialisation section in the neorocks/nvim-best-practices guide.

nvim-lspconfig is quite an old plugin (from the nvim 0.5 days), does not have a good UX and does not lazy-load its LSP client configurations (you need to rely on a plugin manager for that).

In fact, nvim-lspconfig is intended to be a "data only" repo (see its readme), not to be used as a dependency/library. And the lspconfig.utils module is an internal API that could be changed in a breaking manner or even removed without warning or a major version bump. The plugin/rustowl.lua script is already broken for non-lazy.nvim users due to the way Neovim's built-in initialisation sequence works.

Also, the nightly Neovim 0.11 adds vim.lsp.config() which is going to simplify language server setup

Yes, the fact that this is being implemented in core means it is more likely that lspconfig will remove internal APIs once it's released. You can use vim.lsp.config without nvim-lspconfig. rustaceanvim does just that.

but I think that should be done in a different PR.

Sure, but as I mentioned, that would mean churn for users of the rustowl plugin.

Edit: @mawkler I went ahead and opened a PR based on what I did in rustaceanvim: https://github.com/mawkler/rustowl/pull/1. Tested, but in my build, the rustowl server's response doesn't have any decorations.

[cordx56]

Sorry for our late review. Maybe I am most relevant to this issue among maintainers because I am neovim user. But I could not confirm that this PR is working or not. I do not have enough time to review this PR, so it takes a long time. sorry.

This PR has conflicts now, so please resolve conflicts first, And I will review it. I attends a conference until 7 Mar in JST. So I will review this PR after the conference ends.

@mawkler @mrcjkb if you resolve problems in other repository, please feel free to close this PR and open new PRs in our repository. Thank you for your contribution.

[mawkler]

No worries @cordx56! @mrcjkb has added a bunch of improvements in their PR to my branch. I'm guessing that the plan is to merge that PR into my branch that makes up this PR (mawkler:lua-api), and then we can merge this PR after it's been approved here.

[cordx56]

Can I review it? I'm ready to review.

It looks like there are some conflicts. Are you planning to fix them?

[mawkler]

I have now pushed a new rebase on main with the conflicts resolved.

However, I'm having issues with your commit f92d528b19f58ddee07270abaa48b43009f8259a, @mrcjkb, since no highlights appear after doing :RustOwl enable. It seems that Rustowl always responds with the following body:

{
  decorations = {},
  is_analyzed = false,
  path = "..."
}

The previous commit e073eed7d4cf7022181f148d4fb9ce3e5ea6b15c works. I'm not sure what's up with that.

[mrcjkb]

@mawkler I had the same issue (thought it might be my nix rustowl build). My suspicion (based on https://github.com/cordx56/rustowl/issues/30#issuecomment-2679786904) is that there's an issue with the workspace/root directory detection. But :lua =vim.lsp.get_clients({ name = "rustowl" })[1] shows the correct workspece_folders in my test project. So I'm not sure about that. I'll investigate further...

[cordx56]

In https://github.com/cordx56/rustowl/commit/f92d528b19f58ddee07270abaa48b43009f8259a#diff-2b57fb92c0e089d41e642bb34539d18884921ba9832d47500cfaf3d447be9bb0L22 , root dirs are specified to send workspace init command to the LSP server. Maybe the workspace init/change event did not send.

[mrcjkb]

In f92d528#diff-2b57fb92c0e089d41e642bb34539d18884921ba9832d47500cfaf3d447be9bb0L22 , root dirs are specified to send workspace init command to the LSP server. Maybe the workspace init/change event did not send.

@cordx56 I've added support for didChangeWorkspaceFolders in https://github.com/mawkler/rustowl/pull/2/commits/2cc9c74f76904edd7384fbbf224930943641eb5b. A debug print also tells me that is_in_workspace returns true in the on_init hook. I have added a fix for a bug in which we pass root_dir as a function instead of as a string to vim.lsp.start. But I'm still not getting any hightlights on my end. This could be my nix rustowl build, so I'm waiting for feedback from @mawkler before I continue investigating.

[mawkler]

@mrcjkb I tried using RustOwl from your new PR and it does work in a smaller Rust codebase (I tried my advent-of-code repo). However, I keep getting is_analyzed = false when trying RustOwl in RustOwl's own Rust codebase. I also tried another repo with ~3000 lines of Rust code, which also didn't work. Does RustOwl have a timeout to prevent it from running for too long mayhaps?

[cordx56]

RustOwl does not have timeout itself. In my environment, RustOwl code itself can be visualized.

[mrcjkb]

That could be because of the simple root directory detection, which only searches upwards for a Cargo.toml or .git directory. In a multi workspace project, that will only detect the root for the current workspace.

rustaceanvim uses cargo to determine the root manifest in a multi workspace project. Perhaps we should use the same logic here.

Edit: 🤔 rustowl doesn't appear to be a multi workspace project.

[cordx56]

The LSP server searches Cargo.toml up and down from the specified workspace. It is quite a simple but enough to solve these problems.

[mrcjkb]

:thinking: @mawkler does it work with the rustowl codebase if you override the root_dir function to only search for Cargo.toml and not .git?

[mawkler]

@mrcjkb I get the same behaviour regardless of whether I include the .git or not. However, the issue might be that it just takes time for rust-analyzer to finish indexing the repo. After it has done that RustOwl works for most, but not all, variables that I tried in a random file in its own codebase.

An example of a variable that doesn't work is the i on this line. Is that expected behaviour?

[cordx56]

Variable that doesn't work is the i on this line. Is that expected behaviour?

Yes. It is difficult to say that this behavior is a bug. In RustOwl, the green underline, which I said actual lifetime, is the range from the variable’s memory area is acquired, to the variable will be dropped or moved.

However, in this case, variable i is not implemented Drop trait. So the lifetime cannot visualized based on the same logic. Maybe, the invalid lifetime will be visualized as long as this case. So I don’t care about this behavior especially.

[cordx56]

Is this ready for review? If so, please set me as Assignees.

[mawkler]

@cordx56 Yes it's ready for review. I don't seem to have the permission to assign you to the PR 🙂

[cordx56]

@mawkler OK, thank you. I will review.

[cordx56]

@mawkler I got this error:

I tried to identify the cause but i couldn't. My neovim version is 0.10.4.

[cordx56]

https://github.com/cordx56/rustowl/pull/48#issuecomment-2756493320

I found that when enable function called at the first time, the type of bufnr value, which is passed to resolve_bufnr, become function not number. after the second time, it became number. the enable function called many time when the hover.

@mawkler do you know the cause? I'm not familiar with neovim lua. thank you for taking the time.

[mawkler]

@cordx56 Hmm I don't seem to be able to reproduce your issue. I keep getting the buffer number when calling enable multiple times.

Neovim v11.0 was released last week. Could you try upgrading please and see if you're having the same issue? 🙂