Correctly handle mixing named arguments with function name

Before this fix, `function -a arg1 name1` would produce a
function named 'arg1'. After this fix, it will produce a
function named 'name'. See #2068 for more.

5 files changed, 86 insertions, 33 deletions

diff --git a/builtin.cpp b/builtin.cpp
index 71ba8a32..008892de 100644
--- a/builtin.cpp
+++ b/builtin.cpp
@@ -1655,7 +1655,7 @@ static int builtin_functions(parser_t &parser, wchar_t **argv)
             return STATUS_BUILTIN_ERROR;
         }
 
-        if ((wcsfuncname(new_func.c_str()) != 0) || parser_keywords_is_reserved(new_func))
+        if ((wcsfuncname(new_func) != 0) || parser_keywords_is_reserved(new_func))
         {
             append_format(stderr_buffer,
                           _(L"%ls: Illegal function name '%ls'\n"),
@@ -2002,15 +2002,25 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
     int res=STATUS_BUILTIN_OK;
     wchar_t *desc=0;
     std::vector<event_t> events;
-    std::auto_ptr<wcstring_list_t> named_arguments(NULL);
+    
+    bool has_named_arguments = false;
+    wcstring_list_t named_arguments;
     wcstring_list_t inherit_vars;
 
-    wchar_t *name = 0;
     bool shadows = true;
 
     woptind=0;
     
     wcstring_list_t wrap_targets;
+    
+    /* If -a/--argument-names is specified before the function name,
+       then the function name is the last positional, e.g. `function -a arg1 arg2 name`.
+       If it is specified after the function name (or not specified at all) then the
+       function name is the first positional. This is the common case. */
+    bool name_is_first_positional = true;
+    wcstring_list_t positionals;
+    
+    wcstring function_name;
 
     const struct woption long_options[] =
     {
@@ -2032,9 +2042,10 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
     {
         int opt_index = 0;
 
+        // The leading - here specifies RETURN_IN_ORDER
         int opt = wgetopt_long(argc,
                                argv,
-                               L"d:s:j:p:v:e:haSV:",
+                               L"-d:s:j:p:v:e:haSV:",
                                long_options,
                                &opt_index);
         if (opt == -1)
@@ -2176,8 +2187,9 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
             }
 
             case 'a':
-                if (named_arguments.get() == NULL)
-                    named_arguments.reset(new wcstring_list_t);
+                has_named_arguments = true;
+                /* The function name is the first positional unless -a comes before all positionals */
+                name_is_first_positional = ! positionals.empty();
                 break;
 
             case 'S':
@@ -2204,6 +2216,11 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
             case 'h':
                 builtin_print_help(parser, argv[0], stdout_buffer);
                 return STATUS_BUILTIN_OK;
+                
+            case 1:
+                assert(woptarg != NULL);
+                positionals.push_back(woptarg);
+                break;
 
             case '?':
                 builtin_unknown_option(parser, argv[0], argv[woptind-1]);
@@ -2216,87 +2233,99 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
 
     if (!res)
     {
-
-        if (argc == woptind)
+        /* Determine the function name, and remove it from the list of positionals */
+        bool name_is_missing = positionals.empty();
+        if (! name_is_missing)
+        {
+            if (name_is_first_positional)
+            {
+                function_name = positionals.front();
+                positionals.erase(positionals.begin());
+            }
+            else
+            {
+                function_name = positionals.back();
+                positionals.erase(positionals.end() - 1);
+            }
+        }
+        
+        if (name_is_missing)
         {
             append_format(*out_err,
                           _(L"%ls: Expected function name\n"),
                           argv[0]);
             res=1;
         }
-        else if (wcsfuncname(argv[woptind]))
+        else if (wcsfuncname(function_name))
         {
             append_format(*out_err,
                           _(L"%ls: Illegal function name '%ls'\n"),
                           argv[0],
-                          argv[woptind]);
+                          function_name.c_str());
 
             res=1;
         }
-        else if (parser_keywords_is_reserved(argv[woptind]))
+        else if (parser_keywords_is_reserved(function_name))
         {
 
             append_format(*out_err,
                           _(L"%ls: The name '%ls' is reserved,\nand can not be used as a function name\n"),
                           argv[0],
-                          argv[woptind]);
+                          function_name.c_str());
 
             res=1;
         }
-        else if (! wcslen(argv[woptind]))
+        else if (function_name.empty())
         {
             append_format(*out_err, _(L"%ls: No function name given\n"), argv[0]);
             res=1;
         }
         else
         {
-
-            name = argv[woptind++];
-
-            if (named_arguments.get())
+            if (has_named_arguments)
             {
-                while (woptind < argc)
+                /* All remaining positionals are named arguments */
+                named_arguments.swap(positionals);
+                for (size_t i=0; i < named_arguments.size(); i++)
                 {
-                    if (wcsvarname(argv[woptind]))
+                    if (wcsvarname(named_arguments.at(i)))
                     {
                         append_format(*out_err,
                                       _(L"%ls: Invalid variable name '%ls'\n"),
                                       argv[0],
-                                      argv[woptind]);
+                                      named_arguments.at(i).c_str());
                         res = STATUS_BUILTIN_ERROR;
                         break;
                     }
-
-                    named_arguments->push_back(argv[woptind++]);
                 }
             }
-            else if (woptind != argc)
+            else if (! positionals.empty())
             {
+                // +1 because we already got the function name
                 append_format(*out_err,
                               _(L"%ls: Expected one argument, got %d\n"),
                               argv[0],
-                              argc);
+                              positionals.size() + 1);
                 res=1;
-
             }
         }
     }
