patch 8.2.1006: Vim9: require unnecessary return statement

Problem:    Vim9: require unnecessary return statement.
Solution:   Improve the use of the had_return flag. (closes #6270)

diff --git a/src/testdir/test_vim9_disassemble.vim b/src/testdir/test_vim9_disassemble.vim
index e1042588cffbfea2b2e6fdd59b3dcc2af8c18276..2e541d9f924b67926ebdf54f5a31da9b7e831ec7 100644 (file)
--- a/src/testdir/test_vim9_disassemble.vim
+++ b/src/testdir/test_vim9_disassemble.vim
@@ -533,6 +533,30 @@ def Test_disassemble_const_expr()
   assert_notmatch('JUMP', instr)
 enddef
 
+def ReturnInIf(): string
+  if g:cond
+    return "yes"
+  else
+    return "no"
+  endif
+enddef
+
+def Test_disassemble_return_in_if()
+  let instr = execute('disassemble ReturnInIf')
+  assert_match('ReturnInIf\_s*' ..
+        'if g:cond\_s*' ..
+        '0 LOADG g:cond\_s*' ..
+        '1 JUMP_IF_FALSE -> 4\_s*' ..
+        'return "yes"\_s*' ..
+        '2 PUSHS "yes"\_s*' ..
+        '3 RETURN\_s*' ..
+        'else\_s*' ..
+        ' return "no"\_s*' ..
+        '4 PUSHS "no"\_s*' ..
+        '5 RETURN$',
+        instr)
+enddef
+
 def WithFunc()
   let Funky1: func
   let Funky2: func = function("len")

diff --git a/src/testdir/test_vim9_func.vim b/src/testdir/test_vim9_func.vim
index c0a4d9bd65573ae1d5d12168583304fec42c36cd..499699c17ab3e7fb73288a5271b50d21cce4e2d9 100644 (file)
--- a/src/testdir/test_vim9_func.vim
+++ b/src/testdir/test_vim9_func.vim
@@ -31,6 +31,31 @@ def Test_return_something()
   assert_fails('call ReturnGlobal()', 'E1029: Expected number but got string')
 enddef
 
+def Test_missing_return()
+  CheckDefFailure(['def Missing(): number',
+                   '  if g:cond',
+                   '    echo "no return"',
+                   '  else',
+                   '    return 0',
+                   '  endif'
+                   'enddef'], 'E1027:')
+  CheckDefFailure(['def Missing(): number',
+                   '  if g:cond',
+                   '    return 1',
+                   '  else',
+                   '    echo "no return"',
+                   '  endif'
+                   'enddef'], 'E1027:')
+  CheckDefFailure(['def Missing(): number',
+                   '  if g:cond',
+                   '    return 1',
+                   '  else',
+                   '    return 2',
+                   '  endif'
+                   '  return 3'
+                   'enddef'], 'E1095:')
+enddef
+
 let s:nothing = 0
 def ReturnNothing()
   s:nothing = 1

diff --git a/src/version.c b/src/version.c
index 8fe49c57e2550675c02a451f65701b88c2467ee5..080a8b159047103b656967434045cc1a358440f3 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -754,6 +754,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1006,
 /**/
     1005,
 /**/

diff --git a/src/vim9compile.c b/src/vim9compile.c
index c45f620cafe09df98afa7b85a16a2bb351a933a2..8017716b5b5973eb5a0906b562b0e9cb3ea276f3 100644 (file)
--- a/src/vim9compile.c
+++ b/src/vim9compile.c
@@ -23,6 +23,13 @@
 #define DEFINE_VIM9_GLOBALS
 #include "vim9.h"
 
+// values for ctx_skip
+typedef enum {
+    SKIP_NOT,          // condition is a constant, produce code
+    SKIP_YES,          // condition is a constant, do NOT produce code
+    SKIP_UNKNONW       // condition is not a constant, produce code
+} skip_T;
+
 /*
  * Chain of jump instructions where the end label needs to be set.
  */
@@ -36,6 +43,8 @@ struct endlabel_S {
  * info specific for the scope of :if / elseif / else
  */
 typedef struct {
+    int                is_seen_else;
+    int                is_had_return;      // every block ends in :return
     int                is_if_label;        // instruction idx at IF or ELSEIF
     endlabel_T *is_end_label;      // instructions to set end label
 } ifscope_T;
@@ -83,6 +92,7 @@ struct scope_S {
     scope_T    *se_outer;          // scope containing this one
     scopetype_T se_type;
     int                se_local_count;     // ctx_locals.ga_len before scope
+    skip_T     se_skip_save;       // ctx_skip before the block
     union {
        ifscope_T       se_if;
        whilescope_T    se_while;
@@ -103,13 +113,6 @@ typedef struct {
     int                lv_arg;         // when TRUE this is an argument
 } lvar_T;
 
-// values for ctx_skip
-typedef enum {
-    SKIP_NOT,          // condition is a constant, produce code
-    SKIP_YES,          // condition is a constant, do NOT produce code
-    SKIP_UNKNONW       // condition is not a constant, produce code
-} skip_T;
-
 /*
  * Context for compiling lines of Vim script.
  * Stores info about the local variables and condition stack.
@@ -130,6 +133,7 @@ struct cctx_S {
 
     skip_T     ctx_skip;
     scope_T    *ctx_scope;         // current scope, NULL at toplevel
+    int                ctx_had_return;     // last seen statement was "return"
 
     cctx_T     *ctx_outer;         // outer scope for lambda or nested
                                    // function
@@ -5664,6 +5668,7 @@ compile_if(char_u *arg, cctx_T *cctx)
     garray_T   *instr = &cctx->ctx_instr;
     int                instr_count = instr->ga_len;
     scope_T    *scope;
+    skip_T     skip_save = cctx->ctx_skip;
     ppconst_T  ppconst;
 
     CLEAR_FIELD(ppconst);
@@ -5672,10 +5677,11 @@ compile_if(char_u *arg, cctx_T *cctx)
        clear_ppconst(&ppconst);
        return NULL;
     }
-    if (instr->ga_len == instr_count && ppconst.pp_used == 1)
+    if (cctx->ctx_skip == SKIP_YES)
+       clear_ppconst(&ppconst);
+    else if (instr->ga_len == instr_count && ppconst.pp_used == 1)
     {
        // The expression results in a constant.
-       // TODO: how about nesting?
        cctx->ctx_skip = tv2bool(&ppconst.pp_tv[0]) ? SKIP_NOT : SKIP_YES;
        clear_ppconst(&ppconst);
     }
@@ -5690,6 +5696,9 @@ compile_if(char_u *arg, cctx_T *cctx)
     scope = new_scope(cctx, IF_SCOPE);
     if (scope == NULL)
        return NULL;
+    scope->se_skip_save = skip_save;
+    // "is_had_return" will be reset if any block does not end in :return
+    scope->se_u.se_if.is_had_return = TRUE;
 
     if (cctx->ctx_skip == SKIP_UNKNONW)
     {
@@ -5719,6 +5728,8 @@ compile_elseif(char_u *arg, cctx_T *cctx)
        return NULL;
     }
     unwind_locals(cctx, scope->se_local_count);
+    if (!cctx->ctx_had_return)
+       scope->se_u.se_if.is_had_return = FALSE;
 
     if (cctx->ctx_skip == SKIP_UNKNONW)
     {
@@ -5737,7 +5748,9 @@ compile_elseif(char_u *arg, cctx_T *cctx)
        clear_ppconst(&ppconst);
        return NULL;
     }
-    if (instr->ga_len == instr_count && ppconst.pp_used == 1)
+    if (scope->se_skip_save == SKIP_YES)
+       clear_ppconst(&ppconst);
+    else if (instr->ga_len == instr_count && ppconst.pp_used == 1)
     {
        // The expression results in a constant.
        // TODO: how about nesting?
@@ -5774,28 +5787,35 @@ compile_else(char_u *arg, cctx_T *cctx)
        return NULL;
     }
     unwind_locals(cctx, scope->se_local_count);
+    if (!cctx->ctx_had_return)
+       scope->se_u.se_if.is_had_return = FALSE;
+    scope->se_u.se_if.is_seen_else = TRUE;
 
-    // jump from previous block to the end, unless the else block is empty
-    if (cctx->ctx_skip == SKIP_UNKNONW)
+    if (scope->se_skip_save != SKIP_YES)
     {
-       if (compile_jump_to_end(&scope->se_u.se_if.is_end_label,
+       // jump from previous block to the end, unless the else block is empty
+       if (cctx->ctx_skip == SKIP_UNKNONW)
+       {
+           if (!cctx->ctx_had_return
+                   && compile_jump_to_end(&scope->se_u.se_if.is_end_label,
                                                    JUMP_ALWAYS, cctx) == FAIL)
-           return NULL;
-    }
+               return NULL;
+       }
 
-    if (cctx->ctx_skip == SKIP_UNKNONW)
-    {
-       if (scope->se_u.se_if.is_if_label >= 0)
+       if (cctx->ctx_skip == SKIP_UNKNONW)
        {
-           // previous "if" or "elseif" jumps here
-           isn = ((isn_T *)instr->ga_data) + scope->se_u.se_if.is_if_label;
-           isn->isn_arg.jump.jump_where = instr->ga_len;
-           scope->se_u.se_if.is_if_label = -1;
+           if (scope->se_u.se_if.is_if_label >= 0)
+           {
+               // previous "if" or "elseif" jumps here
+               isn = ((isn_T *)instr->ga_data) + scope->se_u.se_if.is_if_label;
+               isn->isn_arg.jump.jump_where = instr->ga_len;
+               scope->se_u.se_if.is_if_label = -1;
+           }
        }
-    }
 
-    if (cctx->ctx_skip != SKIP_UNKNONW)
-       cctx->ctx_skip = cctx->ctx_skip == SKIP_YES ? SKIP_NOT : SKIP_YES;
+       if (cctx->ctx_skip != SKIP_UNKNONW)
+           cctx->ctx_skip = cctx->ctx_skip == SKIP_YES ? SKIP_NOT : SKIP_YES;
+    }
 
     return p;
 }
@@ -5815,6 +5835,8 @@ compile_endif(char_u *arg, cctx_T *cctx)
     }
     ifscope = &scope->se_u.se_if;
     unwind_locals(cctx, scope->se_local_count);
+    if (!cctx->ctx_had_return)
+       ifscope->is_had_return = FALSE;
 
     if (scope->se_u.se_if.is_if_label >= 0)
     {
@@ -5824,8 +5846,11 @@ compile_endif(char_u *arg, cctx_T *cctx)
     }
     // Fill in the "end" label in jumps at the end of the blocks.
     compile_fill_jump_to_end(&ifscope->is_end_label, cctx);
-    // TODO: this should restore the value from before the :if
-    cctx->ctx_skip = SKIP_UNKNONW;
+    cctx->ctx_skip = scope->se_skip_save;
+
+    // If all the blocks end in :return and there is an :else then the
+    // had_return flag is set.
+    cctx->ctx_had_return = ifscope->is_had_return && ifscope->is_seen_else;
 
     drop_scope(cctx);
     return arg;
@@ -6528,7 +6553,6 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
     char_u     *line = NULL;
     char_u     *p;
     char       *errormsg = NULL;       // error message
-    int                had_return = FALSE;
     cctx_T     cctx;
     garray_T   *instr;
     int                called_emsg_before = called_emsg;
@@ -6648,7 +6672,6 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
        }
        emsg_before = called_emsg;
 
-       had_return = FALSE;
        CLEAR_FIELD(ea);
        ea.cmdlinep = &line;
        ea.cmd = skipwhite(line);
@@ -6823,6 +6846,22 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
            continue;
        }
 
+       if (ea.cmdidx != CMD_elseif
+               && ea.cmdidx != CMD_else
+               && ea.cmdidx != CMD_endif
+               && ea.cmdidx != CMD_endfor
+               && ea.cmdidx != CMD_endwhile
+               && ea.cmdidx != CMD_catch
+               && ea.cmdidx != CMD_finally
+               && ea.cmdidx != CMD_endtry)
+       {
+           if (cctx.ctx_had_return)
+           {
+               emsg(_("E1095: Unreachable code after :return"));
+               goto erret;
+           }
+       }
+
        switch (ea.cmdidx)
        {
            case CMD_def:
@@ -6836,7 +6875,7 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
 
            case CMD_return:
                    line = compile_return(p, set_return_type, &cctx);
-                   had_return = TRUE;
+                   cctx.ctx_had_return = TRUE;
                    break;
 
            case CMD_let:
@@ -6861,9 +6900,11 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
                    break;
            case CMD_elseif:
                    line = compile_elseif(p, &cctx);
+                   cctx.ctx_had_return = FALSE;
                    break;
            case CMD_else:
                    line = compile_else(p, &cctx);
+                   cctx.ctx_had_return = FALSE;
                    break;
            case CMD_endif:
                    line = compile_endif(p, &cctx);
@@ -6874,6 +6915,7 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
                    break;
            case CMD_endwhile:
                    line = compile_endwhile(p, &cctx);
+                   cctx.ctx_had_return = FALSE;
                    break;
 
            case CMD_for:
@@ -6881,6 +6923,7 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
                    break;
            case CMD_endfor:
                    line = compile_endfor(p, &cctx);
+                   cctx.ctx_had_return = FALSE;
                    break;
            case CMD_continue:
                    line = compile_continue(p, &cctx);
@@ -6894,12 +6937,15 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
                    break;
            case CMD_catch:
                    line = compile_catch(p, &cctx);
+                   cctx.ctx_had_return = FALSE;
                    break;
            case CMD_finally:
                    line = compile_finally(p, &cctx);
+                   cctx.ctx_had_return = FALSE;
                    break;
            case CMD_endtry:
                    line = compile_endtry(p, &cctx);
+                   cctx.ctx_had_return = FALSE;
                    break;
            case CMD_throw:
                    line = compile_throw(p, &cctx);
@@ -6944,7 +6990,7 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
        goto erret;
     }
 
-    if (!had_return)
+    if (!cctx.ctx_had_return)
     {
        if (ufunc->uf_ret_type->tt_type != VAR_VOID)
        {