helpers: make dynamic_command async (#197)

Author: jfly

doc/HELPERS.md

@@ -105,10 +105,11 @@ directory defaults to the project's root.
 
 ### dynamic_command
 
-Optional callback to set `command` dynamically. Takes one arguments, a `params`
-table. The generator's original command (if set) is available as
-`params.command`. The callback should return a string containing the command to
-run or `nil`, meaning that no command should run.
+Optional callback to set `command` dynamically. Takes two arguments, a `params`
+table and a `done` callback. The generator's original command (if set) is
+available as `params.command`. The `done` callback should be invoked with a
+string containing the command to run or `nil`, meaning that no command should
+run.
 
 `dynamic_command` runs every time its parent generator runs and can affect
 performance, so it's best to cache its output when possible.
@@ -362,17 +363,25 @@ Helpers used to cache output from callbacks and help improve performance.
 
 ### by_bufnr(callback)
 
-Creates a function that caches the result of `callback`, indexed by `bufnr`. 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.
+Creates a function that caches the result of `callback`, indexed by `bufnr`.
+`callback` is a function that takes one argument: a `params` table.
 
-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.
+On the first run of the created function, null-ls will invoke `callback`. On the
+next run, it will directly return the cached value without calling `callback`
+again.
 
-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).
+This is useful when the result 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` resolved to `nil`, the helper will instead cache `false`
+(so that it can determine that it already ran `callback` once and should not run
+it again).
+
+### by_bufnr_async(callback)
+
+Like `by_bufnr`, but `callback` is an async function. That is, `callback` is a
+function that takes two arguments: a `params` table and a `done` callback that
+must be invoked with the result.

lua/null-ls/helpers/cache.lua

@@ -33,4 +33,32 @@ M.by_bufnr = function(cb)
     end
 end
 
+--- creates a function that caches the output of an async callback, indexed by bufnr
+---@param cb function
+---@return fun(params: NullLsParams): any
+M.by_bufnr_async = 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, done)
+        local bufnr = params.bufnr
+        -- if we haven't cached a value yet, get it from cb
+        if M.cache[key][bufnr] == nil then
+            -- make sure we always store a value so we know we've already called cb
+            cb(params, function(result)
+                M.cache[key][bufnr] = result or false
+                done(M.cache[key][bufnr])
+            end)
+        else
+            done(M.cache[key][bufnr])
+        end
+    end
+end
+
+M._reset = function()
+    M.cache = {}
+end
+
 return M

lua/null-ls/helpers/command_resolver.lua

@@ -31,23 +31,26 @@ end
 M.generic = function(prefix)
     ---@param params NullLsParams
     ---@return string|nil
-    return cache.by_bufnr(function(params)
+    return cache.by_bufnr_async(function(params, done)
         local executable_to_find = prefix and u.path.join(prefix, params.command) or params.command
         if not executable_to_find then
+            done(nil)
             return
         end
 
         local stop_path = u.get_vcs_root() or u.get_root()
         local resolved_executable = search_ancestors_for_executable(params.bufname, stop_path, executable_to_find)
-        return resolved_executable
+        done(resolved_executable)
     end)
 end
 
 --- creates a resolver that searches for a local node_modules executable and falls back to a global executable
 M.from_node_modules = function()
     local node_modules_resolver = M.generic(u.path.join("node_modules", ".bin"))
-    return function(params)
-        return node_modules_resolver(params) or params.command
+    return function(params, done)
+        node_modules_resolver(params, function(resolved)
+            done(resolved or params.command)
+        end)
     end
 end
 

lua/null-ls/helpers/generator_factory.lua

@@ -133,6 +133,12 @@ return function(opts)
         end
     end
 
+    if dynamic_command == nil then
+        dynamic_command = function(params, done)
+            done(params.command)
+        end
+    end
+
     local is_nil_table_or_func = function(v)
         return v == nil or vim.tbl_contains({ "function", "table" }, type(v))
     end
@@ -279,65 +285,60 @@ return function(opts)
 
             params.command = command
 
-            local resolved_command
-            if dynamic_command then
-                resolved_command = dynamic_command(params)
-            else
-                resolved_command = command
-            end
-
-            -- if dynamic_command returns nil, don't fall back to command
-            if not resolved_command then
-                log:debug(string.format("unable to resolve command %s; aborting", command))
-                return done()
-            end
+            dynamic_command(params, function(resolved_command)
+                -- if dynamic_command returns nil, don't fall back to command
+                if not resolved_command then
+                    log:debug(string.format("unable to resolve command %s; aborting", command))
+                    return done()
+                end
 
-            local resolved_cwd = cwd and cwd(params) or root
-            params.cwd = resolved_cwd
+                local resolved_cwd = cwd and cwd(params) or root
+                params.cwd = resolved_cwd
 
-            if type(env) == "function" then
-                env = env(params)
-            end
+                if type(env) == "function" then
+                    env = env(params)
+                end
 
-            local spawn_opts = {
-                cwd = resolved_cwd,
-                input = to_stdin and get_content(params) or nil,
-                handler = wrapper,
-                check_exit_code = check_exit_code,
-                timeout = timeout or c.get().default_timeout,
-                env = env,
-            }
-
-            if to_temp_file then
-                local content = get_content(params)
-                local temp_path, cleanup = loop.temp_file(content, params.bufname, temp_dir or c.get().temp_dir)
-
-                spawn_opts.on_stdout_end = function()
-                    if from_temp_file then
-                        params.output = loop.read_file(temp_path)
+                local spawn_opts = {
+                    cwd = resolved_cwd,
+                    input = to_stdin and get_content(params) or nil,
+                    handler = wrapper,
+                    check_exit_code = check_exit_code,
+                    timeout = timeout or c.get().default_timeout,
+                    env = env,
+                }
+
+                if to_temp_file then
+                    local content = get_content(params)
+                    local temp_path, cleanup = loop.temp_file(content, params.bufname, temp_dir or c.get().temp_dir)
+
+                    spawn_opts.on_stdout_end = function()
+                        if from_temp_file then
+                            params.output = loop.read_file(temp_path)
+                        end
+                        cleanup()
                     end
-                    cleanup()
+                    params.temp_path = temp_path
                 end
-                params.temp_path = temp_path
-            end
-
-            local resolved_args = args or {}
-            resolved_args = type(resolved_args) == "function" and resolved_args(params) or resolved_args
-            resolved_args = parse_args(resolved_args, params)
-
-            opts._last_command = resolved_command
-            opts._last_args = resolved_args
-            opts._last_cwd = resolved_cwd
 
-            log:debug(
-                string.format(
-                    "spawning command %s at %s with args %s",
-                    vim.inspect(resolved_command),
-                    resolved_cwd,
-                    vim.inspect(resolved_args)
+                local resolved_args = args or {}
+                resolved_args = type(resolved_args) == "function" and resolved_args(params) or resolved_args
+                resolved_args = parse_args(resolved_args, params)
+
+                opts._last_command = resolved_command
+                opts._last_args = resolved_args
+                opts._last_cwd = resolved_cwd
+
+                log:debug(
+                    string.format(
+                        "spawning command %s at %s with args %s",
+                        vim.inspect(resolved_command),
+                        resolved_cwd,
+                        vim.inspect(resolved_args)
+                    )
                 )
-            )
-            loop.spawn(resolved_command, resolved_args, spawn_opts)
+                loop.spawn(resolved_command, resolved_args, spawn_opts)
+            end)
         end,
         filetypes = opts.filetypes,
         opts = opts,

lua/null-ls/helpers/make_builtin.lua

@@ -98,9 +98,11 @@ local function make_builtin(opts)
         local prefix = type(maybe_prefix) == "string" and maybe_prefix or nil
         local resolver = cmd_resolver.generic(prefix)
 
-        generator_opts.dynamic_command = function(params)
-            local resolved_command = resolver(params) or (prefer_local and params.command)
-            return resolved_command
+        generator_opts.dynamic_command = function(params, done)
+            resolver(params, function(val)
+                local resolved_command = val or (prefer_local and params.command)
+                done(resolved_command)
+            end)
         end
     end
 

test/spec/client_spec.lua

@@ -297,6 +297,18 @@ describe("client", function()
             c._set({ on_attach = on_attach })
         end)
 
+        local api
+
+        -- set up setup_buffer conditions
+        before_each(function()
+            api = mock(vim.api, true)
+            api.nvim_get_option_value.returns("")
+            sources.get.returns({})
+        end)
+        after_each(function()
+            mock.revert(api)
+        end)
+
         it("should do nothing if no client", function()
             client.setup_buffer(mock_bufnr)
 

test/spec/helpers/cache_spec.lua

@@ -1,5 +1,22 @@
 local stub = require("luassert.stub")
 
+function call_sync(async_fn, arg)
+    local co = coroutine.running()
+    assert(co, "not running inside a coroutine")
+
+    local val = nil
+    async_fn(
+        arg,
+        vim.schedule_wrap(function(result)
+            val = result
+            coroutine.resume(co)
+        end)
+    )
+    coroutine.yield()
+
+    return val
+end
+
 describe("cache", function()
     local cache = require("null-ls.helpers").cache
     after_each(function()
@@ -80,4 +97,90 @@ describe("cache", function()
             assert.stub(mock_cb).was_called(2)
         end)
     end)
+
+    describe("by_bufnr_async", function()
+        local mock_params = { bufnr = 1 }
+        local mock_val = "mock_val"
+        local invoked_params = {}
+        local mock_cb = function(params, done)
+            vim.defer_fn(function()
+                table.insert(invoked_params, params)
+                done(mock_val)
+            end, 0)
+        end
+        after_each(function()
+            mock_val = "mock_val"
+            invoked_params = {}
+        end)
+
+        it("should call cb with params", function()
+            local fn = cache.by_bufnr_async(mock_cb)
+
+            call_sync(fn, mock_params)
+
+            assert.are.same({ mock_params }, invoked_params)
+        end)
+
+        it("should return cb return value", function()
+            local fn = cache.by_bufnr_async(mock_cb)
+
+            local val = call_sync(fn, mock_params)
+
+            assert.equals("mock_val", val)
+        end)
+
+        it("should return false if cb returns nil", function()
+            mock_val = nil
+            local fn = cache.by_bufnr_async(mock_cb)
+
+            local val = call_sync(fn, mock_params)
+
+            assert.equals(false, val)
+        end)
+
+        it("should return cached value", function()
+            local co = coroutine.running()
+            assert(co, "not running inside a coroutine")
+
+            local fn = cache.by_bufnr_async(mock_cb)
+
+            local val = call_sync(fn, mock_params)
+            assert.equals("mock_val", val)
+
+            -- Change the return value for the uncached function, and invoke
+            -- the cached function. We shouldn't see the change.
+            mock_val = "other_val"
+            val = call_sync(fn, mock_params)
+
+            assert.equals("mock_val", val)
+        end)
+
+        it("should only call cb once if bufnr is the same", function()
+            local fn = cache.by_bufnr_async(mock_cb)
+
+            call_sync(fn, mock_params)
+            call_sync(fn, mock_params)
+
+            assert.are.same({ mock_params }, invoked_params)
+        end)
+
+        it("should only call cb once if cb returns false", function()
+            mock_val = false
+            local fn = cache.by_bufnr_async(mock_cb)
+
+            call_sync(fn, mock_params)
+            call_sync(fn, mock_params)
+
+            assert.are.same({ mock_params }, invoked_params)
+        end)
+
+        it("should call cb twice if bufnr is different", function()
+            local fn = cache.by_bufnr_async(mock_cb)
+
+            call_sync(fn, mock_params)
+            call_sync(fn, { bufnr = 2 })
+
+            assert.are.same({ mock_params, { bufnr = 2 } }, invoked_params)
+        end)
+    end)
 end)