Can't override flake8 errors to warnings

[levouh]

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

NVIM v0.7.0-dev+812-g2f31e7b88

null-ls config

See below.

Steps to reproduce

Per 742739910ef5ef93efcaed32a5b875d7833b42af, warnings printed by the builtin flake8 diagnostics are highlighted as error even when overridden to be warning.

cat <<EOF >> /tmp/borked.lua
vim.cmd([[
  packadd nvim-lspconfig
  packadd plenary.nvim
  packadd null-ls.nvim
]])

-- No change, just comparing to another source from a different server
require("lspconfig").pyright.setup({})

local n = require("null-ls")
local h = require("null-ls.helpers")

n.setup({
  sources = {
    n.builtins.diagnostics.flake8.with({
      on_output = h.diagnostics.from_pattern(
        [[:(%d+):(%d+): ((%u)%w+) (.*)]],
        { "row", "col", "code", "severity", "message" },
        {
          severities = {
            E = h.diagnostics.severities["warning"], -- Changed to warning!
            W = h.diagnostics.severities["warning"],
            F = h.diagnostics.severities["information"],
            D = h.diagnostics.severities["information"],
            R = h.diagnostics.severities["warning"],
            S = h.diagnostics.severities["warning"],
            I = h.diagnostics.severities["warning"],
            C = h.diagnostics.severities["warning"],
          },
        }
      ),
    }),
  },
})
EOF

mkdir -p /tmp/venvs
python -m venv /tmp/venvs/borked
source /tmp/venvs/borked
python -m pip install flake8
cat <<EOF >> /tmp/borked.py
import os

def foo():
    a=1
EOF

nvim --noplugin -u /tmp/borked.lua /tmp/borked.py

Expected behavior

Overrides should show errors as warnings.

Actual behavior

Errors are shows as errors, not respecting overrides.

Debug log

Empty, even with vim.lsp.set_log_level("debug") so I assume I'm missing something.

Help

Yes

Implementation help

Looking at the code right now but will have to hop off soon, will continue in the morning. The issue comes from this section of the code having entries["severity"] equal to 1 instead of 2 as would be expected.

---

I also confirmed that here the default and overridden severity values are the same (with a print situated just before the tbl_extend call):

Working:

[user] 3: {
[user]   C = 2,
[user]   D = 3,
[user]   E = 1,
[user]   F = 3,
[user]   I = 2,
[user]   R = 2,
[user]   S = 2,
[user]   W = 2
[user] }
[user] 3: {
[user]   C = 2,
[user]   D = 3,
[user]   E = 2,
[user]   F = 3,
[user]   I = 2,
[user]   R = 2,
[user]   S = 2,
[user]   W = 2
[user] }

Borked

[user] 3: {
[user]   C = 2,
[user]   D = 3,
[user]   E = 1,
[user]   F = 3,
[user]   I = 2,
[user]   R = 2,
[user]   S = 2,
[user]   W = 2
[user] }
[user] 3: {
[user]   C = 2,
[user]   D = 3,
[user]   E = 2,
[user]   F = 3,
[user]   I = 2,
[user]   R = 2,
[user]   S = 2,
[user]   W = 2
[user] }

---

More print debugging (yay!) shows that:

Working

[user] 1: E
[user] 2: 2

Borked

[user] 1: E
[user] 2: 1

with 1 being here and 2 being here, both printing the value of entires["severity"].

---

So, as makes sense, it seems that the configuration is being loaded correctly, but just the overriding is not working correctly?

[levouh]

Need to log off, will keep looking at this in the morning and submit a PR if I find the issue before you do @jose-elias-alvarez (assuming you even have time to look :wink:).

[jose-elias-alvarez]

I think the issue is as of the commit you linked, overriding on_output is no longer valid. I would like to think of a better way for users to pass options to on_output to avoid having to override it completely, but I suspect it would require significantly restructuring built-ins. I've re-added the ability to override on_output in d394841cdb3b3ea7dc5f68e597bdf1c5ff48ec78, so could you give that a shot?

[levouh]

Works as expected.

What are the concerns for the effect to overriding some parts of on_output but not all of them? Naively I assume it would just be a tbl_extend call, but I imagine there is some ephemeral state that would make these overrides not permanent?

