refactor(fileio): remove API shell layer encouraging unnecessary allocations

Functions like file_open_new() and file_open_fd_new() which just is a
wrapper around the real functions but with an extra xmalloc/xfree around
is an anti-pattern. If the caller really needs to allocate a
FileDescriptor as a heap object, it can do that directly.

FileDescriptor by itself is pretty much a pointer, or rather two:
the OS fd index and a pointer to a buffer. So most of the time an extra
pointer layer is just wasteful.

In the case of scriptin[curscript] in getchar.c, curscript used
to mean in practice:

N+1 open scripts           when curscript>0
zero or one open scripts   when curscript==0

Which means scriptin[0] had to be compared to NULL to disambiguate the
curscript=0 case.

Instead, use curscript==-1 to mean that are no script,
then all pointer comparisons dissappear and we can just use an array of
structs without extra pointers.

7 files changed, 86 insertions, 198 deletions

diff --git a/src/nvim/getchar.c b/src/nvim/getchar.c
index a010687001..64c9c5a8c3 100644
--- a/src/nvim/getchar.c
+++ b/src/nvim/getchar.c
@@ -65,8 +65,9 @@
 #include "nvim/vim_defs.h"
 
 /// Index in scriptin
-static int curscript = 0;
-FileDescriptor *scriptin[NSCRIPT] = { NULL };
+static int curscript = -1;
+/// Streams to read script from
+static FileDescriptor scriptin[NSCRIPT] = { 0 };
 
 // These buffers are used for storing:
 // - stuffed characters: A command that is translated into another command.
@@ -1176,7 +1177,7 @@ void ungetchars(int len)
 void may_sync_undo(void)
 {
   if ((!(State & (MODE_INSERT | MODE_CMDLINE)) || arrow_used)
-      && scriptin[curscript] == NULL) {
+      && curscript < 0) {
     u_sync(false);
   }
 }
@@ -1216,8 +1217,9 @@ void free_typebuf(void)
 /// restored when "file" has been read completely.
 static typebuf_T saved_typebuf[NSCRIPT];
 
-void save_typebuf(void)
+static void save_typebuf(void)
 {
+  assert(curscript >= 0);
   init_typebuf();
   saved_typebuf[curscript] = typebuf;
   alloc_typebuf();
@@ -1292,18 +1294,13 @@ void openscript(char *name, bool directly)
     return;
   }
 
-  if (scriptin[curscript] != NULL) {    // already reading script
-    curscript++;
-  }
+  curscript++;
   // use NameBuff for expanded name
   expand_env(name, NameBuff, MAXPATHL);
-  int error;
-  if ((scriptin[curscript] = file_open_new(&error, NameBuff,
-                                           kFileReadOnly, 0)) == NULL) {
+  int error = file_open(&scriptin[curscript], NameBuff, kFileReadOnly, 0);
+  if (error) {
     semsg(_(e_notopen_2), name, os_strerror(error));
-    if (curscript) {
-      curscript--;
-    }
+    curscript--;
     return;
   }
   save_typebuf();
@@ -1330,7 +1327,7 @@ void openscript(char *name, bool directly)
       update_topline_cursor();          // update cursor position and topline
       normal_cmd(&oa, false);           // execute one command
       vpeekc();                   // check for end of file
-    } while (scriptin[oldcurscript] != NULL);
+    } while (curscript >= oldcurscript);
 
     State = save_State;
     msg_scroll = save_msg_scroll;
