refactor: use vim.system instead of jobstart (#521)

* refactor: use vim.system

* fix: use schedule wrap in the result callback

* fix: expand the command path

* fix: use uv.kill instead of vim.fn.jobclose

* test: make tests use vim.fn.exepath

* ci: don't run tests on nvim 0.9

* doc: explicitly require Neovim 0.10

* fix: remove jobstart-specific error checks

* refactor: rename jid -> pid

* fix: spawn shell processes if required

* fix: split shell variables

* refactor: delete low-value tests

* test: add a test for shell formatters

* fix: properly build the shell command

* refactor: lua implementation for shell_build_argv

---------

Co-authored-by: Steven Arcangeli <stevearc@stevearc.com>

5 files changed, 173 insertions, 105 deletions

diff --git a/lua/conform/errors.lua b/lua/conform/errors.lua
index 43e9a7b..e99a27c 100644
--- a/lua/conform/errors.lua
+++ b/lua/conform/errors.lua
@@ -7,12 +7,8 @@ local M = {}
 
 ---@enum conform.ERROR_CODE
 M.ERROR_CODE = {
-  -- Command was passed invalid arguments
-  INVALID_ARGS = 1,
-  -- Command was not executable
-  NOT_EXECUTABLE = 2,
-  -- Error occurred during when calling jobstart
-  JOBSTART = 3,
+  -- Error occurred during when calling vim.system
+  VIM_SYSTEM = 3,
   -- Command timed out during execution
   TIMEOUT = 4,
   -- Command was pre-empted by another call to format
@@ -39,10 +35,7 @@ end
 ---@param code conform.ERROR_CODE
 ---@return boolean
 M.is_execution_error = function(code)
-  return code == M.ERROR_CODE.RUNTIME
-    or code == M.ERROR_CODE.NOT_EXECUTABLE
-    or code == M.ERROR_CODE.INVALID_ARGS
-    or code == M.ERROR_CODE.JOBSTART
+  return code == M.ERROR_CODE.RUNTIME or code == M.ERROR_CODE.VIM_SYSTEM
 end
 
 ---@param err1? conform.Error
diff --git a/lua/conform/runner.lua b/lua/conform/runner.lua
index 3aef5fc..029d6ee 100644
--- a/lua/conform/runner.lua
+++ b/lua/conform/runner.lua
@@ -14,12 +14,16 @@ local M = {}
 ---@param formatter_name string
 ---@param ctx conform.Context
 ---@param config conform.JobFormatterConfig
----@return string|string[]
+---@return string[]
 M.build_cmd = function(formatter_name, ctx, config)
   local command = config.command
   if type(command) == "function" then
     command = command(config, ctx)
   end
+  local exepath = vim.fn.exepath(command)
+  if exepath ~= "" then
+    command = exepath
+  end
   ---@type string|string[]
   local args = {}
   if ctx.range and config.range_args then
@@ -29,8 +33,7 @@ M.build_cmd = function(formatter_name, ctx, config)
     local computed_args = config.args
     if type(computed_args) == "function" then
       args = computed_args(config, ctx)
-    else
-      ---@diagnostic disable-next-line: cast-local-type
+    elseif computed_args then
       args = computed_args
     end
   end
@@ -49,10 +52,9 @@ M.build_cmd = function(formatter_name, ctx, config)
       :gsub("$DIRNAME", ctx.dirname)
       :gsub("$RELATIVE_FILEPATH", compute_relative_filepath)
       :gsub("$EXTENSION", ctx.filename:match(".*(%..*)$") or "")
-    return command .. " " .. interpolated
+    return util.shell_build_argv(command .. " " .. interpolated)
   else
     local cmd = { command }
-    ---@diagnostic disable-next-line: param-type-mismatch
     for _, v in ipairs(args) do
       if v == "$FILENAME" then
         v = ctx.filename
@@ -359,40 +361,29 @@ local function run_formatter(bufnr, formatter, config, ctx, input_lines, opts, c
   if env then
     log.debug("Run ENV: %s", env)
   end
-  local stdout
-  local stderr
   local exit_codes = config.exit_codes or { 0 }
-  local jid
-  local ok, jid_or_err = pcall(vim.fn.jobstart, cmd, {
-    cwd = cwd,
-    env = env,
-    stdout_buffered = true,
-    stderr_buffered = true,
-    stdin = config.stdin and "pipe" or "null",
-    on_stdout = function(_, data)
-      if config.stdin then
-        stdout = data
-      end
-    end,
-    on_stderr = function(_, data)
-      stderr = data
-    end,
-    on_exit = function(_, code)
+  local pid
+  local ok, job_or_err = pcall(
+    vim.system,
+    cmd,
+    {
+      cwd = cwd,
+      env = env,
+      stdin = config.stdin and buffer_text,
+      text = true,
+    },
+    vim.schedule_wrap(function(result)
+      local code = result.code
+      local stdout = result.stdout and vim.split(result.stdout, "\n") or {}
+      local stderr = result.stderr and vim.split(result.stderr, "\n") or {}
       if vim.tbl_contains(exit_codes, code) then
-        local output
+        local output = stdout
         if not config.stdin then
           local fd = assert(uv.fs_open(ctx.filename, "r", 448)) -- 0700
           local stat = assert(uv.fs_fstat(fd))
           local content = assert(uv.fs_read(fd, stat.size))
           uv.fs_close(fd)
-          output = vim.split(content, "\r?\n", {})
-        else
-          output = stdout
-          -- trim trailing \r in every line
-          -- so that both branches of this if block behaves the same
-          for i, line in ipairs(output) do
-            output[i] = string.gsub(line, "\r$", "")
-          end
+          output = vim.split(content, "\r?\n")
         end
         -- Remove the trailing newline from the output to convert back to vim lines representation
         if add_extra_newline and output[#output] == "" then
@@ -420,7 +411,7 @@ local function run_formatter(bufnr, formatter, config, ctx, input_lines, opts, c
         end
         if
           vim.api.nvim_buf_is_valid(bufnr)
-          and jid ~= vim.b[bufnr].conform_jid
+          and pid ~= vim.b[bufnr].conform_pid
           and opts.exclusive
         then
           callback({
@@ -434,35 +425,21 @@ local function run_formatter(bufnr, formatter, config, ctx, input_lines, opts, c
           })
         end
       end
-    end,
-  })
+    end)
+  )
   if not ok then
     callback({
-      code = errors.ERROR_CODE.JOBSTART,
-      message = string.format("Formatter '%s' error in jobstart: %s", formatter.name, jid_or_err),
+      code = errors.ERROR_CODE.VIM_SYSTEM,
+      message = string.format("Formatter '%s' error in vim.system: %s", formatter.name, job_or_err),
     })
     return
   end
-  jid = jid_or_err
-  if jid == 0 then
-    callback({
-      code = errors.ERROR_CODE.INVALID_ARGS,
-      message = string.format("Formatter '%s' invalid arguments", formatter.name),
-    })
-  elseif jid == -1 then
-    callback({
-      code = errors.ERROR_CODE.NOT_EXECUTABLE,
-      message = string.format("Formatter '%s' command is not executable", formatter.name),
-    })
-  elseif config.stdin then
-    vim.api.nvim_chan_send(jid, buffer_text)
-    vim.fn.chanclose(jid, "stdin")
-  end
+  pid = job_or_err.pid
   if opts.exclusive then
-    vim.b[bufnr].conform_jid = jid
+    vim.b[bufnr].conform_pid = pid
   end
 
-  return jid
+  return pid
 end
 
 ---@param bufnr integer
@@ -527,9 +504,9 @@ M.format_async = function(bufnr, formatters, range, opts, callback)
   end
 
   -- kill previous jobs for buffer
-  local prev_jid = vim.b[bufnr].conform_jid
-  if prev_jid and opts.exclusive then
-    if vim.fn.jobstop(prev_jid) == 1 then
+  local prev_pid = vim.b[bufnr].conform_pid
+  if prev_pid and opts.exclusive then
+    if uv.kill(prev_pid) == 0 then
       log.info("Canceled previous format job for %s", vim.api.nvim_buf_get_name(bufnr))
     end
   end
@@ -619,9 +596,9 @@ M.format_sync = function(bufnr, formatters, timeout_ms, range, opts)
   local original_lines = vim.api.nvim_buf_get_lines(bufnr, 0, -1, false)
 
   -- kill previous jobs for buffer
-  local prev_jid = vim.b[bufnr].conform_jid
-  if prev_jid and opts.exclusive then
-    if vim.fn.jobstop(prev_jid) == 1 then
+  local prev_pid = vim.b[bufnr].conform_pid
+  if prev_pid and opts.exclusive then
+    if uv.kill(prev_pid) == 0 then
       log.info("Canceled previous format job for %s", vim.api.nvim_buf_get_name(bufnr))
     end
   end
@@ -672,7 +649,7 @@ M.format_lines_sync = function(bufnr, formatters, timeout_ms, range, input_lines
     ---@type conform.FormatterConfig
     local config = assert(require("conform").get_formatter_config(formatter.name, bufnr))
     local ctx = M.build_context(bufnr, config, range)
-    local jid = run_formatter(
+    local pid = run_formatter(
       bufnr,
       formatter,
       config,
@@ -692,8 +669,8 @@ M.format_lines_sync = function(bufnr, formatters, timeout_ms, range, input_lines
     end, 5)
 
     if not wait_result then
-      if jid then
-        vim.fn.jobstop(jid)
+      if pid then
+        uv.kill(pid)
       end
       if wait_reason == -1 then
         return errors.coalesce(final_err, {
diff --git a/lua/conform/util.lua b/lua/conform/util.lua
index 4f9b01b..c08960f 100644
--- a/lua/conform/util.lua
+++ b/lua/conform/util.lua
@@ -209,4 +209,47 @@ M.parse_rust_edition = function(dir)
   end
 end
 
+---@param cmd string
+---@return string[]
+M.shell_build_argv = function(cmd)
+  local argv = {}
+
+  -- If the shell starts with a quote, it contains spaces (from :help 'shell').
+  -- The shell may also have additional arguments in it, separated by spaces.
+  if vim.startswith(vim.o.shell, '"') then
+    local quoted = vim.o.shell:match('^"([^"]+)"')
+    table.insert(argv, quoted)
+    vim.list_extend(argv, vim.split(vim.o.shell:sub(quoted:len() + 3), "%s+", { trimempty = true }))
+  else
+    vim.list_extend(argv, vim.split(vim.o.shell, "%s+"))
+  end
+
+  vim.list_extend(argv, vim.split(vim.o.shellcmdflag, "%s+", { trimempty = true }))
+
+  if vim.o.shellxquote ~= "" then
+    -- When shellxquote is "(", we should escape the shellxescape characters with '^'
+    -- See :help 'shellxescape'
+    if vim.o.shellxquote == "(" and vim.o.shellxescape ~= "" then
+      cmd = cmd:gsub(".", function(char)
+        if string.find(vim.o.shellxescape, char, 1, true) then
+          return "^" .. char
+        else
+          return char
+        end
+      end)
+    end
+
+    if vim.o.shellxquote == "(" then
+      cmd = "(" .. cmd .. ")"
+    elseif vim.o.shellxquote == '"(' then
+      cmd = '"(' .. cmd .. ')"'
+    else
+      cmd = vim.o.shellxquote .. cmd .. vim.o.shellxquote
+    end
+  end
+
+  table.insert(argv, cmd)
+  return argv
+end
+
 return M
diff --git a/tests/runner_spec.lua b/tests/runner_spec.lua
index 5d4fea4..f52ca48 100644
--- a/tests/runner_spec.lua
+++ b/tests/runner_spec.lua
@@ -121,7 +121,7 @@ describe("runner", function()
       local config = assert(conform.get_formatter_config("test"))
       local ctx = runner.build_context(0, config)
       local cmd = runner.build_cmd("", ctx, config)
-      assert.are.same({ "echo", vim.api.nvim_buf_get_name(bufnr) }, cmd)
+      assert.are.same({ vim.fn.exepath("echo"), vim.api.nvim_buf_get_name(bufnr) }, cmd)
     end)
 
     it("replaces $DIRNAME in args", function()
@@ -135,7 +135,10 @@ describe("runner", function()
       local config = assert(conform.get_formatter_config("test"))
       local ctx = runner.build_context(0, config)
       local cmd = runner.build_cmd("", ctx, config)
-      assert.are.same({ "echo", vim.fs.dirname(vim.api.nvim_buf_get_name(bufnr)) }, cmd)
+      assert.are.same(
+        { vim.fn.exepath("echo"), vim.fs.dirname(vim.api.nvim_buf_get_name(bufnr)) },
+        cmd
+      )
     end)
 
     it("resolves arg function", function()
@@ -150,35 +153,7 @@ describe("runner", function()
       local config = assert(conform.get_formatter_config("test"))
       local ctx = runner.build_context(0, config)
       local cmd = runner.build_cmd("", ctx, config)
-      assert.are.same({ "echo", "--stdin" }, cmd)
-    end)
-
-    it("replaces $FILENAME in string args", function()
-      vim.cmd.edit({ args = { "README.md" } })
-      local bufnr = vim.api.nvim_get_current_buf()
-      conform.formatters.test = {
-        meta = { url = "", description = "" },
-        command = "echo",
-        args = "$FILENAME | patch",
-      }
-      local config = assert(conform.get_formatter_config("test"))
-      local ctx = runner.build_context(0, config)
-      local cmd = runner.build_cmd("", ctx, config)
-      assert.equal("echo " .. vim.api.nvim_buf_get_name(bufnr) .. " | patch", cmd)
-    end)
-
-    it("replaces $DIRNAME in string args", function()
-      vim.cmd.edit({ args = { "README.md" } })
-      local bufnr = vim.api.nvim_get_current_buf()
-      conform.formatters.test = {
-        meta = { url = "", description = "" },
-        command = "echo",
-        args = "$DIRNAME | patch",
-      }
-      local config = assert(conform.get_formatter_config("test"))
-      local ctx = runner.build_context(0, config)
-      local cmd = runner.build_cmd("", ctx, config)
-      assert.equal("echo " .. vim.fs.dirname(vim.api.nvim_buf_get_name(bufnr)) .. " | patch", cmd)
+      assert.are.same({ vim.fn.exepath("echo"), "--stdin" }, cmd)
     end)
 
     it("resolves arg function with string results", function()
@@ -193,7 +168,7 @@ describe("runner", function()
       local config = assert(conform.get_formatter_config("test"))
       local ctx = runner.build_context(0, config)
       local cmd = runner.build_cmd("", ctx, config)
-      assert.equal("echo | patch", cmd)
+      assert.are.same(util.shell_build_argv(vim.fn.exepath("echo") .. " | patch"), cmd)
     end)
   end)
 
@@ -410,5 +385,13 @@ print("a")
         assert.are.same({ "a", "d", "c" }, vim.api.nvim_buf_get_lines(0, 0, -1, false))
       end)
     end)
+
+    it("can run the format command in the shell", function()
+      conform.formatters.test = {
+        command = "echo",
+        args = '-e "world\nhello" | sort',
+      }
+      run_formatter_test("", "hello\nworld")
+    end)
   end)
 end)
diff --git a/tests/util_spec.lua b/tests/util_spec.lua
new file mode 100644
index 0000000..9f1456c
--- /dev/null
+++ b/tests/util_spec.lua
@@ -0,0 +1,72 @@
+local test_util = require("tests.test_util")
+local util = require("conform.util")
+
+describe("util", function()
+  local shell = vim.o.shell
+  local shellcmdflag = vim.o.shellcmdflag
+  local shellxescape = vim.o.shellxescape
+  local shellxquote = vim.o.shellxquote
+  after_each(function()
+    test_util.reset_editor()
+    vim.o.shell = shell
+    vim.o.shellcmdflag = shellcmdflag
+    vim.o.shellxescape = shellxescape
+    vim.o.shellxquote = shellxquote
+  end)
+
+  describe("shell_build_argv", function()
+    it("builds simple command", function()
+      vim.o.shell = "/bin/bash"
+      vim.o.shellcmdflag = "-c"
+      vim.o.shellxescape = ""
+      vim.o.shellxquote = ""
+      local argv = util.shell_build_argv("echo hello")
+      assert.are_same({ "/bin/bash", "-c", "echo hello" }, argv)
+    end)
+
+    it("handles shell arguments", function()
+      vim.o.shell = "/bin/bash -f"
+      vim.o.shellcmdflag = "-c"
+      vim.o.shellxescape = ""
+      vim.o.shellxquote = ""
+      local argv = util.shell_build_argv("echo hello")
+      assert.are_same({ "/bin/bash", "-f", "-c", "echo hello" }, argv)
+    end)
+
+    it("handles shell with spaces", function()
+      vim.o.shell = '"c:\\program files\\unix\\sh.exe"'
+      vim.o.shellcmdflag = "-c"
+      vim.o.shellxescape = ""
+      vim.o.shellxquote = ""
+      local argv = util.shell_build_argv("echo hello")
+      assert.are_same({ "c:\\program files\\unix\\sh.exe", "-c", "echo hello" }, argv)
+    end)
+
+    it("handles shell with spaces and args", function()
+      vim.o.shell = '"c:\\program files\\unix\\sh.exe" -f'
+      vim.o.shellcmdflag = "-c"
+      vim.o.shellxescape = ""
+      vim.o.shellxquote = ""
+      local argv = util.shell_build_argv("echo hello")
+      assert.are_same({ "c:\\program files\\unix\\sh.exe", "-f", "-c", "echo hello" }, argv)
+    end)
+
+    it("applies shellxquote", function()
+      vim.o.shell = "/bin/bash"
+      vim.o.shellcmdflag = "-c"
+      vim.o.shellxescape = ""
+      vim.o.shellxquote = "'"
+      local argv = util.shell_build_argv("echo hello")
+      assert.are_same({ "/bin/bash", "-c", "'echo hello'" }, argv)
+    end)
+
+    it("uses shellxescape", function()
+      vim.o.shell = "/bin/bash"
+      vim.o.shellcmdflag = "-c"
+      vim.o.shellxescape = "el"
+      vim.o.shellxquote = "("
+      local argv = util.shell_build_argv("echo hello")
+      assert.are_same({ "/bin/bash", "-c", "(^echo h^e^l^lo)" }, argv)
+    end)
+  end)
+end)