Command highlighting works

highlight.cpp

@@ -1800,23 +1800,9 @@ static void color_children(const parse_node_tree_t &tree, const parse_node_t &pa
     }
 }
 
-/* Color a possibly decorated command */
+/* Determine if a command is valid */
-static void color_command(const wcstring &src, const parse_node_tree_t &tree, const parse_node_t &cmd_node, enum parse_statement_decoration_t decoration, std::vector<int> &color_array, const wcstring &working_directory, const env_vars_snapshot_t &vars)
+static bool command_is_valid(const wcstring &cmd, enum parse_statement_decoration_t decoration, const wcstring &working_directory, const env_vars_snapshot_t &vars)
 {
-    if (! cmd_node.has_source())
-        return;
-    
-    /* Get the source of the command */
-    wcstring cmd(src, cmd_node.source_start, cmd_node.source_length);
-    
-    /* Try expanding it. If we cannot, it's an error. */
-    bool expanded = expand_one(cmd, EXPAND_SKIP_CMDSUBST | EXPAND_SKIP_VARIABLES | EXPAND_SKIP_JOBS);
-    if (! expanded || has_expand_reserved(cmd))
-    {
-        color_node(cmd_node, HIGHLIGHT_ERROR, color_array);
-        return;
-    }
-    
     /* Determine which types we check, based on the decoration */
     bool builtin_ok = true, function_ok = true, abbreviation_ok = true, command_ok = true, implicit_cd_ok = true;
     if (decoration == parse_statement_decoration_command)
@@ -1859,9 +1845,8 @@ static void color_command(const wcstring &src, const parse_node_tree_t &tree, co
     if (! is_valid && implicit_cd_ok)
         is_valid = path_can_be_implicit_cd(cmd, NULL, working_directory.c_str(), vars);
     
-    /* Color the node */
+    /* Return what we got */
-    int color = is_valid ? HIGHLIGHT_COMMAND : HIGHLIGHT_ERROR;
+    return is_valid;
-    color_node(cmd_node, color, color_array);
 }
 
 void highlight_shell_magic(const wcstring &buff, std::vector<int> &color, size_t pos, wcstring_list_t *error, const env_vars_snapshot_t &vars)
@@ -1874,6 +1859,7 @@ void highlight_shell_magic(const wcstring &buff, std::vector<int> &color, size_t
     if (length == 0)
         return;
 
+    /* Start out at zero */
     std::fill(color.begin(), color.end(), 0);
 
     /* Do something sucky and get the current working directory on this background thread. This should really be passed in. */
@@ -1925,25 +1911,45 @@ void highlight_shell_magic(const wcstring &buff, std::vector<int> &color, size_t
             break;
 
             case symbol_redirection:
+            {
                 color_children(parse_tree, node, parse_token_type_string, HIGHLIGHT_REDIRECTION, color);
-                break;
+            }
+            break;
 
             case parse_token_type_background:
             case parse_token_type_end:
+            {
                 color_node(node, HIGHLIGHT_END, color);
-                break;
+            }
+            break;
 
             case symbol_plain_statement:
             {
-                // Color the command
+                // Get the decoration from the parent
-                const parse_node_t *cmd = parse_tree.get_child(node, 0, parse_token_type_string);
+                enum parse_statement_decoration_t decoration = parse_statement_decoration_none;
-                if (cmd != NULL)
+                const parse_node_t *decorated_statement = parse_tree.get_parent(node, symbol_decorated_statement);
+                if (decorated_statement != NULL)
                 {
-                    enum parse_statement_decoration_t decoration = static_cast<enum parse_statement_decoration_t>(node.tag);
+                    decoration = static_cast<enum parse_statement_decoration_t>(decorated_statement->production_idx);
-                    color_command(buff, parse_tree, *cmd, decoration, color, working_directory, vars);
                 }
 
-                // Color arguments
+                /* Color the command */
+                const parse_node_t *cmd_node = parse_tree.get_child(node, 0, parse_token_type_string);
+                if (cmd_node != NULL && cmd_node->has_source())
+                {
+                    bool is_valid_cmd = false;
+                    wcstring cmd(buff, cmd_node->source_start, cmd_node->source_length);
+                    
+                    /* Try expanding it. If we cannot, it's an error. */
+                    bool expanded = expand_one(cmd, EXPAND_SKIP_CMDSUBST | EXPAND_SKIP_VARIABLES | EXPAND_SKIP_JOBS);
+                    if (expanded && ! has_expand_reserved(cmd))
+                    {
+                        is_valid_cmd = command_is_valid(cmd, decoration, working_directory, vars);
+                    }
+                    color_node(*cmd_node, is_valid_cmd ? HIGHLIGHT_COMMAND : HIGHLIGHT_ERROR, color);
+                }
+
+                /* Color arguments */
                 const parse_node_t *arguments = parse_tree.get_child(node, 1, symbol_arguments_or_redirections_list);
                 if (arguments != NULL)
                 {
@@ -1978,8 +1984,13 @@ void highlight_shell_magic(const wcstring &buff, std::vector<int> &color, size_t
         for (parse_node_tree_t::const_iterator iter = parse_tree.begin(); iter != parse_tree.end(); ++iter)
         {
             const parse_node_t &node = *iter;
-            /* See if this node contains the cursor */
+
-            if (node.type == symbol_argument && node.source_contains_location(pos))
+            /* Must be an argument with source */
+            if (node.type != symbol_argument || ! node.has_source())
+                continue;
+            
+            /* See if this node contains the cursor. We check <= source_length so that, when backspacing (and the cursor is just beyond the last token), we may still underline it */
+            if (pos >= node.source_start && pos - node.source_start <= node.source_length)
             {
                 /* See if this is a valid path */
                 if (node_is_potential_path(buff, node, working_directory))

parse_exec.cpp

@@ -259,7 +259,7 @@ class parse_exec_t
         for (;;)
         {
             const parse_node_t &node = parse_tree.at(idx);
-            PARSE_ASSERT(node.type == symbol_argument_list || node.type == symbol_argument_list_nonempty);
+            PARSE_ASSERT(node.type == symbol_argument_list);
             if (node.type == symbol_argument_list)
             {
                 // argument list, may be empty

parse_productions.cpp

@@ -234,16 +234,10 @@ PRODUCTIONS(case_item) =
 };
 RESOLVE_ONLY(case_item)
 
-PRODUCTIONS(argument_list_nonempty) =
-{
-    {parse_token_type_string, symbol_argument_list}
-};
-RESOLVE_ONLY(argument_list_nonempty)
-
 PRODUCTIONS(argument_list) =
 {
     {},
-    {symbol_argument_list_nonempty}
+    {symbol_argument, symbol_argument_list}
 };
 RESOLVE(argument_list)
 {
@@ -451,7 +445,6 @@ const production_t *parse_productions::production_for_token(parse_token_type_t n
             TEST(decorated_statement)
             TEST(case_item_list)
             TEST(case_item)
-            TEST(argument_list_nonempty)
             TEST(argument_list)
             TEST(block_header)
             TEST(for_header)

parse_tree.cpp

@@ -88,8 +88,6 @@ wcstring token_type_description(parse_token_type_t type)
         case symbol_case_item:
             return L"case_item";
 
-        case symbol_argument_list_nonempty:
-            return L"argument_list_nonempty";
         case symbol_argument_list:
             return L"argument_list";
 
@@ -369,24 +367,6 @@ class parse_ll_t
         return symbol_stack.back().type;
     }
 
-    void top_node_set_tag(uint32_t tag)
-    {
-        this->node_for_top_symbol().tag = tag;
-    }
-
-    inline void add_child_to_node(size_t parent_node_idx, parse_stack_element_t *tok)
-    {
-        PARSE_ASSERT(tok->type != token_type_invalid);
-        tok->node_idx = nodes.size();
-        nodes.push_back(parse_node_t(tok->type));
-        nodes.at(parent_node_idx).child_count += 1;
-    }
-
-    inline void symbol_stack_pop()
-    {
-        symbol_stack.pop_back();
-    }
-
     // Pop from the top of the symbol stack, then push the given production, updating node counts. Note that production_t has type "pointer to array" so some care is required.
     inline void symbol_stack_pop_push_production(const production_t *production)
     {
@@ -409,6 +389,8 @@ class parse_ll_t
             if (! count) fprintf(stderr, "\t<empty>\n");
         }
         
+        // Get the parent index. But we can't get the parent parse node yet, since it may be made invalid by adding children
+        const size_t parent_node_idx = symbol_stack.back().node_idx;
 
         // Add the children. Confusingly, we want our nodes to be in forwards order (last token last, so dumps look nice), but the symbols should be reverse order (last token first, so it's lowest on the stack)
         const size_t child_start = nodes.size();
@@ -425,13 +407,14 @@ class parse_ll_t
             {
                 // Generate the parse node. Note that this push_back may invalidate node.
                 parse_token_type_t child_type = production_element_type(elem);
-                nodes.push_back(parse_node_t(child_type));
+                parse_node_t child = parse_node_t(child_type);
+                child.parent = parent_node_idx;
+                nodes.push_back(child);
                 child_count++;
             }
         }
 
         // Update the parent
-        const size_t parent_node_idx = symbol_stack.back().node_idx;
         parse_node_t &parent_node = nodes.at(parent_node_idx);
 
         // Should have no children yet
@@ -815,39 +798,6 @@ static parse_keyword_t keyword_for_token(token_type tok, const wchar_t *tok_txt)
     return result;
 }
 
-// Set type-specific tags for nodes
-// This is not in parse_ll_t because it knows about different node types
-static void tag_nodes(const wcstring &src, parse_node_tree_t *tree)
-{
-    size_t count = tree->size();
-    for (size_t i=0; i < count; i++)
-    {
-        const parse_node_t &node = tree->at(i);
-        switch (node.type)
-        {
-            case symbol_decorated_statement:
-            {
-                // Set a tag on the plain statement to indicate the decoration type
-                // The decoration types matches the production
-                bool is_decorated = (node.production_idx > 0);
-                
-                // Get the plain statement and set the tag equal to the production index we used
-                // This is an enum parse_statement_decoration_t
-                node_offset_t statement_idx = (is_decorated ? 1 : 0);
-                parse_node_t *plain_statement = tree->get_child(node, statement_idx, symbol_plain_statement);
-                if (plain_statement != NULL)
-                {
-                    plain_statement->tag = static_cast<enum parse_statement_decoration_t>(node.production_idx);
-                }
-            }
-            break;
-            
-            default:
-            break;
-        }
-    }
-}
-
 bool parse_t::parse(const wcstring &str, parse_tree_flags_t parse_flags, parse_node_tree_t *output, parse_error_list_t *errors, bool log_it)
 {
     tok_flags_t tok_options = TOK_SQUASH_ERRORS;
@@ -904,9 +854,6 @@ bool parse_t::parse(const wcstring &str, parse_tree_flags_t parse_flags, parse_n
     // Acquire the output from the parser
     this->parser->acquire_output(output, errors);
     
-    // Set node tags
-    tag_nodes(str, output);
-    
     // Indicate if we had a fatal error
     return ! this->parser->has_fatal_error();
 }
@@ -938,28 +885,38 @@ void parse_t::clear()
 const parse_node_t *parse_node_tree_t::get_child(const parse_node_t &parent, node_offset_t which, parse_token_type_t expected_type) const
 {
     const parse_node_t *result = NULL;
-    PARSE_ASSERT(which < parent.child_count);
-    node_offset_t child_offset = parent.child_offset(which);
-    if (child_offset < this->size())
-    {
-        result = &this->at(child_offset);
-    }
     
-    // If we are given an expected type, then the node must be null or that type
+    /* We may get nodes with no children if we had an imcomplete parse. Don't consider than an error */
-    if (result != NULL)
+    if (parent.child_count > 0)
     {
-        assert(expected_type == token_type_invalid || expected_type == result->type);
+        PARSE_ASSERT(which < parent.child_count);
+        node_offset_t child_offset = parent.child_offset(which);
+        if (child_offset < this->size())
+        {
+            result = &this->at(child_offset);
+                        
+            /* If we are given an expected type, then the node must be null or that type */
+            assert(expected_type == token_type_invalid || expected_type == result->type);
+        }
     }
 
     return result;
 }
 
-/* Hackish non-const version of get_child */
+const parse_node_t *parse_node_tree_t::get_parent(const parse_node_t &node, parse_token_type_t expected_type) const
-parse_node_t *parse_node_tree_t::get_child(const parse_node_t &parent, node_offset_t which, parse_token_type_t expected_type)
 {
-    const parse_node_tree_t *const_this = this;
+    const parse_node_t *result = NULL;
-    const parse_node_t *result = const_this->get_child(parent, which, expected_type);
+    if (node.parent != NODE_OFFSET_INVALID)
-    return const_cast<parse_node_t *>(result);
+    {
+        PARSE_ASSERT(node.parent < this->size());
+        const parse_node_t &parent = this->at(node.parent);
+        if (expected_type == token_type_invalid || expected_type == parent.type)
+        {
+            // The type matches (or no type was requested)
+            result = &parent;
+        }
+    }
+    return result;
 }
 
 static void find_nodes_recursive(const parse_node_tree_t &tree, const parse_node_t &parent, parse_token_type_t type, parse_node_tree_t::parse_node_list_t *result)

parse_tree.h

@@ -67,7 +67,6 @@ enum parse_token_type_t
     symbol_arguments_or_redirections_list,
     symbol_argument_or_redirection,
 
-    symbol_argument_list_nonempty,
     symbol_argument_list,
 
     symbol_argument,
@@ -169,6 +168,9 @@ public:
     /* Length of our range in the source code */
     size_t source_length;
     
+    /* Parent */
+    node_offset_t parent;
+
     /* Children */
     node_offset_t child_start;
     node_offset_t child_count;
@@ -183,7 +185,7 @@ public:
     wcstring describe(void) const;
 
     /* Constructor */
-    explicit parse_node_t(parse_token_type_t ty) : type(ty), source_start(-1), source_length(0), child_start(0), child_count(0), tag(0)
+    explicit parse_node_t(parse_token_type_t ty) : type(ty), source_start(-1), source_length(0), parent(NODE_OFFSET_INVALID), child_start(0), child_count(0), tag(0)
     {
     }
 
@@ -198,12 +200,6 @@ public:
     {
         return source_start != (size_t)(-1);
     }
-    
-    /* Indicate if this node's source range contains a given location. The funny math makes this modulo-overflow safe, though overflow is not expected. */
-    bool source_contains_location(size_t where) const
-    {
-        return this->has_source() && where >= source_start && where - source_start < source_length;
-    }
 };
 
 /* The parse tree itself */
@@ -213,13 +209,19 @@ public:
 
     /* Get the node corresponding to a child of the given node, or NULL if there is no such child. If expected_type is provided, assert that the node has that type. */
     const parse_node_t *get_child(const parse_node_t &parent, node_offset_t which, parse_token_type_t expected_type = token_type_invalid) const;    
-    parse_node_t *get_child(const parse_node_t &parent, node_offset_t which, parse_token_type_t expected_type = token_type_invalid);
+    
+    /* Get the node corresponding to the parent of the given node, or NULL if there is no such child. If expected_type is provided, only returns the parent if it is of that type. Note the asymmetry: get_child asserts since the children are known, but get_parent does not, since the parent may not be known. */
+    const parse_node_t *get_parent(const parse_node_t &node, parse_token_type_t expected_type = token_type_invalid) const;
+
 
     /* Find all the nodes of a given type underneath a given node */
     typedef std::vector<const parse_node_t *> parse_node_list_t;
     parse_node_list_t find_nodes(const parse_node_t &parent, parse_token_type_t type) const;
 };
 
+
+/* Node type specific data, stored in the tag field */
+
 /* Statement decorations, stored in the tag of plain_statement. This matches the order of productions in decorated_statement */
 enum parse_statement_decoration_t
 {
@@ -228,6 +230,16 @@ enum parse_statement_decoration_t
     parse_statement_decoration_builtin
 };
 
+/* Argument flags as a bitmask, stored in the tag of argument */
+enum parse_argument_flags_t
+{
+    /* Indicates that this or a prior argument was --, so this should not be treated as an option */
+    parse_argument_no_options = 1 << 0,
+    
+    /* Indicates that the argument is for a cd command */
+    parse_argument_is_for_cd = 1 << 1
+};
+
 /* Fish grammar:
 
 # A job_list is a list of jobs, separated by semicolons or newlines
@@ -260,9 +272,6 @@ enum parse_statement_decoration_t
                     case_item case_item_list
     case_item = CASE argument_list STATEMENT_TERMINATOR job_list
 
-    argument_list_nonempty = <TOK_STRING> argument_list
-    argument_list = <empty> | argument_list_nonempty
-
     block_statement = block_header <TOK_END> job_list <END> arguments_or_redirections_list
     block_header = for_header | while_header | function_header | begin_header
     for_header = FOR var_name IN arguments_or_redirections_list
@@ -280,6 +289,9 @@ enum parse_statement_decoration_t
     decorated_statement = plain_statement | COMMAND plain_statement | BUILTIN plain_statement
     plain_statement = <TOK_STRING> arguments_or_redirections_list optional_background
 
+    argument_list = <empty> | argument argument_list
+
+
     arguments_or_redirections_list = <empty> |
                                      argument_or_redirection arguments_or_redirections_list
     argument_or_redirection = argument | redirection