Don't stop job execution on wildcard errors

Wildcard errors are only reported interactively, and they're also not
really errors. Commands with multiple wildcards would in fact continue
executing if at least one wildcard matched, which is quite surprising.
But they would report an error if there is only one wildcard in the
arguments list and the wildcard has no match, even if there are other
remaining arguments.

Given this inconsistency, and given that sh does not stop execution if a
wildcard fails to match, it seems better to allow execution to continue.
This is better from a scripting perspective anyway, as it means
constructs like `set -l paths foo/*.txt` will actually create the
variable (with an empty value) instead of skipping the `set`
altogether and perhaps causing subsequent code to read or modify a
global or universal variable.

1 files changed, 11 insertions, 51 deletions

diff --git a/parse_execution.cpp b/parse_execution.cpp
index fb9c9718..4bdcf7b6 100644
--- a/parse_execution.cpp
+++ b/parse_execution.cpp
@@ -369,16 +369,8 @@ parse_execution_result_t parse_execution_context_t::run_function_statement(const
     assert(block_end_command.type == symbol_end_command);
     
     /* Get arguments */
-    const parse_node_t *unmatched_wildcard = NULL;
     wcstring_list_t argument_list;
-    parse_execution_result_t result = this->determine_arguments(header, &argument_list, &unmatched_wildcard);
-
-    /* Handle unmatched wildcards */
-    if (result == parse_execution_success && unmatched_wildcard != NULL)
-    {
-        report_unmatched_wildcard_error(*unmatched_wildcard);
-        result = parse_execution_errored;
-    }
+    parse_execution_result_t result = this->determine_arguments(header, &argument_list);
 
     if (result == parse_execution_success)
     {
@@ -469,18 +461,12 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(const pars
     }
 
     /* Get the contents to iterate over. */
-    const parse_node_t *unmatched_wildcard = NULL;
     wcstring_list_t argument_sequence;
-    parse_execution_result_t ret = this->determine_arguments(header, &argument_sequence, &unmatched_wildcard);
+    parse_execution_result_t ret = this->determine_arguments(header, &argument_sequence);
     if (ret != parse_execution_success)
     {
         return ret;
     }
-    
-    if (unmatched_wildcard != NULL)
-    {
-        return report_unmatched_wildcard_error(*unmatched_wildcard);
-    }
 
     for_block_t *fb = new for_block_t();
     parser->push_block(fb);
@@ -601,9 +587,9 @@ parse_execution_result_t parse_execution_context_t::run_switch_statement(const p
             /* Pull out the argument list */
             const parse_node_t &arg_list = *get_child(*case_item, 1, symbol_argument_list);
 
-            /* Expand arguments. We explicitly ignore unmatched_wildcard. That is, a case item list may have a wildcard that fails to expand to anything. We also report case errors, but don't stop execution; i.e. a case item that contains an unexpandable process will report and then fail to match. */
+            /* Expand arguments. A case item list may have a wildcard that fails to expand to anything. We also report case errors, but don't stop execution; i.e. a case item that contains an unexpandable process will report and then fail to match. */
             wcstring_list_t case_args;
-            parse_execution_result_t case_result = this->determine_arguments(arg_list, &case_args, NULL);
+            parse_execution_result_t case_result = this->determine_arguments(arg_list, &case_args);
             if (case_result == parse_execution_success)
             {
                 for (size_t i=0; i < case_args.size(); i++)
@@ -961,20 +947,13 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process(job_t
     else
     {
         /* Form the list of arguments. The command is the first argument. TODO: count hack, where we treat 'count --help' as different from 'count $foo' that expands to 'count --help'. fish 1.x never successfully did this, but it tried to! */
-        const parse_node_t *unmatched_wildcard = NULL;
-        parse_execution_result_t arg_result = this->determine_arguments(statement, &argument_list, &unmatched_wildcard);
+        parse_execution_result_t arg_result = this->determine_arguments(statement, &argument_list);
         if (arg_result != parse_execution_success)
         {
             return arg_result;
         }
         argument_list.insert(argument_list.begin(), cmd);
 
-        /* If we were not able to expand any wildcards, here is the first one that failed */
-        if (unmatched_wildcard != NULL)
-        {
-            return report_unmatched_wildcard_error(*unmatched_wildcard);
-        }
-
         /* The set of IO redirections that we construct for the process */
         if (! this->determine_io_chain(statement, &process_io_chain))
         {
@@ -994,18 +973,12 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process(job_t
     return parse_execution_success;
 }
 
-/* Determine the list of arguments, expanding stuff. Reports any errors caused by expansion. If we have a wildcard and none could be expanded, return the unexpandable wildcard node by reference. */
-parse_execution_result_t parse_execution_context_t::determine_arguments(const parse_node_t &parent, wcstring_list_t *out_arguments, const parse_node_t **out_unmatched_wildcard_node)
+/* Determine the list of arguments, expanding stuff. Reports any errors caused by expansion. If we have a wildcard that could not be expanded, report the error and continue. */
+parse_execution_result_t parse_execution_context_t::determine_arguments(const parse_node_t &parent, wcstring_list_t *out_arguments)
 {
-    /* Whether we failed to match any wildcards, and succeeded in matching any wildcards */
-    bool unmatched_wildcard = false, matched_wildcard = false;
-    
     /* The ultimate result */
     enum parse_execution_result_t result = parse_execution_success;
 
-    /* First node that failed to expand as a wildcard (if any) */
-    const parse_node_t *unmatched_wildcard_node = NULL;
-
     /* Get all argument nodes underneath the statement. We guess we'll have that many arguments (but may have more or fewer, if there are wildcards involved) */
     const parse_node_tree_t::parse_node_list_t argument_nodes = tree.find_nodes(parent, symbol_argument);
     out_arguments->reserve(out_arguments->size() + argument_nodes.size());
@@ -1033,21 +1006,14 @@ parse_execution_result_t parse_execution_context_t::determine_arguments(const pa
 
             case EXPAND_WILDCARD_NO_MATCH:
             {
-                /* Store the node that failed to expand */
-                unmatched_wildcard = true;
-                if (! unmatched_wildcard_node)
-                {
-                    unmatched_wildcard_node = &arg_node;
-                }
+                // report the unmatched wildcard error but don't stop processing.
+                // this will only print an error in interactive mode, though it does set the
+                // process status (similar to a command substitution failing)
+                report_unmatched_wildcard_error(arg_node);
                 break;
             }
 
             case EXPAND_WILDCARD_MATCH:
-            {
-                matched_wildcard = true;
-                break;
-            }
-
             case EXPAND_OK:
             {
                 break;
@@ -1061,12 +1027,6 @@ parse_execution_result_t parse_execution_context_t::determine_arguments(const pa
         }
     }
 
-    /* Return if we had a wildcard problem */
-    if (out_unmatched_wildcard_node != NULL && unmatched_wildcard && ! matched_wildcard)
-    {
-        *out_unmatched_wildcard_node = unmatched_wildcard_node;
-    }
-
     return result;
 }