feat(jobs): jobstart(…,{term=true}), deprecate termopen() #31343

Problem:
`termopen` has long been a superficial wrapper around `jobstart`, and
has no real purpose. Also, `vim.system` and `nvim_open_term` presumably
will replace all features of `jobstart` and `termopen`, so centralizing
the logic will help with that.

Solution:
- Introduce `eval/deprecated.c`, where all deprecated eval funcs will live.
- Introduce "term" flag of `jobstart`.
- Deprecate `termopen`.

9 files changed, 163 insertions, 166 deletions

diff --git a/src/nvim/api/buffer.c b/src/nvim/api/buffer.c
index 2b6aa8b371..6c40eb9b20 100644
--- a/src/nvim/api/buffer.c
+++ b/src/nvim/api/buffer.c
@@ -1181,12 +1181,12 @@ ArrayOf(Integer, 2) nvim_buf_get_mark(Buffer buffer, String name, Arena *arena,
 /// This temporarily switches current buffer to "buffer".
 /// If the current window already shows "buffer", the window is not switched.
 /// If a window inside the current tabpage (including a float) already shows the
-/// buffer, then one of these windows will be set as current window temporarily.
+/// buffer, then one of those windows will be set as current window temporarily.
 /// Otherwise a temporary scratch window (called the "autocmd window" for
 /// historical reasons) will be used.
 ///
 /// This is useful e.g. to call Vimscript functions that only work with the
-/// current buffer/window currently, like |termopen()|.
+/// current buffer/window currently, like `jobstart(…, {'term': v:true})`.
 ///
 /// @param buffer     Buffer handle, or 0 for current buffer
 /// @param fun        Function to call inside the buffer (currently Lua callable
diff --git a/src/nvim/eval.c b/src/nvim/eval.c
index 47f0a13b29..8bdd8dad4c 100644
--- a/src/nvim/eval.c
+++ b/src/nvim/eval.c
@@ -8493,7 +8493,7 @@ char *do_string_sub(char *str, size_t len, char *pat, char *sub, typval_T *expr,
   return ret;
 }
 
-/// common code for getting job callbacks for jobstart, termopen and rpcstart
+/// Common code for getting job callbacks for `jobstart`.
 ///
 /// @return true/false on success/failure.
 bool common_job_callbacks(dict_T *vopts, CallbackReader *on_stdout, CallbackReader *on_stderr,
diff --git a/src/nvim/eval.lua b/src/nvim/eval.lua
index 9de189cc16..72dabd53e9 100644
--- a/src/nvim/eval.lua
+++ b/src/nvim/eval.lua
@@ -5945,7 +5945,7 @@ M.funcs = {
   jobstart = {
     args = { 1, 2 },
     desc = [=[
-      Note: Prefer |vim.system()| in Lua (unless using the `pty` option).
+      Note: Prefer |vim.system()| in Lua (unless using `rpc`, `pty`, or `term`).
 
       Spawns {cmd} as a job.
       If {cmd} is a List it runs directly (no 'shell').
@@ -5953,8 +5953,11 @@ M.funcs = {
         call jobstart(split(&shell) + split(&shellcmdflag) + ['{cmd}'])
       <(See |shell-unquoting| for details.)
 
-      Example: >vim
-        call jobstart('nvim -h', {'on_stdout':{j,d,e->append(line('.'),d)}})
+      Example: start a job and handle its output: >vim
+        call jobstart(['nvim', '-h'], {'on_stdout':{j,d,e->append(line('.'),d)}})
+      <
+      Example: start a job in a |terminal| connected to the current buffer: >vim
+        call jobstart(['nvim', '-h'], {'term':v:true})
       <
       Returns |job-id| on success, 0 on invalid arguments (or job
       table is full), -1 if {cmd}[0] or 'shell' is not executable.
@@ -6019,6 +6022,10 @@ M.funcs = {
         stdin:      (string) Either "pipe" (default) to connect the
       	      job's stdin to a channel or "null" to disconnect
       	      stdin.
+        term:	    (boolean) Spawns {cmd} in a new pseudo-terminal session
+                connected to the current (unmodified) buffer. Implies "pty".
+                Default "height" and "width" are set to the current window
+                dimensions. |jobstart()|. Defaults $TERM to "xterm-256color".
         width:      (number) Width of the `pty` terminal.
 
       {opts} is passed as |self| dictionary to the callback; the
@@ -12271,21 +12278,10 @@ M.funcs = {
     signature = 'tempname()',
   },
   termopen = {
+    deprecated = true,
     args = { 1, 2 },
     desc = [=[
-      Spawns {cmd} in a new pseudo-terminal session connected
-      to the current (unmodified) buffer. Parameters and behavior
-      are the same as |jobstart()| except "pty", "width", "height",
-      and "TERM" are ignored: "height" and "width" are taken from
-      the current window. Note that termopen() implies a "pty" arg
-      to jobstart(), and thus has the implications documented at
-      |jobstart()|.
-
-      Returns the same values as jobstart().
-
-      Terminal environment is initialized as in |jobstart-env|,
-      except $TERM is set to "xterm-256color". Full behavior is
-      described in |terminal|.
+      Use |jobstart()| with `{term: v:true}` instead.
     ]=],
     name = 'termopen',
     params = { { 'cmd', 'string|string[]' }, { 'opts', 'table' } },
diff --git a/src/nvim/eval/deprecated.c b/src/nvim/eval/deprecated.c
new file mode 100644
index 0000000000..67c254dac9
--- /dev/null
+++ b/src/nvim/eval/deprecated.c
@@ -0,0 +1,44 @@
+#include <stdbool.h>                // for true
+
+#include "nvim/errors.h"
+#include "nvim/eval/deprecated.h"
+#include "nvim/eval/funcs.h"
+#include "nvim/eval/typval.h"
+#include "nvim/eval/typval_defs.h"
+#include "nvim/ex_cmds.h"
+#include "nvim/gettext_defs.h"      // for _
+#include "nvim/macros_defs.h"       // for S_LEN
+#include "nvim/message.h"           // for semsg
+#include "nvim/types_defs.h"
+
+#ifdef INCLUDE_GENERATED_DECLARATIONS
+# include "eval/deprecated.c.generated.h"
+#endif
+
+/// "termopen(cmd[, cwd])" function
+void f_termopen(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
+{
+  if (check_secure()) {
+    return;
+  }
+
+  bool must_free = false;
+
+  if (argvars[1].v_type == VAR_UNKNOWN) {
+    must_free = true;
+    argvars[1].v_type = VAR_DICT;
+    argvars[1].vval.v_dict = tv_dict_alloc();
+  }
+
+  if (argvars[1].v_type != VAR_DICT) {
+    // Wrong argument types
+    semsg(_(e_invarg2), "expected dictionary");
+    return;
+  }
+
+  tv_dict_add_bool(argvars[1].vval.v_dict, S_LEN("term"), true);
+  f_jobstart(argvars, rettv, fptr);
+  if (must_free) {
+    tv_dict_free(argvars[1].vval.v_dict);
+  }
+}
diff --git a/src/nvim/eval/deprecated.h b/src/nvim/eval/deprecated.h
new file mode 100644
index 0000000000..b870403aa4
--- /dev/null
+++ b/src/nvim/eval/deprecated.h
@@ -0,0 +1,10 @@
+#pragma once
+
+#include <stdbool.h>                // for true
+
+#include "nvim/eval/typval_defs.h"  // IWYU pragma: keep
+#include "nvim/types_defs.h"  // IWYU pragma: keep
+
+#ifdef INCLUDE_GENERATED_DECLARATIONS
+# include "eval/deprecated.h.generated.h"
+#endif
diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c
index 768d7664f7..23bfcff406 100644
--- a/src/nvim/eval/funcs.c
+++ b/src/nvim/eval/funcs.c
@@ -38,6 +38,7 @@
 #include "nvim/eval.h"
 #include "nvim/eval/buffer.h"
 #include "nvim/eval/decode.h"
+#include "nvim/eval/deprecated.h"
 #include "nvim/eval/encode.h"
 #include "nvim/eval/executor.h"
 #include "nvim/eval/funcs.h"
@@ -3840,8 +3841,8 @@ static const char *required_env_vars[] = {
   NULL
 };
 
-static dict_T *create_environment(const dictitem_T *job_env, const bool clear_env, const bool pty,
-                                  const char * const pty_term_name)
+dict_T *create_environment(const dictitem_T *job_env, const bool clear_env, const bool pty,
+                           const char * const pty_term_name)
 {
   dict_T *env = tv_dict_alloc();
 
@@ -3933,7 +3934,7 @@ static dict_T *create_environment(const dictitem_T *job_env, const bool clear_en
 }
 
 /// "jobstart()" function
-static void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
+void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
 {
   rettv->v_type = VAR_NUMBER;
   rettv->vval.v_number = 0;
@@ -3942,9 +3943,9 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
     return;
   }
 
+  const char *cmd;
   bool executable = true;
-  char **argv = tv_to_argv(&argvars[0], NULL, &executable);
-  dict_T *env = NULL;
+  char **argv = tv_to_argv(&argvars[0], &cmd, &executable);
   if (!argv) {
     rettv->vval.v_number = executable ? 0 : -1;
     return;  // Did error message in tv_to_argv.
@@ -3961,6 +3962,7 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
   bool detach = false;
   bool rpc = false;
   bool pty = false;
+  bool term = false;
   bool clear_env = false;
   bool overlapped = false;
   ChannelStdinMode stdin_mode = kChannelStdinPipe;
@@ -3974,7 +3976,8 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
 
     detach = tv_dict_get_number(job_opts, "detach") != 0;
     rpc = tv_dict_get_number(job_opts, "rpc") != 0;
-    pty = tv_dict_get_number(job_opts, "pty") != 0;
+    term = tv_dict_get_number(job_opts, "term") != 0;
+    pty = term || tv_dict_get_number(job_opts, "pty") != 0;
     clear_env = tv_dict_get_number(job_opts, "clear_env") != 0;
     overlapped = tv_dict_get_number(job_opts, "overlapped") != 0;
 
@@ -3989,6 +3992,14 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
       }
     }
 
+    dictitem_T *const job_term = tv_dict_find(job_opts, S_LEN("term"));
+    if (job_term && VAR_BOOL != job_term->di_tv.v_type) {
+      // Restrict "term" field to boolean, in case we want to allow buffer numbers in the future.
+      semsg(_(e_invarg2), "'term' must be Boolean");
+      shell_free_argv(argv);
+      return;
+    }
+
     if (pty && rpc) {
       semsg(_(e_invarg2), "job cannot have both 'pty' and 'rpc' options set");
       shell_free_argv(argv);
@@ -4032,24 +4043,87 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
   uint16_t height = 0;
   char *term_name = NULL;
 
-  if (pty) {
-    width = (uint16_t)tv_dict_get_number(job_opts, "width");
-    height = (uint16_t)tv_dict_get_number(job_opts, "height");
-    // Legacy method, before env option existed, to specify $TERM.  No longer
-    // documented, but still usable to avoid breaking scripts.
-    term_name = tv_dict_get_string(job_opts, "TERM", false);
-    if (!term_name) {
-      term_name = "ansi";
+  if (term) {
+    if (text_locked()) {
+      text_locked_msg();
+      shell_free_argv(argv);
+      return;
     }
+    if (curbuf->b_changed) {
+      emsg(_("jobstart(...,{term=true}) requires unmodified buffer"));
+      shell_free_argv(argv);
+      return;
+    }
+    assert(!rpc);
+    term_name = "xterm-256color";
+    cwd = cwd ? cwd : ".";
+    overlapped = false;
+    detach = false;
+    stdin_mode = kChannelStdinPipe;
+    width = (uint16_t)MAX(0, curwin->w_width_inner - win_col_off(curwin));
+    height = (uint16_t)curwin->w_height_inner;
   }
 
-  env = create_environment(job_env, clear_env, pty, term_name);
+  if (pty) {
+    width = width ? width : (uint16_t)tv_dict_get_number(job_opts, "width");
+    height = height ? height : (uint16_t)tv_dict_get_number(job_opts, "height");
+    // Deprecated TERM field is from before `env` option existed.
+    term_name = term_name ? term_name : tv_dict_get_string(job_opts, "TERM", false);
+    term_name = term_name ? term_name : "ansi";
+  }
 
+  dict_T *env = create_environment(job_env, clear_env, pty, term_name);
   Channel *chan = channel_job_start(argv, NULL, on_stdout, on_stderr, on_exit, pty,
                                     rpc, overlapped, detach, stdin_mode, cwd,
                                     width, height, env, &rettv->vval.v_number);
-  if (chan) {
+  if (!chan) {
+    return;
+  } else if (!term) {
     channel_create_event(chan, NULL);
+  } else {
+    if (rettv->vval.v_number <= 0) {
+      return;
+    }
+
+    int pid = chan->stream.pty.proc.pid;
+
+    // "./…" => "/home/foo/…"
+    vim_FullName(cwd, NameBuff, sizeof(NameBuff), false);
+    // "/home/foo/…" => "~/…"
+    size_t len = home_replace(NULL, NameBuff, IObuff, sizeof(IObuff), true);
+    // Trim slash.
+    if (len != 1 && (IObuff[len - 1] == '\\' || IObuff[len - 1] == '/')) {
+      IObuff[len - 1] = NUL;
+    }
+
+    if (len == 1 && IObuff[0] == '/') {
+      // Avoid ambiguity in the URI when CWD is root directory.
+      IObuff[1] = '.';
+      IObuff[2] = NUL;
+    }
+
+    // Terminal URI: "term://$CWD//$PID:$CMD"
+    snprintf(NameBuff, sizeof(NameBuff), "term://%s//%d:%s", IObuff, pid, cmd);
+    // Buffer has no terminal associated yet; unset 'swapfile' to ensure no swapfile is created.
+    curbuf->b_p_swf = false;
+
+    apply_autocmds(EVENT_BUFFILEPRE, NULL, NULL, false, curbuf);
+    setfname(curbuf, NameBuff, NULL, true);
+    apply_autocmds(EVENT_BUFFILEPOST, NULL, NULL, false, curbuf);
+
+    Error err = ERROR_INIT;
+    // Set (deprecated) buffer-local vars (prefer 'channel' buffer-local option).
+    dict_set_var(curbuf->b_vars, cstr_as_string("terminal_job_id"),
+                 INTEGER_OBJ((Integer)chan->id), false, false, NULL, &err);
+    api_clear_error(&err);
+    dict_set_var(curbuf->b_vars, cstr_as_string("terminal_job_pid"),
+                 INTEGER_OBJ(pid), false, false, NULL, &err);
+    api_clear_error(&err);
+
+    channel_incref(chan);
+    channel_terminal_open(curbuf, chan);
+    channel_create_event(chan, NULL);
+    channel_decref(chan);
   }
 }
 
@@ -8133,134 +8207,6 @@ static void f_taglist(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
            (char *)tag_pattern, (char *)fname);
 }
 
-/// "termopen(cmd[, cwd])" function
-static void f_termopen(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
-{
-  if (check_secure()) {
-    return;
-  }
-  if (text_locked()) {
-    text_locked_msg();
-    return;
-  }
-  if (curbuf->b_changed) {
-    emsg(_("Can only call this function in an unmodified buffer"));
-    return;
-  }
-
-  const char *cmd;
-  bool executable = true;
-  char **argv = tv_to_argv(&argvars[0], &cmd, &executable);
-  if (!argv) {
-    rettv->vval.v_number = executable ? 0 : -1;
-    return;  // Did error message in tv_to_argv.
-  }
-
-  if (argvars[1].v_type != VAR_DICT && argvars[1].v_type != VAR_UNKNOWN) {
-    // Wrong argument type
-    semsg(_(e_invarg2), "expected dictionary");
-    shell_free_argv(argv);
-    return;
-  }
-
-  CallbackReader on_stdout = CALLBACK_READER_INIT;
-  CallbackReader on_stderr = CALLBACK_READER_INIT;
-  Callback on_exit = CALLBACK_NONE;
-  dict_T *job_opts = NULL;
-  const char *cwd = ".";
-  dict_T *env = NULL;
-  const bool pty = true;
-  bool clear_env = false;
-  dictitem_T *job_env = NULL;
-
-  if (argvars[1].v_type == VAR_DICT) {
-    job_opts = argvars[1].vval.v_dict;
-
-    const char *const new_cwd = tv_dict_get_string(job_opts, "cwd", false);
-    if (new_cwd && *new_cwd != NUL) {
-      cwd = new_cwd;
-      // The new cwd must be a directory.
-      if (!os_isdir(cwd)) {
-        semsg(_(e_invarg2), "expected valid directory");
-        shell_free_argv(argv);
-        return;
-      }
-    }
-
-    job_env = tv_dict_find(job_opts, S_LEN("env"));
-    if (job_env && job_env->di_tv.v_type != VAR_DICT) {
-      semsg(_(e_invarg2), "env");
-      shell_free_argv(argv);
-      return;
-    }
-
-    clear_env = tv_dict_get_number(job_opts, "clear_env") != 0;
-
-    if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) {
-      shell_free_argv(argv);
-      return;
-    }
-  }
-
-  env = create_environment(job_env, clear_env, pty, "xterm-256color");
-
-  const bool rpc = false;
-  const bool overlapped = false;
-  const bool detach = false;
-  ChannelStdinMode stdin_mode = kChannelStdinPipe;
-  uint16_t term_width = (uint16_t)MAX(0, curwin->w_width_inner - win_col_off(curwin));
-  Channel *chan = channel_job_start(argv, NULL, on_stdout, on_stderr, on_exit,
-                                    pty, rpc, overlapped, detach, stdin_mode,
-                                    cwd, term_width, (uint16_t)curwin->w_height_inner,
-                                    env, &rettv->vval.v_number);
-  if (rettv->vval.v_number <= 0) {
-    return;
-  }
-
-  int pid = chan->stream.pty.proc.pid;
-
-  // "./…" => "/home/foo/…"
-  vim_FullName(cwd, NameBuff, sizeof(NameBuff), false);
-  // "/home/foo/…" => "~/…"
-  size_t len = home_replace(NULL, NameBuff, IObuff, sizeof(IObuff), true);
-  // Trim slash.
-  if (len != 1 && (IObuff[len - 1] == '\\' || IObuff[len - 1] == '/')) {
-    IObuff[len - 1] = NUL;
-  }
-
-  if (len == 1 && IObuff[0] == '/') {
-    // Avoid ambiguity in the URI when CWD is root directory.
-    IObuff[1] = '.';
-    IObuff[2] = NUL;
-  }
-
-  // Terminal URI: "term://$CWD//$PID:$CMD"
-  snprintf(NameBuff, sizeof(NameBuff), "term://%s//%d:%s",
-           IObuff, pid, cmd);
-  // at this point the buffer has no terminal instance associated yet, so unset
-  // the 'swapfile' option to ensure no swap file will be created
-  curbuf->b_p_swf = false;
-
-  apply_autocmds(EVENT_BUFFILEPRE, NULL, NULL, false, curbuf);
-  setfname(curbuf, NameBuff, NULL, true);
-  apply_autocmds(EVENT_BUFFILEPOST, NULL, NULL, false, curbuf);
-
-  // Save the job id and pid in b:terminal_job_{id,pid}
-  Error err = ERROR_INIT;
-  // deprecated: use 'channel' buffer option
-  dict_set_var(curbuf->b_vars, cstr_as_string("terminal_job_id"),
-               INTEGER_OBJ((Integer)chan->id), false, false, NULL, &err);
-  api_clear_error(&err);
-  dict_set_var(curbuf->b_vars, cstr_as_string("terminal_job_pid"),
-               INTEGER_OBJ(pid), false, false, NULL, &err);
-  api_clear_error(&err);
-
-  channel_incref(chan);
-  channel_terminal_open(curbuf, chan);
-  channel_create_event(chan, NULL);
-  channel_decref(chan);
-}
-
 /// "timer_info([timer])" function
 static void f_timer_info(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
 {
diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c
index 052bf3b9f7..bd8623397a 100644
--- a/src/nvim/ex_docmd.c
+++ b/src/nvim/ex_docmd.c
@@ -7775,7 +7775,7 @@ static void ex_terminal(exarg_T *eap)
   if (*eap->arg != NUL) {  // Run {cmd} in 'shell'.
     char *name = vim_strsave_escaped(eap->arg, "\"\\");
     snprintf(ex_cmd + len, sizeof(ex_cmd) - len,
-             " | call termopen(\"%s\")", name);
+             " | call jobstart(\"%s\",{'term':v:true})", name);
     xfree(name);
   } else {  // No {cmd}: run the job with tokenized 'shell'.
     if (*p_sh == NUL) {
@@ -7798,7 +7798,7 @@ static void ex_terminal(exarg_T *eap)
     shell_free_argv(argv);
 
     snprintf(ex_cmd + len, sizeof(ex_cmd) - len,
-             " | call termopen([%s])", shell_argv + 1);
+             " | call jobstart([%s], {'term':v:true})", shell_argv + 1);
   }
 
   do_cmdline_cmd(ex_cmd);
diff --git a/src/nvim/generators/gen_eval.lua b/src/nvim/generators/gen_eval.lua
index 443c68e008..0b6ee6cb24 100644
--- a/src/nvim/generators/gen_eval.lua
+++ b/src/nvim/generators/gen_eval.lua
@@ -19,6 +19,7 @@ hashpipe:write([[
 #include "nvim/digraph.h"
 #include "nvim/eval.h"
 #include "nvim/eval/buffer.h"
+#include "nvim/eval/deprecated.h"
 #include "nvim/eval/fs.h"
 #include "nvim/eval/funcs.h"
 #include "nvim/eval/typval.h"
diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c
index 969c539f97..61eede5ae1 100644
--- a/src/nvim/terminal.c
+++ b/src/nvim/terminal.c
@@ -357,7 +357,7 @@ static void term_output_callback(const char *s, size_t len, void *user_data)
 
 /// Initializes terminal properties, and triggers TermOpen.
 ///
-/// The PTY process (TerminalOptions.data) was already started by termopen(),
+/// The PTY process (TerminalOptions.data) was already started by jobstart(),
 /// via ex_terminal() or the term:// BufReadCmd.
 ///
 /// @param buf Buffer used for presentation of the terminal.