executor/converter: Make nlua_pop_Object not recursive

4 files changed, 241 insertions, 95 deletions

diff --git a/src/nvim/api/private/defs.h b/src/nvim/api/private/defs.h
index 223aab09dc..86b549cb44 100644
--- a/src/nvim/api/private/defs.h
+++ b/src/nvim/api/private/defs.h
@@ -76,10 +76,10 @@ typedef struct {
 } Dictionary;
 
 typedef enum {
+  kObjectTypeNil = 0,
   kObjectTypeBuffer,
   kObjectTypeWindow,
   kObjectTypeTabpage,
-  kObjectTypeNil,
   kObjectTypeBoolean,
   kObjectTypeInteger,
   kObjectTypeFloat,
diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c
index 24959e9a59..5d862628cb 100644
--- a/src/nvim/api/vim.c
+++ b/src/nvim/api/vim.c
@@ -197,19 +197,6 @@ Object nvim_eval(String expr, Error *err)
   return rv;
 }
 
-/// Returns object given as argument
-///
-/// This API function is used for testing. One should not rely on its presence
-/// in plugins.
-///
-/// @param[in]  obj  Object to return.
-///
-/// @return its argument.
-Object _vim_id(Object obj)
-{
-  return obj;
-}
-
 /// Calls a VimL function with the given arguments
 ///
 /// On VimL error: Returns a generic error; v:errmsg is not updated.
@@ -843,3 +830,44 @@ static void write_msg(String message, bool to_err)
   --no_wait_return;
   msg_end();
 }
+
+// Functions used for testing purposes
+
+/// Returns object given as argument
+///
+/// This API function is used for testing. One should not rely on its presence
+/// in plugins.
+///
+/// @param[in]  obj  Object to return.
+///
+/// @return its argument.
+Object _vim_id(Object obj)
+{
+  return obj;
+}
+
+/// Returns array given as argument
+///
+/// This API function is used for testing. One should not rely on its presence
+/// in plugins.
+///
+/// @param[in]  arr  Array to return.
+///
+/// @return its argument.
+Array _vim_id_array(Array arr)
+{
+  return arr;
+}
+
+/// Returns dictionary given as argument
+///
+/// This API function is used for testing. One should not rely on its presence
+/// in plugins.
+///
+/// @param[in]  dct  Dictionary to return.
+///
+/// @return its argument.
+Dictionary _vim_id_dictionary(Dictionary dct)
+{
+  return dct;
+}
diff --git a/src/nvim/viml/executor/converter.c b/src/nvim/viml/executor/converter.c
index c8d6848006..6c9b42b38c 100644
--- a/src/nvim/viml/executor/converter.c
+++ b/src/nvim/viml/executor/converter.c
@@ -58,7 +58,7 @@ static LuaTableProps nlua_traverse_table(lua_State *const lstate)
                               // or type keys.
   LuaTableProps ret;
   memset(&ret, 0, sizeof(ret));
