Skip to content

Commit efd8855

Browse files
brammoolbrammool
committed
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)
1 parent 9b68c82 commit efd8855

4 files changed

Lines changed: 128 additions & 31 deletions

File tree

‎src/testdir/test_vim9_disassemble.vim‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,30 @@ def Test_disassemble_const_expr()
533533
assert_notmatch('JUMP', instr)
534534
enddef
535535

536+
def ReturnInIf(): string
537+
if g:cond
538+
return "yes"
539+
else
540+
return "no"
541+
endif
542+
enddef
543+
544+
def Test_disassemble_return_in_if()
545+
let instr = execute('disassemble ReturnInIf')
546+
assert_match('ReturnInIf\_s*' ..
547+
'if g:cond\_s*' ..
548+
'0 LOADG g:cond\_s*' ..
549+
'1 JUMP_IF_FALSE -> 4\_s*' ..
550+
'return "yes"\_s*' ..
551+
'2 PUSHS "yes"\_s*' ..
552+
'3 RETURN\_s*' ..
553+
'else\_s*' ..
554+
' return "no"\_s*' ..
555+
'4 PUSHS "no"\_s*' ..
556+
'5 RETURN$',
557+
instr)
558+
enddef
559+
536560
def WithFunc()
537561
let Funky1: func
538562
let Funky2: func = function("len")

‎src/testdir/test_vim9_func.vim‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,31 @@ def Test_return_something()
3131
assert_fails('call ReturnGlobal()', 'E1029: Expected number but got string')
3232
enddef
3333

34+
def Test_missing_return()
35+
CheckDefFailure(['def Missing(): number',
36+
' if g:cond',
37+
' echo "no return"',
38+
' else',
39+
' return 0',
40+
' endif'
41+
'enddef'], 'E1027:')
42+
CheckDefFailure(['def Missing(): number',
43+
' if g:cond',
44+
' return 1',
45+
' else',
46+
' echo "no return"',
47+
' endif'
48+
'enddef'], 'E1027:')
49+
CheckDefFailure(['def Missing(): number',
50+
' if g:cond',
51+
' return 1',
52+
' else',
53+
' return 2',
54+
' endif'
55+
' return 3'
56+
'enddef'], 'E1095:')
57+
enddef
58+
3459
let s:nothing = 0
3560
def ReturnNothing()
3661
s:nothing = 1

‎src/version.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,8 @@ static char *(features[]) =
754754

