From 865a4647aecbd7bf0fe55cb26f176a6646d4aed0 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 26 Aug 2018 01:41:45 -0700 Subject: [PATCH] Allow variables in commands Syntax highlighting for these coming in next commit. Fixes #154 --- CHANGELOG.md | 1 + src/expand.cpp | 29 ++++++++++++- src/expand.h | 10 +++++ src/fish_tests.cpp | 15 +++---- src/parse_execution.cpp | 83 +++++++++++++++++++++++++------------- src/parse_execution.h | 7 +++- src/parse_util.cpp | 12 +++--- tests/vars_as_commands.err | 12 +++--- tests/vars_as_commands.in | 16 ++++++-- tests/vars_as_commands.out | 3 ++ 10 files changed, 133 insertions(+), 55 deletions(-) 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::vectorreport_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