-  if (!lua_checkstack(lstate, lua_gettop(lstate) + 2)) {
+  if (!lua_checkstack(lstate, lua_gettop(lstate) + 3)) {
     emsgf(_("E1502: Lua failed to grow stack to %i"), lua_gettop(lstate) + 2);
     ret.type = kObjectTypeNil;
     return ret;
@@ -168,7 +168,7 @@ typedef struct {
   bool container;  ///< True if tv is a container.
   bool special;  ///< If true then tv is a _VAL part of special dictionary
                  ///< that represents mapping.
-} PopStackItem;
+} TVPopStackItem;
 
 /// Convert lua object to VimL typval_T
 ///
@@ -182,18 +182,16 @@ typedef struct {
 bool nlua_pop_typval(lua_State *lstate, typval_T *ret_tv)
 {
   bool ret = true;
-#ifndef NDEBUG
   const int initial_size = lua_gettop(lstate);
-#endif
-  kvec_t(PopStackItem) stack = KV_INITIAL_VALUE;
-  kv_push(stack, ((PopStackItem) { ret_tv, false, false }));
+  kvec_t(TVPopStackItem) stack = KV_INITIAL_VALUE;
+  kv_push(stack, ((TVPopStackItem) { ret_tv, false, false }));
   while (ret && kv_size(stack)) {
     if (!lua_checkstack(lstate, lua_gettop(lstate) + 3)) {
       emsgf(_("E1502: Lua failed to grow stack to %i"), lua_gettop(lstate) + 3);
       ret = false;
       break;
     }
-    PopStackItem cur = kv_pop(stack);
+    TVPopStackItem cur = kv_pop(stack);
     if (cur.container) {
       if (cur.special || cur.tv->v_type == VAR_DICT) {
         assert(cur.tv->v_type == (cur.special ? VAR_LIST : VAR_DICT));
@@ -222,14 +220,14 @@ bool nlua_pop_typval(lua_State *lstate, typval_T *ret_tv)
             listitem_T *const  val = listitem_alloc();
             list_append(kv_pair, val);
             kv_push(stack, cur);
-            cur = (PopStackItem) { &val->li_tv, false, false };
+            cur = (TVPopStackItem) { &val->li_tv, false, false };
           } else {
             dictitem_T *const di = dictitem_alloc_len(s, len);
             if (dict_add(cur.tv->vval.v_dict, di) == FAIL) {
               assert(false);
             }
             kv_push(stack, cur);
-            cur = (PopStackItem) { &di->di_tv, false, false };
+            cur = (TVPopStackItem) { &di->di_tv, false, false };
           }
         } else {
           lua_pop(lstate, 1);
@@ -239,14 +237,13 @@ bool nlua_pop_typval(lua_State *lstate, typval_T *ret_tv)
         assert(cur.tv->v_type == VAR_LIST);
         lua_rawgeti(lstate, -1, cur.tv->vval.v_list->lv_len + 1);
         if (lua_isnil(lstate, -1)) {
-          lua_pop(lstate, 1);
-          lua_pop(lstate, 1);
+          lua_pop(lstate, 2);
           continue;
         }
         listitem_T *li = listitem_alloc();
         list_append(cur.tv->vval.v_list, li);
         kv_push(stack, cur);
-        cur = (PopStackItem) { &li->li_tv, false, false };
+        cur = (TVPopStackItem) { &li->li_tv, false, false };
       }
     }
     assert(!cur.container);
@@ -294,14 +291,10 @@ bool nlua_pop_typval(lua_State *lstate, typval_T *ret_tv)
 
         switch (table_props.type) {
           case kObjectTypeArray: {
-            if (table_props.maxidx == 0) {
-              cur.tv->v_type = VAR_LIST;
-              cur.tv->vval.v_list = list_alloc();
-              cur.tv->vval.v_list->lv_refcount++;
-            } else {
-              cur.tv->v_type = VAR_LIST;
-              cur.tv->vval.v_list = list_alloc();
-              cur.tv->vval.v_list->lv_refcount++;
+            cur.tv->v_type = VAR_LIST;
+            cur.tv->vval.v_list = list_alloc();
+            cur.tv->vval.v_list->lv_refcount++;
+            if (table_props.maxidx != 0) {
               cur.container = true;
               kv_push(stack, cur);
             }
@@ -525,7 +518,7 @@ bool nlua_pop_typval(lua_State *lstate, typval_T *ret_tv)
 bool nlua_push_typval(lua_State *lstate, typval_T *const tv)
 {
   const int initial_size = lua_gettop(lstate);
-  if (!lua_checkstack(lstate, initial_size + 1)) {
+  if (!lua_checkstack(lstate, initial_size + 2)) {
     emsgf(_("E1502: Lua failed to grow stack to %i"), initial_size + 4);
     return false;
   }
@@ -873,7 +866,6 @@ Array nlua_pop_Array(lua_State *lstate, Error *err)
 {
   const LuaTableProps table_props = nlua_check_type(lstate, err,
                                                     kObjectTypeArray);
-  lua_pop(lstate, 1);
   if (table_props.type != kObjectTypeArray) {
     return (Array) { .size = 0, .items = NULL };
   }
@@ -954,76 +946,179 @@ Dictionary nlua_pop_Dictionary(lua_State *lstate, Error *err)
   return nlua_pop_Dictionary_unchecked(lstate, table_props, err);
 }
 
+/// Helper structure for nlua_pop_Object
+typedef struct {
+  Object *obj;  ///< Location where conversion result is saved.
+  bool container;  ///< True if tv is a container.
+} ObjPopStackItem;
+
 /// Convert lua table to object
 ///
 /// Always pops one value from the stack.
-Object nlua_pop_Object(lua_State *lstate, Error *err)
-  FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
+Object nlua_pop_Object(lua_State *const lstate, Error *const err)
 {
-  Object ret = { .type = kObjectTypeNil };
-
-  switch (lua_type(lstate, -1)) {
-    case LUA_TNIL: {
-      ret.type = kObjectTypeNil;
-      lua_pop(lstate, 1);
-      break;
-    }
-    case LUA_TSTRING: {
-      ret.type = kObjectTypeString;
-      ret.data.string = nlua_pop_String(lstate, err);
+  Object ret = NIL;
+  const int initial_size = lua_gettop(lstate);
+  kvec_t(ObjPopStackItem) stack = KV_INITIAL_VALUE;
+  kv_push(stack, ((ObjPopStackItem) { &ret, false }));
+  while (!err->set && kv_size(stack)) {
+    if (!lua_checkstack(lstate, lua_gettop(lstate) + 3)) {
+      api_set_error(err, Exception, "Lua failed to grow stack");
       break;
     }
-    case LUA_TNUMBER: {
-      const lua_Number n = lua_tonumber(lstate, -1);
-      if (n > (lua_Number)API_INTEGER_MAX || n < (lua_Number)API_INTEGER_MIN
-          || ((lua_Number)((Integer)n)) != n) {
-        ret.type = kObjectTypeFloat;
-        ret.data.floating = (Float)n;
+    ObjPopStackItem cur = kv_pop(stack);
+    if (cur.container) {
+      if (cur.obj->type == kObjectTypeDictionary) {
+        // stack: …, dict, key
+        if (cur.obj->data.dictionary.size
+            == cur.obj->data.dictionary.capacity) {
+          lua_pop(lstate, 2);
+          continue;
+        }
+        bool next_key_found = false;
+        while (lua_next(lstate, -2)) {
+          // stack: …, dict, new key, val
+          if (lua_type(lstate, -2) == LUA_TSTRING) {
+            next_key_found = true;
+            break;
+          }
+          lua_pop(lstate, 1);
+          // stack: …, dict, new key
+        }
+        if (next_key_found) {
+          // stack: …, dict, new key, val
+          size_t len;
+          const char *s = lua_tolstring(lstate, -2, &len);
+          const size_t idx = cur.obj->data.dictionary.size++;
+          cur.obj->data.dictionary.items[idx].key = (String) {
+            .data = xmemdupz(s, len),
+            .size = len,
+          };
+          kv_push(stack, cur);
+          cur = (ObjPopStackItem) {
+            .obj = &cur.obj->data.dictionary.items[idx].value,
+            .container = false,
+          };
+        } else {
+          // stack: …, dict
+          lua_pop(lstate, 1);
+          // stack: …
+          continue;
+        }
       } else {
-        ret.type = kObjectTypeInteger;
-        ret.data.integer = (Integer)n;
-      }
-      lua_pop(lstate, 1);
-      break;
-    }
-    case LUA_TBOOLEAN: {
-      ret.type = kObjectTypeBoolean;
-      ret.data.boolean = nlua_pop_Boolean(lstate, err);
-      break;
-    }
-    case LUA_TTABLE: {
-      const LuaTableProps table_props = nlua_traverse_table(lstate);
-      ret.type = table_props.type;
-      switch (table_props.type) {
-        case kObjectTypeArray: {
-          ret.data.array = nlua_pop_Array_unchecked(lstate, table_props, err);
-          break;
+        if (cur.obj->data.array.size == cur.obj->data.array.capacity) {
+          lua_pop(lstate, 1);
+          continue;
         }
-        case kObjectTypeDictionary: {
-          ret.data.dictionary = nlua_pop_Dictionary_unchecked(lstate,
-                                                              table_props, err);
-          break;
+        const size_t idx = cur.obj->data.array.size++;
+        lua_rawgeti(lstate, -1, (int)idx + 1);
+        if (lua_isnil(lstate, -1)) {
+          lua_pop(lstate, 2);
+          continue;
         }
-        case kObjectTypeFloat: {
-          ret.data.floating = (Float)table_props.val;
-          break;
+        kv_push(stack, cur);
+        cur = (ObjPopStackItem) {
+          .obj = &cur.obj->data.array.items[idx],
+          .container = false,
+        };
+      }
+    }
+    assert(!cur.container);
+    *cur.obj = NIL;
+    switch (lua_type(lstate, -1)) {
+      case LUA_TNIL: {
+        break;
+      }
+      case LUA_TBOOLEAN: {
+        *cur.obj = BOOLEAN_OBJ(lua_toboolean(lstate, -1));
+        break;
+      }
+      case LUA_TSTRING: {
+        size_t len;
+        const char *s = lua_tolstring(lstate, -1, &len);
+        *cur.obj = STRING_OBJ(((String) {
+          .data = xmemdupz(s, len),
+          .size = len,
+        }));
+        break;
+      }
+      case LUA_TNUMBER: {
+        const lua_Number n = lua_tonumber(lstate, -1);
+        if (n > (lua_Number)API_INTEGER_MAX || n < (lua_Number)API_INTEGER_MIN
+            || ((lua_Number)((Integer)n)) != n) {
+          *cur.obj = FLOATING_OBJ((Float)n);
+        } else {
+          *cur.obj = INTEGER_OBJ((Integer)n);
         }
-        default: {
-          assert(false);
+        break;
+      }
+      case LUA_TTABLE: {
+        const LuaTableProps table_props = nlua_traverse_table(lstate);
+
+        switch (table_props.type) {
+          case kObjectTypeArray: {
+            *cur.obj = ARRAY_OBJ(((Array) {
+              .items = NULL,
+              .size = 0,
+              .capacity = 0,
+            }));
+            if (table_props.maxidx != 0) {
+              cur.obj->data.array.items =
+                  xcalloc(table_props.maxidx,
+                          sizeof(cur.obj->data.array.items[0]));
+              cur.obj->data.array.capacity = table_props.maxidx;
+              cur.container = true;
+              kv_push(stack, cur);
+            }
+            break;
+          }
+          case kObjectTypeDictionary: {
+            *cur.obj = DICTIONARY_OBJ(((Dictionary) {
+              .items = NULL,
+              .size = 0,
+              .capacity = 0,
+            }));
+            if (table_props.string_keys_num != 0) {
+              cur.obj->data.dictionary.items =
+                  xcalloc(table_props.string_keys_num,
+                          sizeof(cur.obj->data.dictionary.items[0]));
+              cur.obj->data.dictionary.capacity = table_props.string_keys_num;
+              cur.container = true;
+              kv_push(stack, cur);
+              lua_pushnil(lstate);
+            }
+            break;
+          }
+          case kObjectTypeFloat: {
+            *cur.obj = FLOATING_OBJ((Float)table_props.val);
+            break;
+          }
+          case kObjectTypeNil: {
+            api_set_error(err, Validation, "Cannot convert given lua table");
+            break;
+          }
+          default: {
+            assert(false);
+          }
         }
+        break;
+      }
+      default: {
+        api_set_error(err, Validation, "Cannot convert given lua type");
+        break;
       }
-      break;
     }
-    default: {
+    if (!cur.container) {
       lua_pop(lstate, 1);
-      set_api_error("Cannot convert given lua type", err);
-      break;
     }
   }
+  kv_destroy(stack);
   if (err->set) {
-    ret.type = kObjectTypeNil;
+    api_free_object(ret);
+    ret = NIL;
+    lua_pop(lstate, lua_gettop(lstate) - initial_size + 1);
   }
-
+  assert(lua_gettop(lstate) == initial_size - 1);
   return ret;
 }
 
diff --git a/test/functional/lua_spec.lua b/test/functional/lua_spec.lua
index e16f41dba2..bd1ed51c2d 100644
--- a/test/functional/lua_spec.lua
+++ b/test/functional/lua_spec.lua
@@ -31,6 +31,9 @@ describe('luaeval() function', function()
     nested_by_level[i] = {o=nested, s=nested_s}
   end
 
+  -- Not checked: funcrefs converted to NIL. To be altered to something more
+  -- meaningful later.
+
   it('correctly evaluates scalars', function()
     eq(1, funcs.luaeval('1'))
     eq(0, eval('type(luaeval("1"))'))
@@ -135,6 +138,10 @@ describe('luaeval() function', function()
                exc_exec('call luaeval("1, 2, 3")'))
     startswith("Vim(call):E5108: Error while calling lua chunk for luaeval(): ",
                exc_exec('call luaeval("(nil)()")'))
+    eq("Vim(call):E5101: Cannot convert given lua type",
+       exc_exec('call luaeval("{42, vim.api}")'))
+    eq("Vim(call):E5101: Cannot convert given lua type",
+       exc_exec('call luaeval("{foo=42, baz=vim.api}")'))
 
     -- The following should not crash: conversion error happens inside
     eq("Vim(call):E5101: Cannot convert given lua type",
@@ -208,9 +215,7 @@ describe('luaeval() function', function()
     eq({foo=1, bar={42, {{baz=true}, 5}}}, funcs.luaeval('vim.api._vim_id({foo=1, bar={42, {{baz=true}, 5}}})'))
   end)
 
-  it('correctly converts containers with type_idx to API objects', function()
-    -- TODO: Similar tests with _vim_array_id and _vim_dictionary_id, that will
-    -- follow slightly different code paths.
+  it('correctly converts container objects with type_idx to API objects', function()
     eq(5, eval('type(luaeval("vim.api._vim_id({[vim.type_idx]=vim.types.float, [vim.val_idx]=0})"))'))
     eq(4, eval([[type(luaeval('vim.api._vim_id({[vim.type_idx]=vim.types.dictionary})'))]]))
     eq(3, eval([[type(luaeval('vim.api._vim_id({[vim.type_idx]=vim.types.array})'))]]))
@@ -223,10 +228,18 @@ describe('luaeval() function', function()
     eq(10, funcs.luaeval('vim.api._vim_id({[vim.type_idx]=vim.types.float, [vim.val_idx]=10, [5]=1, foo=2, [1]=42})'))
     eq({}, funcs.luaeval('vim.api._vim_id({[vim.type_idx]=vim.types.array, [vim.val_idx]=10, [5]=1, foo=2})'))
   end)
-  -- TODO: check what happens when it errors out on second list item
-  -- TODO: check what happens if API function receives wrong number of
-  -- arguments.
-  -- TODO: check what happens if API function receives wrong argument types.
+
+  it('correctly converts arrays with type_idx to API objects', function()
+    eq(3, eval([[type(luaeval('vim.api._vim_id_array({[vim.type_idx]=vim.types.array})'))]]))
+
+    eq({}, funcs.luaeval('vim.api._vim_id_array({[vim.type_idx]=vim.types.array})'))
+
+    -- Presence of type_idx makes Vim ignore some keys
+    eq({42}, funcs.luaeval('vim.api._vim_id_array({[vim.type_idx]=vim.types.array, [vim.val_idx]=10, [5]=1, foo=2, [1]=42})'))
+    eq({{foo=2}}, funcs.luaeval('vim.api._vim_id_array({{[vim.type_idx]=vim.types.dictionary, [vim.val_idx]=10, [5]=1, foo=2, [1]=42}})'))
+    eq({10}, funcs.luaeval('vim.api._vim_id_array({{[vim.type_idx]=vim.types.float, [vim.val_idx]=10, [5]=1, foo=2, [1]=42}})'))
+    eq({}, funcs.luaeval('vim.api._vim_id_array({[vim.type_idx]=vim.types.array, [vim.val_idx]=10, [5]=1, foo=2})'))
+  end)
 
   it('correctly converts self-containing containers', function()
     meths.set_var('l', {})
@@ -243,4 +256,14 @@ describe('luaeval() function', function()
     eq(true, eval('luaeval("_A.d == _A.d.d", {"d": d})'))
     eq(true, eval('luaeval("_A ~= _A.d", {"d": d})'))
   end)
+
+  it('errors out correctly when working with API', function()
+    eq(0, exc_exec([[call luaeval("vim.api.id")]]))
+  end)
+
+  -- TODO: check buffer/window/etc.
+  -- TODO: check what happens when it errors out on second list item
+  -- TODO: check what happens if API function receives wrong number of
+  -- arguments.
+  -- TODO: check what happens if API function receives wrong argument types.
 end)