From 385e40540c904a8d16e8b6fcb46ec892ca98bf67 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sat, 17 Jun 2017 22:36:56 -0700 Subject: [PATCH] fix issues with `builtin_function()` This does several things. It fixes `builtin_function()` so that errors it emits are displayed. As part of doing that I've removed the unnecessary `out_err` parameter to make the interface like every other builtin. This also fixes a regression introduced by #4000 which was attempting to fix a bug introduced by #3649. Fixes #4139 --- src/builtin_function.cpp | 45 ++++++++++++++++++---------------------- src/builtin_function.h | 2 +- src/parse_execution.cpp | 9 ++++---- src/parse_tree.cpp | 10 ++++----- src/parser.cpp | 11 +++++----- src/parser.h | 2 +- src/reader.cpp | 4 ++-- 7 files changed, 38 insertions(+), 45 deletions(-) diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index 37539fdd4..4d73eed41 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include "builtin.h" @@ -51,9 +52,8 @@ static const struct woption long_options[] = {{L"description", required_argument {NULL, 0, NULL, 0}}; static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(high ncss method) - int argc, wchar_t **argv, parser_t &parser, io_streams_t &streams, - wcstring *out_err) { - wchar_t *cmd = argv[0]; + int argc, wchar_t **argv, parser_t &parser, io_streams_t &streams) { + const wchar_t *cmd = L"function"; int opt; wgetopter_t w; while ((opt = w.wgetopt_long(argc, argv, short_options, long_options, NULL)) != -1) { @@ -65,7 +65,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig case 's': { int sig = wcs2sig(w.woptarg); if (sig == -1) { - append_format(*out_err, _(L"%ls: Unknown signal '%ls'"), cmd, w.woptarg); + streams.err.append_format(_(L"%ls: Unknown signal '%ls'"), cmd, w.woptarg); return STATUS_INVALID_ARGS; } opts.events.push_back(event_t::signal_event(sig)); @@ -73,7 +73,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig } case 'v': { if (!valid_var_name(w.woptarg)) { - append_format(*out_err, BUILTIN_ERR_VARNAME, cmd, w.woptarg); + streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, w.woptarg); return STATUS_INVALID_ARGS; } @@ -109,8 +109,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig } if (job_id == -1) { - append_format(*out_err, - _(L"%ls: Cannot find calling job for event handler"), cmd); + streams.err.append_format(_(L"%ls: Cannot find calling job for event handler"), cmd); return STATUS_INVALID_ARGS; } e.type = EVENT_JOB_ID; @@ -118,7 +117,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig } else { pid = fish_wcstoi(w.woptarg); if (errno || pid < 0) { - append_format(*out_err, _(L"%ls: Invalid process id '%ls'"), cmd, + streams.err.append_format(_(L"%ls: Invalid process id '%ls'"), cmd, w.woptarg); return STATUS_INVALID_ARGS; } @@ -143,7 +142,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig } case 'V': { if (!valid_var_name(w.woptarg)) { - append_format(*out_err, BUILTIN_ERR_VARNAME, cmd, w.woptarg); + streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, w.woptarg); return STATUS_INVALID_ARGS; } opts.inherit_vars.push_back(w.woptarg); @@ -173,22 +172,21 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig } static int validate_function_name(int argc, const wchar_t *const *argv, wcstring &function_name, - const wchar_t *cmd, wcstring *out_err) { + const wchar_t *cmd, io_streams_t &streams) { if (argc < 2) { // This is currently impossible but let's be paranoid. - append_format(*out_err, _(L"%ls: Expected function name"), cmd); + streams.err.append_format(_(L"%ls: Expected function name"), cmd); return STATUS_INVALID_ARGS; } function_name = argv[1]; if (!valid_func_name(function_name)) { - append_format(*out_err, _(L"%ls: Illegal function name '%ls'"), cmd, function_name.c_str()); + streams.err.append_format(_(L"%ls: Illegal function name '%ls'"), cmd, function_name.c_str()); return STATUS_INVALID_ARGS; } if (parser_keywords_is_reserved(function_name)) { - append_format( - *out_err, + streams.err.append_format( _(L"%ls: The name '%ls' is reserved,\nand can not be used as a function name"), cmd, function_name.c_str()); return STATUS_INVALID_ARGS; @@ -200,14 +198,11 @@ static int validate_function_name(int argc, const wchar_t *const *argv, wcstring /// Define a function. Calls into `function.cpp` to perform the heavy lifting of defining a /// function. int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, - const wcstring &contents, int definition_line_offset, wcstring *out_err) { - assert(out_err != NULL); - + const wcstring &contents, int definition_line_offset) { // The wgetopt function expects 'function' as the first argument. Make a new wcstring_list with // that property. This is needed because this builtin has a different signature than the other // builtins. - wcstring_list_t args; - args.push_back(L"function"); + wcstring_list_t args = {L"function"}; args.insert(args.end(), c_args.begin(), c_args.end()); // Hackish const_cast matches the one in builtin_run. @@ -215,21 +210,21 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis wchar_t **argv = const_cast(argv_array.get()); wchar_t *cmd = argv[0]; int argc = builtin_count_args(argv); - wcstring function_name; - function_cmd_opts_t opts; // A valid function name has to be the first argument. - int retval = validate_function_name(argc, argv, function_name, cmd, out_err); + wcstring function_name; + int retval = validate_function_name(argc, argv, function_name, cmd, streams); if (retval != STATUS_CMD_OK) return retval; argv++; argc--; + function_cmd_opts_t opts; int optind; - retval = parse_cmd_opts(opts, &optind, argc, argv, parser, streams, out_err); + retval = parse_cmd_opts(opts, &optind, argc, argv, parser, streams); if (retval != STATUS_CMD_OK) return retval; if (opts.print_help) { - builtin_print_help(parser, streams, cmd, streams.out); + builtin_print_help(parser, streams, cmd, streams.err); return STATUS_CMD_OK; } @@ -239,7 +234,7 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis opts.named_arguments.push_back(argv[i]); } } else { - append_format(*out_err, _(L"%ls: Unexpected positional argument '%ls'"), cmd, + streams.err.append_format(_(L"%ls: Unexpected positional argument '%ls'"), cmd, argv[optind]); return STATUS_INVALID_ARGS; } diff --git a/src/builtin_function.h b/src/builtin_function.h index 56e68b7a4..f144924c7 100644 --- a/src/builtin_function.h +++ b/src/builtin_function.h @@ -8,5 +8,5 @@ class parser_t; struct io_streams_t; int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, - const wcstring &contents, int definition_line_offset, wcstring *out_err); + const wcstring &contents, int definition_line_offset); #endif diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 6cd09d6c5..b6ac88c4c 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -388,14 +388,13 @@ parse_execution_result_t parse_execution_context_t::run_function_statement( const wcstring contents_str = wcstring(this->src, contents_start, contents_end - contents_start); int definition_line_offset = this->line_offset_of_character_at_offset(contents_start); - wcstring error_str; io_streams_t streams; int err = builtin_function(*parser, streams, argument_list, contents_str, - definition_line_offset, &error_str); + definition_line_offset); proc_set_last_status(err); - if (!error_str.empty()) { - this->report_error(header, L"%ls", error_str.c_str()); + if (!streams.err.empty()) { + this->report_error(header, L"%ls", streams.err.buffer().c_str()); result = parse_execution_errored; } @@ -690,7 +689,7 @@ parse_execution_result_t parse_execution_context_t::report_errors( // Get a backtrace. wcstring backtrace_and_desc; - parser->get_backtrace(src, error_list, &backtrace_and_desc); + parser->get_backtrace(src, error_list, backtrace_and_desc); // Print it. if (!should_suppress_stderr_for_tests()) { diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp index 613fc100d..792229475 100644 --- a/src/parse_tree.cpp +++ b/src/parse_tree.cpp @@ -29,13 +29,13 @@ static bool production_is_empty(const production_element_t *production) { /// Returns a string description of this parse error. wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring &prefix, bool is_interactive, bool skip_caret) const { - if (skip_caret || source_start >= src.size() || source_start + source_length > src.size()) { - return L""; - } - wcstring result = prefix; result.append(this->text); + if (skip_caret || source_start >= src.size() || source_start + source_length > src.size()) { + return result; + } + // Locate the beginning of this line of source. size_t line_start = 0; @@ -69,7 +69,7 @@ wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring } // Append the line of text. - result.push_back(L'\n'); + if (!result.empty()) result.push_back(L'\n'); result.append(src, line_start, line_end - line_start); // Append the caret line. The input source may include tabs; for that reason we diff --git a/src/parser.cpp b/src/parser.cpp index f3ba54206..61ef22676 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -602,7 +602,7 @@ int parser_t::eval(const wcstring &cmd, const io_chain_t &io, enum block_type_t if (!parse_tree_from_string(cmd, parse_flag_none, &tree, &error_list)) { // Get a backtrace. This includes the message. wcstring backtrace_and_desc; - this->get_backtrace(cmd, error_list, &backtrace_and_desc); + this->get_backtrace(cmd, error_list, backtrace_and_desc); // Print it. fwprintf(stderr, L"%ls\n", backtrace_and_desc.c_str()); @@ -725,8 +725,7 @@ bool parser_t::detect_errors_in_argument_list(const wcstring &arg_list_src, wcst } void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &errors, - wcstring *output) const { - assert(output != NULL); + wcstring &output) const { if (!errors.empty()) { const parse_error_t &err = errors.at(0); @@ -762,10 +761,10 @@ void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &erro const wcstring description = err.describe_with_prefix(src, prefix, is_interactive, skip_caret); if (!description.empty()) { - output->append(description); - output->push_back(L'\n'); + output.append(description); + output.push_back(L'\n'); } - output->append(this->stack_trace()); + output.append(this->stack_trace()); } } diff --git a/src/parser.h b/src/parser.h index 3ebdbda31..852b9c9a6 100644 --- a/src/parser.h +++ b/src/parser.h @@ -327,7 +327,7 @@ class parser_t { profile_item_t *create_profile_item(); void get_backtrace(const wcstring &src, const parse_error_list_t &errors, - wcstring *output) const; + wcstring &output) const; /// Detect errors in the specified string when parsed as an argument list. Returns true if an /// error occurred. diff --git a/src/reader.cpp b/src/reader.cpp index ebd10681c..4a0e67c7b 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -1966,7 +1966,7 @@ parser_test_error_bits_t reader_shell_test(const wchar_t *b) { if (res & PARSER_TEST_ERROR) { wcstring error_desc; - parser_t::principal_parser().get_backtrace(bstr, errors, &error_desc); + parser_t::principal_parser().get_backtrace(bstr, errors, error_desc); // Ensure we end with a newline. Also add an initial newline, because it's likely the user // just hit enter and so there's junk on the current line. @@ -3315,7 +3315,7 @@ static int read_ni(int fd, const io_chain_t &io) { parser.eval(str, io, TOP, std::move(tree)); } else { wcstring sb; - parser.get_backtrace(str, errors, &sb); + parser.get_backtrace(str, errors, sb); fwprintf(stderr, L"%ls", sb.c_str()); res = 1; }