@@ -1342,31 +1339,53 @@ void openscript(char *name, bool directly)
 /// Close the currently active input script.
 static void closescript(void)
 {
+  assert(curscript >= 0);
   free_typebuf();
   typebuf = saved_typebuf[curscript];
 
-  file_free(scriptin[curscript], false);
-  scriptin[curscript] = NULL;
-  if (curscript > 0) {
-    curscript--;
-  }
+  file_close(&scriptin[curscript], false);
+  curscript--;
 }
 
 #if defined(EXITFREE)
 void close_all_scripts(void)
 {
-  while (scriptin[0] != NULL) {
+  while (curscript >= 0) {
     closescript();
   }
 }
 
 #endif
 
+bool open_scriptin(char *scriptin_name)
+  FUNC_ATTR_NONNULL_ALL
+{
+  assert(curscript == -1);
+  curscript++;
+
+  int error;
+  if (strequal(scriptin_name, "-")) {
+    error = file_open_stdin(&scriptin[0]);
+  } else {
+    error = file_open(&scriptin[0], scriptin_name,
+                      kFileReadOnly|kFileNonBlocking, 0);
+  }
+  if (error) {
+    fprintf(stderr, _("Cannot open for reading: \"%s\": %s\n"),
+            scriptin_name, os_strerror(error));
+    curscript--;
+    return false;
+  }
+  save_typebuf();
+
+  return true;
+}
+
 /// Return true when reading keys from a script file.
 int using_script(void)
   FUNC_ATTR_PURE
 {
-  return scriptin[curscript] != NULL;
+  return curscript >= 0;
 }
 
 /// This function is called just before doing a blocking wait.  Thus after
@@ -2801,10 +2820,10 @@ int inchar(uint8_t *buf, int maxlen, long wait_time)
   // Get a character from a script file if there is one.
   // If interrupted: Stop reading script files, close them all.
   ptrdiff_t read_size = -1;
-  while (scriptin[curscript] != NULL && read_size <= 0 && !ignore_script) {
+  while (curscript >= 0 && read_size <= 0 && !ignore_script) {
     char script_char;
     if (got_int
-        || (read_size = file_read(scriptin[curscript], &script_char, 1)) != 1) {
+        || (read_size = file_read(&scriptin[curscript], &script_char, 1)) != 1) {
       // Reached EOF or some error occurred.
       // Careful: closescript() frees typebuf.tb_buf[] and buf[] may
       // point inside typebuf.tb_buf[].  Don't use buf[] after this!
diff --git a/src/nvim/getchar.h b/src/nvim/getchar.h
index 79f4f1f8ba..4e962c9b03 100644
--- a/src/nvim/getchar.h
+++ b/src/nvim/getchar.h
@@ -17,9 +17,6 @@ typedef enum {
 
 enum { NSCRIPT = 15, };  ///< Maximum number of streams to read script from
 
-/// Streams to read script from
-extern FileDescriptor *scriptin[NSCRIPT];
-
 #ifdef INCLUDE_GENERATED_DECLARATIONS
 # include "getchar.h.generated.h"
 #endif
diff --git a/src/nvim/lua/executor.c b/src/nvim/lua/executor.c
index 58f329b76f..3859dd922a 100644
--- a/src/nvim/lua/executor.c
+++ b/src/nvim/lua/executor.c
@@ -1828,7 +1828,11 @@ bool nlua_exec_file(const char *path)
     lua_getglobal(lstate, "loadfile");
     lua_pushstring(lstate, path);
   } else {
-    FileDescriptor *stdin_dup = file_open_stdin();
+    FileDescriptor stdin_dup;
+    int error = file_open_stdin(&stdin_dup);
+    if (error) {
+      return false;
+    }
 
     StringBuilder sb = KV_INITIAL_VALUE;
     kv_resize(sb, 64);
@@ -1837,7 +1841,7 @@ bool nlua_exec_file(const char *path)
       if (got_int) {  // User canceled.
         return false;
       }
-      ptrdiff_t read_size = file_read(stdin_dup, IObuff, 64);
+      ptrdiff_t read_size = file_read(&stdin_dup, IObuff, 64);
       if (read_size < 0) {  // Error.
         return false;
       }
@@ -1849,7 +1853,7 @@ bool nlua_exec_file(const char *path)
       }
     }
     kv_push(sb, NUL);
-    file_free(stdin_dup, false);
+    file_close(&stdin_dup, false);
 
     lua_getglobal(lstate, "loadstring");
     lua_pushstring(lstate, sb.items);
diff --git a/src/nvim/main.c b/src/nvim/main.c
index e18c561879..c2445437e6 100644
--- a/src/nvim/main.c
+++ b/src/nvim/main.c
@@ -426,7 +426,19 @@ int main(int argc, char **argv)
     params.edit_type = EDIT_STDIN;
   }
 
-  open_script_files(&params);
+  if (params.scriptin) {
+    if (!open_scriptin(params.scriptin)) {
+      os_exit(2);
+    }
+  }
+  if (params.scriptout) {
+    scriptout = os_fopen(params.scriptout, params.scriptout_append ? APPENDBIN : WRITEBIN);
+    if (scriptout == NULL) {
+      fprintf(stderr, _("Cannot open for script output: \""));
+      fprintf(stderr, "%s\"\n", params.scriptout);
+      os_exit(2);
+    }
+  }
 
   nlua_init_defaults();
 
@@ -1620,37 +1632,6 @@ static void read_stdin(void)
   check_swap_exists_action();
 }
 
-static void open_script_files(mparm_T *parmp)
-{
-  if (parmp->scriptin) {
-    int error;
-    if (strequal(parmp->scriptin, "-")) {
-      FileDescriptor *stdin_dup = file_open_stdin();
-      scriptin[0] = stdin_dup;
-    } else {
-      scriptin[0] = file_open_new(&error, parmp->scriptin,
-                                  kFileReadOnly|kFileNonBlocking, 0);
-      if (scriptin[0] == NULL) {
-        vim_snprintf(IObuff, IOSIZE,
-                     _("Cannot open for reading: \"%s\": %s\n"),
-                     parmp->scriptin, os_strerror(error));
-        fprintf(stderr, "%s", IObuff);
-        os_exit(2);
-      }
-    }
-    save_typebuf();
-  }
-
-  if (parmp->scriptout) {
-    scriptout = os_fopen(parmp->scriptout, parmp->scriptout_append ? APPENDBIN : WRITEBIN);
-    if (scriptout == NULL) {
-      fprintf(stderr, _("Cannot open for script output: \""));
-      fprintf(stderr, "%s\"\n", parmp->scriptout);
-      os_exit(2);
-    }
-  }
-}
-
 // Create the requested number of windows and edit buffers in them.
 // Also does recovery if "recoverymode" set.
 static void create_windows(mparm_T *parmp)
diff --git a/src/nvim/os/fileio.c b/src/nvim/os/fileio.c
index d24681a156..e14de9e5db 100644
--- a/src/nvim/os/fileio.c
+++ b/src/nvim/os/fileio.c
@@ -129,59 +129,15 @@ int file_open_fd(FileDescriptor *const ret_fp, const int fd, const int flags)
   return 0;
 }
 
