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

@@ -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_cstack = eap->cstack;
-	evalarg->eval_cookie = eap->cookie;
+	if (getline_equal(eap->getline, eap->cookie, getsourceline))
+	{
+	    evalarg->eval_getline = eap->getline;
+	    evalarg->eval_cookie = eap->cookie;
+	}
     }
 }
 

src/evalvars.c

@@ -880,13 +880,7 @@ ex_let(exarg_T *eap)
 
 	    if (eap->skip)
 		++emsg_skip;
-	    CLEAR_FIELD(evalarg);
+	    fill_evalarg_from_eap(&evalarg, eap, eap->skip);
-	    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;
-	    }
 	    expr = skipwhite_and_linebreak(expr, &evalarg);
 	    cur_lnum = SOURCING_LNUM;
 	    i = eval0(expr, &rettv, eap, &evalarg);

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);
+	    fill_evalarg_from_eap(&evalarg, eap, skip);
-	    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;
-	    }
-
 	    if ((cstack->cs_lflags & CSL_HAD_LOOP) != 0)
 	    {
 		// Jumping here from a ":continue" or ":endfor": use the

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.

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<func>
+      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<func>
+      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:')

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)
+	// The function may use script variables from the context.
-	{
+	function_using_block_scopes(fp, cstack);
-	    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;
-	}
 
 	if (parse_argument_types(fp, &argtypes, varargs) == FAIL)
 	{

src/version.c

@@ -755,6 +755,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    3216,
 /**/
     3215,
 /**/

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;
+	hi = hash_find(&si->sn_all_vars.dv_hashtab, di->di_key);
-	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);
 	if (!HASHITEM_EMPTY(hi))
 	{
-	    sallvar_T *sav = HI2SAV(hi);
+	    // Variable with this name exists, either in this block or in
-
+	    // another block.
-	    // variable with this name exists in another block
+	    for (sav = HI2SAV(hi); ; sav = sav->sav_next)
-	    while (sav->sav_next != NULL)
+	    {
-		sav = sav->sav_next;
+		if (sav->sav_block_id == si->sn_current_block_id)
-	    sav->sav_next = newsav;
+		{
+		    // 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;
+
+	    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,
+	    *type = typval2type(tv, get_copyID(), &si->sn_type_list, do_member);
-								    do_member);
 	sv->sv_type = *type;
     }