diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ec2dce9f..f9aae67c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ fish 3.0 is a major release which brings with it both improvements in functional - The `-d` option to `functions` to set the description of an existing function now works; before 3.0 it was documented but unimplemented. Note that the long form `--description` continues to work. (#5105) - `test` and `[` now support floating point values in numeric comparisons. - Autosuggestions try to avoid arguments that are already present in the command line. +- Variables may be used inside commands (#154). ## Other significant changes - Command substitution output is now limited to 10 MB by default (#3822). diff --git a/src/expand.cpp b/src/expand.cpp index 16edc703b..46b9ea07a 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -1065,12 +1065,39 @@ bool expand_one(wcstring &string, expand_flags_t flags, parse_error_list_t *erro if (expand_string(string, &completions, flags | EXPAND_NO_DESCRIPTIONS, errors) && completions.size() == 1) { - string = completions.at(0).completion; + string = std::move(completions.at(0).completion); return true; } return false; } +expand_error_t expand_to_command_and_args(const wcstring &instr, wcstring *out_cmd, + wcstring_list_t *out_args, parse_error_list_t *errors) { + // Fast path. + if (expand_is_clean(instr)) { + *out_cmd = instr; + return EXPAND_OK; + } + + std::vector completions; + expand_error_t expand_err = + expand_string(instr, &completions, + EXPAND_SKIP_CMDSUBST | EXPAND_NO_DESCRIPTIONS | EXPAND_SKIP_JOBS, errors); + if (expand_err == EXPAND_OK || expand_err == EXPAND_WILDCARD_MATCH) { + // The first completion is the command, any remaning are arguments. + bool first = true; + for (auto &comp : completions) { + if (first) { + if (out_cmd) *out_cmd = std::move(comp.completion); + first = false; + } else { + if (out_args) out_args->push_back(std::move(comp.completion)); + } + } + } + return expand_err; +} + // https://github.com/fish-shell/fish-shell/issues/367 // // With them the Seed of Wisdom did I sow, diff --git a/src/expand.h b/src/expand.h index 890a6407b..6cf87645f 100644 --- a/src/expand.h +++ b/src/expand.h @@ -124,6 +124,16 @@ __warn_unused expand_error_t expand_string(const wcstring &input, std::vector; + std::vector highlight_tests; - const highlight_component_t components2[] = { + highlight_tests.push_back( + {{L"echo", highlight_spec_command}, + {L"test/fish_highlight_test/foo", highlight_spec_param | highlight_modifier_valid_path}, + {L"&", highlight_spec_statement_terminator}}); + + highlight_tests.push_back({ {L"command", highlight_spec_command}, {L"echo", highlight_spec_command}, {L"abc", highlight_spec_param}, {L"test/fish_highlight_test/foo", highlight_spec_param | highlight_modifier_valid_path}, {L"&", highlight_spec_statement_terminator}, - {NULL, -1}}; + }); - const highlight_component_t components3[] = { + highlight_tests.push_back({ {L"if command ls", highlight_spec_command}, {L"; ", highlight_spec_statement_terminator}, {L"echo", highlight_spec_command}, @@ -3917,39 +3923,40 @@ static void test_highlighting() { {L"/bin/definitely_not_a_command", highlight_spec_error}, {L"; ", highlight_spec_statement_terminator}, {L"end", highlight_spec_command}, - {NULL, -1}}; + }); // Verify that cd shows errors for non-directories. - const highlight_component_t components4[] = { + highlight_tests.push_back({ {L"cd", highlight_spec_command}, {L"test/fish_highlight_test", highlight_spec_param | highlight_modifier_valid_path}, - {NULL, -1}}; + }); - const highlight_component_t components5[] = { + highlight_tests.push_back({ {L"cd", highlight_spec_command}, {L"test/fish_highlight_test/foo", highlight_spec_error}, - {NULL, -1}}; + }); - const highlight_component_t components6[] = { + highlight_tests.push_back({ {L"cd", highlight_spec_command}, {L"--help", highlight_spec_param}, {L"-h", highlight_spec_param}, {L"definitely_not_a_directory", highlight_spec_error}, - {NULL, -1}}; + }); // Command substitutions. - const highlight_component_t components7[] = {{L"echo", highlight_spec_command}, - {L"param1", highlight_spec_param}, - {L"(", highlight_spec_operator}, - {L"ls", highlight_spec_command}, - {L"param2", highlight_spec_param}, - {L")", highlight_spec_operator}, - {L"|", highlight_spec_statement_terminator}, - {L"cat", highlight_spec_command}, - {NULL, -1}}; + highlight_tests.push_back({ + {L"echo", highlight_spec_command}, + {L"param1", highlight_spec_param}, + {L"(", highlight_spec_operator}, + {L"ls", highlight_spec_command}, + {L"param2", highlight_spec_param}, + {L")", highlight_spec_operator}, + {L"|", highlight_spec_statement_terminator}, + {L"cat", highlight_spec_command}, + }); // Redirections substitutions. - const highlight_component_t components8[] = { + highlight_tests.push_back({ {L"echo", highlight_spec_command}, {L"param1", highlight_spec_param}, @@ -3993,36 +4000,41 @@ static void test_highlighting() { // Just another param. {L"param2", highlight_spec_param}, - {NULL, -1}}; + }); - const highlight_component_t components9[] = {{L"end", highlight_spec_error}, - {L";", highlight_spec_statement_terminator}, - {L"if", highlight_spec_command}, - {L"end", highlight_spec_error}, - {NULL, -1}}; + highlight_tests.push_back({ + {L"end", highlight_spec_error}, + {L";", highlight_spec_statement_terminator}, + {L"if", highlight_spec_command}, + {L"end", highlight_spec_error}, + }); - const highlight_component_t components10[] = { - {L"echo", highlight_spec_command}, {L"'single_quote", highlight_spec_error}, {NULL, -1}}; + highlight_tests.push_back({ + {L"echo", highlight_spec_command}, + {L"'single_quote", highlight_spec_error}, + }); - const highlight_component_t components11[] = {{L"echo", highlight_spec_command}, - {L"$foo", highlight_spec_operator}, - {L"\"", highlight_spec_quote}, - {L"$bar", highlight_spec_operator}, - {L"\"", highlight_spec_quote}, - {L"$baz[", highlight_spec_operator}, - {L"1 2..3", highlight_spec_param}, - {L"]", highlight_spec_operator}, - {NULL, -1}}; + highlight_tests.push_back({ + {L"echo", highlight_spec_command}, + {L"$foo", highlight_spec_operator}, + {L"\"", highlight_spec_quote}, + {L"$bar", highlight_spec_operator}, + {L"\"", highlight_spec_quote}, + {L"$baz[", highlight_spec_operator}, + {L"1 2..3", highlight_spec_param}, + {L"]", highlight_spec_operator}, + }); - const highlight_component_t components12[] = {{L"for", highlight_spec_command}, - {L"i", highlight_spec_param}, - {L"in", highlight_spec_command}, - {L"1 2 3", highlight_spec_param}, - {L";", highlight_spec_statement_terminator}, - {L"end", highlight_spec_command}, - {NULL, -1}}; + highlight_tests.push_back({ + {L"for", highlight_spec_command}, + {L"i", highlight_spec_param}, + {L"in", highlight_spec_command}, + {L"1 2 3", highlight_spec_param}, + {L";", highlight_spec_statement_terminator}, + {L"end", highlight_spec_command}, + }); - const highlight_component_t components13[] = { + highlight_tests.push_back({ {L"echo", highlight_spec_command}, {L"$$foo[", highlight_spec_operator}, {L"1", highlight_spec_param}, @@ -4030,55 +4042,63 @@ static void test_highlighting() { {L"2", highlight_spec_param}, {L"]", highlight_spec_operator}, {L"[3]", highlight_spec_param}, // two dollar signs, so last one is not an expansion - {NULL, -1}}; + }); - const highlight_component_t components14[] = {{L"cat", highlight_spec_command}, - {L"/dev/null", highlight_spec_param}, - {L"|", highlight_spec_statement_terminator}, - {L"less", highlight_spec_command}, - {L"2>", highlight_spec_redirection}, - {NULL, -1}}; + highlight_tests.push_back({ + {L"cat", highlight_spec_command}, + {L"/dev/null", highlight_spec_param}, + {L"|", highlight_spec_statement_terminator}, + {L"less", highlight_spec_command}, + {L"2>", highlight_spec_redirection}, + }); - const highlight_component_t components15[] = {{L"if", highlight_spec_command}, - {L"true", highlight_spec_command}, - {L"&&", highlight_spec_operator}, - {L"false", highlight_spec_command}, - {L";", highlight_spec_statement_terminator}, - {L"or", highlight_spec_operator}, - {L"false", highlight_spec_command}, - {L"||", highlight_spec_operator}, - {L"true", highlight_spec_command}, - {L";", highlight_spec_statement_terminator}, - {L"and", highlight_spec_operator}, - {L"not", highlight_spec_operator}, - {L"!", highlight_spec_operator}, - {L"true", highlight_spec_command}, - {L";", highlight_spec_statement_terminator}, - {L"end", highlight_spec_command}, - {NULL, -1}}; + highlight_tests.push_back({ + {L"if", highlight_spec_command}, + {L"true", highlight_spec_command}, + {L"&&", highlight_spec_operator}, + {L"false", highlight_spec_command}, + {L";", highlight_spec_statement_terminator}, + {L"or", highlight_spec_operator}, + {L"false", highlight_spec_command}, + {L"||", highlight_spec_operator}, + {L"true", highlight_spec_command}, + {L";", highlight_spec_statement_terminator}, + {L"and", highlight_spec_operator}, + {L"not", highlight_spec_operator}, + {L"!", highlight_spec_operator}, + {L"true", highlight_spec_command}, + {L";", highlight_spec_statement_terminator}, + {L"end", highlight_spec_command}, + }); - const highlight_component_t *tests[] = {components1, components2, components3, components4, - components5, components6, components7, components8, - components9, components10, components11, components12, - components13, components14, components15}; - for (size_t which = 0; which < sizeof tests / sizeof *tests; which++) { - const highlight_component_t *components = tests[which]; - // Count how many we have. - size_t component_count = 0; - while (components[component_count].txt != NULL) { - component_count++; - } + // Verify variables and wildcards in commands using /bin/cat. + env_set(L"VARIABLE_IN_COMMAND", ENV_LOCAL, {L"a"}); + env_set(L"VARIABLE_IN_COMMAND2", ENV_LOCAL, {L"at"}); + highlight_tests.push_back( + {{L"/bin/ca", highlight_spec_command, ns}, {L"*", highlight_spec_operator, ns}}); + highlight_tests.push_back({{L"/bin/c", highlight_spec_command, ns}, + {L"{$VARIABLE_IN_COMMAND}", highlight_spec_operator, ns}, + {L"*", highlight_spec_operator, ns}}); + + highlight_tests.push_back({{L"/bin/c", highlight_spec_command, ns}, + {L"{$VARIABLE_IN_COMMAND}", highlight_spec_operator, ns}, + {L"*", highlight_spec_operator, ns}}); + + highlight_tests.push_back({{L"/bin/c", highlight_spec_command, ns}, + {L"$VARIABLE_IN_COMMAND2", highlight_spec_operator, ns}}); + + for (const highlight_component_list_t &components : highlight_tests) { // Generate the text. wcstring text; std::vector expected_colors; - for (size_t i = 0; i < component_count; i++) { - if (i > 0) { + for (const highlight_component_t &comp : components) { + if (!text.empty() && !comp.nospace) { text.push_back(L' '); expected_colors.push_back(0); } - text.append(components[i].txt); - expected_colors.resize(text.size(), components[i].color); + text.append(comp.txt); + expected_colors.resize(text.size(), comp.color); } do_test(expected_colors.size() == text.size()); @@ -4096,13 +4116,14 @@ static void test_highlighting() { if (expected_colors.at(i) != colors.at(i)) { const wcstring spaces(i, L' '); - err(L"Wrong color in test %lu at index %lu in text (expected %#x, actual " + err(L"Wrong color in test at index %lu in text (expected %#x, actual " L"%#x):\n%ls\n%ls^", - which + 1, i, expected_colors.at(i), colors.at(i), text.c_str(), - spaces.c_str()); + i, expected_colors.at(i), colors.at(i), text.c_str(), spaces.c_str()); } } } + env_remove(L"VARIABLE_IN_COMMAND", ENV_DEFAULT); + env_remove(L"VARIABLE_IN_COMMAND2", ENV_DEFAULT); } static void test_wcstring_tok() { @@ -4523,11 +4544,12 @@ static void test_illegal_command_exit_code() { }; const command_result_tuple_t tests[] = { - {L"echo -n", STATUS_CMD_OK}, {L"pwd", STATUS_CMD_OK}, - // a `)` without a matching `(` is now a tokenizer error, and cannot be executed even as an illegal command - // {L")", STATUS_ILLEGAL_CMD}, {L") ", STATUS_ILLEGAL_CMD}, {L") ", STATUS_ILLEGAL_CMD} - {L"*", STATUS_ILLEGAL_CMD}, {L"**", STATUS_ILLEGAL_CMD}, - {L"?", STATUS_ILLEGAL_CMD}, {L"abc?def", STATUS_ILLEGAL_CMD}, + {L"echo -n", STATUS_CMD_OK}, + {L"pwd", STATUS_CMD_OK}, + {L"UNMATCHABLE_WILDCARD*", STATUS_UNMATCHED_WILDCARD}, + {L"UNMATCHABLE_WILDCARD**", STATUS_UNMATCHED_WILDCARD}, + {L"?", STATUS_UNMATCHED_WILDCARD}, + {L"abc?def", STATUS_UNMATCHED_WILDCARD}, }; int res = 0; @@ -4537,9 +4559,9 @@ static void test_illegal_command_exit_code() { for (const auto &test : tests) { res = parser.eval(test.txt, empty_ios, TOP); - int exit_status = res ? STATUS_CMD_UNKNOWN : proc_get_last_status(); + int exit_status = proc_get_last_status(); if (exit_status != test.result) { - err(L"command '%ls': expected exit code %d , got %d", test.txt, test.result, + err(L"command '%ls': expected exit code %d, got %d", test.txt, test.result, exit_status); } } diff --git a/src/highlight.cpp b/src/highlight.cpp index abbc3df50..7d98b896e 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -245,12 +245,9 @@ static bool plain_statement_get_expanded_command(const wcstring &src, wcstring *out_cmd) { // Get the command. Try expanding it. If we cannot, it's an error. maybe_t cmd = command_for_plain_statement(stmt, src); - if (cmd && expand_one(*cmd, EXPAND_SKIP_CMDSUBST | EXPAND_SKIP_VARIABLES | EXPAND_SKIP_JOBS)) { - // Success, return the expanded string by reference. - *out_cmd = std::move(*cmd); - return true; - } - return false; + if (!cmd) return false; + expand_error_t err = expand_to_command_and_args(*cmd, out_cmd, nullptr); + return err == EXPAND_OK || err == EXPAND_WILDCARD_MATCH; } rgb_color_t highlight_get_color(highlight_spec_t highlight, bool is_background) { @@ -437,12 +434,15 @@ static size_t color_variable(const wchar_t *in, size_t in_len, return idx; } -/// This function is a disaster badly in need of refactoring. It colors an argument, without regard -/// to command substitutions. -static void color_argument_internal(const wcstring &buffstr, - std::vector::iterator colors) { +/// This function is a disaster badly in need of refactoring. It colors an argument or command, +/// without regard to command substitutions. +static void color_string_internal(const wcstring &buffstr, highlight_spec_t base_color, + std::vector::iterator colors) { + // Clarify what we expect. + assert((base_color == highlight_spec_param || base_color == highlight_spec_command) && + "Unexpected base color"); const size_t buff_len = buffstr.size(); - std::fill(colors, colors + buff_len, (highlight_spec_t)highlight_spec_param); + std::fill(colors, colors + buff_len, base_color); enum { e_unquoted, e_single_quoted, e_double_quoted } mode = e_unquoted; int bracket_count = 0; @@ -616,7 +616,7 @@ static void color_argument_internal(const wcstring &buffstr, case e_double_quoted: { // Slices are colored in advance, past `in_pos`, and we don't want to overwrite // that. - if (colors[in_pos] == highlight_spec_param) { + if (colors[in_pos] == base_color) { colors[in_pos] = highlight_spec_quote; } switch (c) { @@ -671,6 +671,8 @@ class highlighter_t { color_array_t color_array; // The parse tree of the buff. parse_node_tree_t parse_tree; + // Color a command. + void color_command(tnode_t node); // Color an argument. void color_argument(tnode_t node); // Color a redirection. @@ -720,6 +722,18 @@ void highlighter_t::color_node(const parse_node_t &node, highlight_spec_t color) color); } +void highlighter_t::color_command(tnode_t node) { + auto source_range = node.source_range(); + if (!source_range) return; + + const wcstring cmd_str = node.get_source(this->buff); + + // Get an iterator to the colors associated with the argument. + const size_t arg_start = source_range->start; + const color_array_t::iterator colors = color_array.begin() + arg_start; + color_string_internal(cmd_str, highlight_spec_command, colors); +} + // node does not necessarily have type symbol_argument here. void highlighter_t::color_argument(tnode_t node) { auto source_range = node.source_range(); @@ -732,7 +746,7 @@ void highlighter_t::color_argument(tnode_t node) { const color_array_t::iterator arg_colors = color_array.begin() + arg_start; // Color this argument without concern for command substitutions. - color_argument_internal(arg_str, arg_colors); + color_string_internal(arg_str, highlight_spec_param, arg_colors); // Now do command substitutions. size_t cmdsub_cursor = 0, cmdsub_start = 0, cmdsub_end = 0; @@ -1094,16 +1108,20 @@ const highlighter_t::color_array_t &highlighter_t::highlight() { // We cannot check if the command is invalid, so just assume it's valid. is_valid_cmd = true; } else { + wcstring expanded_cmd; // Check to see if the command is valid. // Try expanding it. If we cannot, it's an error. - bool expanded = expand_one( - *cmd, EXPAND_SKIP_CMDSUBST | EXPAND_SKIP_VARIABLES | EXPAND_SKIP_JOBS); - if (expanded && !has_expand_reserved(*cmd)) { - is_valid_cmd = command_is_valid(*cmd, decoration, working_directory, vars); + bool expanded = plain_statement_get_expanded_command(buff, stmt, &expanded_cmd); + if (expanded && !has_expand_reserved(expanded_cmd)) { + is_valid_cmd = + command_is_valid(expanded_cmd, decoration, working_directory, vars); } } - this->color_node(*cmd_node, - is_valid_cmd ? highlight_spec_command : highlight_spec_error); + if (!is_valid_cmd) { + this->color_node(*cmd_node, highlight_spec_error); + } else { + this->color_command(cmd_node); + } break; } // Only work on root lists, so that we don't re-color child lists. diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index b0ca7fa3d..48e70f271 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -613,7 +613,7 @@ parse_execution_result_t parse_execution_context_t::report_errors( /// Reports an unmatched wildcard error and returns parse_execution_errored. parse_execution_result_t parse_execution_context_t::report_unmatched_wildcard_error( - const parse_node_t &unmatched_wildcard) { + const parse_node_t &unmatched_wildcard) const { proc_set_last_status(STATUS_UNMATCHED_WILDCARD); report_error(unmatched_wildcard, WILDCARD_ERR_MSG, get_source(unmatched_wildcard).c_str()); return parse_execution_errored; @@ -670,13 +670,6 @@ parse_execution_result_t parse_execution_context_t::handle_command_not_found( this->report_error(statement, ERROR_BAD_COMMAND_ASSIGN_ERR_MSG, name_str.c_str(), assigned_val.c_str()); } - } else if (wcschr(cmd, L'$') || wcschr(cmd, VARIABLE_EXPAND_SINGLE) || - wcschr(cmd, VARIABLE_EXPAND)) { - const wchar_t *msg = - _(L"Variables may not be used as commands. In fish, " - L"please define a function or use 'eval %ls'."); - wcstring eval_cmd = reconstruct_orig_str(cmd_str); - this->report_error(statement, msg, eval_cmd.c_str()); } else if (err_code != ENOENT) { this->report_error(statement, _(L"The file '%ls' is not executable by this user"), cmd); } else { @@ -707,6 +700,36 @@ parse_execution_result_t parse_execution_context_t::handle_command_not_found( return parse_execution_errored; } +parse_execution_result_t parse_execution_context_t::expand_command( + tnode_t statement, wcstring *out_cmd, + wcstring_list_t *out_args) const { + // Here we're expanding a command, for example $HOME/bin/stuff or $randomthing. The first + // completion becomes the command itself, everything after becomes arguments. Command + // substitutions are not supported. + parse_error_list_t errors; + + // Get the unexpanded command string. We expect to always get it here. + wcstring unexp_cmd = *command_for_plain_statement(statement, pstree->src); + wcstring cmd; + wcstring_list_t args; + + // Expand the string to produce completions, and report errors. + expand_error_t expand_err = expand_to_command_and_args(unexp_cmd, out_cmd, out_args, &errors); + if (expand_err == EXPAND_ERROR) { + proc_set_last_status(STATUS_ILLEGAL_CMD); + return report_errors(errors); + } else if (expand_err == EXPAND_WILDCARD_NO_MATCH) { + return report_unmatched_wildcard_error(statement); + } + assert(expand_err == EXPAND_OK || expand_err == EXPAND_WILDCARD_MATCH); + + // 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 parse_execution_success; +} + /// Creates a 'normal' (non-block) process. parse_execution_result_t parse_execution_context_t::populate_plain_process( job_t *job, process_t *proc, tnode_t statement) { @@ -716,16 +739,14 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( // We may decide that a command should be an implicit cd. bool use_implicit_cd = false; - // Get the command. We expect to always get it here. - wcstring cmd = *command_for_plain_statement(statement, pstree->src); - - // Expand it as a command. Return an error on failure. - bool expanded = expand_one(cmd, EXPAND_SKIP_CMDSUBST | EXPAND_SKIP_VARIABLES, NULL); - if (!expanded) { - report_error(statement, ILLEGAL_CMD_ERR_MSG, cmd.c_str()); - proc_set_last_status(STATUS_ILLEGAL_CMD); - return parse_execution_errored; + // Get the command and any arguments due to expanding the command. + wcstring cmd; + wcstring_list_t args_from_cmd_expansion; + auto ret = expand_command(statement, &cmd, &args_from_cmd_expansion); + if (ret != parse_execution_success) { + return ret; } + assert(!cmd.empty() && "expand_command should not produce an empty command"); // Determine the process type. enum process_type_t process_type = process_type_for_command(statement, cmd); @@ -776,9 +797,9 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( if (!has_command && get_decoration(statement) == parse_statement_decoration_none) { // Implicit cd requires an empty argument and redirection list. tnode_t args = statement.child<1>(); - if (!args.try_get_child() && !args.try_get_child()) { - // Ok, no arguments or redirections; check to see if the first argument is a - // directory. + if (args_from_cmd_expansion.empty() && !args.try_get_child() && + !args.try_get_child()) { + // Ok, no arguments or redirections; check to see if the command is a directory. wcstring implicit_cd_path; use_implicit_cd = path_can_be_implicit_cd(cmd, &implicit_cd_path); } @@ -790,27 +811,31 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( } } - // The argument list and set of IO redirections that we will construct for the process. + // Produce the full argument list and the set of IO redirections. + wcstring_list_t cmd_args; io_chain_t process_io_chain; - wcstring_list_t argument_list; if (use_implicit_cd) { - /* Implicit cd is simple */ - argument_list.push_back(L"cd"); - argument_list.push_back(cmd); + // Implicit cd is simple. + cmd_args = {L"cd", cmd}; path_to_external_command.clear(); // If we have defined a wrapper around cd, use it, otherwise use the cd builtin. process_type = function_exists(L"cd") ? INTERNAL_FUNCTION : INTERNAL_BUILTIN; } else { + // Not implicit cd. const globspec_t glob_behavior = (cmd == L"set" || cmd == L"count") ? nullglob : failglob; - // Form the list of arguments. The command is the first argument. + // Form the list of arguments. The command is the first argument, followed by any arguments + // from expanding the command, followed by the argument nodes themselves. E.g. if the + // command is '$gco foo' and $gco is git checkout. + cmd_args.push_back(cmd); + cmd_args.insert(cmd_args.end(), args_from_cmd_expansion.begin(), + args_from_cmd_expansion.end()); argument_node_list_t arg_nodes = statement.descendants(); parse_execution_result_t arg_result = - this->expand_arguments_from_nodes(arg_nodes, &argument_list, glob_behavior); + this->expand_arguments_from_nodes(arg_nodes, &cmd_args, glob_behavior); if (arg_result != parse_execution_success) { return arg_result; } - argument_list.insert(argument_list.begin(), cmd); // The set of IO redirections that we construct for the process. if (!this->determine_io_chain(statement.child<1>(), &process_io_chain)) { @@ -823,7 +848,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( // Populate the process. proc->type = process_type; - proc->set_argv(argument_list); + proc->set_argv(cmd_args); proc->set_io_chain(process_io_chain); proc->actual_cmd = path_to_external_command; return parse_execution_success; diff --git a/src/parse_execution.h b/src/parse_execution.h index 01a77b3ce..edd25169d 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -59,7 +59,7 @@ class parse_execution_context_t { // Wildcard error helper. parse_execution_result_t report_unmatched_wildcard_error( - const parse_node_t &unmatched_wildcard); + const parse_node_t &unmatched_wildcard) const; /// Command not found support. parse_execution_result_t handle_command_not_found(const wcstring &cmd, @@ -72,6 +72,11 @@ class parse_execution_context_t { tnode_t job_list, wcstring *out_func_name) const; bool is_function_context() const; + // Expand a command which may contain variables, producing an expand command and possibly + // arguments. Prints an error message on error. + parse_execution_result_t expand_command(tnode_t statement, + wcstring *out_cmd, wcstring_list_t *out_args) const; + /// Return whether we should skip a job with the given bool statement type. bool should_skip(parse_bool_statement_type_t type) const; diff --git a/src/parse_util.cpp b/src/parse_util.cpp index eb60f25e5..8a49686bd 100644 --- a/src/parse_util.cpp +++ b/src/parse_util.cpp @@ -1127,14 +1127,12 @@ static bool detect_errors_in_plain_statement(const wcstring &buff_src, } } - if (maybe_t mcommand = command_for_plain_statement(pst, buff_src)) { - wcstring command = std::move(*mcommand); + if (maybe_t unexp_command = command_for_plain_statement(pst, buff_src)) { + wcstring command; // Check that we can expand the command. - if (!expand_one(command, EXPAND_SKIP_CMDSUBST | EXPAND_SKIP_VARIABLES | EXPAND_SKIP_JOBS, - NULL)) { - // TODO: leverage the resulting errors. - errored = append_syntax_error(parse_errors, source_start, ILLEGAL_CMD_ERR_MSG, - command.c_str()); + if (expand_to_command_and_args(*unexp_command, &command, nullptr, parse_errors) == + EXPAND_ERROR) { + errored = true; } // Check that pipes are sound. diff --git a/tests/vars_as_commands.err b/tests/vars_as_commands.err index f86c062c4..c1eefbf52 100644 --- a/tests/vars_as_commands.err +++ b/tests/vars_as_commands.err @@ -1,6 +1,6 @@ -fish: Variables may not be used as commands. In fish, please define a function or use 'eval $test'. -exec $test - ^ -fish: Variables may not be used as commands. In fish, please define a function or use 'eval "$test"'. -exec "$test" - ^ +fish: The expanded command was empty. +$EMPTY_VARIABLE +^ +fish: The expanded command was empty. +"$EMPTY_VARIABLE" +^ diff --git a/tests/vars_as_commands.in b/tests/vars_as_commands.in index 8e42abee2..93d299454 100644 --- a/tests/vars_as_commands.in +++ b/tests/vars_as_commands.in @@ -1,8 +1,16 @@ # Test that using variables as command names work correctly. -# Both of these should generate errors about using variables as command names. -# Verify that the expected errors are written to stderr. -exec $test -exec "$test" +$EMPTY_VARIABLE +"$EMPTY_VARIABLE" + +set CMD1 echo basic command as variable +$CMD1 + +set CMD2 echo '(' not expanded again +$CMD2 + +# Test implicit cd +set CMD3 /usr/bin +$CMD3 && echo $PWD exit 0 diff --git a/tests/vars_as_commands.out b/tests/vars_as_commands.out index e69de29bb..6f59b82a7 100644 --- a/tests/vars_as_commands.out +++ b/tests/vars_as_commands.out @@ -0,0 +1,3 @@ +basic command as variable +( not expanded again +/usr/bin