# HG changeset patch # User Bram Moolenaar # Date 1592506804 -7200 # Node ID 0d7465881b06d4964c614e4e5174b5ed0483a93f # Parent 4c697217281ee1e698730c60d792621960dd8c9a patch 8.2.1006: Vim9: require unnecessary return statement Commit: https://github.com/vim/vim/commit/efd885559405e1561d577e1b0e6fa827705d285e Author: Bram Moolenaar Date: Thu Jun 18 20:50:10 2020 +0200 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 https://github.com/vim/vim/issues/6270) diff --git a/src/testdir/test_vim9_disassemble.vim b/src/testdir/test_vim9_disassemble.vim --- 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 --- 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 --- 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 --- 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); - - // 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, + 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) + return NULL; + } + + 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 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 } 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 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 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 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 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 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 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 goto erret; } - if (!had_return) + if (!cctx.ctx_had_return) { if (ufunc->uf_ret_type->tt_type != VAR_VOID) {