Further improvements to error reporting with new parser

parse_execution.cpp

@@ -219,7 +219,7 @@ parse_execution_result_t parse_execution_context_t::run_function_statement(const
     
     if (unmatched_wildcard != NULL)
     {
-        append_unmatched_wildcard_error(*unmatched_wildcard);
+        report_unmatched_wildcard_error(*unmatched_wildcard);
         result = parse_execution_errored;
     }
     
@@ -232,7 +232,7 @@ parse_execution_result_t parse_execution_context_t::run_function_statement(const
         
         if (! error_str.empty())
         {
-            this->append_error(header, L"%ls", error_str.c_str());
+            this->report_error(header, L"%ls", error_str.c_str());
             result = parse_execution_errored;
         }
     }
@@ -285,8 +285,13 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(const pars
     const parse_node_t &var_name_node = *get_child(header, 1, parse_token_type_string);
     const wcstring for_var_name = get_source(var_name_node);
     
-    /* Get the contents to iterate over. Here we could do something with unmatched_wildcard. However it seems nicer to not make for loops complain about this, i.e. just iterate over a potentially empty list */
-    wcstring_list_t argument_list = this->determine_arguments(header, NULL);
+    /* Get the contents to iterate over. */
+    const parse_node_t *unmatched_wildcard = NULL;
+    wcstring_list_t argument_list = this->determine_arguments(header, &unmatched_wildcard);
+    if (unmatched_wildcard != NULL)
+    {
+        return report_unmatched_wildcard_error(*unmatched_wildcard);
+    }
     
     parse_execution_result_t ret = parse_execution_success;
     
@@ -303,6 +308,7 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(const pars
         if (should_cancel_execution(fb))
         {
             ret = parse_execution_cancelled;
+            break;
         }
         
         const wcstring &for_variable = fb->variable;
@@ -353,7 +359,7 @@ parse_execution_result_t parse_execution_context_t::run_switch_statement(const p
     {
         case EXPAND_ERROR:
         {
-            result = append_error(switch_value_node,
+            result = report_error(switch_value_node,
                   _(L"Could not expand string '%ls'"),
                   switch_value.c_str());
             break;
@@ -362,7 +368,7 @@ parse_execution_result_t parse_execution_context_t::run_switch_statement(const p
         case EXPAND_WILDCARD_NO_MATCH:
         {
             /* Store the node that failed to expand */
-            append_error(switch_value_node, WILDCARD_ERR_MSG, switch_value.c_str());
+            report_error(switch_value_node, WILDCARD_ERR_MSG, switch_value.c_str());
             ret = parse_execution_errored;
             break;
         }
@@ -376,7 +382,7 @@ parse_execution_result_t parse_execution_context_t::run_switch_statement(const p
     
     if (result == parse_execution_success && switch_values_expanded.size() != 1)
     {
-            result = append_error(switch_value_node,
+            result = report_error(switch_value_node,
                 _(L"switch: Expected exactly one argument, got %lu\n"),
                 switch_values_expanded.size());
     }
@@ -509,8 +515,8 @@ parse_execution_result_t parse_execution_context_t::run_while_statement(const pa
     return ret;
 }
 
-/* Appends an error to the error list. Always returns parse_execution_errored, so you can assign the result to an 'errored' variable */
-parse_execution_result_t parse_execution_context_t::append_error(const parse_node_t &node, const wchar_t *fmt, ...)
+/* Reports an error. Always returns parse_execution_errored, so you can assign the result to an 'errored' variable */
+parse_execution_result_t parse_execution_context_t::report_error(const parse_node_t &node, const wchar_t *fmt, ...)
 {
     parse_error_t error;
     error.source_start = node.source_start;
@@ -522,19 +528,29 @@ parse_execution_result_t parse_execution_context_t::append_error(const parse_nod
     error.text = vformat_string(fmt, va);
     va_end(va);
     
-    //this->errors.push_back(error);
-    
     /* Output the error */
-    fprintf(stderr, "%ls\n", error.describe(this->src).c_str());
+    const wcstring desc = error.describe(this->src);
+    if (! desc.empty())
+    {
+        fprintf(stderr, "%ls\n", desc.c_str());
+    }
     
     return parse_execution_errored;
 }
 
-/* Appends an unmatched wildcard error to the error list, and returns true. */
-parse_execution_result_t parse_execution_context_t::append_unmatched_wildcard_error(const parse_node_t &unmatched_wildcard)
+/* Reoports 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)
 {
     proc_set_last_status(STATUS_UNMATCHED_WILDCARD);
-    return append_error(unmatched_wildcard, WILDCARD_ERR_MSG, get_source(unmatched_wildcard).c_str());
+    /* For reasons I cannot explain, unmatched wildcards are only reported in interactive use. */
+    if (get_is_interactive())
+    {
+        return report_error(unmatched_wildcard, WILDCARD_ERR_MSG, get_source(unmatched_wildcard).c_str());
+    }
+    else
+    {
+        return parse_execution_errored;
+    }
 }
 
 /* Handle the case of command not found */
@@ -565,7 +581,7 @@ void parse_execution_context_t::handle_command_not_found(const wcstring &cmd_str
                 ellipsis_str = L"...";
             
             /* Looks like a command */
-            this->append_error(statement_node,
+            this->report_error(statement_node,
                                _(L"Unknown command '%ls'. Did you mean to run %ls with a modified environment? Try 'env %ls=%ls %ls%ls'. See the help section on the set command by typing 'help set'."),
                                cmd,
                                argument.c_str(),
@@ -576,7 +592,7 @@ void parse_execution_context_t::handle_command_not_found(const wcstring &cmd_str
         }
         else
         {
-            this->append_error(statement_node,
+            this->report_error(statement_node,
                                COMMAND_ASSIGN_ERR_MSG,
                                cmd,
                                name_str.c_str(),
@@ -590,7 +606,7 @@ void parse_execution_context_t::handle_command_not_found(const wcstring &cmd_str
         const wchar_t *val = val_wstr.missing() ? NULL : val_wstr.c_str();
         if (val)
         {
-            this->append_error(statement_node,
+            this->report_error(statement_node,
                                _(L"Variables may not be used as commands. Instead, define a function like 'function %ls; %ls $argv; end' or use the eval builtin instead, like 'eval %ls'. See the help section for the function command by typing 'help function'."),
                                cmd+1,
                                val,
@@ -599,7 +615,7 @@ void parse_execution_context_t::handle_command_not_found(const wcstring &cmd_str
         }
         else
         {
-            this->append_error(statement_node,
+            this->report_error(statement_node,
                                _(L"Variables may not be used as commands. Instead, define a function or use the eval builtin instead, like 'eval %ls'. See the help section for the function command by typing 'help function'."),
                                cmd,
                                cmd);
@@ -607,14 +623,14 @@ void parse_execution_context_t::handle_command_not_found(const wcstring &cmd_str
     }
     else if (wcschr(cmd, L'$'))
     {
-        this->append_error(statement_node,
+        this->report_error(statement_node,
                            _(L"Commands may not contain variables. Use the eval builtin instead, like 'eval %ls'. See the help section for the eval command by typing 'help eval'."),
                            cmd,
                            cmd);
     }
     else if (err_code!=ENOENT)
     {
-        this->append_error(statement_node,
+        this->report_error(statement_node,
                            _(L"The file '%ls' is not executable by this user"),
                            cmd?cmd:L"UNKNOWN");
     }
@@ -631,7 +647,7 @@ void parse_execution_context_t::handle_command_not_found(const wcstring &cmd_str
         event_fire_generic(L"fish_command_not_found", &event_args);
         
         /* Here we want to report an error (so it shows a backtrace), but with no text */
-        this->append_error(statement_node, L"");
+        this->report_error(statement_node, L"");
     }
     
     /* Set the last proc status appropriately */
@@ -657,7 +673,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process(job_t
     bool expanded = expand_one(cmd, EXPAND_SKIP_CMDSUBST | EXPAND_SKIP_VARIABLES);
     if (! expanded)
     {
-        append_error(statement, ILLEGAL_CMD_ERR_MSG, cmd.c_str());
+        report_error(statement, ILLEGAL_CMD_ERR_MSG, cmd.c_str());
         return parse_execution_errored;
     }
     
@@ -718,7 +734,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process(job_t
         if (unmatched_wildcard != NULL)
         {
             job_set_flag(job, JOB_WILDCARD_ERROR, 1);
-            append_unmatched_wildcard_error(*unmatched_wildcard);
+            report_unmatched_wildcard_error(*unmatched_wildcard);
             return parse_execution_errored;
         }
         
@@ -770,7 +786,7 @@ wcstring_list_t parse_execution_context_t::determine_arguments(const parse_node_
         {
             case EXPAND_ERROR:
             {
-                this->append_error(arg_node,
+                this->report_error(arg_node,
                       _(L"Could not expand string '%ls'"),
                       arg_str.c_str());
                 break;
@@ -838,7 +854,7 @@ bool parse_execution_context_t::determine_io_chain(const parse_node_t &statement
         if (! target_expanded || target.empty())
         {
             /* Should improve this error message */
-            errored = append_error(redirect_node,
+            errored = report_error(redirect_node,
                   _(L"Invalid redirection target: %ls"),
                   target.c_str());
         }
@@ -862,7 +878,7 @@ bool parse_execution_context_t::determine_io_chain(const parse_node_t &statement
                     int old_fd = fish_wcstoi(target.c_str(), &end, 10);
                     if (old_fd < 0 || errno || *end)
                     {
-                        errored = append_error(redirect_node,
+                        errored = report_error(redirect_node,
                               _(L"Requested redirection to something that is not a file descriptor %ls"),
                               target.c_str());
                     }

parse_execution.h

@@ -60,9 +60,9 @@ class parse_execution_context_t
     execution_cancellation_reason_t cancellation_reason(const block_t *block) const;
     
     /* Report an error. Always returns true. */
-    parse_execution_result_t append_error(const parse_node_t &node, const wchar_t *fmt, ...);
+    parse_execution_result_t report_error(const parse_node_t &node, const wchar_t *fmt, ...);
     /* Wildcard error helper */
-    parse_execution_result_t append_unmatched_wildcard_error(const parse_node_t &unmatched_wildcard);
+    parse_execution_result_t report_unmatched_wildcard_error(const parse_node_t &unmatched_wildcard);
     
     void handle_command_not_found(const wcstring &cmd, const parse_node_t &statement_node, int err_code);
         

parse_tree.cpp

@@ -1,6 +1,7 @@
 #include "parse_productions.h"
 #include "tokenizer.h"
 #include "fallback.h"
+#include "proc.h"
 #include <vector>
 #include <algorithm>
 
@@ -41,37 +42,47 @@ wcstring parse_error_t::describe(const wcstring &src, bool skip_caret) const
         assert(line_end >= line_start);
         assert(source_start >= line_start);
 
-        // Append the line of text.
-        result.push_back(L'\n');
-        result.append(src, line_start, line_end - line_start);
+        // Don't include the caret and line if we're interactive this is the first line, because then it's obvious
+        bool skip_caret = (get_is_interactive() && source_start == 0);
         
-        // Append the caret line. The input source may include tabs; for that reason we construct a "caret line" that has tabs in corresponding positions
-        wcstring caret_space_line;
-        caret_space_line.reserve(source_start - line_start);
-        for (size_t i=line_start; i < source_start; i++)
+        if (! skip_caret)
         {
-            wchar_t wc = src.at(i);
-            if (wc == L'\t')
+            // Append the line of text.
+            if (! result.empty())
             {
-                caret_space_line.push_back(L'\t');
+                result.push_back(L'\n');
             }
-            else if (wc == L'\n')
+            result.append(src, line_start, line_end - line_start);
+            
+        
+            // Append the caret line. The input source may include tabs; for that reason we construct a "caret line" that has tabs in corresponding positions
+            wcstring caret_space_line;
+            caret_space_line.reserve(source_start - line_start);
+            for (size_t i=line_start; i < source_start; i++)
             {
-                /* It's possible that the source_start points at a newline itself. In that case, pretend it's a space. We only expect this to be at the end of the string. */
-                caret_space_line.push_back(L' ');
-            }
-            else
-            {
-                int width = fish_wcwidth(wc);
-                if (width > 0)
+                wchar_t wc = src.at(i);
+                if (wc == L'\t')
                 {
-                    caret_space_line.append(static_cast<size_t>(width), L' ');
+                    caret_space_line.push_back(L'\t');
+                }
+                else if (wc == L'\n')
+                {
+                    /* It's possible that the source_start points at a newline itself. In that case, pretend it's a space. We only expect this to be at the end of the string. */
+                    caret_space_line.push_back(L' ');
+                }
+                else
+                {
+                    int width = fish_wcwidth(wc);
+                    if (width > 0)
+                    {
+                        caret_space_line.append(static_cast<size_t>(width), L' ');
+                    }
                 }
             }
+            result.push_back(L'\n');
+            result.append(caret_space_line);
+            result.push_back(L'^');
         }
-        result.push_back(L'\n');
-        result.append(caret_space_line);
-        result.push_back(L'^');
     }
     return result;
 }
@@ -249,6 +260,36 @@ wcstring parse_token_t::describe() const
     return result;
 }
 
+/** A string description appropriate for presentation to the user */
+wcstring parse_token_t::user_presentable_description() const
+{
+    if (keyword != parse_keyword_none)
+    {
+        return format_string(L"keyword %ls", keyword_description(keyword).c_str());
+    }
+    
+    switch (type)
+    {
+        /* Hackish. We only support the  */
+        case parse_token_type_string:
+            return L"a string";
+            
+        case parse_token_type_pipe:
+            return L"a pipe";
+            
+        case parse_token_type_redirection:
+            return L"a redirection";
+            
+        case parse_token_type_background:
+            return L"a '&'";
+            
+        case parse_token_type_end:
+            return L"statement terminator";
+            
+        default:
+            return format_string(L"a %ls", this->describe().c_str());
+    }
+}
 
 /* Convert from tokenizer_t's token type to a parse_token_t type */
 static inline parse_token_type_t parse_token_type_from_tokenizer_token(enum token_type tokenizer_token_type)
@@ -382,6 +423,25 @@ struct parse_stack_element_t
         }
         return result;
     }
+    
+    /* Returns a name that we can show to the user, e.g. "a command" */
+    wcstring user_presentable_description(void) const
+    {
+        if (keyword != parse_keyword_none)
+        {
+            return format_string(L"keyword %ls", keyword_description(keyword).c_str());
+        }
+        
+        switch (type)
+        {
+            /* Hackish, the only one we support now */
+            case symbol_statement:
+                return L"a command";
+                
+            default:
+                return format_string(L"a %ls", this->describe().c_str());
+        }
+    }
 };
 
 /* The parser itself, private implementation of class parse_t. This is a hand-coded table-driven LL parser. Most hand-coded LL parsers are recursive descent, but recursive descent parsers are difficult to "pause", unlike table-driven parsers. */
@@ -407,6 +467,7 @@ class parse_ll_t
 
     void parse_error(const wchar_t *expected, parse_token_t token);
     void parse_error(parse_token_t token, parse_error_code_t code, const wchar_t *format, ...);
+    void parse_error_failed_production(struct parse_stack_element_t &elem, parse_token_t token);
     void parse_error_unbalancing_token(parse_token_t token);
     void append_error_callout(wcstring &error_message, parse_token_t token);
 
@@ -671,6 +732,12 @@ void parse_ll_t::parse_error_unbalancing_token(parse_token_t token)
     }
 }
 
+// This is a 'generic' parse error when we can't match the top of the stack element
+void parse_ll_t::parse_error_failed_production(struct parse_stack_element_t &stack_elem, parse_token_t token)
+{
+    this->parse_error(token, parse_error_generic, L"Expected %ls, but instead found %ls", stack_elem.user_presentable_description().c_str(), token.user_presentable_description().c_str());
+}
+
 void parse_ll_t::report_tokenizer_error(parse_token_t token, const wchar_t *tok_error)
 {
     assert(tok_error != NULL);
@@ -861,7 +928,7 @@ void parse_ll_t::accept_tokens(parse_token_t token1, parse_token_t token2)
         {
             if (should_generate_error_messages)
             {
-                this->parse_error(token1, parse_error_generic, L"Unable to produce a '%ls' from input '%ls'", stack_elem.describe().c_str(), token1.describe().c_str());
+                parse_error_failed_production(stack_elem, token1);
             }
             else
             {

parse_tree.h

@@ -51,6 +51,7 @@ struct parse_token_t
     size_t source_length;
 
     wcstring describe() const;
+    wcstring user_presentable_description() const;
 };