konevsa/fish
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Correctly handle mixing named arguments with function name

Browse Source 
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.
This commit is contained in:
ridiculousfish 2015-05-17 14:17:01 -07:00
parent c26d317da5
commit 286c60bc9d
5 changed files with 86 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

89
builtin.cpp
View File

@ -1655,7 +1655,7 @@ static int builtin_functions(parser_t &parser, wchar_t **argv)
return STATUS_BUILTIN_ERROR; 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, append_format(stderr_buffer,
_(L"%ls: Illegal function name '%ls'\n"), _(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; int res=STATUS_BUILTIN_OK;
wchar_t *desc=0; wchar_t *desc=0;
std::vector<event_t> events; 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; wcstring_list_t inherit_vars;
wchar_t *name = 0;
bool shadows = true; bool shadows = true;
woptind=0; woptind=0;
wcstring_list_t wrap_targets; 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[] = 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; int opt_index = 0;
// The leading - here specifies RETURN_IN_ORDER
int opt = wgetopt_long(argc, int opt = wgetopt_long(argc,
argv, argv,
L"d:s:j:p:v:e:haSV:", L"-d:s:j:p:v:e:haSV:",
long_options, long_options,
&opt_index); &opt_index);
if (opt == -1) if (opt == -1)
@ -2176,8 +2187,9 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
} }
case 'a': case 'a':
if (named_arguments.get() == NULL) has_named_arguments = true;
named_arguments.reset(new wcstring_list_t); /* The function name is the first positional unless -a comes before all positionals */
name_is_first_positional = ! positionals.empty();
break; break;
case 'S': case 'S':
@ -2204,6 +2216,11 @@ int define_function(parser_t &parser, const wcstring_list_t &c_args, const wcstr
case 'h': case 'h':
builtin_print_help(parser, argv[0], stdout_buffer); builtin_print_help(parser, argv[0], stdout_buffer);
return STATUS_BUILTIN_OK; return STATUS_BUILTIN_OK;
case 1:
assert(woptarg != NULL);
positionals.push_back(woptarg);
break;
case '?': case '?':
builtin_unknown_option(parser, argv[0], argv[woptind-1]); 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 (!res)
{ {
/* Determine the function name, and remove it from the list of positionals */
if (argc == woptind) 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, append_format(*out_err,
_(L"%ls: Expected function name\n"), _(L"%ls: Expected function name\n"),
argv[0]); argv[0]);
res=1; res=1;
} }
else if (wcsfuncname(argv[woptind])) else if (wcsfuncname(function_name))
{ {
append_format(*out_err, append_format(*out_err,
_(L"%ls: Illegal function name '%ls'\n"), _(L"%ls: Illegal function name '%ls'\n"),
argv[0], argv[0],
argv[woptind]); function_name.c_str());
res=1; res=1;
} }
else if (parser_keywords_is_reserved(argv[woptind])) else if (parser_keywords_is_reserved(function_name))
{ {
append_format(*out_err, append_format(*out_err,
_(L"%ls: The name '%ls' is reserved,\nand can not be used as a function name\n"), _(L"%ls: The name '%ls' is reserved,\nand can not be used as a function name\n"),
argv[0], argv[0],
argv[woptind]); function_name.c_str());
res=1; 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]); append_format(*out_err, _(L"%ls: No function name given\n"), argv[0]);
res=1; res=1;
} }
else else
{ {
if (has_named_arguments)
name = argv[woptind++];
if (named_arguments.get())
{ {
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, append_format(*out_err,
_(L"%ls: Invalid variable name '%ls'\n"), _(L"%ls: Invalid variable name '%ls'\n"),
argv[0], argv[0],
argv[woptind]); named_arguments.at(i).c_str());
res = STATUS_BUILTIN_ERROR; res = STATUS_BUILTIN_ERROR;
break; 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, append_format(*out_err,
_(L"%ls: Expected one argument, got %d\n"), _(L"%ls: Expected one argument, got %d\n"),
argv[0], argv[0],
argc); positionals.size() + 1);
res=1; res=1;
} }
} }
} }
if (res) if (res)
{ {
builtin_print_help(parser, argv[0], *out_err); builtin_print_help(parser, argv[0], *out_err);
} }
else else
{ {
/* Here we actually define the function! */
function_data_t d; function_data_t d;
d.name = name; d.name = function_name;
if (desc) if (desc)
d.description = desc; d.description = desc;
d.events.swap(events); d.events.swap(events);
d.shadows = shadows; 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); d.inherit_vars.swap(inherit_vars);
for (size_t i=0; i<d.events.size(); i++) 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 // Handle wrap targets
for (size_t w=0; w < wrap_targets.size(); w++) 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));
} }
} }

9
common.cpp
View File

@ -481,9 +481,14 @@ const wchar_t *wcsvarname(const wchar_t *str)
return NULL; 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'/');
} }

3
common.h
View File

@ -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 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 \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 Test if the given string is valid in a variable name

14
tests/function.in
View File

@ -30,3 +30,17 @@ set foo 'bad foo'
set bar 'bad bar' set bar 'bad bar'
set baz 'bad baz' set baz 'bad baz'
frob 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

4
tests/function.out
View File

@ -18,3 +18,7 @@ $bar: (5)
4: '' 4: ''
5: '3' 5: '3'
$baz: (0) $baz: (0)
Function name1 found
Function name2 found
Function name3 found
Function name4 found