From e7ab186ee9aeafc29f1f41647564edd3defe9ab9 Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Sun, 15 Sep 2024 06:23:36 -0700 Subject: [PATCH 1/5] Add `by_bufroot` cache I'm building a [`nix fmt`](https://nix.dev/manual/nix/latest/command-ref/new-cli/nix3-fmt) formatter. The tricky thing about `nix fmt` is that for nontrivial projects, the entrypoint (`nix fmt`) can be slow enough to be a bad experience to run on every save, and can even trigger [neovim's lsp timeout](https://github.com/nvimtools/none-ls.nvim?tab=readme-ov-file#i-am-seeing-a-formatting-timeout-error-message). It's not the that underlying formatter it invokes are slow, it's that it can take 1-2 seconds for nix to evaluate the `flake.nix` and determine what command `nix fmt` should run under the hood. Since the underlying command shouldn't change very often for a given project, I feel like it would be reasonable to cache this result per project, hence this new `by_bufroot` callback. --- doc/HELPERS.md | 17 +++++++++++++++++ lua/null-ls/helpers/cache.lua | 21 +++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/doc/HELPERS.md b/doc/HELPERS.md index 8ccfbe83..2aa72c99 100644 --- a/doc/HELPERS.md +++ b/doc/HELPERS.md @@ -376,3 +376,20 @@ be too performance-intensive to include out-of-the-box. Note that if `callback` returns `nil`, the helper will override the return value and instead cache `false` (so that it can determine that it already ran `callback` once and should not run it again). + +### by_bufroot(callback) + +Creates a function that caches the result of `callback`, indexed by `root`. On +the first run of the created function, null-ls will call `callback` with a +`params` table. On the next run, it will directly return the cached value +without calling `callback` again. + +This is useful when the return value of `callback` is not expected to change +over the lifetime of the buffer, which works well for `cwd` and +`runtime_condition` callbacks. Users can use it as a simple shortcut to improve +performance, and built-in authors can use it to add logic that would otherwise +be too performance-intensive to include out-of-the-box. + +Note that if `callback` returns `nil`, the helper will override the return value +and instead cache `false` (so that it can determine that it already ran +`callback` once and should not run it again). diff --git a/lua/null-ls/helpers/cache.lua b/lua/null-ls/helpers/cache.lua index 89a48a82..9795e440 100644 --- a/lua/null-ls/helpers/cache.lua +++ b/lua/null-ls/helpers/cache.lua @@ -25,6 +25,27 @@ M.by_bufnr = function(cb) end end +--- creates a function that caches the output of a callback, indexed by project root +---@param cb function +---@return fun(params: NullLsParams): any +M.by_bufroot = function(cb) + -- assign next available key, since we just want to avoid collisions + local key = next_key + M.cache[key] = {} + next_key = next_key + 1 + + return function(params) + local root = params.root + -- if we haven't cached a value yet, get it from cb + if M.cache[key][root] == nil then + -- make sure we always store a value so we know we've already called cb + M.cache[key][root] = cb(params) or false + end + + return M.cache[key][root] + end +end + M._reset = function() M.cache = {} end From 2096a580b39a5c7778f405c0ec472886a8ae2e8f Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Sun, 15 Sep 2024 08:18:40 -0700 Subject: [PATCH 2/5] Add "nix flake fmt" formatter This uses `nix fmt` under the hood. I introduced some caching make it performant, as `nix fmt` can take a little while to evaluate all your nix code to figure out the formatter entrypoint. --- doc/HELPERS.md | 2 +- .../builtins/formatting/nix_flake_fmt.lua | 154 ++++++++++++++++++ 2 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 lua/null-ls/builtins/formatting/nix_flake_fmt.lua diff --git a/doc/HELPERS.md b/doc/HELPERS.md index 2aa72c99..4e218305 100644 --- a/doc/HELPERS.md +++ b/doc/HELPERS.md @@ -169,7 +169,7 @@ Not compatible with `ignore_stderr`. Reads the contents of the temp file created by `to_temp_file` after running `command` and assigns it to `params.output`. Useful for formatters that don't -output to `stdin` (see `formatter_factory`). +output to `stdout` (see `formatter_factory`). This option depends on `to_temp_file`. diff --git a/lua/null-ls/builtins/formatting/nix_flake_fmt.lua b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua new file mode 100644 index 00000000..1bf80ed8 --- /dev/null +++ b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua @@ -0,0 +1,154 @@ +local h = require("null-ls.helpers") +local methods = require("null-ls.methods") +local log = require("null-ls.logger") + +local FORMATTING = methods.internal.FORMATTING + +--- Return the command that `nix fmt` would run, or nil if we're not in a +--- flake. +--- +--- The formatter must follow treefmt's [formatter +--- spec](https://github.com/numtide/treefmt/blob/main/docs/formatter-spec.md). +--- +--- This basically re-implements the "entrypoint discovery" that `nix fmt` does. +--- So why are we doing this ourselves rather than just invoking `nix fmt`? +--- Unfortunately, it can take a few moments to evaluate all your nix code to +--- figure out the formatter entrypoint. It can even be slow enough to exceed +--- Neovim's default LSP timeout. +--- By doing this ourselves, we can cache the result. +--- +---@return string|nil +local find_nix_fmt = function(params) + -- Discovering currentSystem here lets us keep the *next* eval pure. + -- We want to keep that part pure as a performance improvement: an impure + -- eval that references the flake would copy *all* files (including + -- gitignored files!), which can be quite expensive if you've got many GiB + -- of artifacts in the directory. This optimization can probably go away + -- once the [Lazy trees PR] lands. + -- + -- [Lazy trees PR]: https://github.com/NixOS/nix/pull/6530 + local cp = vim.system({ "nix", "--extra-experimental-features", "nix-command", "config", "show", "system" }):wait() + if cp.code ~= 0 then + log:warn(string.format("unable to discover builtins.currentSystem from nix. stderr: %s", cp.stderr)) + return nil + end + local nix_current_system = cp.stdout:match("^(.-)%s+") -- remove trailing newline + + local eval_nix_formatter = [[ + let + currentSystem = "]] .. nix_current_system .. [["; + # Various functions vendored from nixpkgs lib (to avoid adding a + # dependency on nixpkgs). + lib = rec { + getOutput = output: pkg: + if ! pkg ? outputSpecified || ! pkg.outputSpecified + then pkg.${output} or pkg.out or pkg + else pkg; + getBin = getOutput "bin"; + # Simplified by removing various type assertions. + getExe' = x: y: "${getBin x}/bin/${y}"; + # getExe is simplified to assume meta.mainProgram is specified. + getExe = x: getExe' x x.meta.mainProgram; + }; + in + formatterBySystem: + if formatterBySystem ? ${currentSystem} then + let + formatter = formatterBySystem.${currentSystem}; + drv = formatter.drvPath; + bin = lib.getExe formatter; + in + drv + "\n" + bin + "\n" + else + "" + ]] + + cp = vim.system({ + "nix", + "--extra-experimental-features", + "nix-command flakes", + "eval", + ".#formatter", + "--raw", + "--apply", + eval_nix_formatter, + }, { cwd = params.root }):wait() + if cp.code ~= 0 then + -- Dirty hack to check if the flake actually defines a formatter. + -- + -- I cannot for the *life* of me figure out a less hacky way of + -- checking if a flake defines a formatter. Things I've tried: + -- + -- - `nix eval . --apply '...'`: This doesn't not give me the flake + -- itself, it gives me the default package. + -- - `builtins.getFlake`: Every incantation I've tried requires + -- `--impure`, which has the performance downside described above. + -- - `nix flake show --json .`: This works, but it can be quite slow: + -- we end up evaluating all outputs, which can take a while for + -- `nixosConfigurations`. + if cp.stderr:find("error: flake .+ does not provide attribute .+ or 'formatter'") then + log:warn("this flake does not define a `nix fmt` entrypoint") + else + log:error(string.format("unable discover 'nix fmt' command. stderr: %s", cp.stderr)) + end + + return nil + end + + if cp.stdout == "" then + log:warn("this flake does not define a formatter for your system: %s", nix_current_system) + return nil + end + + -- stdout has 2 lines of output: + -- 1. drv path + -- 2. exe path + local drv_path, nix_fmt_path = cp.stdout:match("([^\n]+)\n([^\n]+)\n") + + -- Build the derivation. This ensures that `nix_fmt_path` exists. + cp = vim.system({ "nix", "--extra-experimental-features", "nix-command", "build", "--no-link", drv_path .. "^out" }) + :wait() + if cp.code ~= 0 then + log:warn(string.format("unable to build 'nix fmt' entrypoint. stderr: %s", cp.stderr)) + return nil + end + + return nix_fmt_path +end + +return h.make_builtin({ + name = "nix flake fmt", + meta = { + url = "https://nix.dev/manual/nix/latest/command-ref/new-cli/nix3-fmt", + description = "`nix fmt` - reformat your code in the standard style (this is a generic formatter, not to be confused with nixfmt, a formatter for .nix files)", + }, + method = FORMATTING, + filetypes = {}, + generator_opts = { + -- It can take a few moments to find the `nix fmt` entrypoint. The + -- underlying command shouldn't change very often for a given + -- project, so cache it for the project root. + dynamic_command = h.cache.by_bufroot(find_nix_fmt), + args = { + -- `--walk` is specific to treefmt, and this formatter is supposed + -- to be generic. + -- Note: this could get converted to the new `TREEFMT_WALK` + -- environment variable once + -- + -- is fixed, which would at least play nicely with other types of + -- `nix fmt` entrypoints. + -- + -- However, IMO, the real fix is to change treefmt itself to be + -- willing to format files passed explicitly, even if they're + -- gitignored: + -- https://github.com/numtide/treefmt/issues/435 + "--walk=filesystem", + "$FILENAME", + }, + to_temp_file = true, + }, + condition = function(utils) + return utils.root_has_file("flake.nix") + end, + factory = h.formatter_factory, +}) From ff1abef2662d00e2f8792b43c1bf37487e845067 Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Sun, 13 Oct 2024 13:24:13 -0500 Subject: [PATCH 3/5] Clarify comment --- lua/null-ls/builtins/formatting/nix_flake_fmt.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lua/null-ls/builtins/formatting/nix_flake_fmt.lua b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua index 1bf80ed8..3a99ce8e 100644 --- a/lua/null-ls/builtins/formatting/nix_flake_fmt.lua +++ b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua @@ -5,7 +5,7 @@ local log = require("null-ls.logger") local FORMATTING = methods.internal.FORMATTING --- Return the command that `nix fmt` would run, or nil if we're not in a ---- flake. +--- flake with a formatter, or if we fail to discover the formatter somehow. --- --- The formatter must follow treefmt's [formatter --- spec](https://github.com/numtide/treefmt/blob/main/docs/formatter-spec.md). From 0ba235a1c7b07e29d4b39ddb60fa94694019595d Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Sun, 13 Oct 2024 13:24:28 -0500 Subject: [PATCH 4/5] Store a gcroot for the build This protects us against garbage collection so long as vim is running. --- lua/null-ls/builtins/formatting/nix_flake_fmt.lua | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lua/null-ls/builtins/formatting/nix_flake_fmt.lua b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua index 3a99ce8e..e475b629 100644 --- a/lua/null-ls/builtins/formatting/nix_flake_fmt.lua +++ b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua @@ -106,8 +106,15 @@ local find_nix_fmt = function(params) local drv_path, nix_fmt_path = cp.stdout:match("([^\n]+)\n([^\n]+)\n") -- Build the derivation. This ensures that `nix_fmt_path` exists. - cp = vim.system({ "nix", "--extra-experimental-features", "nix-command", "build", "--no-link", drv_path .. "^out" }) - :wait() + cp = vim.system({ + "nix", + "--extra-experimental-features", + "nix-command", + "build", + "--out-link", + vim.fn.tempname(), + drv_path .. "^out", + }):wait() if cp.code ~= 0 then log:warn(string.format("unable to build 'nix fmt' entrypoint. stderr: %s", cp.stderr)) return nil From 328131d576bd61112711a2607ba9a150404d3deb Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Sun, 13 Oct 2024 13:24:55 -0500 Subject: [PATCH 5/5] Update comment about treefmt blocker --- lua/null-ls/builtins/formatting/nix_flake_fmt.lua | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/lua/null-ls/builtins/formatting/nix_flake_fmt.lua b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua index e475b629..dd8dc51a 100644 --- a/lua/null-ls/builtins/formatting/nix_flake_fmt.lua +++ b/lua/null-ls/builtins/formatting/nix_flake_fmt.lua @@ -137,18 +137,8 @@ return h.make_builtin({ -- project, so cache it for the project root. dynamic_command = h.cache.by_bufroot(find_nix_fmt), args = { - -- `--walk` is specific to treefmt, and this formatter is supposed - -- to be generic. - -- Note: this could get converted to the new `TREEFMT_WALK` - -- environment variable once - -- - -- is fixed, which would at least play nicely with other types of - -- `nix fmt` entrypoints. - -- - -- However, IMO, the real fix is to change treefmt itself to be - -- willing to format files passed explicitly, even if they're - -- gitignored: - -- https://github.com/numtide/treefmt/issues/435 + -- Needed until is + -- fixed, and a version of treefmt is released with the fix. "--walk=filesystem", "$FILENAME", },