755755
static int included_patches[] =
756756
{ /* Add new patch number below this line */
757+
/**/
758+
1006,
757759
/**/
758760
1005,
759761
/**/

‎src/vim9compile.c‎

Lines changed: 77 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,13 @@
2323
#define DEFINE_VIM9_GLOBALS
2424
#include "vim9.h"
2525

26+
// values for ctx_skip
27+
typedef enum {
28+
SKIP_NOT, // condition is a constant, produce code
29+
SKIP_YES, // condition is a constant, do NOT produce code
30+
SKIP_UNKNONW // condition is not a constant, produce code
31+
} skip_T;
32+
2633
/*
2734
* Chain of jump instructions where the end label needs to be set.
2835
*/
@@ -36,6 +43,8 @@ struct endlabel_S {
3643
* info specific for the scope of :if / elseif / else
3744
*/
3845
typedef struct {
46+
int is_seen_else;
47+
int is_had_return; // every block ends in :return
3948
int is_if_label; // instruction idx at IF or ELSEIF
4049
endlabel_T *is_end_label; // instructions to set end label
4150
} ifscope_T;
@@ -83,6 +92,7 @@ struct scope_S {
8392
scope_T *se_outer; // scope containing this one
8493
scopetype_T se_type;
8594
int se_local_count; // ctx_locals.ga_len before scope
95+
skip_T se_skip_save; // ctx_skip before the block
8696
union {
8797
ifscope_T se_if;
8898
whilescope_T se_while;
@@ -103,13 +113,6 @@ typedef struct {
103113
int lv_arg; // when TRUE this is an argument
104114
} lvar_T;
105115

106-
// values for ctx_skip
107-
typedef enum {
108-
SKIP_NOT, // condition is a constant, produce code
109-
SKIP_YES, // condition is a constant, do NOT produce code
110-
SKIP_UNKNONW // condition is not a constant, produce code
111-
} skip_T;
112-
113116
/*
114117
* Context for compiling lines of Vim script.
115118
* Stores info about the local variables and condition stack.
@@ -130,6 +133,7 @@ struct cctx_S {
130133

131134
skip_T ctx_skip;
132135
scope_T *ctx_scope; // current scope, NULL at toplevel
136+
int ctx_had_return; // last seen statement was "return"
133137

134138
cctx_T *ctx_outer; // outer scope for lambda or nested
135139
// function
@@ -5664,6 +5668,7 @@ compile_if(char_u *arg, cctx_T *cctx)
56645668
garray_T *instr = &cctx->ctx_instr;
56655669
int instr_count = instr->ga_len;
56665670
scope_T *scope;
5671+
skip_T skip_save = cctx->ctx_skip;
56675672
ppconst_T ppconst;
56685673

56695674
CLEAR_FIELD(ppconst);
@@ -5672,10 +5677,11 @@ compile_if(char_u *arg, cctx_T *cctx)
56725677
clear_ppconst(&ppconst);
56735678
return NULL;
56745679
}
5675-
if (instr->ga_len == instr_count && ppconst.pp_used == 1)
5680+
if (cctx->ctx_skip == SKIP_YES)
5681+
clear_ppconst(&ppconst);
5682+
else if (instr->ga_len == instr_count && ppconst.pp_used == 1)
56765683
{
56775684
// The expression results in a constant.
5678-
// TODO: how about nesting?
56795685
cctx->ctx_skip = tv2bool(&ppconst.pp_tv[0]) ? SKIP_NOT : SKIP_YES;
56805686
clear_ppconst(&ppconst);
56815687
}
@@ -5690,6 +5696,9 @@ compile_if(char_u *arg, cctx_T *cctx)
56905696
scope = new_scope(cctx, IF_SCOPE);
56915697
if (scope == NULL)
56925698
return NULL;
5699+
scope->se_skip_save = skip_save;
5700+
// "is_had_return" will be reset if any block does not end in :return
5701+
scope->se_u.se_if.is_had_return = TRUE;
56935702

56945703
if (cctx->ctx_skip == SKIP_UNKNONW)
56955704
{
@@ -5719,6 +5728,8 @@ compile_elseif(char_u *arg, cctx_T *cctx)
57195728
return NULL;
57205729
}
57215730
unwind_locals(cctx, scope->se_local_count);
5731+
if (!cctx->ctx_had_return)
5732+
scope->se_u.se_if.is_had_return = FALSE;
57225733

57235734
if (cctx->ctx_skip == SKIP_UNKNONW)
57245735
{
@@ -5737,7 +5748,9 @@ compile_elseif(char_u *arg, cctx_T *cctx)
57375748
clear_ppconst(&ppconst);
57385749
return NULL;
57395750
}
5740-
if (instr->ga_len == instr_count && ppconst.pp_used == 1)
5751+
if (scope->se_skip_save == SKIP_YES)
5752+
clear_ppconst(&ppconst);
5753+
else if (instr->ga_len == instr_count && ppconst.pp_used == 1)
57415754
{
57425755
// The expression results in a constant.
57435756
// TODO: how about nesting?
@@ -5774,28 +5787,35 @@ compile_else(char_u *arg, cctx_T *cctx)
57745787
return NULL;
57755788
}
57765789
unwind_locals(cctx, scope->se_local_count);
5790+
if (!cctx->ctx_had_return)
5791+
scope->se_u.se_if.is_had_return = FALSE;
5792+
scope->se_u.se_if.is_seen_else = TRUE;
57775793

5778-
// jump from previous block to the end, unless the else block is empty
5779-
if (cctx->ctx_skip == SKIP_UNKNONW)
5794+
if (scope->se_skip_save != SKIP_YES)
57805795
{
5781-
if (compile_jump_to_end(&scope->se_u.se_if.is_end_label,
5796+
// jump from previous block to the end, unless the else block is empty
5797+
if (cctx->ctx_skip == SKIP_UNKNONW)
5798+
{
5799+
if (!cctx->ctx_had_return
5800+
&& compile_jump_to_end(&scope->se_u.se_if.is_end_label,
57825801
JUMP_ALWAYS, cctx) == FAIL)
5783-
return NULL;
5784-
}
5802+
return NULL;
5803+
}
57855804

5786-
if (cctx->ctx_skip == SKIP_UNKNONW)
5787-
{
5788-
if (scope->se_u.se_if.is_if_label >= 0)
5805+
if (cctx->ctx_skip == SKIP_UNKNONW)
57895806
{
5790-
// previous "if" or "elseif" jumps here
5791-
isn = ((isn_T *)instr->ga_data) + scope->se_u.se_if.is_if_label;
5792-
isn->isn_arg.jump.jump_where = instr->ga_len;
5793-
scope->se_u.se_if.is_if_label = -1;
5807+
if (scope->se_u.se_if.is_if_label >= 0)
5808+
{
5809+
// previous "if" or "elseif" jumps here
5810+
isn = ((isn_T *)instr->ga_data) + scope->se_u.se_if.is_if_label;
5811+
isn->isn_arg.jump.jump_where = instr->ga_len;
5812+
scope->se_u.se_if.is_if_label = -1;
5813+
}
57945814
}
5795-
}
57965815

5797-
if (cctx->ctx_skip != SKIP_UNKNONW)
5798-
cctx->ctx_skip = cctx->ctx_skip == SKIP_YES ? SKIP_NOT : SKIP_YES;
5816+
if (cctx->ctx_skip != SKIP_UNKNONW)
5817+
cctx->ctx_skip = cctx->ctx_skip == SKIP_YES ? SKIP_NOT : SKIP_YES;
5818+
}
57995819

58005820
return p;
58015821
}
@@ -5815,6 +5835,8 @@ compile_endif(char_u *arg, cctx_T *cctx)
58155835
}
58165836
ifscope = &scope->se_u.se_if;
58175837
unwind_locals(cctx, scope->se_local_count);
5838+
if (!cctx->ctx_had_return)
5839+
ifscope->is_had_return = FALSE;
58185840

58195841
if (scope->se_u.se_if.is_if_label >= 0)
58205842
{
@@ -5824,8 +5846,11 @@ compile_endif(char_u *arg, cctx_T *cctx)
58245846
}
58255847
// Fill in the "end" label in jumps at the end of the blocks.
58265848
compile_fill_jump_to_end(&ifscope->is_end_label, cctx);
5827-
// TODO: this should restore the value from before the :if
5828-
cctx->ctx_skip = SKIP_UNKNONW;
5849+
cctx->ctx_skip = scope->se_skip_save;
5850+
5851+
// If all the blocks end in :return and there is an :else then the
5852+
// had_return flag is set.
5853+
cctx->ctx_had_return = ifscope->is_had_return && ifscope->is_seen_else;
58295854

58305855
drop_scope(cctx);
58315856
return arg;
@@ -6528,7 +6553,6 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
65286553
char_u *line = NULL;
65296554
char_u *p;
65306555
char *errormsg = NULL; // error message
6531-
int had_return = FALSE;
65326556
cctx_T cctx;
65336557
garray_T *instr;
65346558
int called_emsg_before = called_emsg;
@@ -6648,7 +6672,6 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
66486672
}
66496673
emsg_before = called_emsg;
66506674

6651-
had_return = FALSE;
66526675
CLEAR_FIELD(ea);
66536676
ea.cmdlinep = &line;
66546677
ea.cmd = skipwhite(line);
@@ -6823,6 +6846,22 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
68236846
continue;
68246847
}
68256848

6849+
if (ea.cmdidx != CMD_elseif
6850+
&& ea.cmdidx != CMD_else
6851+
&& ea.cmdidx != CMD_endif
6852+
&& ea.cmdidx != CMD_endfor
6853+
&& ea.cmdidx != CMD_endwhile
6854+
&& ea.cmdidx != CMD_catch
6855+
&& ea.cmdidx != CMD_finally
6856+
&& ea.cmdidx != CMD_endtry)
6857+
{
6858+
if (cctx.ctx_had_return)
6859+
{
6860+
emsg(_("E1095: Unreachable code after :return"));
6861+
goto erret;
6862+
}
6863+
}
6864+
68266865
switch (ea.cmdidx)
68276866
{
68286867
case CMD_def:
@@ -6836,7 +6875,7 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
68366875

68376876
case CMD_return:
68386877
line = compile_return(p, set_return_type, &cctx);
6839-
had_return = TRUE;
6878+
cctx.ctx_had_return = TRUE;
68406879
break;
68416880

68426881
case CMD_let:
@@ -6861,9 +6900,11 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
68616900
break;
68626901
case CMD_elseif:
68636902
line = compile_elseif(p, &cctx);
6903+
cctx.ctx_had_return = FALSE;
68646904
break;
68656905
case CMD_else:
68666906
line = compile_else(p, &cctx);
6907+
cctx.ctx_had_return = FALSE;
68676908
break;
68686909
case CMD_endif:
68696910
line = compile_endif(p, &cctx);
@@ -6874,13 +6915,15 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
68746915
break;
68756916
case CMD_endwhile:
68766917
line = compile_endwhile(p, &cctx);
6918+
cctx.ctx_had_return = FALSE;
68776919
break;
68786920

68796921
case CMD_for:
68806922
line = compile_for(p, &cctx);
68816923
break;
68826924
case CMD_endfor:
68836925
line = compile_endfor(p, &cctx);
6926+
cctx.ctx_had_return = FALSE;
68846927
break;
68856928
case CMD_continue:
68866929
line = compile_continue(p, &cctx);
@@ -6894,12 +6937,15 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
68946937
break;
68956938
case CMD_catch:
68966939
line = compile_catch(p, &cctx);
6940+
cctx.ctx_had_return = FALSE;
68976941
break;
68986942
case CMD_finally:
68996943
line = compile_finally(p, &cctx);
6944+
cctx.ctx_had_return = FALSE;
69006945
break;
69016946
case CMD_endtry:
69026947
line = compile_endtry(p, &cctx);
6948+
cctx.ctx_had_return = FALSE;
69036949
break;
69046950
case CMD_throw:
69056951
line = compile_throw(p, &cctx);
@@ -6944,7 +6990,7 @@ compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx)
69446990
goto erret;
69456991
}
69466992

6947-
if (!had_return)
6993+
if (!cctx.ctx_had_return)
69486994
{
69496995
if (ufunc->uf_ret_type->tt_type != VAR_VOID)
69506996
{

0 commit comments

Comments
 (0)