Allow variables in commands

Syntax highlighting for these coming in next commit.

Fixes #154

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).

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<completion_t> 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,

src/expand.h

@@ -124,6 +124,16 @@ __warn_unused expand_error_t expand_string(const wcstring &input, std::vector<co
 /// \return Whether expansion succeded
 bool expand_one(wcstring &inout_str, expand_flags_t flags, parse_error_list_t *errors = NULL);
 
+/// Expand a command string like $HOME/bin/cmd into a command and list of arguments.
+/// Return the command and arguments by reference.
+/// If the expansion resulted in no or an empty command, the command will be an empty string. Note
+/// that API does not distinguish between expansion resulting in an empty command (''), and
+/// expansion resulting in no command (e.g. unset variable).
+// \return an expand error.
+expand_error_t expand_to_command_and_args(const wcstring &instr, wcstring *out_cmd,
+                                          wcstring_list_t *out_args,
+                                          parse_error_list_t *errors = NULL);
+
 /// Convert the variable value to a human readable form, i.e. escape things, handle arrays, etc.
 /// Suitable for pretty-printing.
 wcstring expand_escape_variable(const env_var_t &var);

src/fish_tests.cpp

@@ -4520,11 +4520,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;
@@ -4534,9 +4535,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);
         }
     }

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<grammar::plain_statement> 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<grammar::plain_statement> 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<g::arguments_or_redirections_list> args = statement.child<1>();
-            if (!args.try_get_child<g::argument, 0>() && !args.try_get_child<g::redirection, 0>()) {
-                // 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<g::argument, 0>() &&
+                !args.try_get_child<g::redirection, 0>()) {
+                // 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<g::argument>();
         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;

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<grammar::job_list> 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<grammar::plain_statement> 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;
 

src/parse_util.cpp

@@ -1127,14 +1127,12 @@ static bool detect_errors_in_plain_statement(const wcstring &buff_src,
         }
     }
 
-    if (maybe_t<wcstring> mcommand = command_for_plain_statement(pst, buff_src)) {
-        wcstring command = std::move(*mcommand);
+    if (maybe_t<wcstring> 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.

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"
+^

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

tests/vars_as_commands.out

@@ -0,0 +1,3 @@
+basic command as variable
+( not expanded again
+/usr/bin