feat: migrate to diagnostic API and support linting on save (feat. golangci-lint)

[jose-elias-alvarez]

Closes #166 and supersedes #318.

The main change in this PR is to migrate to the vim.diagnostic API when available, the biggest advantage of which is the ability to use separate namespaces for each source. This lets us push multiple sets of diagnostics at a time, which enables running linters on save (more on this in a bit) and lays the groundwork for better source management, as mentioned in #337. It's also simpler to use, both internally and from a user config perspective.

Previously, pushing diagnostics on textDocument/didSave notifications wasn't possible for the reasons discussed in #166, but with the diagnostic API in place, adding support was simple. The one issue was handling textDocument/didOpen notifications, since these should prompt null-ls to run both on-change and on-save linters. I'm not thrilled with the workaround (checking a separate overrides table) and want to see if I can come up with something better.

I combined the implementations in #318 and #69 to add support for golangci-lint as the first on-save source. I don't use Go at all and only have a sample repository for testing, so I'd appreciate feedback from @zalegrala and anyone else who's able to test this out in a real development environment.

Finally, this PR drops support for Neovim version 0.5.0, which is a breaking change but shouldn't be a big deal.

[zalegrala]

I'm currently running 0.5.1 from Archlinux, but would like to test this out.

[jose-elias-alvarez]

So the issue with 0.5.1 is that diagnostics are correctly generated and on save, but every time you make a change to the buffer, the diagnostics are wiped out until the next save, which (in my opinion) is not a good experience and is inconsistent with how the VS Code extension works. Do you think that's acceptable?

[zalegrala]

That doesn't seem like a great experience, but if what happens when you upgrade is that you get a better experience, then maybe that is acceptable. I just looked for 0.6.0 source for neovim, but see there are many issues still in the milestone, so not sure on timeline there.

I just checked out the branch and enabled the linter in my config, and what shows up when I run golangci-lint manually does not appear in the buffer for me. Do you think that is related to my version, or perhaps I've not configured something correctly?

Using lunarvim (which I can skip if you think its related) I have this config:

local linters = require("lvim.lsp.null-ls.linters")
linters.setup({
	 { exe = "golangci_lint", filetypes = { "go" }, },
})

[jose-elias-alvarez]

I just pushed a commit disabling the version check so you can try it out. I have no idea about setting up Lunarvim, but all you need to do is pull the latest commit, then run the following code after null-ls is loaded and set up:

local null_ls = require("null-ls")
null_ls.register(null_ls.builtins.diagnostics.golangci_lint)

[zalegrala]

That works. Now I see what your saying about the buffer change. If its the best we can do while we wait for 0.6.0 I'd take it, but I do agree it isn't ideal.

[marcomayer]

Ah just found this and that you're already discussing the issue I mentioned in #318. If it's fixed with nvim 0.6 I also think it's a better-than-nothing start 👍

[David-Else]

Finally, this PR drops support for Neovim version 0.5.0, which is a breaking change but shouldn't be a big deal.

Could you make a 0.5.1 compat branch when this happens? They do that with treesitter https://github.com/nvim-treesitter/nvim-treesitter/tree/0.5-compat , you could delete it as soon as 0.6 is out officially :) If that is annoying some kind of instructions to freeze the version in vim-plug/packer?

[rafikdraoui]

Hello! Here is what I experienced after installing this branch.

1. Runtime error from golangci-lint isn't handled

If golangci-lint is run in a directory that doesn't have a go.mod file, it returns with exit code 5 and the message ERRO Running error: context loading failed: no go files to analyze

Opening the Go file in neovim, the following error message is displayed:

null-ls: failed to run generator: ...e/pack/packer/start/null-ls.nvim/lua/null-ls/helpers.lua:192: error in generator output: level=error msg="Running error: context loading failed: no go files to analyze"

2. params.output.Issues can be null in on_output callback

After adding a go.mod file to make golangci-lint happy:

module hello

go 1.17

The linter now runs, but there is still an error message:

null-ls: failed to run generator: ....nvim/lua/null-ls/builtins/diagnostics/golangci_lint.lua:32: bad argument #1 to 'ipairs' (table expected, got userdata)

It turns out that params.output.Issues can be vim.NIL in some cases:

[DEBUG Sat 13 Nov 2021 09:18:03 PM] ...ite/pack/packer/start/null-ls.nvim/lua/null-ls/utils.lua:74: output: {"Issues":null,"Report":{"Linters":[...]}}

$ golangci-lint run --fast --out-format=json | jq .Issues
null

This feels like an issue in golangci-lint: ideally it would set Issues to an empty list rather than null for consistency. I have made a PR about it: golangci/golangci-lint#2358

In the mean time, we could have a null-check on params.output.Issues before passing it to ipairs in

null-ls.nvim/lua/null-ls/builtins/diagnostics/golangci_lint.lua

Line 28 in fe252e5

for _, d in ipairs(params.output.Issues) do

[jose-elias-alvarez]

Finally, this PR drops support for Neovim version 0.5.0, which is a breaking change but shouldn't be a big deal.

Could you make a 0.5.1 compat branch when this happens? They do that with treesitter https://github.com/nvim-treesitter/nvim-treesitter/tree/0.5-compat , you could delete it as soon as 0.6 is out officially :) If that is annoying some kind of instructions to freeze the version in vim-plug/packer?

0.5.1 will still be supported until 0.6.0 release (and probably for a couple of months afterwards). This PR just removes 0.5.0 support, since I don't think many users are still on it and accounting for 3 potential diagnostic APIs was too much.

[jose-elias-alvarez]

@rafikdraoui Thanks for trying this out and for the detailed feedback! I've addressed the second issue for now by adding a typecheck.

For the first issue, I believe we have at least a few linters that will error out if the user attempts to run them without a config file. We could either ask users to set a condition for the source that checks for the presence of a config file or do something like what we do with ESLint, where we inject the error as a diagnostic (a few VS Code extensions have this behavior too). Its presence in this PR is more of a proof-of-concept than anything else, and I'd rather leave it for actual users to improve it in subsequent PRs.

[jose-elias-alvarez]

I've gone back and forth on whether to block on-save linters on 0.5.1, and I think @marcomayer's experience shows that users who are unaware of the limitation are likely to conclude that null-ls itself is broken when encountering the behavior.

Either way, it seems that 0.6 release is not that far out. What I'll do for now is merge this to hopefully get some more feedback on this from 0.6.0 users (who are surely more used to things breaking) and make sure the migration to the diagnostics API is all set for release. Thanks to everyone who tried this out / gave feedback!