Patch 8.2.1006

[Bram Moolenaar]

Patch 8.2.1006
Problem: Vim9: require unnecessary return statement.
Solution: Improve the use of the had_return flag. (closes #6270)
Files: src/vim9compile.c, src/testdir/test_vim9_disassemble.vim,
src/testdir/test_vim9_func.vim


*** ../vim-8.2.1005/src/vim9compile.c 2020-06-18 19:31:04.626688594 +0200
--- src/vim9compile.c 2020-06-18 20:37:00.343573396 +0200
***************
*** 23,28 ****
--- 23,35 ----
#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,41 ****
--- 43,50 ----
* 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,88 ****
--- 92,98 ----
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,115 ****
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.
--- 113,118 ----
***************
*** 130,135 ****
--- 133,139 ----

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,5669 ****
--- 5668,5674 ----
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,5681 ****
clear_ppconst(&ppconst);
return NULL;
}
! 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);
}
--- 5677,5687 ----
clear_ppconst(&ppconst);
return NULL;
}
! 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.
cctx->ctx_skip = tv2bool(&ppconst.pp_tv[0]) ? SKIP_NOT : SKIP_YES;
clear_ppconst(&ppconst);
}
***************
*** 5690,5695 ****
--- 5696,5704 ----
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,5724 ****
--- 5728,5735 ----
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,5743 ****
clear_ppconst(&ppconst);
return NULL;
}
! if (instr->ga_len == instr_count && ppconst.pp_used == 1)
{
// The expression results in a constant.
// TODO: how about nesting?
--- 5748,5756 ----
clear_ppconst(&ppconst);
return NULL;
}
! 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,5801 ****
return NULL;
}
unwind_locals(cctx, scope->se_local_count);

! // jump from previous block to the end, unless the else block is empty
! if (cctx->ctx_skip == SKIP_UNKNONW)
{
! if (compile_jump_to_end(&scope->se_u.se_if.is_end_label,
JUMP_ALWAYS, cctx) == FAIL)
! return NULL;
! }

! if (cctx->ctx_skip == SKIP_UNKNONW)
! {
! 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;

return p;
}
--- 5787,5821 ----
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;

! if (scope->se_skip_save != SKIP_YES)
{
! // 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;
! }

! if (cctx->ctx_skip == SKIP_UNKNONW)
{
! 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;
! }

return p;
}
***************
*** 5815,5820 ****
--- 5835,5842 ----
}
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,5831 ****
}
// 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;

drop_scope(cctx);
return arg;
--- 5846,5856 ----
}
// Fill in the "end" label in jumps at the end of the blocks.
compile_fill_jump_to_end(&ifscope->is_end_label, cctx);
! 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,6534 ****
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;
--- 6553,6558 ----
***************
*** 6648,6654 ****
}
emsg_before = called_emsg;

- had_return = FALSE;
CLEAR_FIELD(ea);
ea.cmdlinep = &line;
ea.cmd = skipwhite(line);
--- 6672,6677 ----
***************
*** 6823,6828 ****
--- 6846,6867 ----
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,6842 ****

case CMD_return:
line = compile_return(p, set_return_type, &cctx);
! had_return = TRUE;
break;

case CMD_let:
--- 6875,6881 ----

case CMD_return:
line = compile_return(p, set_return_type, &cctx);
! cctx.ctx_had_return = TRUE;
break;

case CMD_let:
***************
*** 6861,6869 ****
--- 6900,6910 ----
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,6879 ****
--- 6915,6921 ----
break;
case CMD_endwhile:
line = compile_endwhile(p, &cctx);
+ cctx.ctx_had_return = FALSE;
break;

case CMD_for:
***************
*** 6881,6886 ****
--- 6923,6929 ----
break;
case CMD_endfor:
line = compile_endfor(p, &cctx);
+ cctx.ctx_had_return = FALSE;
break;
case CMD_continue:
line = compile_continue(p, &cctx);
***************
*** 6894,6905 ****
--- 6937,6951 ----
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,6950 ****
goto erret;
}

! if (!had_return)
{
if (ufunc->uf_ret_type->tt_type != VAR_VOID)
{
--- 6990,6996 ----
goto erret;
}

! if (!cctx.ctx_had_return)
{
if (ufunc->uf_ret_type->tt_type != VAR_VOID)
{
*** ../vim-8.2.1005/src/testdir/test_vim9_disassemble.vim 2020-05-24 23:00:06.444196001 +0200
--- src/testdir/test_vim9_disassemble.vim 2020-06-18 20:37:44.663493853 +0200
***************
*** 533,538 ****
--- 533,562 ----
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")
*** ../vim-8.2.1005/src/testdir/test_vim9_func.vim 2020-06-18 18:45:46.001900050 +0200
--- src/testdir/test_vim9_func.vim 2020-06-18 20:47:15.114273602 +0200
***************
*** 31,36 ****
--- 31,61 ----
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
*** ../vim-8.2.1005/src/version.c 2020-06-18 19:31:04.626688594 +0200
--- src/version.c 2020-06-18 20:47:37.458220916 +0200
***************
*** 756,757 ****
--- 756,759 ----
{ /* Add new patch number below this line */
+ /**/
+ 1006,
/**/

--
Proofread carefully to see if you any words out.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///