Vim9: no error for using `extend()` to change type of argument

[lacygoill]

Steps to reproduce

Run this shell command:

vim -Nu NONE -S <(tee <<'EOF'
    vim9script
    var d: dict<any> = {a: 0}
    d->extend({b: ''})
EOF
)

No error is given.

Expected behavior

A type mismatch error is given. Maybe this one:

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

Version of Vim

8.2 Included patches: 1-3978

Environment

Operating system: Ubuntu 20.04.3 LTS Terminal: xterm Value of $TERM: xterm-256color Shell: zsh 5.8

Additional Context

In a function, the exact same code does give an error at runtime:

vim9script
def Func()
    var d: dict<any> = {a: 0}
    d->extend({b: ''})
enddef
Func()

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

Which is expected, but also inconsistent with regards to what happens at the script level.

---

I know why no error is given. It's because d is declared with dict<any> (and not dict<number>). I still think this is not working as intended. See this comment for the reason why.

---

The same issue probably applies to other functions which can change the type of their arguments. Like map():

vim9script
var d: dict<any> = {a: 0}
d->map((k, v) => true)

no error

vim9script
def Func()
    var d: dict<any> = {a: 0}
    d->map((k, v) => true)
enddef
Func()

E1012: Type mismatch; expected number but got bool in map()

[brammool]

Steps to reproduce

Run this shell command:

vim -Nu NONE -S <(tee <<'EOF'
    vim9script
    var d: dict<any> = {a: 0}
    d->extend({b: ''})
EOF
)

No error is given.

Expected behavior

A type mismatch error is given. Maybe this one:

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

In this case the variable is declared as dict. It just happens to be initialzed with a number value, but that should not change the type. The type declaration is "stronger" than the value.

Version of Vim

8.2 Included patches: 1-3978

Environment

Operating system: Ubuntu 20.04.3 LTS Terminal: xterm Value of $TERM: xterm-256color Shell: zsh 5.8

Additional Context

In a function, the exact same code does give an error at runtime:

vim9script
def Func()
    var d: dict<any> = {a: 0}
    d->extend({b: ''})
enddef
Func()

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

Which is expected, but also inconsistent with regards to what happens at the script level.

I argue that the compiled function is wrong. I haven't yet checked where the error comes from, but there is no instruction to check it:

 d->extend({b: ''})

4 LOAD $0 5 PUSHS "b" 6 PUSHS "" 7 NEWDICT size 1 8 BCALL extend(argc 2) 9 DROP

The same issue probably applies to other functions which can change the type of their arguments. Like map():

vim9script
var d: dict<any> = {a: 0}
d->map((k, v) => true)

no error

vim9script
def Func()
    var d: dict<any> = {a: 0}
    d->map((k, v) => true)
enddef
Func()

E1012: Type mismatch; expected number but got bool in map()

I also think this error should not be given.

-- Some of the well known MS-Windows errors: EMULTI Multitasking attempted, system confused EKEYBOARD Keyboard locked, try getting out of this one! EXPLAIN Unexplained error, please tell us what happened EFUTURE Reserved for our future mistakes

/// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net \\ /// \\ \\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\ help me help AIDS victims -- http://ICCF-Holland.org ///

[lacygoill]

In this case the variable is declared as dict. It just happens to be initialzed with a number value, but that should not change the type. The type declaration is "stronger" than the value.

I think this would be a good change, because it would mean fewer useless/annoying errors.

But it would be a significant change. For example, in runtime/autoload/ccomplete.vim, we might replace some (all?) of the 10 extendnew() with extend(), which would improve the performance. More generally, in a function, some invocations of *new() functions (mapnew(), extendnew(), flattennew()) could then be replaced with their counterpart without the new suffix.

[lacygoill]

I think this would be a good change, because it would mean fewer useless/annoying errors.

I quoted the wrong excerpt. Anyway, I meant that not giving an error in the function was a good idea.

[brammool]

I have attempted to implement this in 8.2.3991. Hardly any existing tests fails, one test no longer fails. Thus the impact of this change is limited. It's a bit tricky though, sometimes we need to use the declared type, sometimes the actual type (which can be more specific). Open an issue if you find a place where it doesn't look right.

[lacygoill]

Thus the impact of this change is limited.