-/// Like file_open(), but allocate and return ret_fp
-///
-/// @param[out]  error  Error code, or 0 on success. @see os_strerror()
-/// @param[in]  fname  File name to open.
-/// @param[in]  flags  Flags, @see FileOpenFlags.
-/// @param[in]  mode  Permissions for the newly created file (ignored if flags
-///                   does not have kFileCreate\*).
-///
-/// @return [allocated] Opened file or NULL in case of error.
-FileDescriptor *file_open_new(int *const error, const char *const fname, const int flags,
-                              const int mode)
-  FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
-{
-  FileDescriptor *const fp = xmalloc(sizeof(*fp));
-  if ((*error = file_open(fp, fname, flags, mode)) != 0) {
-    xfree(fp);
-    return NULL;
-  }
-  return fp;
-}
-
-/// Like file_open_fd(), but allocate and return ret_fp
-///
-/// @param[out]  error  Error code, or 0 on success. @see os_strerror()
-/// @param[in]  fd  File descriptor to wrap.
-/// @param[in]  flags  Flags, @see FileOpenFlags.
-/// @param[in]  mode  Permissions for the newly created file (ignored if flags
-///                   does not have FILE_CREATE\*).
-///
-/// @return [allocated] Opened file or NULL in case of error.
-FileDescriptor *file_open_fd_new(int *const error, const int fd, const int flags)
-  FUNC_ATTR_NONNULL_ALL FUNC_ATTR_MALLOC FUNC_ATTR_WARN_UNUSED_RESULT
-{
-  FileDescriptor *const fp = xmalloc(sizeof(*fp));
-  if ((*error = file_open_fd(fp, fd, flags)) != 0) {
-    xfree(fp);
-    return NULL;
-  }
-  return fp;
-}
-
 /// Opens standard input as a FileDescriptor.
