Bugfix: Recognise ERB files correctly in Neovim

[tsvallender]

Motivation

Though ruby-lsp supports ERB files, in Neovim these were either not being parsed at all, or, following this update to lspconfig, parsed as Ruby (with all the associated errors that brings).

Implementation

Checking for "eruby" as the filetype, as well as "erb" gets things working.

There is one caveat: I still have a single error in each file being thrown by Rubocop: Lint/Syntax: unexpected token tLT This offense is not auto-correctable. I presume this does not occur in VS Code but can’t see any code preventing Rubocop in ERB files, so would appreciate any thoughts on that. Still worth merging as-is, I believe, as it reduces a screen full of errors and no functionality to one error plus functionality.

Automated Tests

Not applicable

Manual Tests

In VS Code there should be no change, in Neovim ERB HTML files should now be parsed correctly.

[tsvallender]

I have signed the CLA!

[adam12]

Confirming this patch corrects the errors seen in Neovim.

[tsvallender]

I’ll also note the Rubocop warning I mentioned can be removed simply by telling Rubocop to ignore ERB files. Should Rubocop be able to successfully run in ERB files normally though? (i.e. am I missing out on some other functionality by doing that?)

[vinistock]

I’ll also note the Rubocop warning I mentioned can be removed simply by telling Rubocop to ignore ERB files. Should Rubocop be able to successfully run in ERB files normally though? (i.e. am I missing out on some other functionality by doing that?)

No, RuboCop should only run in Ruby files. We turn off diagnostics for ERB documents, but eventually we'd like to offer diagnostics + formatting through erb-lint.

Confirming this patch corrects the errors seen in Neovim.

Checking other Ruby language servers for NeoVim, it seems like it standardizes on eruby as the language ID. Not sure why that was picked over erb, but I think it makes sense to move forward with this fix.

[mbriggs]

Would it be possible to cut a release for this? If you are an erb enjoyer, this is a pretty disruptive bug :)

[adam12]

Would it be possible to cut a release for this? If you are an erb enjoyer, this is a pretty disruptive bug :)

Might be worth adjusting your nvim config temporarily to remove eruby from the filetype.

Here's two examples, tho I wasn't able to test Mason directly because I only use it for managing non-Ruby LSPs.

local lspconfig = require('lspconfig')
lspconfig.ruby_lsp.setup({
  filetypes = { 'ruby' }
})

require('mason-lspconfig').setup_handlers {
  function (server_name) -- default handler
    require('lspconfig')[server_name].setup({})
  end,

  ['ruby_lsp'] = function ()
    require('lspconfig').ruby_lsp.setup({
      filetypes = { 'ruby' },
    })
  end,
}

[rodrigolj]

Is this a regex issue, perhaps? If we need to narrow the diagnostics only between <% %> tags and such, I could spend some time on this. I just need someone to point me where to look, because I haven't worked on an LSP before.

[adam12]

Is this a regex issue, perhaps?

The parser is already aware of ERB formatted files. The issue corrected here was the filetype sent by vim/neovim wasn't the same filetype sent by the VSCode extension.

[vinistock]

Just cut a release with the fix https://github.com/Shopify/ruby-lsp/releases/tag/v0.17.13.

To force the upgrade, you can either

• rm -r .ruby-lsp delete the custom bundle directory altogether and then re-launch the editor to create it from scratch
• BUNDLE_GEMFILE=.ruby-lsp/Gemfile bundle update ruby-lsp update the LSP manually

Is this a regex issue, perhaps?

No, this was a mismatch of the language ID for ERB documents expected by the server vs the one provided by NeoVim - which does not match the one used by VS Code.

[mbriggs]

Just upgraded, everything works great, thank you to everyone involved :D

[adam12]

Just to round out this PR, I checked Zed's support for the Ruby LSP and it appears that it sends erb as well, so no special handling required in that case.

https://github.com/zed-industries/zed/blob/88a12b60a98f5d7980054cfef007fb79f8fc7be3/extensions/ruby/extension.toml#L15

[AvidRambo]

Does this fix this issue?

[mbriggs]

Hey, checking in from #2448 and #2442, while this fixed that specific problem, apparently diagnostics should not be running in erb files. Maybe these are parse errors that neovim is surfacing but vsc does not?

[vinistock]

When the Ruby LSP broadcasts its diagnostic capabilities, it scopes it to only Ruby files

ruby-lsp/lib/ruby_lsp/requests/diagnostics.rb

Line 28 in e18d456

document_selector: [Interface::DocumentFilter.new(language: "ruby")],

Maybe NeoVim is not respecting the server's document selector for diagnostics? If that's the case, it's a NeoVim bug since the spec prescribes the possibility for the server to dictate which files it wants to handle for diagnostics.

See Diagnostic registration options, which extends Text document registration options and allows for a document selector.

In addition to correctness, this is important for performance reasons too. We don't want the client to keep sending requests that the server doesn't want to handle. VS Code respects the server selection and never sends diagnostic requests for ERB files.

In terms of tactics on how to move forward with this, our 3 person team does not have NeoVim experience. My suggestion is to revert neovim/nvim-lspconfig#3266 and then someone with more NeoVim experience would need to do a more in depth investigation to understand what's not going right before shipping that configuration PR again (to avoid disruption for NeoVim users).

[elken]

Seems that neovim does indeed not yet support the needed interface neovim/neovim#24412 (the PR that added initial diagnosticProvider work cites back to that as being the ticket for DiagnosticRegistrationOptions even if it is slightly misnamed)

@vinistock as someone who has experience already implemented this on the VSCode side; would this be a big piece of work? Is this just something that is checked on startup then before every diagnostic request?

In the meantime, I also agree that the PR should be reverted.

[adam12]

In the meantime, I also agree that the PR should be reverted.

I agree. I've submitted a PR to do so.

[elken]

Great job!

I would really like this in, so I might look into what's needed at the weekend but no promises 😄

[vinistock]

@vinistock as someone who has experience already implemented this on the VSCode side; would this be a big piece of work? Is this just something that is checked on startup then before every diagnostic request?

I don't have experience implementing the client side. The VS Code team provides an official package for the client side which implements the entire specification for us. We only implement the server side of the equation.

I would imagine that the client needs to remember the document selector on a per feature basis since some of the features (if not all of them?) can have it overridden by the server. When receiving the response from the initialize request, it needs to go over the capabilities and adjust the document selectors according to what the server responded with.

[elken]

I see, thank you. That's cleared some of it up. 😄