It is limited in the sense that it should not break anything, because it removes a useless constraint: there was no need to give an error in a function.

But it might have an impact in terms of performance optimizations. For example, I think runtime/autoload/ccomplete.vim no longer requires any extendnew():

diff --git a/runtime/autoload/ccomplete.vim b/runtime/autoload/ccomplete.vim
index d8675faf6..b3f886a56 100644
--- a/runtime/autoload/ccomplete.vim
+++ b/runtime/autoload/ccomplete.vim
@@ -202,7 +202,7 @@ def ccomplete#Complete(findstart: bool, abase: string): any # {{{1
               || !v['static']
               || bufnr('%') == bufnr(v['filename']))

-    res = extendnew(res, tags->map((_, v: dict<any>) => Tag2item(v)))
+    res = extend(res, tags->map((_, v: dict<any>) => Tag2item(v)))
   endif

   if len(res) == 0
@@ -216,9 +216,9 @@ def ccomplete#Complete(findstart: bool, abase: string): any # {{{1
     for i: number in len(diclist)->range()
       # New ctags has the "typeref" field.  Patched version has "typename".
       if diclist[i]->has_key('typename')
-        res = extendnew(res, diclist[i]['typename']->StructMembers(items[1 :], true))
+        res = extend(res, diclist[i]['typename']->StructMembers(items[1 :], true))
       elseif diclist[i]->has_key('typeref')
-        res = extendnew(res, diclist[i]['typeref']->StructMembers(items[1 :], true))
+        res = extend(res, diclist[i]['typeref']->StructMembers(items[1 :], true))
       endif

       # For a variable use the command, which must be a search pattern that
@@ -227,7 +227,7 @@ def ccomplete#Complete(findstart: bool, abase: string): any # {{{1
         var line: string = diclist[i]['cmd']
         if line[: 1] == '/^'
           var col: number = line->charidx(match(line, '\<' .. items[0] .. '\>'))
-          res = extendnew(res, line[2 : col - 1]->Nextitem(items[1 :], 0, true))
+          res = extend(res, line[2 : col - 1]->Nextitem(items[1 :], 0, true))
         endif
       endif
     endfor
