From 2eb6fc3b52148f961e804ec2be361d531ff770d8 Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Sun, 25 Jul 2021 14:13:53 +0200 Subject: [PATCH] patch 8.2.3216: Vim9: crash when using variable in a loop at script level Problem: Vim9: crash when using variable in a loop at script level. Solution: Do not clear the variable if a function was defined. Do not create a new entry in sn_var_vals every time. (closes #8628) --- src/eval.c | 10 ++-- src/evalvars.c | 8 +-- src/ex_eval.c | 18 ++----- src/structs.h | 3 ++ src/testdir/test_vim9_script.vim | 28 +++++++++++ src/userfunc.c | 58 +++++++++++++--------- src/version.c | 2 + src/vim9script.c | 84 ++++++++++++++++++++------------ 8 files changed, 135 insertions(+), 76 deletions(-) diff --git a/src/eval.c b/src/eval.c index fdee0089b..fc2266225 100644 --- a/src/eval.c +++ b/src/eval.c @@ -146,10 +146,14 @@ fill_evalarg_from_eap(evalarg_T *evalarg, exarg_T *eap, int skip) { CLEAR_FIELD(*evalarg); evalarg->eval_flags = skip ? 0 : EVAL_EVALUATE; - if (eap != NULL && getline_equal(eap->getline, eap->cookie, getsourceline)) + if (eap != NULL) { - evalarg->eval_getline = eap->getline; - evalarg->eval_cookie = eap->cookie; + evalarg->eval_cstack = eap->cstack; + if (getline_equal(eap->getline, eap->cookie, getsourceline)) + { + evalarg->eval_getline = eap->getline; + evalarg->eval_cookie = eap->cookie; + } } } diff --git a/src/evalvars.c b/src/evalvars.c index 1f781dad7..534163c54 100644 --- a/src/evalvars.c +++ b/src/evalvars.c @@ -880,13 +880,7 @@ ex_let(exarg_T *eap) if (eap->skip) ++emsg_skip; - CLEAR_FIELD(evalarg); - evalarg.eval_flags = eap->skip ? 0 : EVAL_EVALUATE; - if (getline_equal(eap->getline, eap->cookie, getsourceline)) - { - evalarg.eval_getline = eap->getline; - evalarg.eval_cookie = eap->cookie; - } + fill_evalarg_from_eap(&evalarg, eap, eap->skip); expr = skipwhite_and_linebreak(expr, &evalarg); cur_lnum = SOURCING_LNUM; i = eval0(expr, &rettv, eap, &evalarg); diff --git a/src/ex_eval.c b/src/ex_eval.c index 6491955d6..827f9cdcd 100644 --- a/src/ex_eval.c +++ b/src/ex_eval.c @@ -972,9 +972,6 @@ leave_block(cstack_T *cstack) hide_script_var(si, i, func_defined); } - // TODO: is this needed? - cstack->cs_script_var_len[cstack->cs_idx] = si->sn_var_vals.ga_len; - if (cstack->cs_idx == 0) si->sn_current_block_id = 0; else @@ -1178,6 +1175,8 @@ ex_while(exarg_T *eap) { scriptitem_T *si = SCRIPT_ITEM(current_sctx.sc_sid); int i; + int func_defined = cstack->cs_flags[cstack->cs_idx] + & CSF_FUNC_DEF; // Any variables defined in the previous round are no longer // visible. @@ -1192,10 +1191,8 @@ ex_while(exarg_T *eap) if (sv->sv_name != NULL) // Remove a variable declared inside the block, if it // still exists, from sn_vars. - hide_script_var(si, i, FALSE); + hide_script_var(si, i, func_defined); } - cstack->cs_script_var_len[cstack->cs_idx] = - si->sn_var_vals.ga_len; } } cstack->cs_flags[cstack->cs_idx] = @@ -1222,14 +1219,7 @@ ex_while(exarg_T *eap) /* * ":for var in list-expr" */ - CLEAR_FIELD(evalarg); - evalarg.eval_flags = skip ? 0 : EVAL_EVALUATE; - if (getline_equal(eap->getline, eap->cookie, getsourceline)) - { - evalarg.eval_getline = eap->getline; - evalarg.eval_cookie = eap->cookie; - } - + fill_evalarg_from_eap(&evalarg, eap, skip); if ((cstack->cs_lflags & CSL_HAD_LOOP) != 0) { // Jumping here from a ":continue" or ":endfor": use the diff --git a/src/structs.h b/src/structs.h index 265fe0253..bf8102cd7 100644 --- a/src/structs.h +++ b/src/structs.h @@ -1888,6 +1888,9 @@ typedef struct { // used when compiling a :def function, NULL otherwise cctx_T *eval_cctx; + // used when executing commands from a script, NULL otherwise + cstack_T *eval_cstack; + // Used to collect lines while parsing them, so that they can be // concatenated later. Used when "eval_ga.ga_itemsize" is not zero. // "eval_ga.ga_data" is a list of pointers to lines. diff --git a/src/testdir/test_vim9_script.vim b/src/testdir/test_vim9_script.vim index 8c80e1801..14684c096 100644 --- a/src/testdir/test_vim9_script.vim +++ b/src/testdir/test_vim9_script.vim @@ -2592,6 +2592,34 @@ def Test_for_loop() CheckDefAndScriptSuccess(lines) enddef +def Test_for_loop_with_closure() + var lines =<< trim END + var flist: list + for i in range(5) + var inloop = i + flist[i] = () => inloop + endfor + for i in range(5) + assert_equal(4, flist[i]()) + endfor + END + CheckDefAndScriptSuccess(lines) + + lines =<< trim END + var flist: list + for i in range(5) + var inloop = i + flist[i] = () => { + return inloop + } + endfor + for i in range(5) + assert_equal(4, flist[i]()) + endfor + END + CheckDefAndScriptSuccess(lines) +enddef + def Test_for_loop_fails() CheckDefAndScriptFailure2(['for '], 'E1097:', 'E690:') CheckDefAndScriptFailure2(['for x'], 'E1097:', 'E690:') diff --git a/src/userfunc.c b/src/userfunc.c index be1eaa957..7c9634742 100644 --- a/src/userfunc.c +++ b/src/userfunc.c @@ -613,6 +613,35 @@ is_function_cmd(char_u **cmd) return FALSE; } +/* + * Called when defining a function: The context may be needed for script + * variables declared in a block that is visible now but not when the function + * is compiled or called later. + */ + static void +function_using_block_scopes(ufunc_T *fp, cstack_T *cstack) +{ + if (cstack != NULL && cstack->cs_idx >= 0) + { + int count = cstack->cs_idx + 1; + int i; + + fp->uf_block_ids = ALLOC_MULT(int, count); + if (fp->uf_block_ids != NULL) + { + mch_memmove(fp->uf_block_ids, cstack->cs_block_id, + sizeof(int) * count); + fp->uf_block_depth = count; + } + + // Set flag in each block to indicate a function was defined. This + // is used to keep the variable when leaving the block, see + // hide_script_var(). + for (i = 0; i <= cstack->cs_idx; ++i) + cstack->cs_flags[i] |= CSF_FUNC_DEF; + } +} + /* * Read the body of a function, put every line in "newlines". * This stops at "}", "endfunction" or "enddef". @@ -1195,6 +1224,8 @@ lambda_function_body( ufunc->uf_script_ctx.sc_lnum += sourcing_lnum_top; set_function_type(ufunc); + function_using_block_scopes(ufunc, evalarg->eval_cstack); + rettv->vval.v_partial = pt; rettv->v_type = VAR_PARTIAL; ufunc = NULL; @@ -1442,6 +1473,8 @@ get_lambda_tv( fp->uf_script_ctx = current_sctx; fp->uf_script_ctx.sc_lnum += SOURCING_LNUM - newlines.ga_len; + function_using_block_scopes(fp, evalarg->eval_cstack); + pt->pt_func = fp; pt->pt_refcount = 1; rettv->vval.v_partial = pt; @@ -1459,6 +1492,7 @@ theend: vim_free(tofree2); if (types_optional) ga_clear_strings(&argtypes); + return OK; errret: @@ -4313,28 +4347,8 @@ define_function(exarg_T *eap, char_u *name_arg) // error messages are for the first function line SOURCING_LNUM = sourcing_lnum_top; - if (cstack != NULL && cstack->cs_idx >= 0) - { - int count = cstack->cs_idx + 1; - int i; - - // The block context may be needed for script variables declared in - // a block that is visible now but not when the function is called - // later. - fp->uf_block_ids = ALLOC_MULT(int, count); - if (fp->uf_block_ids != NULL) - { - mch_memmove(fp->uf_block_ids, cstack->cs_block_id, - sizeof(int) * count); - fp->uf_block_depth = count; - } - - // Set flag in each block to indicate a function was defined. This - // is used to keep the variable when leaving the block, see - // hide_script_var(). - for (i = 0; i <= cstack->cs_idx; ++i) - cstack->cs_flags[i] |= CSF_FUNC_DEF; - } + // The function may use script variables from the context. + function_using_block_scopes(fp, cstack); if (parse_argument_types(fp, &argtypes, varargs) == FAIL) { diff --git a/src/version.c b/src/version.c index f12b9dc04..ea1411dff 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 */ +/**/ + 3216, /**/ 3215, /**/ diff --git a/src/vim9script.c b/src/vim9script.c index 605d0be96..9b4a2c0fb 100644 --- a/src/vim9script.c +++ b/src/vim9script.c @@ -758,47 +758,72 @@ update_vim9_script_var( { scriptitem_T *si = SCRIPT_ITEM(current_sctx.sc_sid); hashitem_T *hi; - svar_T *sv; + svar_T *sv = NULL; if (create) { - sallvar_T *newsav; + sallvar_T *newsav; + sallvar_T *sav = NULL; // Store a pointer to the typval_T, so that it can be found by index // instead of using a hastab lookup. if (ga_grow(&si->sn_var_vals, 1) == FAIL) return; - sv = ((svar_T *)si->sn_var_vals.ga_data) + si->sn_var_vals.ga_len; - newsav = (sallvar_T *)alloc_clear( - sizeof(sallvar_T) + STRLEN(di->di_key)); - if (newsav == NULL) - return; - - sv->sv_tv = &di->di_tv; - sv->sv_const = (flags & ASSIGN_FINAL) ? ASSIGN_FINAL - : (flags & ASSIGN_CONST) ? ASSIGN_CONST : 0; - sv->sv_export = is_export; - newsav->sav_var_vals_idx = si->sn_var_vals.ga_len; - ++si->sn_var_vals.ga_len; - STRCPY(&newsav->sav_key, di->di_key); - sv->sv_name = newsav->sav_key; - newsav->sav_di = di; - newsav->sav_block_id = si->sn_current_block_id; - - hi = hash_find(&si->sn_all_vars.dv_hashtab, newsav->sav_key); + hi = hash_find(&si->sn_all_vars.dv_hashtab, di->di_key); if (!HASHITEM_EMPTY(hi)) { - sallvar_T *sav = HI2SAV(hi); + // Variable with this name exists, either in this block or in + // another block. + for (sav = HI2SAV(hi); ; sav = sav->sav_next) + { + if (sav->sav_block_id == si->sn_current_block_id) + { + // variable defined in a loop, re-use the entry + sv = ((svar_T *)si->sn_var_vals.ga_data) + + sav->sav_var_vals_idx; + // unhide the variable + if (sv->sv_tv == &sav->sav_tv) + { + clear_tv(&sav->sav_tv); + sv->sv_tv = &di->di_tv; + sav->sav_di = di; + } + break; + } + if (sav->sav_next == NULL) + break; + } + } + + if (sv == NULL) + { + // Variable not defined or not defined in current block: Add a + // svar_T and create a new sallvar_T. + sv = ((svar_T *)si->sn_var_vals.ga_data) + si->sn_var_vals.ga_len; + newsav = (sallvar_T *)alloc_clear( + sizeof(sallvar_T) + STRLEN(di->di_key)); + if (newsav == NULL) + return; - // variable with this name exists in another block - while (sav->sav_next != NULL) - sav = sav->sav_next; - sav->sav_next = newsav; + sv->sv_tv = &di->di_tv; + sv->sv_const = (flags & ASSIGN_FINAL) ? ASSIGN_FINAL + : (flags & ASSIGN_CONST) ? ASSIGN_CONST : 0; + sv->sv_export = is_export; + newsav->sav_var_vals_idx = si->sn_var_vals.ga_len; + ++si->sn_var_vals.ga_len; + STRCPY(&newsav->sav_key, di->di_key); + sv->sv_name = newsav->sav_key; + newsav->sav_di = di; + newsav->sav_block_id = si->sn_current_block_id; + + if (HASHITEM_EMPTY(hi)) + // new variable name + hash_add(&si->sn_all_vars.dv_hashtab, newsav->sav_key); + else if (sav != NULL) + // existing name in a new block, append to the list + sav->sav_next = newsav; } - else - // new variable name - hash_add(&si->sn_all_vars.dv_hashtab, newsav->sav_key); } else { @@ -807,8 +832,7 @@ update_vim9_script_var( if (sv != NULL) { if (*type == NULL) - *type = typval2type(tv, get_copyID(), &si->sn_type_list, - do_member); + *type = typval2type(tv, get_copyID(), &si->sn_type_list, do_member); sv->sv_type = *type; } -- 2.40.0