jose-elias-alvarez / null-ls.nvim

Use Neovim as a language server to inject LSP diagnostics, code actions, and more via Lua.
Other
3.64k stars 791 forks source link

[Request] Exclude Running Null-LS by Filetypes and Buftypes #49

Closed tigorlazuardi closed 3 years ago

tigorlazuardi commented 3 years ago

I found some compability issues of null-ls with nvim-tree. I got double entries if I use builtins that use filetypes of "*" (like misspell or gitsigns). I assume that's because null-ls will run on that buffer and nvim-tree go haywire.

Screenshot:

nvim-tree

This perhaps could be caused by other plugins or configurations, but debugging that deep will take a huge time. Hopefully simple exclusion will solve the problem.

Ideally an exclude per sources, but perhaps adding global_exclude parameter when calling require('null-ls').config should be suffice for this.

jose-elias-alvarez commented 3 years ago

I'm definitely not opposed to adding a list of filetypes to exclude, but recent versions of null-ls that integrate with nvim-lspconfig actually shouldn't attach to buffers like the one created by nvim-tree where buftype is set to nofile. I tried this out on my end, and I see that nvim-tree does correctly set its buftype, but it doesn't always do so before it sets filetype (which is what triggers null-ls to attempt to attach). I'm going to open up an issue on their end and see if we can get it solved there, and if not, I'll add the exclude option.

I can see that null-ls can mistakenly attach to the nvim-tree buffer, but I wasn't able to replicate what I see in your screenshot. Does the same thing happen in every directory, and does commenting out null-ls from your config fix the issue?

Edit: I opened a PR for nvim-tree that should take care of the issue. If you have a chance to check it out and see if it fixes the issue, that would be great, because if it doesn't, it's caused by something else.

gegoune commented 3 years ago

Is getting

Error executing vim.schedule lua callback: ...ack/packer/start/null-ls.nvim/lua/null-ls/formatting.lua:41: Invalid window id: 1003

after quiting Neogit's buffer related to this issue?

jose-elias-alvarez commented 3 years ago

@gegoune I don't think so, I just checked and Neogit correctly sets buftypebefore filetype, so null-ls won't attach (on the latest main commit, at least). Could you tell me how to reproduce this?

gegoune commented 3 years ago

I have run Neogit commit, made a commit and error popped out after Neogit commit window closed.

jose-elias-alvarez commented 3 years ago

Hmm, I see that null-ls is attaching to the gitcommit buffer, but I'm not seeing an error. Are you quitting with :wq or with another command? Also, are you using an autocommand for formatting (and if so, what is it)?

Either way, I think I'll put in a check at the line that's giving you an error to make sure the window is valid before attempting to do anything with it, which should take care of the issue.

Edit: hopefully solved by 52286ee4d11016b3366481935c12211c44d43777, but let me know.

gegoune commented 3 years ago

Won't answer your questions, sorry, as it seems to work with latest change. Thank you! ❤️

tigorlazuardi commented 3 years ago

Hey sorry for not answering these few days. But I can conclude it's very most likely the double entries are not your fault (by logic alone, after reading the source code, it's impossible), but must be nvim-tree's fault.

auto-session seems to also trigger the double entries if enabled auto session load, but bypass the autoload by calling neovim directly to open a file.

jose-elias-alvarez commented 3 years ago

No idea about auto-session, but if you're able to put together reproduction steps, I can investigate if there's anything we can do to fix that.

gegoune commented 3 years ago

Double entries is well known yet not fully investigated issue of nvim-tree.

akinsho commented 3 years ago

So I'm also seeing this issue quite consistently with null-ls and neogit I don't notice anything in nvim-tree but I use that infrequently. Every time I finish committing and save and quit after leaving the neogit tab I see an error.

Error detected while processing CursorHold Autocommands for "*":
E5108: Error executing lua ...l/Cellar/neovim/0.5.0/share/nvim/runtime/lua/vim/lsp.lua:262: Invalid buffer id: 47

It appears to relate to my custom on attach which I also pass to null-ls which has an autocommand for document highlights (ignore the custom helpers).

  if client and client.resolved_capabilities.document_highlight then
    as.augroup('LspCursorCommands', {
      {
        events = { 'CursorHold' },
        targets = { '<buffer>' },
        command = vim.lsp.buf.document_highlight,
      },
      {
        events = { 'CursorHoldI' },
        targets = { '<buffer>' },
        command = vim.lsp.buf.document_highlight,
      },
      {
        events = { 'CursorMoved' },
        targets = { '<buffer>' },
        command = vim.lsp.buf.clear_references,
      },
    })
  end

I tend to quit with :wq or rather a mapping of q to wq. One difference I think there is in my setup is that I use a custom gitmessages file so that is what is loaded as my commit message template.