@@ -473,11 +473,11 @@ def Nextitem( # {{{1

       # New ctags has the "typeref" field.  Patched version has "typename".
       if item->has_key('typeref')
-        res = extendnew(res, item['typeref']->StructMembers(items, all))
+        res = extend(res, item['typeref']->StructMembers(items, all))
         continue
       endif
       if item->has_key('typename')
-        res = extendnew(res, item['typename']->StructMembers(items, all))
+        res = extend(res, item['typename']->StructMembers(items, all))
         continue
       endif

@@ -511,11 +511,11 @@ def Nextitem( # {{{1
               endif
             endfor
             if name != ''
-              res = extendnew(res, StructMembers(cmdtokens[0] .. ':' .. name, items, all))
+              res = extend(res, StructMembers(cmdtokens[0] .. ':' .. name, items, all))
             endif
           elseif depth < 10
             # Could be "typedef other_T some_T".
-            res = extendnew(res, cmdtokens[0]->Nextitem(items, depth + 1, all))
+            res = extend(res, cmdtokens[0]->Nextitem(items, depth + 1, all))
           endif
         endif
       endif
@@ -674,7 +674,7 @@ def SearchMembers( # {{{1
     endif

     if typename != ''
-      res = extendnew(res, StructMembers(typename, items, all))
+      res = extend(res, StructMembers(typename, items, all))
     else
       # Use the search command (the declaration itself).
       var sb: number = line->match('\t\zs/^')
@@ -683,7 +683,7 @@ def SearchMembers( # {{{1
         var e: number = line
           ->charidx(match(line, '\<' .. matches[i]['match'] .. '\>', sb))
         if e > 0
-          res = extendnew(res, line[s : e - 1]->Nextitem(items, 0, all))
+          res = extend(res, line[s : e - 1]->Nextitem(items, 0, all))
         endif
       endif
     endif

That's 10 locations where the Vim script code no longer needs to make a copy of a possible huge list of dictionaries, which took time. That must have a positive impact on performance. I have 163 other invocations of *new() functions in my config where I need to check whether I can remove the new suffix.

[lacygoill]

Actually, now, we no longer even need an assignment:

diff --git a/runtime/autoload/ccomplete.vim b/runtime/autoload/ccomplete.vim
index d8675faf6..f50a67f4f 100644
--- a/runtime/autoload/ccomplete.vim
+++ b/runtime/autoload/ccomplete.vim
@@ -202,7 +202,7 @@ def ccomplete#Complete(findstart: bool, abase: string): any # {{{1
               || !v['static']
               || bufnr('%') == bufnr(v['filename']))

-    res = extendnew(res, tags->map((_, v: dict<any>) => Tag2item(v)))
+    res->extend(tags->map((_, v: dict<any>) => Tag2item(v)))
   endif

   if len(res) == 0
@@ -216,9 +216,9 @@ def ccomplete#Complete(findstart: bool, abase: string): any # {{{1
     for i: number in len(diclist)->range()
       # New ctags has the "typeref" field.  Patched version has "typename".
       if diclist[i]->has_key('typename')
-        res = extendnew(res, diclist[i]['typename']->StructMembers(items[1 :], true))
+        res->extend(diclist[i]['typename']->StructMembers(items[1 :], true))
       elseif diclist[i]->has_key('typeref')
-        res = extendnew(res, diclist[i]['typeref']->StructMembers(items[1 :], true))
+        res->extend(diclist[i]['typeref']->StructMembers(items[1 :], true))
       endif

       # For a variable use the command, which must be a search pattern that
@@ -227,7 +227,7 @@ def ccomplete#Complete(findstart: bool, abase: string): any # {{{1
         var line: string = diclist[i]['cmd']
         if line[: 1] == '/^'
           var col: number = line->charidx(match(line, '\<' .. items[0] .. '\>'))
-          res = extendnew(res, line[2 : col - 1]->Nextitem(items[1 :], 0, true))
+          res->extend(line[2 : col - 1]->Nextitem(items[1 :], 0, true))
         endif
       endif
     endfor
@@ -473,11 +473,11 @@ def Nextitem( # {{{1

       # New ctags has the "typeref" field.  Patched version has "typename".
       if item->has_key('typeref')
-        res = extendnew(res, item['typeref']->StructMembers(items, all))
+        res->extend(item['typeref']->StructMembers(items, all))
         continue
       endif
       if item->has_key('typename')
-        res = extendnew(res, item['typename']->StructMembers(items, all))
+        res->extend(item['typename']->StructMembers(items, all))
         continue
       endif

@@ -511,11 +511,11 @@ def Nextitem( # {{{1
               endif
             endfor
             if name != ''
-              res = extendnew(res, StructMembers(cmdtokens[0] .. ':' .. name, items, all))
+              res->extend(StructMembers(cmdtokens[0] .. ':' .. name, items, all))
             endif
           elseif depth < 10
             # Could be "typedef other_T some_T".
-            res = extendnew(res, cmdtokens[0]->Nextitem(items, depth + 1, all))
+            res->extend(cmdtokens[0]->Nextitem(items, depth + 1, all))
           endif
         endif
       endif
@@ -674,7 +674,7 @@ def SearchMembers( # {{{1
     endif

     if typename != ''
-      res = extendnew(res, StructMembers(typename, items, all))
+      res->extend(StructMembers(typename, items, all))
     else
       # Use the search command (the declaration itself).
       var sb: number = line->match('\t\zs/^')
@@ -683,7 +683,7 @@ def SearchMembers( # {{{1
         var e: number = line
           ->charidx(match(line, '\<' .. matches[i]['match'] .. '\>', sb))
         if e > 0
-          res = extendnew(res, line[s : e - 1]->Nextitem(items, 0, all))
+          res->extend(line[s : e - 1]->Nextitem(items, 0, all))
         endif
       endif
     endif

[lacygoill]

What about variables which are not declared, like global variables?

vim9script
g:name = {}
extend(g:name, {a: 0})
extend(g:name, {b: ''})

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

Why give an error here? What purpose does it serve? A global variable is not declared, thus has no type. There is no reason for Vim to expect any particular type.

Is it a case where we're supposed to use a type cast? It doesn't work:

vim9script
g:name = {}
extend(g:name, {a: 0})
extend(<dict<any>>g:name, {b: ''})

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

[brammool]

What about variables which are not declared, like global variables?

vim9script
g:name = {}
extend(g:name, {a: 0})
extend(g:name, {b: ''})

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

Why give an error here? What purpose does it serve? A global variable is not declared, thus has no type. There is no reason for Vim to expect any particular type.

Hmm, it's true that the variable has no declared type. I think the reason the change of the actual type causes an error here is that we did this for when we did restrict the type. Keep in mind that extend() does not know where its arguments come from, the argument can be any expression, not necessarily a variable.

If we make it this way, then this also won't fail: extend([1, 2], ["x"]) Which is probably OK, since the list type was not declared.

When the type is infered it will also be used:

var l = [1, 2]
extend(l, ["x"])  # error

-- hundred-and-one symptoms of being an internet addict:

206. You religiously respond immediately to e-mail, while ignoring your growing pile of snail mail.

     /// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net \\ /// \\ \\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\ help me help AIDS victims -- http://ICCF-Holland.org ///

[brammool]

Thus the impact of this change is limited.

It is limited in the sense that it should not break anything, because it removes a useless constraint: there was no need to give an error in a function.

Yes, the main impact is that it's more logical: if you declare the members to be "any" there must not be an error for using one member type after another member type.

But it might have an impact in terms of performance optimizations. For example, I think runtime/autoload/ccomplete.vim no longer requires any extendnew():

Please make a PR for this update.

A disadvantage is though, that when manipulating a dict or list it has to go through the actual values to find out the current type. Previously it just used the type stored with the dict or list header. But that is read-only, which is most likely quicker than copying the dict or list.

-- hundred-and-one symptoms of being an internet addict:

203. You're an active member of more than 20 newsgroups.

     /// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net \\ /// \\ \\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\ help me help AIDS victims -- http://ICCF-Holland.org ///

[brammool]

I wrote:

What about variables which are not declared, like global variables?

vim9script
g:name = {}
extend(g:name, {a: 0})
extend(g:name, {b: ''})

E1013: Argument 2: type mismatch, expected dict<number> but got dict<string> in extend()

Why give an error here? What purpose does it serve? A global variable is not declared, thus has no type. There is no reason for Vim to expect any particular type.

Hmm, it's true that the variable has no declared type. I think the reason the change of the actual type causes an error here is that we did this for when we did restrict the type. Keep in mind that extend() does not know where its arguments come from, the argument can be any expression, not necessarily a variable.

If we make it this way, then this also won't fail: extend([1, 2], ["x"]) Which is probably OK, since the list type was not declared.

Actually, when trying this out, there is an error at compile time. Because the type is taken from the constant list, and for extend() we check if the type is correct. This does not make a difference between an inferred type and a declared type.

If we want to do it properly, then this would fail:

var l: list<number> = [1, 2]
extend(l, ["x"])

But these do not:

extend([1, 2], ["x"])

g:l = [1, 2]
extend(g:l, ["x"])

var l: list<any> = [1, 2]
extend(l, ["x"])

That's not so easy to implement...

-- hundred-and-one symptoms of being an internet addict:

207. You're given one phone call in prison and you ask them for a laptop.

     /// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net \\ /// \\ \\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\ help me help AIDS victims -- http://ICCF-Holland.org ///

[lacygoill]

Keep in mind that extend() does not know where its arguments come from, the argument can be any expression, not necessarily a variable.

I know, which is why – in the original report – I thought the issue was the other way around. We already had this discussion in #7565. The conclusion was that a builtin function which operates on a dictionary/list in-place should never change the type of its members. Whether the dictionary/list was declared or not should not matter:

Does that really matter? Compare these two:

var l = [0] map(l, Func) and: map([0], Func)

Do you expect them to behave differently? That would be hard to explain. It can easily get more complicated:

map(GetList(), Func)

Now you can't see where the list is coming from. Can the item type be changed or not?

What I did now was use the rule that map() can change the item value, but not the type. If you want to change the type, use mapnew().

It may not be perfect, but at least it's a simple rule.

[...]

The counter argument is that the rules should not depend on where a value comes from, whether it's a literal or from a variable. It makes the rules hard to understand. And also difficult to implement, we would need to keep track where a value came from.

Now, it seems we're trying to make Vim smarter, by lifting this requirement for everything except for variables which have been declared. I'm all for it. I've always thought it made more sense. Not only that, but in practice, this requirement has always been the source of the most frequent, annoying, and useless errors.

But this change has broad implications.

The patch 8.2.3994 has started to make Vim smarter, by removing the requirement for extend() (but only at the script level; there is a FIXME for compiled code):

vim9script
echo [1, 2]->extend(['x'])

[1, 2, 'x']

But if we go in that direction, then to be consistent (and give fewer annoying/useless errors), we should probably do the same for map():

vim9script
echo [1, 2]->map((..._) => 'x')

E1012: Type mismatch; expected number but got string in map()

Also, in a future patch, we might want to allow flatten() in Vim9:

vim9script
echo [1, [2, 3], 4]->flatten()

E1158: Cannot use flatten() in Vim9 script

Here, [1, [2, 3], 4] was not declared. Since extend() doesn't care about changing the type of the items in its first list argument, I don't see why flatten() should care either.

[lacygoill]

Now, it seems we're trying to make Vim smarter, by lifting this requirement for everything except for variables which have been declared. I'm all for it. I've always thought it made more sense.

Although, I do have one concern, which was already raised in the previous discussion. The case where the first list/dictionary argument of extend()/map()/flatten() is the output of another function. I'm not sure whether we should allow to change the type of the members then.

BTW, while I was trying to figure out how Vim handled this case, I found what seems to be a bug:

vim9script
def GetList(): list<number>
    var l: list<number> = [0]
    return l
enddef
echo GetList()
    ->extend(['a'])

E1013: Argument 2: type mismatch, expected list<number> but got list<string> in extend()

vim9script
def GetList(): list<number>
    var l = [0]
    return l
enddef
echo GetList()
    ->extend(['a'])

[0, 'a']

There is no reason for the 2 snippets to give different results. In the second snippet, type inferrence infers that l has the type list<number>, and no error is given. So, in the first snippet, why does Vim give an error when we explicitly write the exact same type?

[brammool]

Now, it seems we're trying to make Vim smarter, by lifting this requirement for everything except for variables which have been declared. I'm all for it. I've always thought it made more sense.

Although, I do have one concern, which was already raised in the previous discussion. The case where the first list/dictionary argument of extend()/map()/flatten() is the output of another function. I'm not sure whether we should allow to change the type of the members then.

BTW, while I was trying to figure out how Vim handled this case, I found what seems to be a bug:

vim9script
def GetList(): list<number>
    var l: list<number> = [0]
    return l
enddef
echo GetList()
    ->extend(['a'])

E1013: Argument 2: type mismatch, expected list<number> but got list<string> in extend()

vim9script
def GetList(): list<number>
    var l = [0]
    return l
enddef
echo GetList()
    ->extend(['a'])

[0, 'a']

There is no reason for the 2 snippets to give different results. In the second snippet, type inferrence infers that l has the type list<number>, and no error is given. So, in the first snippet, why does Vim give an error when we explicitly write the exact same type?

Right.

Thinking about this a bit more, we currently just don't have enough information to decide whether a list or dict can be changed in a certain way. We only store one type. We actually need two types: what the current value represents and what the declared type is.

If we have just a list constant [1, 2] then there is no declared type, but we do know it's currently a list. Thus passing it to a function that requires a list is OK, passing it to a function that requires list is an error.

At the same time, if map() or extend() change the members of the list, the type can change. Since there is no declared type for [1, 2], that is permitted. But if the list was declared as list then it should be an error.

This solved the problem mentioned before:

g:l = []
g:l->extend([1])
g:l->extend(['x'])

That should work, even though the first extend() changed the type to list it becomes list in the second extend() call. We must allow this, since there is no way to specify list for this list.

If we would use a declared list: var l: list = [] g:l = l g:l->extend([1]) g:l->extend(['x'])

Then the second extend() should give an error. That is only possible if we add the declared type to the list.

I'll have a go at it, I expect it will require some tweaking later.

-- hundred-and-one symptoms of being an internet addict:

212. Your Internet group window has more icons than your Accessories window.

     /// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net \\ /// \\ \\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\ help me help AIDS victims -- http://ICCF-Holland.org ///