-FileDescriptor *file_open_stdin(void)
-  FUNC_ATTR_MALLOC FUNC_ATTR_WARN_UNUSED_RESULT
+int file_open_stdin(FileDescriptor *fp)
+  FUNC_ATTR_WARN_UNUSED_RESULT
 {
-  int error;
-  FileDescriptor *const stdin_dup = file_open_fd_new(&error, os_open_stdin_fd(),
-                                                     kFileReadOnly|kFileNonBlocking);
-  assert(stdin_dup != NULL);
+  int error = file_open_fd(fp, os_open_stdin_fd(), kFileReadOnly|kFileNonBlocking);
   if (error != 0) {
     ELOG("failed to open stdin: %s", os_strerror(error));
   }
-  return stdin_dup;
+  return error;
 }
 
 /// Close file and free its buffer
@@ -202,20 +158,6 @@ int file_close(FileDescriptor *const fp, const bool do_fsync)
   return flush_error;
 }
 
-/// Close and free file obtained using file_open_new()
-///
-/// @param[in,out]  fp  File to close.
-/// @param[in]  do_fsync  If true, use fsync() to write changes to disk.
-///
-/// @return 0 or error code.
-int file_free(FileDescriptor *const fp, const bool do_fsync)
-  FUNC_ATTR_NONNULL_ALL
-{
-  const int ret = file_close(fp, do_fsync);
-  xfree(fp);
-  return ret;
-}
-
 /// Flush file modifications to disk
 ///
 /// @param[in,out]  fp  File to work with.
