Indent switch + case properly. https://github.com/fish-shell/fish-shell/issues/530

1 files changed, 106 insertions, 159 deletions

diff --git a/parser.cpp b/parser.cpp
index 11b3745f..ca524b8d 100644
--- a/parser.cpp
+++ b/parser.cpp
@@ -46,13 +46,6 @@ The fish parser. Contains functions for parsing and evaluating code.
 #include "complete.h"
 
 /**
-   Maximum number of block levels in code. This is not the same as
-   maximum recursion depth, this only has to do with how many block
-   levels are legal in the source code, not at evaluation.
-*/
-#define BLOCK_MAX_COUNT 64
-
-/**
    Maximum number of function calls, i.e. recursion depth.
 */
 #define MAX_RECURSION_DEPTH 128
@@ -304,65 +297,21 @@ struct block_lookup_entry
 */
 static const struct block_lookup_entry block_lookup[]=
 {
-    {
-        WHILE, L"while", WHILE_BLOCK
-    }
-    ,
-    {
-        FOR, L"for", FOR_BLOCK
-    }
-    ,
-    {
-        IF, L"if", IF_BLOCK
-    }
-    ,
-    {
-        FUNCTION_DEF, L"function", FUNCTION_DEF_BLOCK
-    }
-    ,
-    {
-        FUNCTION_CALL, 0, FUNCTION_CALL_BLOCK
-    }
-    ,
-    {
-        FUNCTION_CALL_NO_SHADOW, 0, FUNCTION_CALL_NO_SHADOW_BLOCK
-    }
-    ,
-    {
-        SWITCH, L"switch", SWITCH_BLOCK
-    }
-    ,
-    {
-        FAKE, 0, FAKE_BLOCK
-    }
-    ,
-    {
-        TOP, 0, TOP_BLOCK
-    }
-    ,
-    {
-        SUBST, 0, SUBST_BLOCK
-    }
-    ,
-    {
-        BEGIN, L"begin", BEGIN_BLOCK
-    }
-    ,
-    {
-        SOURCE, L".", SOURCE_BLOCK
-    }
-    ,
-    {
-        EVENT, 0, EVENT_BLOCK
-    }
-    ,
-    {
-        BREAKPOINT, L"breakpoint", BREAKPOINT_BLOCK
-    }
-    ,
-    {
-        (block_type_t)0, 0, 0
-    }
+    { WHILE, L"while", WHILE_BLOCK },
+    { FOR, L"for", FOR_BLOCK },
+    { IF, L"if", IF_BLOCK },
+    { FUNCTION_DEF, L"function", FUNCTION_DEF_BLOCK },
+    { FUNCTION_CALL, 0, FUNCTION_CALL_BLOCK },
+    { FUNCTION_CALL_NO_SHADOW, 0, FUNCTION_CALL_NO_SHADOW_BLOCK },
+    { SWITCH, L"switch", SWITCH_BLOCK },
+    { FAKE, 0, FAKE_BLOCK },
+    { TOP, 0, TOP_BLOCK },
+    { SUBST, 0, SUBST_BLOCK },
+    { BEGIN, L"begin", BEGIN_BLOCK },
+    { SOURCE, L".", SOURCE_BLOCK },
+    { EVENT, 0, EVENT_BLOCK },
+    { BREAKPOINT, L"breakpoint", BREAKPOINT_BLOCK },
+    { (block_type_t)0, 0, 0 }
 };
 
 static bool job_should_skip_elseif(const job_t *job, const block_t *current_block);
@@ -413,19 +362,6 @@ void parser_t::skip_all_blocks(void)
     }
 }
 
