Improve error reporting for unclosed blocks

4 files changed, 119 insertions, 8 deletions

diff --git a/parse_tree.cpp b/parse_tree.cpp
index ea0b0abd..2a0a14a3 100644
--- a/parse_tree.cpp
+++ b/parse_tree.cpp
@@ -276,7 +276,7 @@ static wcstring token_type_user_presentable_description(parse_token_type_t type,
 
     switch (type)
     {
-            /* Hackish. We only support the following types. */
+        /* Hackish. We only support the following types. */
         case symbol_statement:
             return L"a command";
 
@@ -298,11 +298,41 @@ static wcstring token_type_user_presentable_description(parse_token_type_t type,
         case parse_token_type_end:
             return L"end of the statement";
 
+        case parse_token_type_terminate:
+            return L"end of the input";
+
         default:
             return format_string(L"a %ls", token_type_description(type).c_str());
     }
 }
 
+static wcstring block_type_user_presentable_description(parse_token_type_t type)
+{
+    switch (type)
+    {
+        case symbol_for_header:
+            return L"for loop";
+
+        case symbol_while_header:
+            return L"while loop";
+
+        case symbol_function_header:
+            return L"function definition";
+
+        case symbol_begin_header:
+            return L"begin";
+
+        case symbol_if_statement:
+            return L"if statement";
+
+        case symbol_switch_statement:
+            return L"switch statement";
+
+        default:
+            return token_type_description(type);
+    }
+}
+
 /** Returns a string description of the given parse node */
 wcstring parse_node_t::describe(void) const
 {
@@ -506,11 +536,15 @@ class parse_ll_t
     /* The symbol stack can contain terminal types or symbols. Symbols go on to do productions, but terminal types are just matched against input tokens. */
     bool top_node_handle_terminal_types(parse_token_t token);
 
-    void parse_error(const wchar_t *expected, parse_token_t token);
+    void parse_error_unexpected_token(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_at_location(size_t location, 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);
 
+    /* Reports an error for an unclosed block, e.g. 'begin;'. Returns true on success, false on failure (e.g. it is not an unclosed block) */
+    bool report_error_for_unclosed_block();
+
     void dump_stack(void) const;
 
     // Get the node corresponding to the top element of the stack
@@ -558,7 +592,7 @@ class parse_ll_t
         assert(child_start_big < NODE_OFFSET_INVALID);
         node_offset_t child_start = static_cast<node_offset_t>(child_start_big);
 
-        // To avoid constructing multiple nodes, we push_back a single one that we modify
+        // To avoid constructing multiple nodes, we make a single one that we modify
         parse_node_t representative_child(token_type_invalid);
         representative_child.parent = parent_node_idx;
 
@@ -769,6 +803,26 @@ void parse_ll_t::parse_error(parse_token_t token, parse_error_code_t code, const
     }
 }
 
+void parse_ll_t::parse_error_at_location(size_t source_location, parse_error_code_t code, const wchar_t *fmt, ...)
+{
+    this->fatal_errored = true;
+    if (this->should_generate_error_messages)
+    {
+        //this->dump_stack();
+        parse_error_t err;
+
+        va_list va;
+        va_start(va, fmt);
+        err.text = vformat_string(fmt, va);
+        err.code = code;
+        va_end(va);
+
+        err.source_start = source_location;
+        err.source_length = 0;
+        this->errors.push_back(err);
+    }
+}
+
 // Unbalancing token. This includes 'else' or 'case' or 'end' outside of the appropriate block
 // This essentially duplicates some logic from resolving the production for symbol_statement_list - yuck
 void parse_ll_t::parse_error_unbalancing_token(parse_token_t token)
@@ -844,7 +898,7 @@ void parse_ll_t::parse_error_failed_production(struct parse_stack_element_t &sta
         if (! done)
         {
             const wcstring expected = stack_elem.user_presentable_description();
-            this->parse_error(expected.c_str(), token);
+            this->parse_error_unexpected_token(expected.c_str(), token);
         }
     }
 }
@@ -876,7 +930,7 @@ void parse_ll_t::report_tokenizer_error(parse_token_t token, int tok_err_code, c
     this->parse_error(token, parse_error_code, L"%ls", tok_error);
 }
 
-void parse_ll_t::parse_error(const wchar_t *expected, parse_token_t token)
+void parse_ll_t::parse_error_unexpected_token(const wchar_t *expected, parse_token_t token)
 {
     fatal_errored = true;
     if (this->should_generate_error_messages)
@@ -920,6 +974,42 @@ static bool type_is_terminal_type(parse_token_type_t type)
     }
 }
 
