To: vim_dev@googlegroups.com Subject: Patch 8.2.1006 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ 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 -- Bram@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 ///