Asking as I am happy to work on it and submit a PR, but just want to have the right foresight beforehand.

[jose-elias-alvarez]

on_output is a function, so I'm not sure if we can override parts of it without overriding the whole thing. There are a few solutions I can think of:

1. Add another option, something like on_output_opts, and make that available via params on run. This is dirty and would require some refactoring but is the easiest.
2. Refactor built-ins as proper classes so that any method can access the built-in's options via self. This has lots of benefits and is the overall cleanest solution but is complicated and might require breaking changes.
3. Refactor the diagnostic helpers to return an on_output table with a __call metamethod. That would let us assign options to the table and handle them when on_output is called. This isn't that hard in terms of implementation but also wouldn't handle sources that don't use the diagnostic helpers.

Happy to provide my thoughts / review PRs for any of the above approaches (or anything else you can think of).

[jose-elias-alvarez]

This diff hopefully illustrates what I had in mind for 3:

diff --git a/lua/null-ls/helpers/diagnostics.lua b/lua/null-ls/helpers/diagnostics.lua
index c06f754..88eb5e1 100644
--- a/lua/null-ls/helpers/diagnostics.lua
+++ b/lua/null-ls/helpers/diagnostics.lua
@@ -95,16 +95,24 @@ local from_pattern = function(pattern, groups, overrides)
     local offsets = overrides.offsets or {}
     local attr_adapters = make_attr_adapters(severities, overrides.adapters or {})

-    return function(line, params)
-        local results = { line:match(pattern) }
-        local entries = {}
+    return setmetatable({}, {
+        __call = function(t, line, params)
+            -- this is dumb and doesn't handle other keys, but you hopefully get the idea
+            if t.opts and t.opts.severities then
+                severities = vim.tbl_extend("force", severities, t.opts.severities)
+                attr_adapters = make_attr_adapters(severities, overrides.adapters or {})
+            end

-        for i, match in ipairs(results) do
-            entries[groups[i]] = match
-        end
+            local results = { line:match(pattern) }
+            local entries = {}

-        return make_diagnostic(entries, defaults, attr_adapters, params, offsets)
-    end
+            for i, match in ipairs(results) do
+                entries[groups[i]] = match
+            end
+
+            return make_diagnostic(entries, defaults, attr_adapters, params, offsets)
+        end,
+    })
 end

 --- Parse a linter's output using a errorformat
diff --git a/lua/null-ls/helpers/generator_factory.lua b/lua/null-ls/helpers/generator_factory.lua
index fc6badc..1a5de64 100644
--- a/lua/null-ls/helpers/generator_factory.lua
+++ b/lua/null-ls/helpers/generator_factory.lua
@@ -140,7 +140,13 @@ return function(opts)
                 "table",
                 true,
             },
-            on_output = { on_output, "function" },
+            on_output = {
+                on_output,
+                function(v)
+                    return type(v) == "function" or type(v) == "table" and type(getmetatable(v).__call) == "function"
+                end,
+                "function or callable table",
+            },
             format = {
                 format,
                 function(a)
diff --git a/lua/null-ls/helpers/make_builtin.lua b/lua/null-ls/helpers/make_builtin.lua
index 4a3153f..44923a3 100644
--- a/lua/null-ls/helpers/make_builtin.lua
+++ b/lua/null-ls/helpers/make_builtin.lua
@@ -32,6 +32,10 @@ local function make_builtin(opts)
         on_output = opts.on_output,
     })

