eval: Fix max_min functions

Found two bugs:

1. Multiple unneeded error messages, vim/vim#1039.
2. Unformatted error string, vim/vim#1040.

src/nvim/eval.c

@@ -12463,53 +12463,49 @@ static void f_matchstrpos(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   find_some_match(argvars, rettv, 4);
 }
 
-static void max_min(typval_T *argvars, typval_T *rettv, int domax)
+/// Get maximal/minimal number value in a list or dictionary
+///
+/// @param[in]  tv  List or dictionary to work with. If it contains something
+///                 that is not an integer number (or cannot be coerced to
+///                 it) error is given.
+/// @param[out]  rettv  Location where result will be saved. Only assigns
+///                     vval.v_number, type is not touched. Returns zero for
+///                     empty lists/dictionaries.
+/// @param[in]  domax  Determines whether maximal or minimal value is desired.
+static void max_min(const typval_T *const tv, typval_T *const rettv,
+                    const bool domax)
+  FUNC_ATTR_NONNULL_ALL
 {
-  long n = 0;
-  long i;
+  varnumber_T n = 0;
   bool error = false;
 
-  if (argvars[0].v_type == VAR_LIST) {
-    list_T          *l;
-    listitem_T      *li;
-
-    l = argvars[0].vval.v_list;
-    if (l != NULL) {
-      li = l->lv_first;
-      if (li != NULL) {
-        n = tv_get_number_chk(&li->li_tv, &error);
-        for (;; ) {
-          li = li->li_next;
-          if (li == NULL) {
-            break;
-          }
-          i = tv_get_number_chk(&li->li_tv, &error);
-          if (domax ? i > n : i < n) {
-            n = i;
-          }
+  if (tv->v_type == VAR_LIST) {
+    const list_T *const l = tv->vval.v_list;
+    if (tv_list_len(l) != 0) {
+      n = tv_get_number_chk(&l->lv_first->li_tv, &error);
+      for (const listitem_T *li = l->lv_first->li_next; li != NULL && !error;
+           li = li->li_next) {
+        const varnumber_T i = tv_get_number_chk(&li->li_tv, &error);
+        if (domax ? i > n : i < n) {
+          n = i;
         }
       }
     }
-  } else if (argvars[0].v_type == VAR_DICT) {
-    dict_T          *d;
-    int first = TRUE;
-    hashitem_T      *hi;
-    int todo;
-
-    d = argvars[0].vval.v_dict;
-    if (d != NULL) {
-      todo = (int)d->dv_hashtab.ht_used;
-      for (hi = d->dv_hashtab.ht_array; todo > 0; ++hi) {
-        if (!HASHITEM_EMPTY(hi)) {
-          todo--;
-          i = tv_get_number_chk(&TV_DICT_HI2DI(hi)->di_tv, &error);
-          if (first) {
-            n = i;
-            first = FALSE;
-          } else if (domax ? i > n : i < n)
-            n = i;
+  } else if (tv->v_type == VAR_DICT) {
+    if (tv->vval.v_dict != NULL) {
+      bool first = true;
+      TV_DICT_ITER(tv->vval.v_dict, di, {
+        const varnumber_T i = tv_get_number_chk(&di->di_tv, &error);
+        if (error) {
+          break;
         }
-      }
+        if (first) {
+          n = i;
+          first = true;
+        } else if (domax ? i > n : i < n) {
+          n = i;
+        }
+      });
     }
   } else {
     EMSG2(_(e_listdictarg), domax ? "max()" : "min()");

src/nvim/eval/typval.h

@@ -279,13 +279,13 @@ typedef struct list_stack_S {
 #define TV_DICT_HI2DI(hi) \
     ((dictitem_T *)((hi)->hi_key - offsetof(dictitem_T, di_key)))
 
-static inline long tv_list_len(list_T *const l)
+static inline long tv_list_len(const list_T *const l)
   REAL_FATTR_PURE REAL_FATTR_WARN_UNUSED_RESULT;
 
 /// Get the number of items in a list
 ///
 /// @param[in]  l  List to check.
-static inline long tv_list_len(list_T *const l)
+static inline long tv_list_len(const list_T *const l)
 {
   if (l == NULL) {
     return 0;

test/functional/eval/minmax_functions_spec.lua

@@ -0,0 +1,51 @@
+local helpers = require('test.functional.helpers')(after_each)
+
+local eq = helpers.eq
+local eval = helpers.eval
+local clear = helpers.clear
+local funcs = helpers.funcs
+local redir_exec = helpers.redir_exec
+
+before_each(clear)
+for _, func in ipairs({'min', 'max'}) do
+  describe(func .. '()', function()
+    it('gives a single error message when multiple values failed conversions',
+    function()
+      eq('\nE745: Using a List as a Number\n0',
+         redir_exec('echo ' .. func .. '([-5, [], [], [], 5])'))
+      eq('\nE745: Using a List as a Number\n0',
+         redir_exec('echo ' .. func .. '({1:-5, 2:[], 3:[], 4:[], 5:5})'))
+      for errmsg, errinput in pairs({
+        ['E745: Using a List as a Number'] = '[]',
+        ['E805: Using a Float as a Number'] = '0.0',
+        ['E703: Using a Funcref as a Number'] = 'function("tr")',
+        ['E728: Using a Dictionary as a Number'] = '{}',
+      }) do
+        eq('\n' .. errmsg .. '\n0',
+           redir_exec('echo ' .. func .. '([' .. errinput .. '])'))
+        eq('\n' .. errmsg .. '\n0',
+           redir_exec('echo ' .. func .. '({1:' .. errinput .. '})'))
+      end
+    end)
+    it('works with arrays/dictionaries with zero items', function()
+      eq(0, funcs[func]({}))
+      eq(0, eval(func .. '({})'))
+    end)
+    it('works with arrays/dictionaries with one item', function()
+      eq(5, funcs[func]({5}))
+      eq(5, funcs[func]({test=5}))
+    end)
+    it('works with NULL arrays/dictionaries', function()
+      eq(0, eval(func .. '(v:_null_list)'))
+      eq(0, eval(func .. '(v:_null_dict)'))
+    end)
+    it('errors out for invalid types', function()
+      for _, errinput in ipairs({'1', 'v:true', 'v:false', 'v:null',
+                                 'function("tr")', '""'}) do
+        eq(('\nE712: Argument of %s() must be a List or Dictionary\n0'):format(
+            func),
+           redir_exec('echo ' .. func .. '(' .. errinput .. ')'))
+      end
+    end)
+  end)
+end