+bool parse_ll_t::report_error_for_unclosed_block()
+{
+    bool reported_error = false;
+    /* Unclosed block, for example, 'while true ; '. We want to show the block node that opened it. */
+    const parse_node_t &top_node = this->node_for_top_symbol();
+
+    /* Hacktastic. We want to point at the source location of the block, but our block doesn't have a source range yet - only the terminal tokens do. So get the block statement corresponding to this end command. In general this block may be of a variety of types: if_statement, switch_statement, etc., each with different node structures. But keep descending the first child and eventually you hit a keyword: begin, if, etc. That's the keyword we care about. */
+    const parse_node_t *end_command = this->nodes.get_parent(top_node, symbol_end_command);
+    const parse_node_t *block_node = end_command ? this->nodes.get_parent(*end_command) : NULL;
+
+    if (block_node && block_node->type == symbol_block_statement)
+    {
+        // Get the header
+        block_node = this->nodes.get_child(*block_node, 0, symbol_block_header);
+        block_node = this->nodes.get_child(*block_node, 0); // specific statement
+    }
+    if (block_node != NULL)
+    {
+        // block_node is now an if_statement, switch_statement, for_header, while_header, function_header, or begin_header
+        // Hackish: descend down the first node until we reach the bottom. This will be a keyword node like SWITCH, which will have the source range. Ordinarily the source range would be known by the parent node too, but we haven't completed parsing yet, so we haven't yet propagated source ranges
+        const parse_node_t *cursor = block_node;
+        while (cursor->child_count > 0)
+        {
+            cursor = this->nodes.get_child(*cursor, 0);
+            assert(cursor != NULL);
+        }
+        if (cursor->source_start != NODE_OFFSET_INVALID)
+        {
+            const wcstring node_desc = block_type_user_presentable_description(block_node->type);
+            this->parse_error_at_location(cursor->source_start, parse_error_generic, L"Missing end to balance this %ls", node_desc.c_str());
+            reported_error = true;
+        }
+    }
+    return reported_error;
+}
+
 bool parse_ll_t::top_node_handle_terminal_types(parse_token_t token)
 {
     PARSE_ASSERT(! symbol_stack.empty());
@@ -994,10 +1084,14 @@ bool parse_ll_t::top_node_handle_terminal_types(parse_token_t token)
                     }
                 }
             }
+            else if (stack_top.keyword == parse_keyword_end && token.type == parse_token_type_terminate && this->report_error_for_unclosed_block())
+            {
+                // Handled by report_error_for_unclosed_block
+            }
             else
             {
                 const wcstring expected = stack_top.user_presentable_description();
-                this->parse_error(expected.c_str(), token);
+                this->parse_error_unexpected_token(expected.c_str(), token);
             }
         }
 
@@ -1231,7 +1325,7 @@ const parse_node_t *parse_node_tree_t::get_child(const parse_node_t &parent, nod
 {
     const parse_node_t *result = NULL;
 
-    /* We may get nodes with no children if we had an imcomplete parse. Don't consider than an error */
+    /* We may get nodes with no children if we had an incomplete parse. Don't consider than an error */
     if (parent.child_count > 0)
     {
         PARSE_ASSERT(which < parent.child_count);
diff --git a/parse_util.cpp b/parse_util.cpp
index e491177b..e89883c9 100644
--- a/parse_util.cpp
+++ b/parse_util.cpp
@@ -1271,7 +1271,7 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars
 
     // Parse the input string into a parse tree
     // Some errors are detected here
-    bool parsed = parse_tree_from_string(buff_src, parse_flag_leave_unterminated, &node_tree, &parse_errors);
+    bool parsed = parse_tree_from_string(buff_src, allow_incomplete ? parse_flag_leave_unterminated : parse_flag_none, &node_tree, &parse_errors);
 
     if (allow_incomplete)
     {
diff --git a/tests/test9.err b/tests/test9.err
index f439e8db..db67e030 100644
--- a/tests/test9.err
+++ b/tests/test9.err
@@ -1 +1,7 @@
 emit: expected event name
+Missing end to balance this begin
+Missing end to balance this while loop
+Missing end to balance this for loop
+Missing end to balance this switch statement
+Missing end to balance this function definition
+Missing end to balance this if statement
diff --git a/tests/test9.in b/tests/test9.in
index 95a75843..35df1737 100644
--- a/tests/test9.in
+++ b/tests/test9.in
@@ -97,4 +97,15 @@ else
     echo 'psub file was deleted'
 end
 
+# Test support for unbalanced blocks
+function try_unbalanced_block
+    ../fish -c "echo $argv | source " 2>&1 | grep "Missing end" 1>&2
+end
+try_unbalanced_block 'begin'
+try_unbalanced_block 'while true'
+try_unbalanced_block 'for x in 1 2 3'
+try_unbalanced_block 'switch abc'
+try_unbalanced_block 'function anything'
+try_unbalanced_block 'if false'
+
 false