diff --git a/src/nvim/shada.c b/src/nvim/shada.c
index f4a729d13d..412bf54516 100644
--- a/src/nvim/shada.c
+++ b/src/nvim/shada.c
@@ -402,7 +402,7 @@ typedef ptrdiff_t (*ShaDaFileWriter)(ShaDaWriteDef *const sd_writer,
 struct sd_write_def {
   ShaDaFileWriter write;   ///< Writer function.
   ShaDaWriteCloser close;  ///< Close function.
-  void *cookie;            ///< Data describing object written to.
+  FileDescriptor cookie;            ///< Data describing object written to.
   const char *error;       ///< Error message in case of error.
 };
 
@@ -643,7 +643,7 @@ static ptrdiff_t write_file(ShaDaWriteDef *const sd_writer, const void *const de
                             const size_t size)
   FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
 {
-  const ptrdiff_t ret = file_write(sd_writer->cookie, dest, size);
+  const ptrdiff_t ret = file_write(&sd_writer->cookie, dest, size);
   if (ret < 0) {
     sd_writer->error = os_strerror((int)ret);
     return -1;
@@ -656,13 +656,14 @@ static void close_sd_reader(ShaDaReadDef *const sd_reader)
   FUNC_ATTR_NONNULL_ALL
 {
   close_file(sd_reader->cookie);
+  xfree(sd_reader->cookie);
 }
 
 /// Wrapper for closing file descriptors opened for writing
 static void close_sd_writer(ShaDaWriteDef *const sd_writer)
   FUNC_ATTR_NONNULL_ALL
 {
-  close_file(sd_writer->cookie);
+  close_file(&sd_writer->cookie);
 }
 
 /// Wrapper for read that reads to IObuff and ignores bytes read
@@ -731,8 +732,6 @@ static ShaDaReadResult sd_reader_skip(ShaDaReadDef *const sd_reader, const size_
 static int open_shada_file_for_reading(const char *const fname, ShaDaReadDef *sd_reader)
   FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL
 {
-  int error;
-
   *sd_reader = (ShaDaReadDef) {
     .read = &read_file,
     .close = &close_sd_reader,
@@ -740,9 +739,11 @@ static int open_shada_file_for_reading(const char *const fname, ShaDaReadDef *sd
     .error = NULL,
     .eof = false,
     .fpos = 0,
-    .cookie = file_open_new(&error, fname, kFileReadOnly, 0),
+    .cookie = xmalloc(sizeof(FileDescriptor)),
   };
-  if (sd_reader->cookie == NULL) {
+  int error = file_open(sd_reader->cookie, fname, kFileReadOnly, 0);
+  if (error) {
+    XFREE_CLEAR(sd_reader->cookie);
     return error;
   }
 
@@ -754,7 +755,7 @@ static int open_shada_file_for_reading(const char *const fname, ShaDaReadDef *sd
 /// Wrapper for closing file descriptors
 static void close_file(void *cookie)
 {
-  const int error = file_free(cookie, !!p_fs);
+  const int error = file_close(cookie, !!p_fs);
   if (error != 0) {
     semsg(_(SERR "System error while closing ShaDa file: %s"),
           os_strerror(error));
@@ -3003,6 +3004,7 @@ int shada_write_file(const char *const file, bool nomerge)
     .error = NULL,
   };
   ShaDaReadDef sd_reader = { .close = NULL };
+  bool did_open_writer = false;
 
   if (!nomerge) {
     int error;
@@ -3032,8 +3034,8 @@ int shada_write_file(const char *const file, bool nomerge)
     // 3: If somebody happened to delete the file after it was opened for
     //    reading use u=rw permissions.
 shada_write_file_open: {}
-    sd_writer.cookie = file_open_new(&error, tempname, kFileCreateOnly|kFileNoSymlink, perm);
-    if (sd_writer.cookie == NULL) {
+    error = file_open(&sd_writer.cookie, tempname, kFileCreateOnly|kFileNoSymlink, perm);
+    if (error) {
       if (error == UV_EEXIST || error == UV_ELOOP) {
         // File already exists, try another name
         char *const wp = tempname + strlen(tempname) - 1;
@@ -3054,6 +3056,8 @@ shada_write_file_open: {}
         semsg(_(SERR "System error while opening temporary ShaDa file %s "
                 "for writing: %s"), tempname, os_strerror(error));
       }
+    } else {
+      did_open_writer = true;
     }
   }
   if (nomerge) {
@@ -3076,16 +3080,16 @@ shada_write_file_nomerge: {}
       }
       *tail = tail_save;
     }
-    int error;
-    sd_writer.cookie = file_open_new(&error, fname, kFileCreate|kFileTruncate,
-                                     0600);
-    if (sd_writer.cookie == NULL) {
+    int error = file_open(&sd_writer.cookie, fname, kFileCreate|kFileTruncate, 0600);
+    if (error) {
       semsg(_(SERR "System error while opening ShaDa file %s for writing: %s"),
             fname, os_strerror(error));
+    } else {
+      did_open_writer = true;
     }
   }
 
-  if (sd_writer.cookie == NULL) {
+  if (!did_open_writer) {
     xfree(fname);
     xfree(tempname);
     if (sd_reader.cookie != NULL) {
@@ -3132,7 +3136,7 @@ shada_write_file_nomerge: {}
             || old_info.stat.st_gid != getgid()) {
           const uv_uid_t old_uid = (uv_uid_t)old_info.stat.st_uid;
           const uv_gid_t old_gid = (uv_gid_t)old_info.stat.st_gid;
-          const int fchown_ret = os_fchown(file_fd(sd_writer.cookie),
+          const int fchown_ret = os_fchown(file_fd(&sd_writer.cookie),
                                            old_uid, old_gid);
           if (fchown_ret != 0) {
             semsg(_(RNERR "Failed setting uid and gid for file %s: %s"),
diff --git a/test/unit/os/fileio_spec.lua b/test/unit/os/fileio_spec.lua
index 385f4fa4b0..cc5a0d84c3 100644
--- a/test/unit/os/fileio_spec.lua
+++ b/test/unit/os/fileio_spec.lua
@@ -55,24 +55,12 @@ local function file_open(fname, flags, mode)
   return ret1, ret2
 end
 
-local function file_open_new(fname, flags, mode)
-  local ret1 = ffi.new('int[?]', 1, { 0 })
-  local ret2 = ffi.gc(m.file_open_new(ret1, fname, flags, mode), nil)
-  return ret1[0], ret2
-end
-
 local function file_open_fd(fd, flags)
   local ret2 = ffi.new('FileDescriptor')
   local ret1 = m.file_open_fd(ret2, fd, flags)
   return ret1, ret2
 end
 
-local function file_open_fd_new(fd, flags)
-  local ret1 = ffi.new('int[?]', 1, { 0 })
-  local ret2 = ffi.gc(m.file_open_fd_new(ret1, fd, flags), nil)
-  return ret1[0], ret2
-end
-
 local function file_write(fp, buf)
   return m.file_write(fp, buf, #buf)
 end
@@ -131,26 +119,6 @@ describe('file_open_fd', function()
   end)
 end)
 
-describe('file_open_fd_new', function()
-  itp('can use file descriptor returned by os_open for reading', function()
-    local fd = m.os_open(file1, m.kO_RDONLY, 0)
-    local err, fp = file_open_fd_new(fd, m.kFileReadOnly)
-    eq(0, err)
-    eq({ #fcontents, fcontents }, { file_read(fp, #fcontents) })
-    eq(0, m.file_free(fp, false))
-  end)
-  itp('can use file descriptor returned by os_open for writing', function()
-    eq(nil, uv.fs_stat(filec))
-    local fd = m.os_open(filec, m.kO_WRONLY + m.kO_CREAT, 384)
-    local err, fp = file_open_fd_new(fd, m.kFileWriteOnly)
-    eq(0, err)
-    eq(4, file_write(fp, 'test'))
-    eq(0, m.file_free(fp, false))
-    eq(4, uv.fs_stat(filec).size)
-    eq('test', io.open(filec):read('*a'))
-  end)
-end)
-
 describe('file_open', function()
   itp('can create a rwx------ file with kFileCreate', function()
     local err, fp = file_open(filec, m.kFileCreate, 448)
@@ -276,21 +244,6 @@ describe('file_open', function()
   end)
 end)
 
-describe('file_open_new', function()
-  itp('can open a file read-only', function()
-    local err, fp = file_open_new(file1, 0, 384)
-    eq(0, err)
-    eq(false, fp.wr)
-    eq(0, m.file_free(fp, false))
-  end)
-
-  itp('fails to open an existing file with kFileCreateOnly', function()
-    local err, fp = file_open_new(file1, m.kFileCreateOnly, 384)
-    eq(m.UV_EEXIST, err)
-    eq(nil, fp)
-  end)
-end)
-
 describe('file_close', function()
   itp('can flush writes to disk also with true argument', function()
     local err, fp = file_open(filec, m.kFileCreateOnly, 384)
@@ -303,18 +256,6 @@ describe('file_close', function()
   end)
 end)
 
-describe('file_free', function()
-  itp('can flush writes to disk also with true argument', function()
-    local err, fp = file_open_new(filec, m.kFileCreateOnly, 384)
-    eq(0, err)
-    local wsize = file_write(fp, 'test')
-    eq(4, wsize)
-    eq(0, uv.fs_stat(filec).size)
-    eq(0, m.file_free(fp, true))
-    eq(wsize, uv.fs_stat(filec).size)
-  end)
-end)
-
 describe('file_fsync', function()
   itp('can flush writes to disk', function()
     local err, fp = file_open(filec, m.kFileCreateOnly, 384)