+    if opts.on_output_opts and type(generator_opts.on_output) == "table" then
+        generator_opts.on_output.opts = opts.on_output_opts
+    end
+
     local builtin = {
         method = method,
         filetypes = filetypes,

This same pattern could be used in generator_factory to inject on_output_opts into params. That's potentially a decent halfway point.

[levouh]

Awesome, I will take a look at a PR here soon unless someone else gets to it first. Will post when I start.

[fragov]

As I understand, there is no way now to change severity from error to warning (i.e., for cspell)?

[jose-elias-alvarez]

@fragov That's correct. I mentioned this in #494, but sources like cspell that don't specify severities fall back to a default severity that's currently hard-coded to error. I could add a way to configure the fallback severity as a stopgap until we add the ability to configure severity for each linter.

[wookayin]

ALE supports some diagnostics level mapping as follows (this is quite generic and not specific to linter source). Why don't null-ls.nvim allow a similar feature of remapping diagnostics level? May I know what is blocking implement this, if any?

let g:ale_type_map = {
      \ 'pycodestyle' : {'ES' : 'WS', 'E': 'W'},
      \ 'flake8' : {'E', 'E': 'W'}
      \}

[jose-elias-alvarez]

@wookayin Using a global config variable is technically possible, but it's not a good fit for the null-ls architecture / user expectations. We could actually achieve something similar with a simple change:

diff --git a/lua/null-ls/diagnostics.lua b/lua/null-ls/diagnostics.lua
index 0646089..7830c0c 100644
--- a/lua/null-ls/diagnostics.lua
+++ b/lua/null-ls/diagnostics.lua
@@ -50,7 +50,10 @@ local postprocess = function(diagnostic, _, generator)
     diagnostic.end_lnum = range["end"].line
     diagnostic.col = range["start"].character
     diagnostic.end_col = range["end"].character
+
     diagnostic.severity = diagnostic.severity or c.get().fallback_severity
+    diagnostic.severity = generator and generator.opts.severities and generator.opts.severities[diagnostic.severity]
+        or diagnostic.severity

     diagnostic.source = diagnostic.source or generator.opts.name or generator.opts.command or "null-ls"
     if diagnostic.filename and not diagnostic.bufnr then
diff --git a/lua/null-ls/helpers/make_builtin.lua b/lua/null-ls/helpers/make_builtin.lua
index 2136ff7..5349c77 100644
--- a/lua/null-ls/helpers/make_builtin.lua
+++ b/lua/null-ls/helpers/make_builtin.lua
@@ -33,6 +33,7 @@ local function make_builtin(opts)
         dynamic_command = opts.dynamic_command,
         runtime_condition = opts.runtime_condition,
         timeout = opts.timeout,
+        severities = opts.severities,
         -- this isn't ideal, but since we don't have a way to modify on_output's behavior,
         -- it's better than nothing
         on_output = opts.on_output,

This would let users override severities by doing the following:

local sources = {
    null_ls.builtins.diagnostics.selene.with({
        severities = {
            [vim.diagnostic.severity.ERROR] = vim.diagnostic.severity.INFO,
        },
    }),
}

This doesn't fully handle the case in the OP, though, since that requires modifying how the built-in handles flake8 output. Something like this would probably be the best solution until built-ins are rewritten to give on_output() access to the underlying options, which is a major and likely breaking change.

[wookayin]

Of course, I didn't mean the use of global variables but some source-specific configs (using .with) -- just like in your proposed snippet. Overriding on_output is most powerful method to deal, but personally I find this configuration quite overly complicated. I probably don't understand the gist of why simple severity mapping is not easy --- maybe it's already mapped to vim.diagnostic.severity.ERROR while parsing --- but if that's the case, this is something that could be improved, not limited to flake8 but for the sake of other sources as well.

[jose-elias-alvarez]

If you look at the flake8 built-in, you can see that the map of flake8 severities to Neovim severities is hardcoded in on_output(). Since on_output() doesn't have access to the built-in or its options, there's no simple way for users to override the map (though you can imagine how a global variable could technically work).

Ideally, whatever method we use would not only allow overriding severities but also allow passing other options that on_output() can use. If we rewrote built-ins into an OO style so that each method had access to its parent built-in, we could solve this, but as I said that's a big change. I'm definitely open to other solutions, too.

[jose-elias-alvarez]

51f336638100ba1462e9c1fb0831464e44d02aef added a diagnostics_postprocess hook that (if I understand the original issue correctly) handles this specific case, since you can override any error diagnostic to a warning.

This isn't a general solution, though, since we lose the original severity, so if a user wanted to set F diagnostics as warnings, they wouldn't be able to distinguish between F and D diagnostics (a potentially nonsensical example but hopefully it makes sense). One solution might be to store the original diagnostic on the processed diagnostic object, which would let users check that and assign the severity accordingly.

[jose-elias-alvarez]

Closing this as diagnostics_postprocess is going to be our best solution until per-source configuration is possible, which will require a larger rewrite.