-
+    
     if (res)
     {
         builtin_print_help(parser, argv[0], *out_err);
     }
     else
     {
+        /* Here we actually define the function! */
         function_data_t d;
 
-        d.name = name;
+        d.name = function_name;
         if (desc)
             d.description = desc;
         d.events.swap(events);
         d.shadows = shadows;
-        if (named_arguments.get())
-            d.named_arguments.swap(*named_arguments);
+        d.named_arguments.swap(named_arguments);
         d.inherit_vars.swap(inherit_vars);
 
         for (size_t i=0; i<d.events.size(); i++)
@@ -2312,7 +2341,7 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
         // Handle wrap targets
         for (size_t w=0; w < wrap_targets.size(); w++)
         {
-            complete_add_wrapper(name, wrap_targets.at(w));
+            complete_add_wrapper(function_name, wrap_targets.at(w));
         }
     }
 
diff --git a/common.cpp b/common.cpp
index 3440f075..6767e351 100644
--- a/common.cpp
+++ b/common.cpp
@@ -481,9 +481,14 @@ const wchar_t *wcsvarname(const wchar_t *str)
     return NULL;
 }
 
-const wchar_t *wcsfuncname(const wchar_t *str)
+const wchar_t *wcsvarname(const wcstring &str)
 {
-    return wcschr(str, L'/');
+    return wcsvarname(str.c_str());
+}
+
+const wchar_t *wcsfuncname(const wcstring &str)
+{
+    return wcschr(str.c_str(), L'/');
 }
 
 
diff --git a/common.h b/common.h
index a4bee08f..f0d55894 100644
--- a/common.h
+++ b/common.h
@@ -696,6 +696,7 @@ void append_formatv(wcstring &str, const wchar_t *format, va_list ap);
 */
 
 const wchar_t *wcsvarname(const wchar_t *str);
+const wchar_t *wcsvarname(const wcstring &str);
 
 
 /**
@@ -704,7 +705,7 @@ const wchar_t *wcsvarname(const wchar_t *str);
    \return null if this is a valid name, and a pointer to the first invalid character otherwise
 */
 
-const wchar_t *wcsfuncname(const wchar_t *str);
+const wchar_t *wcsfuncname(const wcstring &str);
 
 /**
    Test if the given string is valid in a variable name
diff --git a/tests/function.in b/tests/function.in
index e533b25b..747bbcec 100644
--- a/tests/function.in
+++ b/tests/function.in
@@ -30,3 +30,17 @@ set foo 'bad foo'
 set bar 'bad bar'
 set baz 'bad baz'
 frob
+
+# Test that -a does not mix up the function name with arguments
+# See #2068
+function name1 -a arg1 arg2 ; end
+function -a arg1 arg2 name2 ; end
+function name3 --argument-names arg1 arg2 ; end
+function --argument-names arg1 arg2 name4 ; end
+for i in (seq 4)
+    if functions -q name$i
+        echo "Function name$i found"
+    else
+        echo "Function name$i not found, but should have been"
+    end
+end
diff --git a/tests/function.out b/tests/function.out
index 3fa70990..5a3da619 100644
--- a/tests/function.out
+++ b/tests/function.out
@@ -18,3 +18,7 @@ $bar: (5)
 4: ''
 5: '3'
 $baz: (0)
+Function name1 found
+Function name2 found
+Function name3 found
+Function name4 found