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

Symbol are not supported in neovim #596

Closed
Ethnical opened this issue Sep 2, 2024 · 8 comments · Fixed by #598
Closed

Symbol are not supported in neovim #596

Ethnical opened this issue Sep 2, 2024 · 8 comments · Fixed by #598
Assignees

Comments

@Ethnical
Copy link

Ethnical commented Sep 2, 2024

In a normal solidity file we would like to have the symbols working (really useful with telescope). However, got issue because seems not be supported for now:

Expected Results:
I took a codebase with golang code just as an example, how what is expected when symbols for LSP are showing up:

CleanShot.2024-09-02.at.08.41.48.mp4

The cmd to reproduce is:

Telescope lsp_document_symbols

Produced Results:
This is a video on solidity files for example no symbol will appear:

CleanShot.2024-09-02.at.08.54.32.mp4

This command will produce:

[telescope.builtin.lsp_*]: server does not support textDocument/documentSymbol
@kanej
Copy link
Member

kanej commented Sep 2, 2024

Our documentSymbol support is currently behind a feature flag. We are rolling out full support over the next few weeks.

@Ethnical
Copy link
Author

Ethnical commented Sep 2, 2024

Oh thanks @kanej
Do you have the flag to help to test before released haha?
Would love to make feedback about it!

@Ethnical
Copy link
Author

Ethnical commented Sep 13, 2024

Okay seems to be located this for future people: #550

@fvictorio
Copy link
Member

@Ethnical did you get this working with Telescope? We might need to do something extra to enable it, because even after upgrading the LSP server with :LspInstall solidity_ls_nomicfoundation I still can't get them to work.

@Ethnical
Copy link
Author

Ethnical commented Sep 13, 2024

@fvictorio I didn't tried yet! will try maybe this weekend :)

@fvictorio
Copy link
Member

Ok, found the issue on my side.

The symbols provider is behind a feature flag, which uses the machine id to enable it for some percentage of users. The latest version sets that percentage to 100%, so in practice it's not a feature flag anymore, but it still goes through that mechanism. This means that if the machine id is not available, then all features that are behind flags will be disabled.

For some neovim-configuration-hell reason in my setup, the solidity server doesn't receive a machine id, so it just disables symbols and semantic highlighting. The fix is to add init_options in... some place. In my case it's something like this (I'm using lsp-zero):

local lsp = require('lsp-zero').preset({})

local lspconfig = require("lspconfig")
local configs = require("lspconfig.configs")

-- ...

configs.solidity = {
  default_config = {
    cmd = {'nomicfoundation-solidity-language-server', '--stdio'},
    filetypes = { 'solidity' },
    root_dir = lspconfig.util.find_git_ancestor,
    single_file_support = true,
  },
}

lspconfig.solidity.setup {
  init_options  = {
    machineId = "machineId"
  }
}

I added those init_options in lspconfig.solidity.setup with an arbitrary value for the machine id, and it started working.

@kanej @OmarTawfik since we are setting this to 100% now and we are unlikely to set it back to a non-100% value, should we just change this so that they are always enabled without going through isFeatureEnabled?

@fvictorio fvictorio changed the title Symbol are not supported Symbol are not supported in neovim Sep 13, 2024
@OmarTawfik
Copy link
Contributor

OmarTawfik commented Sep 16, 2024

@kanej @OmarTawfik since we are setting this to 100% now and we are unlikely to set it back to a non-100% value, should we just change this so that they are always enabled without going through isFeatureEnabled?

Thank you for the investigation. That would be the next step, yes, given that there are no issues/regressions found so far, and it has been rolled out to %100 for two weeks already (#597). This way, users wouldn't need to worry about providing a machineId.

@kanej
Copy link
Member

kanej commented Sep 16, 2024

Agreed, we should now switch to not using the feature flag system for these features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants