Make parse_util_locate_cmdsubst return the innermost command substitution instead of the outermost.

Fixes https://github.com/fish-shell/fish-shell/issues/913

6 files changed, 73 insertions, 80 deletions

diff --git a/expand.cpp b/expand.cpp
index 6dfa9e44..e25109fc 100644
--- a/expand.cpp
+++ b/expand.cpp
@@ -1336,10 +1336,7 @@ static int expand_cmdsubst(parser_t &parser, const wcstring &input, std::vector<
     const wchar_t * const in = input.c_str();
 
     int parse_ret;
-    switch (parse_ret = parse_util_locate_cmdsubst(in,
-                        &paran_begin,
-                        &paran_end,
-                        0))
+    switch (parse_ret = parse_util_locate_cmdsubst(in, &paran_begin, &paran_end, false))
     {
         case -1:
             parser.error(SYNTAX_ERROR,
@@ -1628,10 +1625,7 @@ int expand_string(const wcstring &input, std::vector<completion_t> &output, expa
     {
         wchar_t *begin, *end;
 
-        if (parse_util_locate_cmdsubst(input.c_str(),
-                                       &begin,
-                                       &end,
-                                       1) != 0)
+        if (parse_util_locate_cmdsubst(input.c_str(), &begin, &end, true) != 0)
         {
             parser.error(CMDSUBST_ERROR, -1, L"Command substitutions not allowed");
             return EXPAND_ERROR;
diff --git a/fish_tests.cpp b/fish_tests.cpp
index 2572e8d6..fad9b7ec 100644
--- a/fish_tests.cpp
+++ b/fish_tests.cpp
@@ -60,6 +60,7 @@
 #include "postfork.h"
 #include "signal.h"
 #include "highlight.h"
+#include "parse_util.h"
 
 /**
    The number of tests to run
@@ -527,6 +528,30 @@ static void test_parser()
     }
 }
 
+static void test_utils()
+{
+    say(L"Testing utils");
+    const wchar_t *a = L"echo (echo (echo hi";
+    
+    const wchar_t *begin = NULL, *end = NULL;
+    parse_util_cmdsubst_extent(a, 0, &begin, &end);
+    if (begin != a || end - begin != wcslen(begin)) err(L"parse_util_cmdsubst_extent failed on line %ld", (long)__LINE__);
+    parse_util_cmdsubst_extent(a, 1, &begin, &end);
+    if (begin != a || end - begin != wcslen(begin)) err(L"parse_util_cmdsubst_extent failed on line %ld", (long)__LINE__);
+    parse_util_cmdsubst_extent(a, 2, &begin, &end);
+    if (begin != a || end - begin != wcslen(begin)) err(L"parse_util_cmdsubst_extent failed on line %ld", (long)__LINE__);
+    parse_util_cmdsubst_extent(a, 3, &begin, &end);
+    if (begin != a || end - begin != wcslen(begin)) err(L"parse_util_cmdsubst_extent failed on line %ld", (long)__LINE__);
+    
+    parse_util_cmdsubst_extent(a, 8, &begin, &end);
+    if (begin != a + wcslen(L"echo (")) err(L"parse_util_cmdsubst_extent failed on line %ld", (long)__LINE__);
+
+    parse_util_cmdsubst_extent(a, 17, &begin, &end);
+    if (begin != a + wcslen(L"echo (echo (")) err(L"parse_util_cmdsubst_extent failed on line %ld", (long)__LINE__);
+
+    
+}
+
 class lru_node_test_t : public lru_node_t
 {
 public:
@@ -1747,6 +1772,7 @@ int main(int argc, char **argv)
     test_tok();
     test_fork();
     test_parser();
+    test_utils();
     test_lru();
     test_expand();
     test_fuzzy_match();
diff --git a/highlight.cpp b/highlight.cpp
index 8ea0989e..265fd40a 100644
--- a/highlight.cpp
+++ b/highlight.cpp
@@ -1319,7 +1319,7 @@ void highlight_shell(const wcstring &buff, std::vector<int> &color, size_t pos,
 
     std::fill(color.begin(), color.end(), -1);
 
-    /* Do something sucky and get the current working directory on this background thread. This should really be passed in. Note that we also need this as a vector (of one directory). */
+    /* Do something sucky and get the current working directory on this background thread. This should really be passed in. */
     const wcstring working_directory = env_get_pwd_slash();
 
     /* Tokenize the string */
@@ -1335,7 +1335,7 @@ void highlight_shell(const wcstring &buff, std::vector<int> &color, size_t pos,
     {
         wchar_t *begin, *end;
 
-        if (parse_util_locate_cmdsubst(subpos, &begin, &end, 1) <= 0)
+        if (parse_util_locate_cmdsubst(subpos, &begin, &end, true) <= 0)
         {
             break;
         }
diff --git a/parse_util.cpp b/parse_util.cpp
index a7614f0c..e6d04a3f 100644
--- a/parse_util.cpp
+++ b/parse_util.cpp
@@ -153,10 +153,7 @@ size_t parse_util_get_offset(const wcstring &str, int line, long line_offset)
 }
 
 
-int parse_util_locate_cmdsubst(const wchar_t *in,
-                               wchar_t **begin,
-                               wchar_t **end,
-                               int allow_incomplete)
+int parse_util_locate_cmdsubst(const wchar_t *in, wchar_t **begin, wchar_t **end, bool allow_incomplete)
 {
     wchar_t *pos;
     wchar_t prev=0;
@@ -243,73 +240,57 @@ int parse_util_locate_cmdsubst(const wchar_t *in,
     return 1;
 }
 
-
-void parse_util_cmdsubst_extent(const wchar_t *buff,
-                                size_t cursor_pos,
-                                const wchar_t **a,
-                                const wchar_t **b)
+void parse_util_cmdsubst_extent(const wchar_t *buff, size_t cursor_pos, const wchar_t **a, const wchar_t **b)
 {
-    wchar_t *begin, *end;
-    wchar_t *pos;
-    const wchar_t *cursor = buff + cursor_pos;
+    const wchar_t * const cursor = buff + cursor_pos;
 
     CHECK(buff,);
-
-    if (a)
-    {
-        *a = (wchar_t *)buff;
-    }
-
-    if (b)
-    {
-        *b = (wchar_t *)buff+wcslen(buff);
-    }
-
-    pos = (wchar_t *)buff;
-
-    while (1)
-    {
-        if (parse_util_locate_cmdsubst(pos,
-                                       &begin,
-                                       &end,
-                                       1) <= 0)
+    
+    const size_t bufflen = wcslen(buff);
+    assert(cursor_pos <= bufflen);
+    
+    /* ap and bp are the beginning and end of the tightest command substitition found so far */
+    const wchar_t *ap = buff, *bp = buff + bufflen;
+    const wchar_t *pos = buff;
+    for (;;)
+    {
+        wchar_t *begin = NULL, *end = NULL;
+        if (parse_util_locate_cmdsubst(pos, &begin, &end, true) <= 0)
         {
-            /*
-              No subshell found
-            */
+            /* No subshell found, all done */
             break;
         }
-
-        if (!end)
+        
+        /* Intrepret NULL to mean the end */
+        if (end == NULL)
         {
-            end = (wchar_t *)buff + wcslen(buff);
+            end = const_cast<wchar_t *>(buff) + bufflen;
         }
-
-        if ((begin < cursor) && (end >= cursor))
+        
+        if (begin < cursor && end >= cursor)
         {
+            /* This command substitution surrounds the cursor, so it's a tighter fit */
             begin++;
-
-            if (a)
-            {
-                *a = begin;
-            }
-
-            if (b)
-            {
-                *b = end;
-            }
-
-            break;
+            ap = begin;
+            bp = end;
+            pos = begin + 1;
         }
-
-        if (!*end)
+        else if (begin >= cursor)
         {
+            /* This command substitution starts at or after the cursor. Since it was the first command substitution in the string, we're done. */
             break;
         }
-
-        pos = end+1;
+        else
+        {
+            /* This command substitution ends before the cursor. Skip it. */
+            assert(end < cursor);
+            pos = end + 1;
+            assert(pos <= buff + bufflen);
+        }
     }
-
+    
+    if (a != NULL) *a = ap;
+    if (b != NULL) *b = bp;
 }
 
 /**
@@ -432,7 +413,6 @@ void parse_util_token_extent(const wchar_t *buff,
 {
     const wchar_t *begin, *end;
     long pos;
-    wchar_t *buffcpy;
 
     const wchar_t *a = NULL, *b = NULL, *pa = NULL, *pb = NULL;
 
@@ -459,14 +439,9 @@ void parse_util_token_extent(const wchar_t *buff,
     assert(end >= begin);
     assert(end <= (buff+wcslen(buff)));
 
-    buffcpy = wcsndup(begin, end-begin);
+    const wcstring buffcpy = wcstring(begin, end-begin);
 
-    if (!buffcpy)
-    {
-        DIE_MEM();
-    }
-
-    tokenizer_t tok(buffcpy, TOK_ACCEPT_UNFINISHED | TOK_SQUASH_ERRORS);
+    tokenizer_t tok(buffcpy.c_str(), TOK_ACCEPT_UNFINISHED | TOK_SQUASH_ERRORS);
     for (; tok_has_next(&tok); tok_next(&tok))
     {
         size_t tok_begin = tok_get_pos(&tok);
@@ -512,8 +487,6 @@ void parse_util_token_extent(const wchar_t *buff,
         }
     }
 
-    free(buffcpy);
-
     if (tok_begin)
     {
         *tok_begin = a;
@@ -679,7 +652,7 @@ static wchar_t get_quote(const wchar_t *cmd, size_t len)
             {
                 const wchar_t *end = quote_end(&cmd[i]);
                 //fwprintf( stderr, L"Jump %d\n",  end-cmd );
-                if ((end == 0) || (!*end) || (end-cmd > len))
+                if ((end == 0) || (!*end) || (end > cmd + len))
                 {
                     res = cmd[i];
                     break;
diff --git a/parse_util.h b/parse_util.h
index b8e370f7..24147e18 100644
--- a/parse_util.h
+++ b/parse_util.h
@@ -18,14 +18,14 @@
    \param in the string to search for subshells
    \param begin the starting paranthesis of the subshell
    \param end the ending paranthesis of the subshell
-   \param flags set this variable to ACCEPT_INCOMPLETE if in tab_completion mode
+   \param accept_incomplete whether to permit missing closing parenthesis
    \return -1 on syntax error, 0 if no subshells exist and 1 on sucess
 */
 
 int parse_util_locate_cmdsubst(const wchar_t *in,
                                wchar_t **begin,
                                wchar_t **end,
-                               int flags);
+                               bool accept_incomplete);
 
 /**
    Find the beginning and end of the command substitution under the
diff --git a/parser.cpp b/parser.cpp
index 1bc262ea..a8390af8 100644
--- a/parser.cpp
+++ b/parser.cpp
@@ -2716,7 +2716,7 @@ int parser_t::parser_test_argument(const wchar_t *arg, wcstring *out, const wcha
         switch (parse_util_locate_cmdsubst(arg_cpy,
                                            &paran_begin,
                                            &paran_end,
-                                           0))
+                                           false))
         {
             case -1:
                 err=1;