My null-ls config is

    use {
      'jose-elias-alvarez/null-ls.nvim',
      requires = { 'nvim-lua/plenary.nvim', 'neovim/nvim-lspconfig' },
      config = function()
        local null_ls = require 'null-ls'
        null_ls.config {
          debounce = 150,
          sources = {
            null_ls.builtins.code_actions.gitsigns, -- <-- seems to be the cause
            null_ls.builtins.formatting.stylua,
            null_ls.builtins.formatting.prettier.with {
              filetypes = { 'html', 'json', 'yaml', 'graphql', 'markdown' },
            },
          },
        }
        require('lspconfig')['null-ls'].setup {
          on_attach = as.lsp.on_attach,
        }
      end,

Removing the gitsigns code actions seems to resolve this issue.

jose-elias-alvarez commented 3 years ago

@akinsho Thanks to your info, I was able to find the source of the issue, which appears to be the fact that writing and quitting a Git commit buffer leaves a pending change in the client's change-tracking state, which is flushed on the next request. That change attempts to get the buffer content for the Git commit buffer by its buffer number, and since the buffer isn't loaded, it throws an error.

Checking whether the buffer is loaded fixes the issue, but I don't think the Neovim folks would be thrilled about fixing a problem that is (as far as I know) only triggered by null-ls. I have no idea why this happens, and I have no idea why it only seems to happen for Git commit buffers. I wondered if someone might come up with a source that wants to use the gitcommit filetype, but if I can't figure out a fix, I'll just make sure it's always ignored and deal with it later if necessary.

I've never heard of using a custom file for commits, but does it also set filetype=gitcommit?

akinsho commented 3 years ago

@jose-elias-alvarez so the actual GIT_COMMIT buffer has it's filetype set in filetype.vim by checking against some filenames, but neogit calls it's file NEOGIT_GIT_COMMIT or something so it doesn't get set by nvim. It does seem to be being specified in neogit

https://github.com/neovim/neovim/blob/93443d59a9680a6055d4602653d34516bc2c2571/runtime/filetype.vim#L656

https://github.com/TimUntersberger/neogit/blob/bf148e2534097988e61ed334edaf31b67134369b/lua/neogit/popups/commit.lua#L24

but I'm not sure if there's a race conditions or what. One thing I'm seeing on occasion is neogit isn't setting it to gitcommit in time maybe and filetype.vim has another check with checks if a file has # as the starting character and as a fallback sets it to a filetype of conf

https://github.com/neovim/neovim/blob/93443d59a9680a6055d4602653d34516bc2c2571/runtime/filetype.vim#L2333-L2338

I hadn't seen that behaviour before installing null-ls I'm not sure if the content is being manipulated at some point and the buffer is being reset in some way. I tried adding a modeline specifying the filetype manually myself but that seems to not work either.

I think if just ignoring it is easiest then (biased ofc) I'd be fine with that. I personally really don't need any lsp functionality in the commit buffer.

jose-elias-alvarez commented 3 years ago

Thanks for looking into the filetype side of things! I can't say for sure about the behavior you are seeing, but if the filetype isn't set correctly, I imagine it's a Neogit issue, since to my knowledge there's nothing null-ls does that would alter that in any way.

Fixing the filetype on our end is definitely the easiest fix, but if I'm not crazy, I think this is actually a Neovim bug that happens when a) a client is using the debounce_text_changes flags and doesn't allow incremental sync, and b) the user edits, writes, and removes a buffer from the buffer list in close succession (similar to the situation we have here). I've been able to reproduce it with actual language servers and regular files, so I'm going to open a Neovim issue, but in the meantime I'll put together a quick fix to simply exclude gitcommit from null-ls. Does that sound reasonable?

akinsho commented 3 years ago

That sounds good 👍🏿, yeah I figure the filetype issue is probably down to neogit so will see about raising an issue for that.

Out of curiosity, I don't specifically prevent incremental sync in my config, is this something that the gitsigns or maybe null-ls in general is doing? (doesn't really matter, just wondering if I'm not using incremental sync, I thought it was on by default now)

jose-elias-alvarez commented 3 years ago

It's null-ls – we have it off so we can avoid an extra API call and have Neovim send us the entire buffer content on every change.

I just pushed a small change to disable attaching to gitcommit buffers (I realized we'll have to do the same for 0.5, even after we get the upstream bug fixed) so let me know if that takes care of the issue. I see that the nvim-tree PR I put in got merged, so this issue should be OK to close after that.

akinsho commented 3 years ago

@jose-elias-alvarez I believe that has resolved the issue (a little hard to be 100% certain due to the conf neogit issue which means that the conf filetype can also trigger it, but that shouldn't be happening in neogit at all)

jose-elias-alvarez commented 3 years ago

@akinsho Right, the conf issue will likely trigger the same issue, since null-ls will attach, but hopefully we can deal with that on Neogit's end. Going to close this for now – thank you for the help!