eval: Fix extend() behaviour with NULL lists and dictionaries

Ref #4615
Ref vim/vim#768

src/nvim/eval.c

@@ -8155,15 +8155,20 @@ static void f_extend(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   const size_t arg_errmsg_len = strlen(arg_errmsg);
 
   if (argvars[0].v_type == VAR_LIST && argvars[1].v_type == VAR_LIST) {
-    list_T          *l1, *l2;
-    listitem_T      *item;
     long before;
     bool error = false;
 
-    l1 = argvars[0].vval.v_list;
+    list_T *const l1 = argvars[0].vval.v_list;
-    l2 = argvars[1].vval.v_list;
+    list_T *const l2 = argvars[1].vval.v_list;
-    if (l1 != NULL && !tv_check_lock(l1->lv_lock, arg_errmsg, arg_errmsg_len)
+    if (l1 == NULL) {
-        && l2 != NULL) {
+      const bool locked = tv_check_lock(VAR_FIXED, arg_errmsg, arg_errmsg_len);
+      (void)locked;
+      assert(locked == true);
+    } else if (l2 == NULL) {
+      // Do nothing
+      tv_copy(&argvars[0], rettv);
+    } else if (!tv_check_lock(l1->lv_lock, arg_errmsg, arg_errmsg_len)) {
+      listitem_T *item;
       if (argvars[2].v_type != VAR_UNKNOWN) {
         before = tv_get_number_chk(&argvars[2], &error);
         if (error) {
@@ -8187,13 +8192,16 @@ static void f_extend(typval_T *argvars, typval_T *rettv, FunPtr fptr)
     }
   } else if (argvars[0].v_type == VAR_DICT && argvars[1].v_type ==
              VAR_DICT) {
-    dict_T *d1;
+    dict_T *const d1 = argvars[0].vval.v_dict;
-    dict_T *d2;
+    dict_T *const d2 = argvars[1].vval.v_dict;
-
+    if (d1 == NULL) {
-    d1 = argvars[0].vval.v_dict;
+      const bool locked = tv_check_lock(VAR_FIXED, arg_errmsg, arg_errmsg_len);
-    d2 = argvars[1].vval.v_dict;
+      (void)locked;
-    if (d1 != NULL && !tv_check_lock(d1->dv_lock, arg_errmsg, arg_errmsg_len)
+      assert(locked == true);
-        && d2 != NULL) {
+    } else if (d2 == NULL) {
+      // Do nothing
+      tv_copy(&argvars[0], rettv);
+    } else if (!tv_check_lock(d1->dv_lock, arg_errmsg, arg_errmsg_len)) {
       const char *action = "force";
       // Check the third argument.
       if (argvars[2].v_type != VAR_UNKNOWN) {

test/functional/eval/null_spec.lua

@@ -20,13 +20,12 @@ describe('NULL', function()
   after_each(function()
     os.remove(tmpfname)
   end)
-  describe('list', function()
+  local null_test = function(name, cmd, err)
-    local null_list_test = function(name, cmd, err)
     it(name, function()
       eq(err, exc_exec(cmd))
     end)
   end
-    local null_list_expr_test = function(name, expr, err, val, after)
+  local null_expr_test = function(name, expr, err, val, after)
     it(name, function()
       eq((err == 0) and ('') or ('\n' .. err),
          redir_exec('let g:_var = ' .. expr))
@@ -40,100 +39,100 @@ describe('NULL', function()
       end
     end)
   end
-
+  describe('list', function()
     -- Incorrect behaviour
 
     -- FIXME map() should not return 0 without error
-    null_list_expr_test('does not crash map()', 'map(L, "v:val")', 0, 0)
+    null_expr_test('does not crash map()', 'map(L, "v:val")', 0, 0)
     -- FIXME map() should not return 0 without error
-    null_list_expr_test('does not crash filter()', 'filter(L, "1")', 0, 0)
+    null_expr_test('does not crash filter()', 'filter(L, "1")', 0, 0)
     -- FIXME map() should at least return L
-    null_list_expr_test('makes map() return v:_null_list', 'map(L, "v:val") is# L', 0, 0)
+    null_expr_test('makes map() return v:_null_list', 'map(L, "v:val") is# L', 0, 0)
     -- FIXME filter() should at least return L
-    null_list_expr_test('makes filter() return v:_null_list', 'map(L, "1") is# L', 0, 0)
+    null_expr_test('makes filter() return v:_null_list', 'map(L, "1") is# L', 0, 0)
     -- FIXME add() should not return 1 at all
-    null_list_expr_test('does not crash add()', 'add(L, 0)', 0, 1)
+    null_expr_test('does not crash add()', 'add(L, 0)', 0, 1)
-    -- FIXME extend() should not return 0 without error
+    null_expr_test('does not crash extend()', 'extend(L, [1])', 'E742: Cannot change value of extend() argument', 0)
-    null_list_expr_test('does not crash extend()', 'extend(L, [1])', 0, 0)
+    null_expr_test('does not crash extend() (second position)', 'extend([1], L)', 0, {1})
-    -- FIXME extend() should not return 0 at all
-    null_list_expr_test('does not crash extend() (second position)', 'extend([1], L)', 0, 0)
     -- FIXME should be accepted by inputlist()
-    null_list_expr_test('is accepted as an empty list by inputlist()',
+    null_expr_test('is accepted as an empty list by inputlist()',
                         '[feedkeys("\\n"), inputlist(L)]', 'E686: Argument of inputlist() must be a List', {0, 0})
     -- FIXME should be accepted by writefile(), return {0, {}}
-    null_list_expr_test('is accepted as an empty list by writefile()',
+    null_expr_test('is accepted as an empty list by writefile()',
                         ('[writefile(L, "%s"), readfile("%s")]'):format(tmpfname, tmpfname),
                         'E484: Can\'t open file ' .. tmpfname, {0, {}})
     -- FIXME should give error message
-    null_list_expr_test('does not crash remove()', 'remove(L, 0)', 0, 0)
+    null_expr_test('does not crash remove()', 'remove(L, 0)', 0, 0)
     -- FIXME should return 0
-    null_list_expr_test('is accepted by setqflist()', 'setqflist(L)', 0, -1)
+    null_expr_test('is accepted by setqflist()', 'setqflist(L)', 0, -1)
     -- FIXME should return 0
-    null_list_expr_test('is accepted by setloclist()', 'setloclist(1, L)', 0, -1)
+    null_expr_test('is accepted by setloclist()', 'setloclist(1, L)', 0, -1)
     -- FIXME should return 0
-    null_list_expr_test('is accepted by setmatches()', 'setmatches(L)', 0, -1)
+    null_expr_test('is accepted by setmatches()', 'setmatches(L)', 0, -1)
     -- FIXME should return empty list or error out
-    null_list_expr_test('is accepted by sort()', 'sort(L)', 0, 0)
+    null_expr_test('is accepted by sort()', 'sort(L)', 0, 0)
     -- FIXME Should return 1
-    null_list_expr_test('is accepted by sort()', 'sort(L) is L', 0, 0)
+    null_expr_test('is accepted by sort()', 'sort(L) is L', 0, 0)
     -- FIXME should not error out
-    null_list_test('is accepted by :cexpr', 'cexpr L', 'Vim(cexpr):E777: String or List expected')
+    null_test('is accepted by :cexpr', 'cexpr L', 'Vim(cexpr):E777: String or List expected')
     -- FIXME should not error out
-    null_list_test('is accepted by :lexpr', 'lexpr L', 'Vim(lexpr):E777: String or List expected')
+    null_test('is accepted by :lexpr', 'lexpr L', 'Vim(lexpr):E777: String or List expected')
-    null_list_test('is accepted by :for', 'for x in L|throw x|endfor', 0)
+    null_test('is accepted by :for', 'for x in L|throw x|endfor', 0)
 
     -- Subjectable behaviour
 
     -- FIXME Should return 1
-    null_list_expr_test('is equal to empty list', 'L == []', 0, 0)
+    null_expr_test('is equal to empty list', 'L == []', 0, 0)
     -- FIXME Should return 1
-    null_list_expr_test('is equal to empty list (reverse order)', '[] == L', 0, 0)
+    null_expr_test('is equal to empty list (reverse order)', '[] == L', 0, 0)
     -- FIXME Should return 1
-    null_list_expr_test('is not locked', 'islocked("v:_null_list")', 0, 0)
+    null_expr_test('is not locked', 'islocked("v:_null_list")', 0, 0)
 
     -- Crashes
 
-    -- null_list_expr_test('does not crash setreg', 'setreg("x", L)', 0, 0)
+    -- null_expr_test('does not crash setreg', 'setreg("x", L)', 0, 0)
-    -- null_list_expr_test('does not crash setline', 'setline(1, L)', 0, 0)
+    -- null_expr_test('does not crash setline', 'setline(1, L)', 0, 0)
-    -- null_list_expr_test('does not crash system()', 'system("cat", L)', 0, '')
+    -- null_expr_test('does not crash system()', 'system("cat", L)', 0, '')
-    -- null_list_expr_test('does not crash systemlist()', 'systemlist("cat", L)', 0, {})
+    -- null_expr_test('does not crash systemlist()', 'systemlist("cat", L)', 0, {})
 
     -- Correct behaviour
-    null_list_expr_test('does not crash append()', 'append(1, L)', 0, 0, function()
+    null_expr_test('does not crash append()', 'append(1, L)', 0, 0, function()
       eq({''}, curbufmeths.get_lines(0, -1, false))
     end)
-    null_list_expr_test('is identical to itself', 'L is L', 0, 1)
+    null_expr_test('is identical to itself', 'L is L', 0, 1)
-    null_list_expr_test('can be sliced', 'L[:]', 0, {})
+    null_expr_test('can be sliced', 'L[:]', 0, {})
-    null_list_expr_test('can be copied', 'copy(L)', 0, {})
+    null_expr_test('can be copied', 'copy(L)', 0, {})
-    null_list_expr_test('can be deepcopied', 'deepcopy(L)', 0, {})
+    null_expr_test('can be deepcopied', 'deepcopy(L)', 0, {})
-    null_list_expr_test('does not crash when indexed', 'L[1]',
+    null_expr_test('does not crash when indexed', 'L[1]',
                         'E684: list index out of range: 1\nE15: Invalid expression: L[1]', nil)
-    null_list_expr_test('does not crash call()', 'call("arglistid", L)', 0, 0)
+    null_expr_test('does not crash call()', 'call("arglistid", L)', 0, 0)
-    null_list_expr_test('does not crash col()', 'col(L)', 0, 0)
+    null_expr_test('does not crash col()', 'col(L)', 0, 0)
-    null_list_expr_test('does not crash virtcol()', 'virtcol(L)', 0, 0)
+    null_expr_test('does not crash virtcol()', 'virtcol(L)', 0, 0)
-    null_list_expr_test('does not crash line()', 'line(L)', 0, 0)
+    null_expr_test('does not crash line()', 'line(L)', 0, 0)
-    null_list_expr_test('does not crash count()', 'count(L, 1)', 0, 0)
+    null_expr_test('does not crash count()', 'count(L, 1)', 0, 0)
-    null_list_expr_test('does not crash cursor()', 'cursor(L)', 'E474: Invalid argument', -1)
+    null_expr_test('does not crash cursor()', 'cursor(L)', 'E474: Invalid argument', -1)
-    null_list_expr_test('is empty', 'empty(L)', 0, 1)
+    null_expr_test('is empty', 'empty(L)', 0, 1)
-    null_list_expr_test('does not crash get()', 'get(L, 1, 10)', 0, 10)
+    null_expr_test('does not crash get()', 'get(L, 1, 10)', 0, 10)
-    null_list_expr_test('has zero length', 'len(L)', 0, 0)
+    null_expr_test('has zero length', 'len(L)', 0, 0)
-    null_list_expr_test('is accepted as an empty list by max()', 'max(L)', 0, 0)
+    null_expr_test('is accepted as an empty list by max()', 'max(L)', 0, 0)
-    null_list_expr_test('is accepted as an empty list by min()', 'min(L)', 0, 0)
+    null_expr_test('is accepted as an empty list by min()', 'min(L)', 0, 0)
-    null_list_expr_test('is stringified correctly', 'string(L)', 0, '[]')
+    null_expr_test('is stringified correctly', 'string(L)', 0, '[]')
-    null_list_expr_test('is JSON encoded correctly', 'json_encode(L)', 0, '[]')
+    null_expr_test('is JSON encoded correctly', 'json_encode(L)', 0, '[]')
-    null_list_test('does not crash lockvar', 'lockvar! L', 0)
+    null_test('does not crash lockvar', 'lockvar! L', 0)
-    null_list_expr_test('can be added to itself', '(L + L)', 0, {})
+    null_expr_test('can be added to itself', '(L + L)', 0, {})
-    null_list_expr_test('can be added to itself', '(L + L) is L', 0, 1)
+    null_expr_test('can be added to itself', '(L + L) is L', 0, 1)
-    null_list_expr_test('can be added to non-empty list', '([1] + L)', 0, {1})
+    null_expr_test('can be added to non-empty list', '([1] + L)', 0, {1})
-    null_list_expr_test('can be added to non-empty list (reversed)', '(L + [1])', 0, {1})
+    null_expr_test('can be added to non-empty list (reversed)', '(L + [1])', 0, {1})
-    null_list_expr_test('is equal to itself', 'L == L', 0, 1)
+    null_expr_test('is equal to itself', 'L == L', 0, 1)
-    null_list_expr_test('is not not equal to itself', 'L != L', 0, 0)
+    null_expr_test('is not not equal to itself', 'L != L', 0, 0)
-    null_list_expr_test('counts correctly', 'count([L], L)', 0, 1)
+    null_expr_test('counts correctly', 'count([L], L)', 0, 1)
   end)
   describe('dict', function()
     it('does not crash when indexing NULL dict', function()
       eq('\nE716: Key not present in Dictionary: test\nE15: Invalid expression: v:_null_dict.test',
          redir_exec('echo v:_null_dict.test'))
     end)
+    null_expr_test('makes extend error out', 'extend(D, {})', 'E742: Cannot change value of extend() argument', 0)
+    null_expr_test('makes extend do nothing', 'extend({1: 2}, D)', 0, {['1']=2})
   end)
 end)