patch 8.2.1006: Vim9: require unnecessary return statementv8.2.1006

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

4 files changed, 128 insertions, 31 deletions

diff --git a/src/testdir/test_vim9_disassemble.vim b/src/testdir/test_vim9_disassemble.vim
index e1042588c..2e541d9f9 100644
--- 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 c0a4d9bd6..499699c17 100644
--- 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 8fe49c57e..080a8b159 100644
--- a/src/version.c
+++ b/src/version.c
@@ -755,6 +755,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    1006,
+/**/
     1005,
 /**/
     1004,
diff --git a/src/vim9compile.c b/src/vim9compile.c
index c45f620ca..8017716b5 100644
--- 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)
 	{