Highlight the entire variable name, not just the dollar sign. Fixes #1201

fish_tests.cpp

@@ -2709,8 +2709,22 @@ static void test_highlighting(void)
         {NULL, -1}
     };
     
+    const highlight_component_t components11[] =
+    {
+        {L"echo", highlight_spec_command},
+        {L"$foo", highlight_spec_operator},
+        {L"\"", highlight_spec_quote},
+        {L"$bar", highlight_spec_operator},
+        {L"\"", highlight_spec_quote},
+        {L"$baz[", highlight_spec_operator},
+        {L"1 2..3", highlight_spec_param},
+        {L"]", highlight_spec_operator},
+        {NULL, -1}
+    };
 
-    const highlight_component_t *tests[] = {components1, components2, components3, components4, components5, components6, components7, components8, components9, components10};
+
+
+    const highlight_component_t *tests[] = {components1, components2, components3, components4, components5, components6, components7, components8, components9, components10, components11};
     for (size_t which = 0; which < sizeof tests / sizeof *tests; which++)
     {
         const highlight_component_t *components = tests[which];

highlight.cpp

@@ -46,7 +46,7 @@
 
 static void highlight_universal_internal(const wcstring &buff, std::vector<highlight_spec_t> &color, size_t pos);
 
-/** The environment variables used to specify the color of different tokens. This matchest the order in highlight_spec_t */
+/** The environment variables used to specify the color of different tokens. This matches the order in highlight_spec_t */
 static const wchar_t * const highlight_var[] =
 {
     L"fish_color_normal",
@@ -1393,7 +1393,51 @@ void highlight_shell_classic(const wcstring &buff, std::vector<highlight_spec_t>
     }
 }
 
-/* This function is a disaster badly in need of refactoring. */
+/* Highlights the variable starting with 'in', setting colors within the 'colors' array. Returns the number of characters consumed. */
+static size_t color_variable(const wchar_t *in, size_t in_len, std::vector<highlight_spec_t>::iterator colors)
+{
+    assert(in_len > 0);
+    assert(in[0] == L'$');
+    
+    // Handle an initial run of $s.
+    size_t idx = 0;
+    while (in[idx] == '$')
+    {
+        // Our color depends on the next char
+        wchar_t next = in[idx + 1];
+        if (next == L'$' || wcsvarchr(next))
+        {
+            colors[idx] = highlight_spec_operator;
+        }
+        else
+        {
+            colors[idx] = highlight_spec_error;
+        }
+        idx++;
+    }
+    
+    // Handle a sequence of variable characters
+    while (wcsvarchr(in[idx]))
+    {
+        colors[idx++] = highlight_spec_operator;
+    }
+    
+    // Handle a slice. Note that we currently don't do any validation of the slice's contents, e.g. $foo[blah] will not show an error even though it's invalid.
+    if (in[idx] == L'[')
+    {
+        wchar_t *slice_begin = NULL, *slice_end = NULL;
+        if (1 == parse_util_locate_slice(in, &slice_begin, &slice_end, false))
+        {
+            size_t slice_begin_idx = slice_begin - in, slice_end_idx = slice_end - in;
+            assert(slice_end_idx > slice_begin_idx);
+            colors[slice_begin_idx] = highlight_spec_operator;
+            colors[slice_end_idx] = highlight_spec_operator;
+        }
+    }
+    return idx;
+}
+
+/* This function is a disaster badly in need of refactoring. It colors an argument, without regard to command substitutions. */
 static void color_argument_internal(const wcstring &buffstr, std::vector<highlight_spec_t>::iterator colors)
 {
     const size_t buff_len = buffstr.size();
@@ -1535,14 +1579,7 @@ static void color_argument_internal(const wcstring &buffstr, std::vector<highlig
                         case L'$':
                         {
                             assert(in_pos < buff_len);
-                            int dollar_color = highlight_spec_error;
+                            in_pos += color_variable(buffstr.c_str() + in_pos, buff_len - in_pos, colors + in_pos);
-                            if (in_pos + 1 < buff_len)
-                            {
-                                wchar_t next = buffstr.at(in_pos + 1);
-                                if (next == L'$' || wcsvarchr(next))
-                                    dollar_color = highlight_spec_operator;
-                            }
-                            colors[in_pos] = dollar_color;
                             break;
                         }
 
@@ -1658,14 +1695,7 @@ static void color_argument_internal(const wcstring &buffstr, std::vector<highlig
 
                     case L'$':
                     {
-                        int dollar_color = highlight_spec_error;
+                        in_pos += color_variable(buffstr.c_str() + in_pos, buff_len - in_pos, colors + in_pos);
-                        if (in_pos + 1 < buff_len)
-                        {
-                            wchar_t next = buffstr.at(in_pos + 1);
-                            if (next == L'$' || wcsvarchr(next))
-                                dollar_color = highlight_spec_operator;
-                        }
-                        colors[in_pos] = dollar_color;
                         break;
                     }
 

parse_execution.cpp

@@ -657,7 +657,7 @@ parse_execution_result_t parse_execution_context_t::report_error(const parse_nod
         const parse_error_list_t error_list = parse_error_list_t(1, error);
         parser->get_backtrace(src, error_list, &backtrace_and_desc);
 
-        fprintf(stderr, "%ls", backtrace_and_desc.c_str());
+        fprintf(stderr, "<%ls>", backtrace_and_desc.c_str());
     }
 
     return parse_execution_errored;

parse_util.cpp

@@ -144,12 +144,11 @@ size_t parse_util_get_offset(const wcstring &str, int line, long line_offset)
     }
 
     return off + line_offset2;
-
 }
 
-
+static int parse_util_locate_brackets_of_type(const wchar_t *in, wchar_t **begin, wchar_t **end, bool allow_incomplete, wchar_t open_type, wchar_t close_type)
-int parse_util_locate_cmdsubst(const wchar_t *in, wchar_t **begin, wchar_t **end, bool allow_incomplete)
 {
+    /* open_type is typically ( or [, and close type is the corresponding value */
     wchar_t *pos;
     wchar_t prev=0;
     int syntax_error=0;
@@ -177,7 +176,7 @@ int parse_util_locate_cmdsubst(const wchar_t *in, wchar_t **begin, wchar_t **end
             }
             else
             {
-                if (*pos == '(')
+                if (*pos == open_type)
                 {
                     if ((paran_count == 0)&&(paran_begin==0))
                     {
@@ -186,7 +185,7 @@ int parse_util_locate_cmdsubst(const wchar_t *in, wchar_t **begin, wchar_t **end
                     
                     paran_count++;
                 }
-                else if (*pos == ')')
+                else if (*pos == close_type)
                 {
                     
                     paran_count--;
@@ -235,7 +234,19 @@ int parse_util_locate_cmdsubst(const wchar_t *in, wchar_t **begin, wchar_t **end
     return 1;
 }
 
-int parse_util_locate_cmdsubst_range(const wcstring &str, size_t *inout_cursor_offset, wcstring *out_contents, size_t *out_start, size_t *out_end, bool accept_incomplete)
+
+int parse_util_locate_cmdsubst(const wchar_t *in, wchar_t **begin, wchar_t **end, bool accept_incomplete)
+{
+    return parse_util_locate_brackets_of_type(in, begin, end, accept_incomplete, L'(', L')');
+}
+
+int parse_util_locate_slice(const wchar_t *in, wchar_t **begin, wchar_t **end, bool accept_incomplete)
+{
+    return parse_util_locate_brackets_of_type(in, begin, end, accept_incomplete, L'[', L']');
+}
+
+
+static int parse_util_locate_brackets_range(const wcstring &str, size_t *inout_cursor_offset, wcstring *out_contents, size_t *out_start, size_t *out_end, bool accept_incomplete, wchar_t open_type, wchar_t close_type)
 {
     /* Clear the return values */
     out_contents->clear();
@@ -249,21 +260,21 @@ int parse_util_locate_cmdsubst_range(const wcstring &str, size_t *inout_cursor_o
     /* Defer to the wonky version */
     const wchar_t * const buff = str.c_str();
     const wchar_t * const valid_range_start = buff + *inout_cursor_offset, *valid_range_end = buff + str.size();
-    wchar_t *cmdsub_begin = NULL, *cmdsub_end = NULL;
+    wchar_t *bracket_range_begin = NULL, *bracket_range_end = NULL;
-    int ret = parse_util_locate_cmdsubst(valid_range_start, &cmdsub_begin, &cmdsub_end, accept_incomplete);
+    int ret = parse_util_locate_brackets_of_type(valid_range_start, &bracket_range_begin, &bracket_range_end, accept_incomplete, open_type, close_type);
     if (ret > 0)
     {
         /* The command substitutions must not be NULL and must be in the valid pointer range, and the end must be bigger than the beginning */
-        assert(cmdsub_begin != NULL && cmdsub_begin >= valid_range_start && cmdsub_begin <= valid_range_end);
+        assert(bracket_range_begin != NULL && bracket_range_begin >= valid_range_start && bracket_range_begin <= valid_range_end);
-        assert(cmdsub_end != NULL && cmdsub_end > cmdsub_begin && cmdsub_end >= valid_range_start && cmdsub_end <= valid_range_end);
+        assert(bracket_range_end != NULL && bracket_range_end > bracket_range_begin && bracket_range_end >= valid_range_start && bracket_range_end <= valid_range_end);
 
         /* Assign the substring to the out_contents */
-        const wchar_t *interior_begin = cmdsub_begin + 1;
+        const wchar_t *interior_begin = bracket_range_begin + 1;
-        out_contents->assign(interior_begin, cmdsub_end - interior_begin);
+        out_contents->assign(interior_begin, bracket_range_end - interior_begin);
 
         /* Return the start and end */
-        *out_start = cmdsub_begin - buff;
+        *out_start = bracket_range_begin - buff;
-        *out_end = cmdsub_end - buff;
+        *out_end = bracket_range_end - buff;
 
         /* Update the inout_cursor_offset. Note this may cause it to exceed str.size(), though overflow is not likely */
         *inout_cursor_offset = 1 + *out_end;
@@ -271,6 +282,16 @@ int parse_util_locate_cmdsubst_range(const wcstring &str, size_t *inout_cursor_o
     return ret;
 }
 
+int parse_util_locate_cmdsubst_range(const wcstring &str, size_t *inout_cursor_offset, wcstring *out_contents, size_t *out_start, size_t *out_end, bool accept_incomplete)
+{
+    return parse_util_locate_brackets_range(str, inout_cursor_offset, out_contents, out_start, out_end, accept_incomplete, L'(', L')');
+}
+
+int parse_util_locate_slice_range(const wcstring &str, size_t *inout_cursor_offset, wcstring *out_contents, size_t *out_start, size_t *out_end, bool accept_incomplete)
+{
+    return parse_util_locate_brackets_range(str, inout_cursor_offset, out_contents, out_start, out_end, accept_incomplete, L'[', L']');
+}
+
 void parse_util_cmdsubst_extent(const wchar_t *buff, size_t cursor_pos, const wchar_t **a, const wchar_t **b)
 {
     const wchar_t * const cursor = buff + cursor_pos;

parse_util.h

@@ -28,6 +28,12 @@ int parse_util_locate_cmdsubst(const wchar_t *in,
                                wchar_t **end,
                                bool accept_incomplete);
 
+/** Same as parse_util_locate_cmdsubst, but handles square brackets [ ] */
+int parse_util_locate_slice(const wchar_t *in,
+                            wchar_t **begin,
+                            wchar_t **end,
+                            bool accept_incomplete);
+
 /**
    Alternative API. Iterate over command substitutions.