-/**
-   Return the current number of block nestings
-*/
-/*
-static int block_count( block_t *b )
-{
-
-  if( b==0)
-    return 0;
-  return( block_count(b->outer)+1);
-}
-*/
-
 void parser_t::push_block(block_t *newv)
 {
     const enum block_type_t type = newv->type();
@@ -495,9 +431,7 @@ void parser_t::pop_block()
 
 const wchar_t *parser_t::get_block_desc(int block) const
 {
-    int i;
-
-    for (i=0; block_lookup[i].desc; i++)
+    for (size_t i=0; block_lookup[i].desc; i++)
     {
         if (block_lookup[i].type == block)
         {
@@ -2747,9 +2681,7 @@ int parser_t::eval(const wcstring &cmdStr, const io_chain_t &io, enum block_type
 */
 block_type_t parser_get_block_type(const wcstring &cmd)
 {
-    int i;
-
-    for (i=0; block_lookup[i].desc; i++)
+    for (size_t i=0; block_lookup[i].desc; i++)
     {
         if (block_lookup[i].name && cmd == block_lookup[i].name)
         {
@@ -2764,16 +2696,14 @@ block_type_t parser_get_block_type(const wcstring &cmd)
 */
 const wchar_t *parser_get_block_command(int type)
 {
-    int i;
-
-    for (i=0; block_lookup[i].desc; i++)
+    for (size_t i=0; block_lookup[i].desc; i++)
     {
         if (block_lookup[i].type == type)
         {
             return block_lookup[i].name;
         }
     }
-    return 0;
+    return NULL;
 }
 
 /**
@@ -2966,10 +2896,16 @@ int parser_t::test_args(const  wchar_t * buff, wcstring *out, const wchar_t *pre
     return err;
 }
 
-int parser_t::test(const  wchar_t * buff,
-                   int *block_level,
-                   wcstring *out,
-                   const wchar_t *prefix)
+// helper type used in parser::test below
+struct block_info_t {
+    int position; //tokenizer position
+    block_type_t type; //type of the block
+    int indentation; //indentation associated with the block
+    
+    bool has_had_case; //if we are a switch, whether we've encountered a case
+};
+
+int parser_t::test(const wchar_t *buff, int *block_level, wcstring *out, const wchar_t *prefix)
 {
     ASSERT_IS_MAIN_THREAD();
 
@@ -2983,10 +2919,10 @@ int parser_t::test(const  wchar_t * buff,
 
     tokenizer_t * const previous_tokenizer=current_tokenizer;
     const int previous_pos=current_tokenizer_pos;
-
-    int block_pos[BLOCK_MAX_COUNT] = {};
-    block_type_t block_type[BLOCK_MAX_COUNT] = {};
-    int count = 0;
+    
+    // These are very nearly stacks, but sometimes we have to inspect non-top elements (e.g. return)
+    std::vector<struct block_info_t> block_infos;
+    int indentation_sum = 0; //sum of indentation in block_infos
     int res = 0;
 
     /*
@@ -3100,8 +3036,29 @@ int parser_t::test(const  wchar_t * buff,
                     if (command == L"end")
                     {
                         tok_next(&tok);
-                        count--;
                         tok_set_pos(&tok, mark);
+                        
+                        /* Test that end is not used when not inside any block */
+                        if (block_infos.empty())
+                        {
+                            err = 1;
+                            if (out)
+                            {
+                                error(SYNTAX_ERROR,
+                                      tok_get_pos(&tok),
+                                      INVALID_END_ERR_MSG);
+                                print_errors(*out, prefix);
+                                const wcstring h = builtin_help_get(*this, L"end");
+                                if (! h.empty())
+                                    append_format(*out, L"%ls", h.c_str());
+                            }
+                        }
+                        else
+                        {
+                            indentation_sum -= block_infos.back().indentation;
+                            block_infos.pop_back();
+                            
+                        }
                     }
 
                     /*
@@ -3109,10 +3066,32 @@ int parser_t::test(const  wchar_t * buff,
                       _after_ checking for end commands, but _before_
                       checking for block opening commands.
                     */
-                    bool is_else_or_elseif = (command == L"else");
-                    if (block_level)
+                    if (block_level != NULL)
                     {
-                        block_level[tok_get_pos(&tok)] = count + (is_else_or_elseif?-1:0);
+                        int indentation_adjust = 0;
+                        if (command == L"else")
+                        {
+                            // if or else if goes back
+                            indentation_adjust = -1;
+                        }
+                        else if (command == L"case")
+                        {
+                            if (! block_infos.empty() && block_infos.back().type == SWITCH)
+                            {
+                                // mark that we've encountered a case, and increase the indentation
+                                // by doing this now, we avoid overly indenting the first case as the user types it
+                                if (! block_infos.back().has_had_case)
+                                {
+                                    block_infos.back().has_had_case = true;
+                                    block_infos.back().indentation += 1;
+                                    indentation_sum += 1;
+                                }
+                                // unindent this case
+                                indentation_adjust = -1;
+                            }
+                        }
+
+                        block_level[tok_get_pos(&tok)] = indentation_sum + indentation_adjust;
                     }
 
                     /*
@@ -3120,25 +3099,11 @@ int parser_t::test(const  wchar_t * buff,
                     */
                     if (parser_keywords_is_block(command))
                     {
-                        if (count >= BLOCK_MAX_COUNT)
-                        {
-                            if (out)
-                            {
-                                error(SYNTAX_ERROR,
-                                      tok_get_pos(&tok),
-                                      BLOCK_ERR_MSG);
-
-                                print_errors(*out, prefix);
-                            }
-                        }
-                        else
-                        {
-                            block_type[count] = parser_get_block_type(command);
-                            block_pos[count] = current_tokenizer_pos;
-                            tok_next(&tok);
-                            count++;
-                            tok_set_pos(&tok, mark);
-                        }
+                        struct block_info_t info = {current_tokenizer_pos, parser_get_block_type(command), 1 /* indent */};
+                        block_infos.push_back(info);
+                        indentation_sum += info.indentation;
+                        tok_next(&tok);
+                        tok_set_pos(&tok, mark);
                     }
 
                     /*
@@ -3203,7 +3168,7 @@ int parser_t::test(const  wchar_t * buff,
                     */
                     if (command == L"case")
                     {
-                        if (!count || block_type[count-1]!=SWITCH)
+                        if (block_infos.empty() || block_infos.back().type != SWITCH)
                         {
                             err=1;
 
@@ -3226,13 +3191,13 @@ int parser_t::test(const  wchar_t * buff,
                     */
                     if (command == L"return")
                     {
-                        int found_func=0;
-                        int i;
-                        for (i=count-1; i>=0; i--)
+                        bool found_func = false;
+                        size_t block_idx = block_infos.size();
+                        while (block_idx--)
                         {
-                            if (block_type[i]==FUNCTION_DEF)
+                            if (block_infos.at(block_idx).type == FUNCTION_DEF)
                             {
-                                found_func=1;
+                                found_func = true;
                                 break;
                             }
                         }
@@ -3281,14 +3246,14 @@ int parser_t::test(const  wchar_t * buff,
                     */
                     if (contains(command, L"break", L"continue"))
                     {
-                        int found_loop=0;
-                        int i;
-                        for (i=count-1; i>=0; i--)
+                        bool found_loop = false;
+                        size_t block_idx = block_infos.size();
+                        while (block_idx--)
                         {
-                            if ((block_type[i]==WHILE) ||
-                                    (block_type[i]==FOR))
+                            block_type_t type = block_infos.at(block_idx).type;
+                            if (type == WHILE || type == FOR)
                             {
-                                found_loop=1;
+                                found_loop = true;
                                 break;
                             }
                         }
@@ -3336,7 +3301,7 @@ int parser_t::test(const  wchar_t * buff,
                     */
                     if (command == L"else")
                     {
-                        if (!count || block_type[count-1]!=IF)
+                        if (block_infos.empty() || block_infos.back().type != IF)
                         {
                             err=1;
                             if (out)
@@ -3350,25 +3315,6 @@ int parser_t::test(const  wchar_t * buff,
                             }
                         }
                     }
-
-                    /*
-                      Test that end is not used when not inside any block
-                    */
-                    if (count < 0)
-                    {
-                        err = 1;
-                        if (out)
-                        {
-                            error(SYNTAX_ERROR,
-                                  tok_get_pos(&tok),
-                                  INVALID_END_ERR_MSG);
-                            print_errors(*out, prefix);
-                            const wcstring h = builtin_help_get(*this, L"end");
-                            if (h.size())
-                                append_format(*out, L"%ls", h.c_str());
-                        }
-                    }
-
                 }
                 else
                 {
@@ -3677,17 +3623,17 @@ int parser_t::test(const  wchar_t * buff,
     }
 
 
-    if (out && count>0)
+    if (out != NULL && ! block_infos.empty())
     {
         const wchar_t *cmd;
+        int bad_pos = block_infos.back().position;
+        block_type_t bad_type = block_infos.back().type;
 
-        error(SYNTAX_ERROR,
-              block_pos[count-1],
-              BLOCK_END_ERR_MSG);
+        error(SYNTAX_ERROR, bad_pos, BLOCK_END_ERR_MSG);
 
         print_errors(*out, prefix);
 
-        cmd = parser_get_block_command(block_type[count -1]);
+        cmd = parser_get_block_command(bad_type);
         if (cmd)
         {
             const wcstring h = builtin_help_get(*this, cmd);
@@ -3736,19 +3682,20 @@ int parser_t::test(const  wchar_t * buff,
           validator had at exit. This makes sure a new line is
           correctly indented even if it is empty.
         */
+        int last_indent = block_infos.empty() ? 0 : block_infos.back().indentation;
         size_t suffix_idx = len;
         while (suffix_idx--)
         {
             if (!wcschr(L" \n\t\r", buff[suffix_idx]))
                 break;
-            block_level[suffix_idx] = count;
+            block_level[suffix_idx] = last_indent;
         }
     }
 
     /*
       Calculate exit status
     */
-    if (count!= 0)
+    if (! block_infos.empty())
         unfinished = 1;
 
     if (err)