Interplay of NULL_LS_DIAGNOSTICS and NULL_LS_DIAGNOSTICS_ON_SAVE with pylint prevents errors from showing

bwells commented 2 years ago

FAQ

[X] I have checked the FAQ and it didn't resolve my problem.

Issues

[X] I have checked existing issues and there are no issues with the same problem.

Neovim Version

v0.5.1

null-ls config

local null_ls = require("null-ls")
null_ls.config({
    debug = true,
    sources = {
        null_ls.builtins.diagnostics.pylint
    },
})

Steps to reproduce

Open a sample python file with any obvious error such as

banana = food

Make a change to the file that will trigger textDocument/didChange, but do not yet save the file
Pylint is very slow, wait for the error to appear.
Save, triggering textDocument/didSave
Observe that now no errors are displayed.

Expected behavior

On save I expect that previously reported errors would not disappear when the document has not otherwise changed.

Actual behavior

All reported errors disappear.

This appears to be due to the the result of NULL_LS_DIAGNOSTICS_ON_SAVE triggering with no configured generators.

received LSP notification for method textDocument/didSave
running generators for method NULL_LS_DIAGNOSTICS_ON_SAVE
no generators available
received diagnostics from generators
{}

Furthermore, even though errors on document change are working correctly, if you write right after a change null-ls rejects the diagnostics generated by the change event (buffer changed; ignoring received diagnostics) even though the contents have not actually changed between the change event and save event.

Debug log

[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: received LSP notification for method textDocument/didChange
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: running generators for method NULL_LS_DIAGNOSTICS
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: spawning command pylint with args:
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: { "--from-stdin", "/Users/kremlan/workspace/Portal-Site/portal/controllers/overview.py", "-f", "json" }
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: received LSP notification for method textDocument/didSave
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: running generators for method NULL_LS_DIAGNOSTICS_ON_SAVE
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: no generators available
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: received diagnostics from generators
[DEBUG Mon Nov 15 10:00:32 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: {}
[DEBUG Mon Nov 15 10:00:35 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: error output: nil
[DEBUG Mon Nov 15 10:00:35 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: output: [
    {
        "type": "convention",
        "module": "portal.controllers.overview",
        "obj": "",
        "line": 10,
        "column": 0,
        "path": "portal/controllers/overview.py",
        "symbol": "wrong-import-order",
        "message": "third party import \"from sqlalchemy import and_, or_\" should be placed before \"from paste.deploy.converters import asbool\"",
        "message-id": "C0411"
    }
]

[DEBUG Mon Nov 15 10:00:35 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: received diagnostics from generators
[DEBUG Mon Nov 15 10:00:35 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: { {
    code = "C0411",
    col = 1,
    message = 'third party import "from sqlalchemy import and_, or_" should be placed before "from paste.deploy.converters import asbool"',
    range = {
      end = {
        character = 0,
        line = 10
      },
      start = {
        character = 0,
        line = 9
      }
    },
    row = 10,
    severity = 3,
    source = "pylint"
  } }
[DEBUG Mon Nov 15 10:00:35 2021] .../.config/nvim/plugged/null-ls.nvim/lua/null-ls/utils.lua:74: buffer changed; ignoring received diagnostics

Help

Yes, but I don't know how to start. I would need guidance

Implementation help

I'm happy to help improve either null-ls or the pylint builtin. I see several approaches:

Configure pylint to activate on save rather than change. Pylint takes so long that this is my previous workflow for how to work with it anyway (via ALE).
Configure pylint to provide generators for both change and save.
Improve null-ls to not wipe out existing diagnostics when no generator is configured for an event.
Improve null-ls to prevent a save event from counting as a document change, preventing other events from helping.

jose-elias-alvarez commented 2 years ago

Thanks for the detailed report! The fundamental issue (and the reason running linters on save is not supported on 0.5.1) is that the LSP API doesn't give us a good way to publish 2 sets of diagnostics separately, so they wipe each other out. I think the easiest fix is to simply disable the notifications completely on 0.5.1, which is what I did in 765613c5a7499d92d6cf7aac6ccd7e4946bd5ec8. Could you update and let me know if that fixes your issue?

Your issue also made me realize that we didn't have an easy way to override a built-in's default method, so I added that in 2143761a0a3991308a482d7d5624ce9c9cef93b7. There's an example there for how to configure pylint to run only on save, so you can try that out once 0.6 releases (or earlier if you build from source).

bwells commented 2 years ago

Thanks so much for such a quick fix and for null-ls in general. I can confirm that 765613c resolves the issue for me.

jose-elias-alvarez commented 2 years ago

Great! You're also right that there is a potential timing issue with changedtick tracking, but it shouldn't cause problems on 0.5.1 for now, so I'll solve that separately.

jose-elias-alvarez / null-ls.nvim