diff --git a/src/builtin_eval.cpp b/src/builtin_eval.cpp index 289024874..41512e63f 100644 --- a/src/builtin_eval.cpp +++ b/src/builtin_eval.cpp @@ -24,19 +24,16 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) { new_cmd += argv[i]; } - const auto cached_exec_count = parser.libdata().exec_count; int status = STATUS_CMD_OK; if (argc > 1) { - if (parser.eval(new_cmd, *streams.io_chain) != end_execution_reason_t::ok) { - status = STATUS_CMD_ERROR; - } else if (cached_exec_count == parser.libdata().exec_count) { + auto res = parser.eval(new_cmd, *streams.io_chain); + if (res.was_empty) { // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc. // where we have an argument but nothing is executed. status = STATUS_CMD_OK; } else { - status = parser.get_last_status(); + status = res.status.status_value(); } } - return status; } diff --git a/src/common.h b/src/common.h index d453e4b1d..6a0dcc656 100644 --- a/src/common.h +++ b/src/common.h @@ -817,6 +817,8 @@ enum { STATUS_ILLEGAL_CMD = 123, /// The status code used when `read` is asked to consume too much data. STATUS_READ_TOO_MUCH = 122, + /// The status code when an expansion fails, for example, "$foo[" + STATUS_EXPAND_ERROR = 121, }; /* Normally casting an expression to void discards its value, but GCC diff --git a/src/complete.cpp b/src/complete.cpp index 088c0ec47..507c1e9b6 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -615,41 +615,40 @@ void completer_t::complete_cmd_desc(const wcstring &str) { // systems with a large set of manuals, but it should be ok since apropos is only called once. lookup.clear(); list.clear(); - if (exec_subshell(lookup_cmd, *ctx.parser, list, false /* don't apply exit status */) != -1) { - // Then discard anything that is not a possible completion and put the result into a - // hashtable with the completion as key and the description as value. - // - // Should be reasonably fast, since no memory allocations are needed. - // mqudsi: I don't know if the above were ever true, but it's certainly not any more. - // Plenty of allocations below. - for (const wcstring &elstr : list) { - if (elstr.length() < cmd.length()) continue; - const wcstring fullkey(elstr, cmd.length()); + (void)exec_subshell(lookup_cmd, *ctx.parser, list, false /* don't apply exit status */); + // Then discard anything that is not a possible completion and put the result into a + // hashtable with the completion as key and the description as value. + // + // Should be reasonably fast, since no memory allocations are needed. + // mqudsi: I don't know if the above were ever true, but it's certainly not any more. + // Plenty of allocations below. + for (const wcstring &elstr : list) { + if (elstr.length() < cmd.length()) continue; + const wcstring fullkey(elstr, cmd.length()); - size_t tab_idx = fullkey.find(L'\t'); - if (tab_idx == wcstring::npos) continue; + size_t tab_idx = fullkey.find(L'\t'); + if (tab_idx == wcstring::npos) continue; - const wcstring key(fullkey, 0, tab_idx); - wcstring val(fullkey, tab_idx + 1); + const wcstring key(fullkey, 0, tab_idx); + wcstring val(fullkey, tab_idx + 1); - // And once again I make sure the first character is uppercased because I like it that - // way, and I get to decide these things. - if (!val.empty()) val[0] = towupper(val[0]); - lookup[key] = val; - } + // And once again I make sure the first character is uppercased because I like it that + // way, and I get to decide these things. + if (!val.empty()) val[0] = towupper(val[0]); + lookup[key] = val; + } - // Then do a lookup on every completion and if a match is found, change to the new - // description. - // - // This needs to do a reallocation for every description added, but there shouldn't be that - // many completions, so it should be ok. - for (auto &completion : completions) { - const wcstring &el = completion.completion; - if (el.empty()) continue; + // Then do a lookup on every completion and if a match is found, change to the new + // description. + // + // This needs to do a reallocation for every description added, but there shouldn't be that + // many completions, so it should be ok. + for (auto &completion : completions) { + const wcstring &el = completion.completion; + if (el.empty()) continue; - auto new_desc_iter = lookup.find(el); - if (new_desc_iter != lookup.end()) completion.description = new_desc_iter->second; - } + auto new_desc_iter = lookup.find(el); + if (new_desc_iter != lookup.end()) completion.description = new_desc_iter->second; } } @@ -683,7 +682,7 @@ void completer_t::complete_cmd(const wcstring &str_cmd) { if (result == expand_result_t::cancel) { return; } - if (result != expand_result_t::error && this->wants_descriptions()) { + if (result == expand_result_t::ok && this->wants_descriptions()) { this->complete_cmd_desc(str_cmd); } @@ -1592,7 +1591,7 @@ void completer_t::perform() { auto expand_ret = expand_string(expression, &expression_expanded, expand_flag::no_descriptions, ctx); wcstring_list_t vals; - if (expand_ret != expand_result_t::error) { + if (expand_ret == expand_result_t::ok) { for (auto &completion : expression_expanded) vals.emplace_back(std::move(completion.completion)); ctx.parser->vars().set(variable_name, ENV_LOCAL | ENV_EXPORT, diff --git a/src/exec.cpp b/src/exec.cpp index 89550712c..0076d4262 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -706,18 +706,7 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job, const parsed_source_ref_t &source = p->block_node_source; tnode_t node = p->internal_block_node; assert(source && node && "Process is missing node info"); - return [=](parser_t &parser) { - end_execution_reason_t res = parser.eval_node(source, node, lineage); - switch (res) { - case end_execution_reason_t::ok: - case end_execution_reason_t::error: - case end_execution_reason_t::cancelled: - return proc_status_t::from_exit_code(parser.get_last_status()); - case end_execution_reason_t::control_flow: - default: - DIE("end_execution_reason_t::control_flow should not be returned from eval_node"); - } - }; + return [=](parser_t &parser) { return parser.eval_node(source, node, lineage).status; }; } else { assert(p->type == process_type_t::function); auto props = function_get_properties(p->argv0()); @@ -729,25 +718,15 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job, return [=](parser_t &parser) { // Pull out the job list from the function. tnode_t body = props->func_node.child<1>(); - const auto &ld = parser.libdata(); - auto saved_exec_count = ld.exec_count; const block_t *fb = function_prepare_environment(parser, *argv, *props); auto res = parser.eval_node(props->parsed_source, body, lineage); function_restore_environment(parser, fb); - switch (res) { - case end_execution_reason_t::ok: - // If the function did not execute anything, treat it as success. - return proc_status_t::from_exit_code(saved_exec_count == ld.exec_count - ? EXIT_SUCCESS - : parser.get_last_status()); - case end_execution_reason_t::error: - case end_execution_reason_t::cancelled: - return proc_status_t::from_exit_code(parser.get_last_status()); - default: - case end_execution_reason_t::control_flow: - DIE("end_execution_reason_t::control_flow should not be returned from eval_node"); + // If the function did not execute anything, treat it as success. + if (res.was_empty) { + res = proc_status_t::from_exit_code(EXIT_SUCCESS); } + return res.status; }; } } @@ -879,7 +858,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, const std::share } if (p->type != process_type_t::block_node) { - // An simple `begin ... end` should not be considered an execution of a command. + // A simple `begin ... end` should not be considered an execution of a command. parser.libdata().exec_count++; } @@ -1121,60 +1100,10 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const job_lineage_t return true; } -static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstring_list_t *lst, - bool apply_exit_status, bool is_subcmd) { - ASSERT_IS_MAIN_THREAD(); - auto &ld = parser.libdata(); - bool prev_subshell = ld.is_subshell; - auto prev_statuses = parser.get_last_statuses(); - bool split_output = false; - - auto prev_read_limit = ld.read_limit; - ld.read_limit = is_subcmd ? read_byte_limit : 0; - - const auto ifs = parser.vars().get(L"IFS"); - if (!ifs.missing_or_empty()) { - split_output = true; - } - - ld.is_subshell = true; - auto subcommand_statuses = statuses_t::just(-1); // assume the worst - - // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may - // be null. - std::shared_ptr buffer; - if (auto bufferfill = io_bufferfill_t::create(fd_set_t{}, ld.read_limit)) { - if (parser.eval(cmd, io_chain_t{bufferfill}, block_type_t::subst) == end_execution_reason_t::ok) { - subcommand_statuses = parser.get_last_statuses(); - } - buffer = io_bufferfill_t::finish(std::move(bufferfill)); - } - - if (buffer && buffer->buffer().discarded()) { - subcommand_statuses = statuses_t::just(STATUS_READ_TOO_MUCH); - } - - // If the caller asked us to preserve the exit status, restore the old status. Otherwise set the - // status of the subcommand. - if (apply_exit_status) { - // Hack: If the evaluation failed, avoid returning -1 to the user. - if (subcommand_statuses.status == -1) { - parser.set_last_statuses(statuses_t::just(255)); - } else { - parser.set_last_statuses(subcommand_statuses); - } - } else { - parser.set_last_statuses(std::move(prev_statuses)); - } - - ld.is_subshell = prev_subshell; - ld.read_limit = prev_read_limit; - - if (lst == nullptr || !buffer) { - return subcommand_statuses.status; - } +/// Populate \p lst with the output of \p buffer, perhaps splitting lines according to \p split. +static void populate_subshell_output(wcstring_list_t *lst, const io_buffer_t &buffer, bool split) { // Walk over all the elements. - for (const auto &elem : buffer->buffer().elements()) { + for (const auto &elem : buffer.buffer().elements()) { if (elem.is_explicitly_separated()) { // Just append this one. lst->push_back(str2wcstring(elem.contents)); @@ -1185,7 +1114,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin assert(!elem.is_explicitly_separated() && "should not be explicitly separated"); const char *begin = elem.contents.data(); const char *end = begin + elem.contents.size(); - if (split_output) { + if (split) { const char *cursor = begin; while (cursor < end) { // Look for the next separator. @@ -1210,17 +1139,73 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin lst->push_back(str2wcstring(begin, end - begin)); } } +} - return subcommand_statuses.status; +/// Execute \p cmd in a subshell in \p parser. If \p lst is not null, populate it with the output. +/// Return $status in \p out_status. +/// If \p apply_exit_status is false, then reset $status back to its original value. +/// \p is_subcmd controls whether we apply a read limit. +/// \p break_expand is used to propagate whether the result should be "expansion breaking" in the +/// sense that subshells used during string expansion should halt that expansion. \return the value +/// of $status. +static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstring_list_t *lst, + bool *break_expand, bool apply_exit_status, bool is_subcmd) { + ASSERT_IS_MAIN_THREAD(); + auto &ld = parser.libdata(); + + scoped_push is_subshell(&ld.is_subshell, true); + scoped_push read_limit(&ld.read_limit, is_subcmd ? read_byte_limit : 0); + + auto prev_statuses = parser.get_last_statuses(); + const cleanup_t put_back([&] { + if (!apply_exit_status) { + parser.set_last_statuses(prev_statuses); + } + }); + + const bool split_output = !parser.vars().get(L"IFS").missing_or_empty(); + + // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may + // be null. + auto bufferfill = io_bufferfill_t::create(fd_set_t{}, ld.read_limit); + if (!bufferfill) { + *break_expand = true; + return STATUS_CMD_ERROR; + } + eval_res_t eval_res = parser.eval(cmd, io_chain_t{bufferfill}, block_type_t::subst); + std::shared_ptr buffer = io_bufferfill_t::finish(std::move(bufferfill)); + if (buffer->buffer().discarded()) { + *break_expand = true; + return STATUS_READ_TOO_MUCH; + } + + if (eval_res.break_expand) { + *break_expand = true; + return eval_res.status.status_value(); + } + + if (lst) { + populate_subshell_output(lst, *buffer, split_output); + } + *break_expand = false; + return eval_res.status.status_value(); +} + +int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs) { + ASSERT_IS_MAIN_THREAD(); + bool break_expand = false; + int ret = exec_subshell_internal(cmd, parser, &outputs, &break_expand, true, true); + // Only return an error code if we should break expansion. + return break_expand ? ret : STATUS_CMD_OK; +} + +int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status) { + bool break_expand = false; + return exec_subshell_internal(cmd, parser, nullptr, &break_expand, apply_exit_status, false); } int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs, - bool apply_exit_status, bool is_subcmd) { - ASSERT_IS_MAIN_THREAD(); - return exec_subshell_internal(cmd, parser, &outputs, apply_exit_status, is_subcmd); -} - -int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status, bool is_subcmd) { - ASSERT_IS_MAIN_THREAD(); - return exec_subshell_internal(cmd, parser, nullptr, apply_exit_status, is_subcmd); + bool apply_exit_status) { + bool break_expand = false; + return exec_subshell_internal(cmd, parser, &outputs, &break_expand, apply_exit_status, false); } diff --git a/src/exec.h b/src/exec.h index af3f1481b..7f9b03fab 100644 --- a/src/exec.h +++ b/src/exec.h @@ -17,17 +17,22 @@ struct job_lineage_t; class parser_t; bool exec_job(parser_t &parser, const std::shared_ptr &j, const job_lineage_t &lineage); -/// Evaluate the expression cmd in a subshell, add the outputs into the list l. On return, the -/// status flag as returned bu \c proc_gfet_last_status will not be changed. +/// Evaluate a command. /// /// \param cmd the command to execute -/// \param outputs The list to insert output into. +/// \param parser the parser with which to execute code +/// \param outputs the list to insert output into. +/// \param apply_exit_status if set, update $status within the parser, otherwise do not. /// -/// \return the status of the last job to exit, or -1 if en error was encountered. +/// \return a value appropriate for populating $status. +int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status); int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs, - bool apply_exit_status, bool is_subcmd = false); -int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status, - bool is_subcmd = false); + bool apply_exit_status); + +/// Like exec_subshell, but only returns expansion-breaking errors. That is, a zero return means +/// "success" (even though the command may have failed), a non-zero return means that we should +/// halt expansion. +int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs); /// Loops over close until the syscall was run without being interrupted. void exec_close(int fd); diff --git a/src/expand.cpp b/src/expand.cpp index 990295037..c175180de 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -528,7 +528,7 @@ static expand_result_t expand_braces(const wcstring &instr, expand_flags_t flags if (syntax_error) { append_syntax_error(errors, SOURCE_LOCATION_UNKNOWN, _(L"Mismatched braces")); - return expand_result_t::error; + return expand_result_t::make_error(STATUS_EXPAND_ERROR); } if (brace_begin == nullptr) { @@ -574,9 +574,10 @@ static expand_result_t expand_braces(const wcstring &instr, expand_flags_t flags return expand_result_t::ok; } -/// Perform cmdsubst expansion. -static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t *out_list, - parse_error_list_t *errors) { +/// Expand a command substitution \p input, executing on \p parser, and inserting the results into +/// \p out_list, or any errors into \p errors. \return an expand result. +static expand_result_t expand_cmdsubst(wcstring input, parser_t &parser, + completion_list_t *out_list, parse_error_list_t *errors) { wchar_t *paren_begin = nullptr, *paren_end = nullptr; wchar_t *tail_begin = nullptr; size_t i, j; @@ -586,11 +587,11 @@ static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t switch (parse_util_locate_cmdsubst(in, &paren_begin, &paren_end, false)) { case -1: { append_syntax_error(errors, SOURCE_LOCATION_UNKNOWN, L"Mismatched parenthesis"); - return false; + return expand_result_t::make_error(STATUS_EXPAND_ERROR); } case 0: { append_completion(out_list, std::move(input)); - return true; + return expand_result_t::ok; } case 1: { break; @@ -603,18 +604,23 @@ static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t wcstring_list_t sub_res; const wcstring subcmd(paren_begin + 1, paren_end - paren_begin - 1); - if (exec_subshell(subcmd, parser, sub_res, true /* apply_exit_status */, - true /* is_subcmd */) == -1) { - append_cmdsub_error(errors, SOURCE_LOCATION_UNKNOWN, - L"Unknown error while evaluating command substitution"); - return false; - } - - if (parser.get_last_status() == STATUS_READ_TOO_MUCH) { - append_cmdsub_error( - errors, in - paren_begin, - _(L"Too much data emitted by command substitution so it was discarded\n")); - return false; + int subshell_status = exec_subshell_for_expand(subcmd, parser, sub_res); + if (subshell_status != 0) { + // TODO: Ad-hoc switch, how can we enumerate the possible errors more safely? + const wchar_t *err; + switch (subshell_status) { + case STATUS_READ_TOO_MUCH: + err = L"Too much data emitted by command substitution so it was discarded"; + break; + case STATUS_CMD_ERROR: + err = L"Too many active file descriptors"; + break; + default: + err = L"Unknown error while evaluating command substitution"; + break; + } + append_cmdsub_error(errors, in - paren_begin, _(err)); + return expand_result_t::make_error(subshell_status); } tail_begin = paren_end + 1; @@ -627,7 +633,7 @@ static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t bad_pos = parse_slice(slice_begin, &slice_end, slice_idx, sub_res.size()); if (bad_pos != 0) { append_syntax_error(errors, slice_begin - in + bad_pos, L"Invalid index value"); - return false; + return expand_result_t::make_error(STATUS_EXPAND_ERROR); } wcstring_list_t sub_res2; @@ -684,7 +690,7 @@ static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t } } - return parser.get_last_status() != STATUS_READ_TOO_MUCH; + return expand_result_t::ok; } // Given that input[0] is HOME_DIRECTORY or tilde (ugh), return the user's name. Return the empty @@ -885,21 +891,18 @@ expand_result_t expander_t::stage_cmdsubst(wcstring input, completion_list_t *ou switch (parse_util_locate_cmdsubst_range(input, &cur, nullptr, &start, &end, true)) { case 0: append_completion(out, std::move(input)); - break; + return expand_result_t::ok; case 1: append_cmdsub_error(errors, start, L"Command substitutions not allowed"); /* intentionally falls through */ case -1: default: - return expand_result_t::error; + return expand_result_t::make_error(STATUS_EXPAND_ERROR); } } else { assert(ctx.parser && "Must have a parser to expand command substitutions"); - bool cmdsubst_ok = expand_cmdsubst(std::move(input), *ctx.parser, out, errors); - if (!cmdsubst_ok) return expand_result_t::error; + return expand_cmdsubst(std::move(input), *ctx.parser, out, errors); } - - return expand_result_t::ok; } expand_result_t expander_t::stage_variables(wcstring input, completion_list_t *out) { @@ -918,7 +921,7 @@ expand_result_t expander_t::stage_variables(wcstring input, completion_list_t *o } else { size_t size = next.size(); if (!expand_variables(std::move(next), out, size, ctx.vars, errors)) { - return expand_result_t::error; + return expand_result_t::make_error(STATUS_EXPAND_ERROR); } } return expand_result_t::ok; @@ -1085,7 +1088,7 @@ expand_result_t expander_t::expand_string(wcstring input, completion_list_t *out total_result = expand_result_t::ok; } - if (total_result != expand_result_t::error) { + if (total_result == expand_result_t::ok) { // Hack to un-expand tildes (see #647). if (!(flags & expand_flag::skip_home_directories)) { unexpand_tildes(input, ctx.vars, &completions); @@ -1113,7 +1116,7 @@ bool expand_one(wcstring &string, expand_flags_t flags, const operation_context_ } if (expand_string(std::move(string), &completions, flags | expand_flag::no_descriptions, ctx, - errors) != expand_result_t::error && + errors) == expand_result_t::ok && completions.size() == 1) { string = std::move(completions.at(0).completion); return true; diff --git a/src/expand.h b/src/expand.h index 8aa2915cc..c4cf99a04 100644 --- a/src/expand.h +++ b/src/expand.h @@ -100,16 +100,39 @@ enum : wchar_t { }; /// These are the possible return values for expand_string. -enum class expand_result_t { - /// There was a syntax error, for example, unmatched braces. - error, - /// Expansion succeeded. - ok, - /// Expansion was cancelled (e.g. control-C). - cancel, - /// Expansion succeeded, but a wildcard in the string matched no files, - /// so the output is empty. - wildcard_no_match, +struct expand_result_t { + enum result_t { + /// There was an error, for example, unmatched braces. + error, + /// Expansion succeeded. + ok, + /// Expansion was cancelled (e.g. control-C). + cancel, + /// Expansion succeeded, but a wildcard in the string matched no files, + /// so the output is empty. + wildcard_no_match, + }; + + /// The result of expansion. + result_t result; + + /// If expansion resulted in an error, this is an appropriate value with which to populate + /// $status. + int status{0}; + + /* implicit */ expand_result_t(result_t result) : result(result) {} + + /// operator== allows for comparison against result_t values. + bool operator==(result_t rhs) const { return result == rhs; } + bool operator!=(result_t rhs) const { return !(*this == rhs); } + + /// Make an error value with the given status. + static expand_result_t make_error(int status) { + assert(status != 0 && "status cannot be 0 for an error result"); + expand_result_t result(error); + result.status = status; + return result; + } }; /// The string represented by PROCESS_EXPAND_SELF diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 998cf9e36..904bc93bd 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -1033,16 +1033,13 @@ static void test_1_cancellation(const wchar_t *src) { usleep(delay * 1E6); pthread_kill(thread, SIGINT); }); - end_execution_reason_t ret = parser_t::principal_parser().eval(src, io_chain_t{filler}); + eval_res_t res = parser_t::principal_parser().eval(src, io_chain_t{filler}); auto buffer = io_bufferfill_t::finish(std::move(filler)); if (buffer->buffer().size() != 0) { err(L"Expected 0 bytes in out_buff, but instead found %lu bytes, for command %ls\n", buffer->buffer().size(), src); } - // TODO: cancelling out of command substitutions is currently reported as an error, not a - // cancellation. - // do_test(ret == end_execution_reason_t::cancelled); - (void)ret; + do_test(res.status.signal_exited() && res.status.signal_code() == SIGINT); iothread_drain_all(); } diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 6dc3e81f7..6bc339ef8 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -259,8 +259,8 @@ end_execution_reason_t parse_execution_context_t::run_if_statement( tnode_t condition_head = if_clause.child<1>(); tnode_t condition_boolean_tail = if_clause.child<3>(); - // Check the condition and the tail. We treat end_execution_reason_t::error here as failure, in - // accordance with historic behavior. + // Check the condition and the tail. We treat end_execution_reason_t::error here as failure, + // in accordance with historic behavior. end_execution_reason_t cond_ret = run_job_conjunction(condition_head, associated_block); if (cond_ret == end_execution_reason_t::ok) { cond_ret = run_job_list(condition_boolean_tail, associated_block); @@ -350,10 +350,8 @@ end_execution_reason_t parse_execution_context_t::run_function_statement( wcstring errtext = streams.err.contents(); if (!errtext.empty()) { - this->report_error(header, L"%ls", errtext.c_str()); - result = end_execution_reason_t::error; + return this->report_error(err, header, L"%ls", errtext.c_str()); } - return result; } @@ -385,8 +383,8 @@ end_execution_reason_t parse_execution_context_t::run_for_statement( tnode_t var_name_node = header.child<1>(); wcstring for_var_name = get_source(var_name_node); if (!expand_one(for_var_name, expand_flags_t{}, ctx)) { - report_error(var_name_node, FAILED_EXPANSION_VARIABLE_NAME_ERR_MSG, for_var_name.c_str()); - return end_execution_reason_t::error; + return report_error(STATUS_EXPAND_ERROR, var_name_node, + FAILED_EXPANSION_VARIABLE_NAME_ERR_MSG, for_var_name.c_str()); } // Get the contents to iterate over. @@ -399,9 +397,9 @@ end_execution_reason_t parse_execution_context_t::run_for_statement( auto var = parser->vars().get(for_var_name, ENV_DEFAULT); if (var && var->read_only()) { - report_error(var_name_node, L"You cannot use read-only variable '%ls' in a for loop", - for_var_name.c_str()); - return end_execution_reason_t::error; + return report_error(STATUS_INVALID_ARGS, var_name_node, + L"You cannot use read-only variable '%ls' in a for loop", + for_var_name.c_str()); } int retval; if (var) { @@ -412,8 +410,8 @@ end_execution_reason_t parse_execution_context_t::run_for_statement( assert(retval == ENV_OK); if (!valid_var_name(for_var_name)) { - report_error(var_name_node, BUILTIN_ERR_VARNAME, L"for", for_var_name.c_str()); - return end_execution_reason_t::error; + return report_error(STATUS_INVALID_ARGS, var_name_node, BUILTIN_ERR_VARNAME, L"for", + for_var_name.c_str()); } trace_if_enabled(*parser, L"for", arguments); @@ -462,19 +460,20 @@ end_execution_reason_t parse_execution_context_t::run_switch_statement( expand_flag::no_descriptions, ctx, &errors); parse_error_offset_source_start(&errors, switch_value_n.source_range()->start); - switch (expand_ret) { + switch (expand_ret.result) { case expand_result_t::error: - return report_errors(errors); + return report_errors(expand_ret.status, errors); case expand_result_t::cancel: return end_execution_reason_t::cancelled; case expand_result_t::wildcard_no_match: - return report_unmatched_wildcard_error(switch_value_n); + return report_error(STATUS_UNMATCHED_WILDCARD, switch_value_n, WILDCARD_ERR_MSG, + get_source(switch_value_n).c_str()); case expand_result_t::ok: if (switch_values_expanded.size() > 1) { - return report_error(switch_value_n, + return report_error(STATUS_INVALID_ARGS, switch_value_n, _(L"switch: Expected at most one argument, got %lu\n"), switch_values_expanded.size()); } @@ -617,9 +616,8 @@ end_execution_reason_t parse_execution_context_t::run_while_statement( return ret; } -// Reports an error. Always returns end_execution_reason_t::error, so you can assign the result to an -// 'errored' variable. -end_execution_reason_t parse_execution_context_t::report_error(const parse_node_t &node, +// Reports an error. Always returns end_execution_reason_t::error. +end_execution_reason_t parse_execution_context_t::report_error(int status, const parse_node_t &node, const wchar_t *fmt, ...) const { // Create an error. parse_error_list_t error_list = parse_error_list_t(1); @@ -633,12 +631,11 @@ end_execution_reason_t parse_execution_context_t::report_error(const parse_node_ error->text = vformat_string(fmt, va); va_end(va); - this->report_errors(error_list); - return end_execution_reason_t::error; + return this->report_errors(status, error_list); } end_execution_reason_t parse_execution_context_t::report_errors( - const parse_error_list_t &error_list) const { + int status, const parse_error_list_t &error_list) const { if (!parser->cancellation_signal) { if (error_list.empty()) { FLOG(error, L"Error reported but no error text found."); @@ -652,15 +649,10 @@ end_execution_reason_t parse_execution_context_t::report_errors( if (!should_suppress_stderr_for_tests()) { std::fwprintf(stderr, L"%ls", backtrace_and_desc.c_str()); } - } - return end_execution_reason_t::error; -} -/// Reports an unmatched wildcard error and returns end_execution_reason_t::error. -end_execution_reason_t parse_execution_context_t::report_unmatched_wildcard_error( - const parse_node_t &unmatched_wildcard) const { - parser->set_last_statuses(statuses_t::just(STATUS_UNMATCHED_WILDCARD)); - report_error(unmatched_wildcard, WILDCARD_ERR_MSG, get_source(unmatched_wildcard).c_str()); + // Mark status. + parser->set_last_statuses(statuses_t::just(status)); + } return end_execution_reason_t::error; } @@ -672,7 +664,8 @@ end_execution_reason_t parse_execution_context_t::handle_command_not_found( const wchar_t *const cmd = cmd_str.c_str(); if (err_code != ENOENT) { - this->report_error(statement, _(L"The file '%ls' is not executable by this user"), cmd); + return this->report_error(STATUS_NOT_EXECUTABLE, statement, + _(L"The file '%ls' is not executable by this user"), cmd); } else { // Handle unrecognized commands with standard command not found handler that can make better // error messages. @@ -692,14 +685,8 @@ end_execution_reason_t parse_execution_context_t::handle_command_not_found( event_fire_generic(*parser, L"fish_command_not_found", &event_args); // Here we want to report an error (so it shows a backtrace), but with no text. - this->report_error(statement, L""); + return this->report_error(STATUS_CMD_UNKNOWN, statement, L""); } - - // Set the last proc status appropriately. - int status = err_code == ENOENT ? STATUS_CMD_UNKNOWN : STATUS_NOT_EXECUTABLE; - parser->set_last_statuses(statuses_t::just(status)); - - return end_execution_reason_t::error; } end_execution_reason_t parse_execution_context_t::expand_command( @@ -718,21 +705,22 @@ end_execution_reason_t parse_execution_context_t::expand_command( expand_result_t expand_err = expand_to_command_and_args(unexp_cmd, ctx, out_cmd, out_args, &errors); if (expand_err == expand_result_t::error) { - parser->set_last_statuses(statuses_t::just(STATUS_ILLEGAL_CMD)); // Issue #5812 - the expansions were done on the command token, // excluding prefixes such as " " or "if ". // This means that the error positions are relative to the beginning // of the token; we need to make them relative to the original source. for (auto &error : errors) error.source_start += pos_of_command_token; - return report_errors(errors); + return report_errors(STATUS_ILLEGAL_CMD, errors); } else if (expand_err == expand_result_t::wildcard_no_match) { - return report_unmatched_wildcard_error(statement); + return report_error(STATUS_UNMATCHED_WILDCARD, statement, WILDCARD_ERR_MSG, + get_source(statement).c_str()); } assert(expand_err == expand_result_t::ok); // Complain if the resulting expansion was empty, or expanded to an empty string. if (out_cmd->empty()) { - return this->report_error(statement, _(L"The expanded command was empty.")); + return this->report_error(STATUS_ILLEGAL_CMD, statement, + _(L"The expanded command was empty.")); } return end_execution_reason_t::ok; } @@ -840,8 +828,9 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process( } // The set of IO redirections that we construct for the process. - if (!this->determine_redirections(statement.child<1>(), &redirections)) { - return end_execution_reason_t::error; + auto reason = this->determine_redirections(statement.child<1>(), &redirections); + if (reason != end_execution_reason_t::ok) { + return reason; } // Determine the process type. @@ -876,18 +865,19 @@ end_execution_reason_t parse_execution_context_t::expand_arguments_from_nodes( auto expand_ret = expand_string(arg_str, &arg_expanded, expand_flag::no_descriptions, ctx, &errors); parse_error_offset_source_start(&errors, arg_node.source_range()->start); - switch (expand_ret) { + switch (expand_ret.result) { case expand_result_t::error: { - return this->report_errors(errors); + return this->report_errors(expand_ret.status, errors); } + case expand_result_t::cancel: { return end_execution_reason_t::cancelled; } case expand_result_t::wildcard_no_match: { if (glob_behavior == failglob) { // Report the unmatched wildcard error and stop processing. - report_unmatched_wildcard_error(arg_node); - return end_execution_reason_t::error; + return report_error(STATUS_UNMATCHED_WILDCARD, arg_node, WILDCARD_ERR_MSG, + get_source(arg_node).c_str()); } break; } @@ -916,7 +906,7 @@ end_execution_reason_t parse_execution_context_t::expand_arguments_from_nodes( return end_execution_reason_t::ok; } -bool parse_execution_context_t::determine_redirections( +end_execution_reason_t parse_execution_context_t::determine_redirections( tnode_t node, redirection_spec_list_t *out_redirections) { // Get all redirection nodes underneath the statement. while (auto redirect_node = node.next_in_list()) { @@ -925,9 +915,8 @@ bool parse_execution_context_t::determine_redirections( if (!redirect || !redirect->is_valid()) { // TODO: figure out if this can ever happen. If so, improve this error message. - report_error(redirect_node, _(L"Invalid redirection: %ls"), - redirect_node.get_source(pstree->src).c_str()); - return false; + return report_error(STATUS_INVALID_ARGS, redirect_node, _(L"Invalid redirection: %ls"), + redirect_node.get_source(pstree->src).c_str()); } // PCA: I can't justify this skip_variables flag. It was like this when I got here. @@ -935,8 +924,8 @@ bool parse_execution_context_t::determine_redirections( expand_one(target, no_exec() ? expand_flag::skip_variables : expand_flags_t{}, ctx); if (!target_expanded || target.empty()) { // TODO: Improve this error message. - report_error(redirect_node, _(L"Invalid redirection target: %ls"), target.c_str()); - return false; + return report_error(STATUS_INVALID_ARGS, redirect_node, + _(L"Invalid redirection target: %ls"), target.c_str()); } // Make a redirection spec from the redirect token. @@ -948,8 +937,7 @@ bool parse_execution_context_t::determine_redirections( if (spec.mode == redirection_mode_t::fd && !spec.is_close() && !spec.get_target_as_fd()) { const wchar_t *fmt = _(L"Requested redirection to '%ls', which is not a valid file descriptor"); - report_error(redirect_node, fmt, spec.target.c_str()); - return false; + return report_error(STATUS_INVALID_ARGS, redirect_node, fmt, spec.target.c_str()); } out_redirections->push_back(std::move(spec)); @@ -959,7 +947,7 @@ bool parse_execution_context_t::determine_redirections( out_redirections->push_back(get_stderr_merge()); } } - return true; + return end_execution_reason_t::ok; } end_execution_reason_t parse_execution_context_t::populate_not_process( @@ -970,8 +958,7 @@ end_execution_reason_t parse_execution_context_t::populate_not_process( if (optional_time.tag() == parse_optional_time_time) { flags.has_time_prefix = true; if (!job->mut_flags().foreground) { - this->report_error(not_statement, ERROR_TIME_BACKGROUND); - return end_execution_reason_t::error; + return this->report_error(STATUS_INVALID_ARGS, not_statement, ERROR_TIME_BACKGROUND); } } return this->populate_job_process( @@ -996,15 +983,14 @@ end_execution_reason_t parse_execution_context_t::populate_block_process( // TODO: fix this ugly find_child. auto arguments = specific_statement.template find_child(); redirection_spec_list_t redirections; - if (!this->determine_redirections(arguments, &redirections)) { - return end_execution_reason_t::error; + auto reason = this->determine_redirections(arguments, &redirections); + if (reason == end_execution_reason_t::ok) { + proc->type = process_type_t::block_node; + proc->block_node_source = pstree; + proc->internal_block_node = statement; + proc->set_redirection_specs(std::move(redirections)); } - - proc->type = process_type_t::block_node; - proc->block_node_source = pstree; - proc->internal_block_node = statement; - proc->set_redirection_specs(std::move(redirections)); - return end_execution_reason_t::ok; + return reason; } end_execution_reason_t parse_execution_context_t::apply_variable_assignments( @@ -1027,18 +1013,17 @@ end_execution_reason_t parse_execution_context_t::apply_variable_assignments( expand_flag::no_descriptions, ctx, &errors); parse_error_offset_source_start( &errors, variable_assignment.source_range()->start + *equals_pos + 1); - switch (expand_ret) { - case expand_result_t::error: { - this->report_errors(errors); - return end_execution_reason_t::error; - } - case expand_result_t::cancel: { + switch (expand_ret.result) { + case expand_result_t::error: + return this->report_errors(expand_ret.status, errors); + + case expand_result_t::cancel: return end_execution_reason_t::cancelled; - } + case expand_result_t::wildcard_no_match: // nullglob (equivalent to set) - case expand_result_t::ok: { + case expand_result_t::ok: break; - } + default: { DIE("unexpected expand_string() return value"); break; @@ -1066,7 +1051,7 @@ end_execution_reason_t parse_execution_context_t::populate_job_process( cleanup_t scope([&]() { if (block) parser->pop_block(block); }); - if (result != end_execution_reason_t::ok) return end_execution_reason_t::error; + if (result != end_execution_reason_t::ok) return result; switch (specific_statement.type) { case symbol_not_statement: { @@ -1122,8 +1107,7 @@ end_execution_reason_t parse_execution_context_t::populate_job_from_job_node( if (optional_time.tag() == parse_optional_time_time) { j->mut_flags().has_time_prefix = true; if (job_node_is_background(job_node)) { - this->report_error(job_node, ERROR_TIME_BACKGROUND); - return end_execution_reason_t::error; + return this->report_error(STATUS_INVALID_ARGS, job_node, ERROR_TIME_BACKGROUND); } } end_execution_reason_t result = @@ -1144,7 +1128,8 @@ end_execution_reason_t parse_execution_context_t::populate_job_from_job_node( auto parsed_pipe = pipe_or_redir_t::from_string(get_source(pipe)); assert(parsed_pipe.has_value() && parsed_pipe->is_pipe && "Failed to parse valid pipe"); if (!parsed_pipe->is_valid()) { - result = report_error(pipe, ILLEGAL_FD_ERR_MSG, get_source(pipe).c_str()); + result = report_error(STATUS_INVALID_ARGS, pipe, ILLEGAL_FD_ERR_MSG, + get_source(pipe).c_str()); break; } processes.back()->pipe_write_fd = parsed_pipe->fd; @@ -1200,6 +1185,7 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t job_ if (parser->is_interactive() && tcgetattr(STDIN_FILENO, &tmodes)) { // Need real error handling here. wperror(L"tcgetattr"); + parser->set_last_statuses(statuses_t::just(STATUS_CMD_ERROR)); return end_execution_reason_t::error; } @@ -1452,13 +1438,13 @@ end_execution_reason_t parse_execution_context_t::eval_node(tnode_t this->infinite_recursive_statement_in_job_list(job_list, &func_name); if (infinite_recursive_node) { // We have an infinite recursion. - return this->report_error(infinite_recursive_node, INFINITE_FUNC_RECURSION_ERR_MSG, - func_name.c_str()); + return this->report_error(STATUS_CMD_ERROR, infinite_recursive_node, + INFINITE_FUNC_RECURSION_ERR_MSG, func_name.c_str()); } // Check for stack overflow. The TOP check ensures we only do this for function calls. if (associated_block->type() == block_type_t::top && parser->function_stack_is_overflowing()) { - return this->report_error(job_list, CALL_STACK_LIMIT_EXCEEDED_ERR_MSG); + return this->report_error(STATUS_CMD_ERROR, job_list, CALL_STACK_LIMIT_EXCEEDED_ERR_MSG); } return this->run_job_list(job_list, associated_block); } diff --git a/src/parse_execution.h b/src/parse_execution.h index 436bbe6ec..35b6fd4b6 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -52,13 +52,11 @@ class parse_execution_context_t { // This will never return end_execution_reason_t::ok. maybe_t check_end_execution() const; - // Report an error. Always returns 'end_execution_reason_t::error'. - end_execution_reason_t report_error(const parse_node_t &node, const wchar_t *fmt, ...) const; - end_execution_reason_t report_errors(const parse_error_list_t &error_list) const; - - // Wildcard error helper. - end_execution_reason_t report_unmatched_wildcard_error( - const parse_node_t &unmatched_wildcard) const; + // Report an error, setting $status to \p status. Always returns + // 'end_execution_reason_t::error'. + end_execution_reason_t report_error(int status, const parse_node_t &node, const wchar_t *fmt, + ...) const; + end_execution_reason_t report_errors(int status, const parse_error_list_t &error_list) const; /// Command not found support. end_execution_reason_t handle_command_not_found(const wcstring &cmd, @@ -122,10 +120,10 @@ class parse_execution_context_t { wcstring_list_t *out_arguments, globspec_t glob_behavior); - // Determines the list of redirections for a node. Returns none() on failure, for example, an - // invalid fd. - bool determine_redirections(tnode_t node, - redirection_spec_list_t *out_redirections); + // Determines the list of redirections for a node. + end_execution_reason_t determine_redirections( + tnode_t node, + redirection_spec_list_t *out_redirections); end_execution_reason_t run_1_job(tnode_t job, const block_t *associated_block); end_execution_reason_t run_job_conjunction(tnode_t job_expr, diff --git a/src/parser.cpp b/src/parser.cpp index ab5644761..f5a564b88 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -614,8 +614,7 @@ profile_item_t *parser_t::create_profile_item() { return result; } -end_execution_reason_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, - enum block_type_t block_type) { +eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, enum block_type_t block_type) { // Parse the source into a tree, if we can. parse_error_list_t error_list; if (parsed_source_ref_t ps = parse_source(cmd, parse_flag_none, &error_list)) { @@ -627,12 +626,16 @@ end_execution_reason_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, // Print it. std::fwprintf(stderr, L"%ls\n", backtrace_and_desc.c_str()); - return end_execution_reason_t::error; + + // Set a valid status. + this->set_last_statuses(statuses_t::just(STATUS_ILLEGAL_CMD)); + bool break_expand = true; + return eval_res_t{proc_status_t::from_exit_code(STATUS_ILLEGAL_CMD), break_expand}; } } -end_execution_reason_t parser_t::eval(const parsed_source_ref_t &ps, const io_chain_t &io, - enum block_type_t block_type) { +eval_res_t parser_t::eval(const parsed_source_ref_t &ps, const io_chain_t &io, + enum block_type_t block_type) { assert(block_type == block_type_t::top || block_type == block_type_t::subst); if (!ps->tree.empty()) { job_lineage_t lineage; @@ -640,13 +643,17 @@ end_execution_reason_t parser_t::eval(const parsed_source_ref_t &ps, const io_ch // Execute the first node. tnode_t start{&ps->tree, &ps->tree.front()}; return this->eval_node(ps, start, std::move(lineage), block_type); + } else { + auto status = proc_status_t::from_exit_code(get_last_status()); + bool break_expand = false; + bool was_empty = true; + return eval_res_t{status, break_expand, was_empty}; } - return end_execution_reason_t::ok; } template -end_execution_reason_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_t node, - job_lineage_t lineage, block_type_t block_type) { +eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_t node, + job_lineage_t lineage, block_type_t block_type) { static_assert( std::is_same::value || std::is_same::value, "Unexpected node type"); @@ -655,7 +662,7 @@ end_execution_reason_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_ // not empty, we are still in the process of cancelling; refuse to evaluate anything. if (this->cancellation_signal) { if (!block_list.empty()) { - return end_execution_reason_t::cancelled; + return proc_status_t::from_signal(this->cancellation_signal); } this->cancellation_signal = 0; } @@ -674,25 +681,33 @@ end_execution_reason_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_ using exc_ctx_ref_t = std::unique_ptr; scoped_push exc(&execution_context, make_unique( ps, this, op_ctx, std::move(lineage))); - end_execution_reason_t res = execution_context->eval_node(node, scope_block); + + // Check the exec count so we know if anything got executed. + const size_t prev_exec_count = libdata().exec_count; + end_execution_reason_t reason = execution_context->eval_node(node, scope_block); + const size_t new_exec_count = libdata().exec_count; + exc.restore(); this->pop_block(scope_block); job_reap(*this, false); // reap again - // control_flow is used internally to react to break and return. - // Here we treat that as success. - if (res == end_execution_reason_t::control_flow) { - res = end_execution_reason_t::ok; + if (this->cancellation_signal) { + // We were signalled. + return proc_status_t::from_signal(this->cancellation_signal); + } else { + auto status = proc_status_t::from_exit_code(this->get_last_status()); + bool break_expand = (reason == end_execution_reason_t::error); + bool was_empty = !break_expand && prev_exec_count == new_exec_count; + return eval_res_t{status, break_expand, was_empty}; } - return res; } // Explicit instantiations. TODO: use overloads instead? -template end_execution_reason_t parser_t::eval_node(const parsed_source_ref_t &, tnode_t, - job_lineage_t, block_type_t); -template end_execution_reason_t parser_t::eval_node(const parsed_source_ref_t &, tnode_t, - job_lineage_t, block_type_t); +template eval_res_t parser_t::eval_node(const parsed_source_ref_t &, tnode_t, + job_lineage_t, block_type_t); +template eval_res_t parser_t::eval_node(const parsed_source_ref_t &, tnode_t, + job_lineage_t, block_type_t); void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &errors, wcstring &output) const { diff --git a/src/parser.h b/src/parser.h index 494fe7164..2cb261235 100644 --- a/src/parser.h +++ b/src/parser.h @@ -200,6 +200,24 @@ struct library_data_t { class operation_context_t; +/// The result of parser_t::eval family. +struct eval_res_t { + /// The value for $status. + proc_status_t status; + + /// If set, there was an error that should be considered a failed expansion, such as + /// command-not-found. For example, `touch (not-a-command)` will not invoke 'touch' because + /// command-not-found will mark break_expand. + bool break_expand; + + /// If set, no commands were executed and there we no errors. + bool was_empty{false}; + + /* implicit */ eval_res_t(proc_status_t status, bool break_expand = false, + bool was_empty = false) + : status(status), break_expand(break_expand), was_empty(was_empty) {} +}; + class parser_t : public std::enable_shared_from_this { friend class parse_execution_context_t; @@ -266,22 +284,23 @@ class parser_t : public std::enable_shared_from_this { /// \param io io redirections to perform on all started jobs /// \param block_type The type of block to push on the block stack, which must be either 'top' /// or 'subst'. + /// \param break_expand If not null, return by reference whether the error ought to be an expand + /// error. This includes nested expand errors, and command-not-found. /// - /// \return the eval result, - end_execution_reason_t eval(const wcstring &cmd, const io_chain_t &io, - block_type_t block_type = block_type_t::top); + /// \return the result of evaluation. + eval_res_t eval(const wcstring &cmd, const io_chain_t &io, + block_type_t block_type = block_type_t::top); /// Evaluate the parsed source ps. /// Because the source has been parsed, a syntax error is impossible. - end_execution_reason_t eval(const parsed_source_ref_t &ps, const io_chain_t &io, - block_type_t block_type = block_type_t::top); + eval_res_t eval(const parsed_source_ref_t &ps, const io_chain_t &io, + block_type_t block_type = block_type_t::top); /// Evaluates a node. /// The node type must be grammar::statement or grammar::job_list. template - end_execution_reason_t eval_node(const parsed_source_ref_t &ps, tnode_t node, - job_lineage_t lineage, - block_type_t block_type = block_type_t::top); + eval_res_t eval_node(const parsed_source_ref_t &ps, tnode_t node, job_lineage_t lineage, + block_type_t block_type = block_type_t::top); /// Evaluate line as a list of parameters, i.e. tokenize it and perform parameter expansion and /// cmdsubst execution on the tokens. Errors are ignored. If a parser is provided, it is used diff --git a/src/reader.cpp b/src/reader.cpp index 62c7d4938..f7eb97ecc 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -874,8 +874,8 @@ void reader_write_title(const wcstring &cmd, parser_t &parser, bool reset_cursor } wcstring_list_t lst; - if (exec_subshell(fish_title_command, parser, lst, false /* ignore exit status */) != -1 && - !lst.empty()) { + (void)exec_subshell(fish_title_command, parser, lst, false /* ignore exit status */); + if (!lst.empty()) { std::fputws(L"\x1B]0;", stdout); for (const auto &i : lst) { std::fputws(i.c_str(), stdout); diff --git a/tests/checks/cmdsub-limit.fish b/tests/checks/cmdsub-limit.fish index 6fa25d233..1f5bca97f 100644 --- a/tests/checks/cmdsub-limit.fish +++ b/tests/checks/cmdsub-limit.fish @@ -29,7 +29,6 @@ set --show b #CHECK: $b: not set in global scope #CHECK: $b: not set in universal scope #CHECKERR: {{.*}}: Too much data emitted by command substitution so it was discarded -#CHECKERR: #CHECKERR: set b (string repeat -n 512 x) #CHECKERR: ^ @@ -43,7 +42,6 @@ set --show c #CHECK: $c[1]: length=0 value=|| #CHECK: $c: not set in universal scope #CHECKERR: {{.*}}: Too much data emitted by command substitution so it was discarded -#CHECKERR: #CHECKERR: set -l x (string repeat -n $argv x) #CHECKERR: ^ #CHECKERR: in function 'subme' with arguments '513' @@ -62,6 +60,5 @@ test $saved_status -eq 122 or echo expected status 122, saw $saved_status >&2 #CHECKERR: {{.*}}: Too much data emitted by command substitution so it was discarded -#CHECKERR: #CHECKERR: echo this will fail (string repeat --max 513 b) to output anything #CHECKERR: ^ diff --git a/tests/checks/eval.fish b/tests/checks/eval.fish index e33050de8..e484f0aed 100644 --- a/tests/checks/eval.fish +++ b/tests/checks/eval.fish @@ -21,14 +21,14 @@ echo $status false eval "(" echo $status -# CHECK: 1 +# CHECK: 123 # CHECKERR: {{.*}}checks/eval.fish (line {{\d+}}): Unexpected end of string, expecting ')' # CHECKERR: ( # CHECKERR: ^ false eval '""' echo $status -# CHECK: 1 +# CHECK: 123 # CHECKERR: {{.*}}checks/eval.fish (line {{\d+}}): The expanded command was empty. # CHECKERR: "" # CHECKERR: ^ diff --git a/tests/checks/invocation.fish b/tests/checks/invocation.fish index 89f41fb04..f10809118 100644 --- a/tests/checks/invocation.fish +++ b/tests/checks/invocation.fish @@ -3,6 +3,10 @@ $fish -c "echo 1.2.3.4." # CHECK: 1.2.3.4. +PATH= $fish -c "command a" 2>/dev/null +echo $status +# CHECK: 127 + PATH= $fish -c "echo (command a)" 2>/dev/null echo $status -# CHECK: 255 +# CHECK: 127 diff --git a/tests/checks/status-value.fish b/tests/checks/status-value.fish new file mode 100644 index 000000000..7ca2ac73d --- /dev/null +++ b/tests/checks/status-value.fish @@ -0,0 +1,26 @@ +# RUN: %fish %s + +# Empty commands should be 123 +set empty_var +$empty_var +echo $status +# CHECK: 123 +# CHECKERR: {{.*}} The expanded command was empty. +# CHECKERR: $empty_var +# CHECKERR: ^ + +# Failed expansions +echo "$abc[" +echo $status +# CHECK: 121 +# CHECKERR: {{.*}} Invalid index value +# CHECKERR: echo "$abc[" +# CHECKERR: ^ + +# Failed wildcards +echo *gibberishgibberishgibberish* +echo $status +# CHECK: 124 +# CHECKERR: {{.*}} No matches for wildcard '*gibberishgibberishgibberish*'. {{.*}} +# CHECKERR: echo *gibberishgibberishgibberish* +# CHECKERR: ^