patch 8.2.2321: Vim9: cannot nest closures

Problem:    Vim9: cannot nest closures.
Solution:   Add the nesting level to ISN_LOADOUTER and ISN_STOREOUTER.
            (closes #7150, closes #7635)

src/structs.h

@@ -1978,6 +1978,8 @@ struct partial_S
     // For a compiled closure: the arguments and local variables.
     garray_T	*pt_ectx_stack;	    // where to find local vars
     int		pt_ectx_frame;	    // index of function frame in uf_ectx_stack
+    garray_T	*pt_outer_stack;    // pt_ectx_stack one level up
+    int		pt_outer_frame;	    // pt_ectx_frame one level up.
     funcstack_T	*pt_funcstack;	    // copy of stack, used after context
 				    // function returns
 

src/testdir/test_vim9_disassemble.vim

@@ -567,17 +567,17 @@ def Test_disassemble_closure()
   var res = execute('disass g:Append')
   assert_match('<lambda>\d\_s*' ..
         'local ..= arg\_s*' ..
-        '\d LOADOUTER $0\_s*' ..
+        '\d LOADOUTER level 1 $0\_s*' ..
         '\d LOAD arg\[-1\]\_s*' ..
         '\d CONCAT\_s*' ..
-        '\d STOREOUTER $0\_s*' ..
+        '\d STOREOUTER level 1 $0\_s*' ..
         '\d RETURN 0',
         res)
 
   res = execute('disass g:Get')
   assert_match('<lambda>\d\_s*' ..
         'return local\_s*' ..
-        '\d LOADOUTER $0\_s*' ..
+        '\d LOADOUTER level 1 $0\_s*' ..
         '\d RETURN',
         res)
 

src/testdir/test_vim9_func.vim

@@ -1812,6 +1812,16 @@ def Test_list_lambda()
   assert_match('def <lambda>\d\+(_: any, ...): number\n1  return 0\n   enddef', body)
 enddef
 
+def DoFilterThis(a: string): list<string>
+  # closure nested inside another closure using argument
+  var Filter = (l) => filter(l, (_, v) => stridx(v, a) == 0)
+  return ['x', 'y', 'a', 'x2', 'c']->Filter()
+enddef
+
+def Test_nested_closure_using_argument()
+  assert_equal(['x', 'x2'], DoFilterThis('x'))
+enddef
+
 func Test_silent_echo()
   CheckScreendump
 

src/version.c

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

src/vim9.h

@@ -33,7 +33,7 @@ typedef enum {
     ISN_LOADWDICT,  // push w: dict
     ISN_LOADTDICT,  // push t: dict
     ISN_LOADS,	    // push s: variable isn_arg.loadstore
-    ISN_LOADOUTER,  // push variable from outer scope isn_arg.number
+    ISN_LOADOUTER,  // push variable from outer scope isn_arg.outer
     ISN_LOADSCRIPT, // push script-local variable isn_arg.script.
     ISN_LOADOPT,    // push option isn_arg.string
     ISN_LOADENV,    // push environment variable isn_arg.string
@@ -47,7 +47,7 @@ typedef enum {
     ISN_STOREW,	    // pop into window-local variable isn_arg.string
     ISN_STORET,	    // pop into tab-local variable isn_arg.string
     ISN_STORES,	    // pop into script variable isn_arg.loadstore
-    ISN_STOREOUTER,  // pop variable into outer scope isn_arg.number
+    ISN_STOREOUTER,  // pop variable into outer scope isn_arg.outer
     ISN_STORESCRIPT, // pop into script variable isn_arg.script
     ISN_STOREOPT,    // pop into option isn_arg.string
     ISN_STOREENV,    // pop into environment variable isn_arg.string
@@ -303,6 +303,12 @@ typedef struct {
     int		unp_semicolon;	// last item gets list of remainder
 } unpack_T;
 
+// arguments to ISN_LOADOUTER and ISN_STOREOUTER
+typedef struct {
+    int		outer_idx;	// index
+    int		outer_depth;	// nesting level, stack frames to go up
+} outer_T;
+
 /*
  * Instruction
  */
@@ -342,6 +348,7 @@ struct isn_S {
 	put_T		    put;
 	cmod_T		    cmdmod;
 	unpack_T	    unpack;
+	outer_T		    outer;
     } isn_arg;
 };
 

src/vim9compile.c

@@ -108,7 +108,7 @@ typedef struct {
     char_u	*lv_name;
     type_T	*lv_type;
     int		lv_idx;		// index of the variable on the stack
-    int		lv_from_outer;	// when TRUE using ctx_outer scope
+    int		lv_from_outer;	// nesting level, using ctx_outer scope
     int		lv_const;	// when TRUE cannot be assigned to
     int		lv_arg;		// when TRUE this is an argument
 } lvar_T;
@@ -149,7 +149,7 @@ static void delete_def_function_contents(dfunc_T *dfunc, int mark_deleted);
 
 /*
  * Lookup variable "name" in the local scope and return it in "lvar".
- * "lvar->lv_from_outer" is set accordingly.
+ * "lvar->lv_from_outer" is incremented accordingly.
  * If "lvar" is NULL only check if the variable can be found.
  * Return FAIL if not found.
  */
@@ -172,7 +172,7 @@ lookup_local(char_u *name, size_t len, lvar_T *lvar, cctx_T *cctx)
 	    if (lvar != NULL)
 	    {
 		*lvar = *lvp;
-		lvar->lv_from_outer = FALSE;
+		lvar->lv_from_outer = 0;
 	    }
 	    return OK;
 	}
@@ -186,7 +186,7 @@ lookup_local(char_u *name, size_t len, lvar_T *lvar, cctx_T *cctx)
 	    if (lvar != NULL)
 	    {
 		cctx->ctx_outer_used = TRUE;
-		lvar->lv_from_outer = TRUE;
+		++lvar->lv_from_outer;
 	    }
 	    return OK;
 	}
@@ -258,7 +258,7 @@ arg_exists(
 	if (arg_exists(name, len, idxp, type, gen_load_outer, cctx->ctx_outer)
 									 == OK)
 	{
-	    *gen_load_outer = TRUE;
+	    ++*gen_load_outer;
 	    return OK;
 	}
     }
@@ -1175,6 +1175,23 @@ generate_STORE(cctx_T *cctx, isntype_T isn_type, int idx, char_u *name)
     return OK;
 }
 
+/*
+ * Generate an ISN_STOREOUTER instruction.
+ */
+    static int
+generate_STOREOUTER(cctx_T *cctx, int idx, int level)
+{
+    isn_T	*isn;
+
+    RETURN_OK_IF_SKIP(cctx);
+    if ((isn = generate_instr_drop(cctx, ISN_STOREOUTER, 1)) == NULL)
+	return FAIL;
+    isn->isn_arg.outer.outer_idx = idx;
+    isn->isn_arg.outer.outer_depth = level;
+
+    return OK;
+}
+
 /*
  * Generate an ISN_STORENR instruction (short for ISN_PUSHNR + ISN_STORE)
  */
@@ -1233,6 +1250,27 @@ generate_LOAD(
     return OK;
 }
 
+/*
+ * Generate an ISN_LOADOUTER instruction
+ */
+    static int
+generate_LOADOUTER(
+	cctx_T	    *cctx,
+	int	    idx,
+	int	    nesting,
+	type_T	    *type)
+{
+    isn_T	*isn;
+
+    RETURN_OK_IF_SKIP(cctx);
+    if ((isn = generate_instr_type(cctx, ISN_LOADOUTER, type)) == NULL)
+	return FAIL;
+    isn->isn_arg.outer.outer_idx = idx;
+    isn->isn_arg.outer.outer_depth = nesting;
+
+    return OK;
+}
+
 /*
  * Generate an ISN_LOADV instruction for v:var.
  */
@@ -1439,6 +1477,11 @@ generate_FUNCREF(cctx_T *cctx, ufunc_T *ufunc)
     isn->isn_arg.funcref.fr_func = ufunc->uf_dfunc_idx;
     cctx->ctx_has_closure = 1;
 
+    // if the referenced function is a closure, it may use items further up in
+    // the nested context, including this one.
+    if (ufunc->uf_flags & FC_CLOSURE)
+	cctx->ctx_ufunc->uf_flags |= FC_CLOSURE;
+
     if (ga_grow(stack, 1) == FAIL)
 	return FAIL;
     ((type_T **)stack->ga_data)[stack->ga_len] =
@@ -2589,7 +2632,7 @@ compile_load(
 	size_t	    len = end - *arg;
 	int	    idx;
 	int	    gen_load = FALSE;
-	int	    gen_load_outer = FALSE;
+	int	    gen_load_outer = 0;
 
 	name = vim_strnsave(*arg, end - *arg);
 	if (name == NULL)
@@ -2597,7 +2640,7 @@ compile_load(
 
 	if (arg_exists(*arg, len, &idx, &type, &gen_load_outer, cctx) == OK)
 	{
-	    if (!gen_load_outer)
+	    if (gen_load_outer == 0)
 		gen_load = TRUE;
 	}
 	else
@@ -2608,8 +2651,8 @@ compile_load(
 	    {
 		type = lvar.lv_type;
 		idx = lvar.lv_idx;
-		if (lvar.lv_from_outer)
+		if (lvar.lv_from_outer != 0)
-		    gen_load_outer = TRUE;
+		    gen_load_outer = lvar.lv_from_outer;
 		else
 		    gen_load = TRUE;
 	    }
@@ -2631,9 +2674,9 @@ compile_load(
 	}
 	if (gen_load)
 	    res = generate_LOAD(cctx, ISN_LOAD, idx, NULL, type);
-	if (gen_load_outer)
+	if (gen_load_outer > 0)
 	{
-	    res = generate_LOAD(cctx, ISN_LOADOUTER, idx, NULL, type);
+	    res = generate_LOADOUTER(cctx, idx, gen_load_outer, type);
 	    cctx->ctx_outer_used = TRUE;
 	}
     }
@@ -5120,9 +5163,9 @@ generate_loadvar(
 	    generate_LOADV(cctx, name + 2, TRUE);
 	    break;
 	case dest_local:
-	    if (lvar->lv_from_outer)
+	    if (lvar->lv_from_outer > 0)
-		generate_LOAD(cctx, ISN_LOADOUTER, lvar->lv_idx,
+		generate_LOADOUTER(cctx, lvar->lv_idx, lvar->lv_from_outer,
-							   NULL, type);
+									 type);
 	    else
 		generate_LOAD(cctx, ISN_LOAD, lvar->lv_idx, NULL, type);
 	    break;
@@ -6178,7 +6221,7 @@ compile_assignment(char_u *arg, exarg_T *eap, cmdidx_T cmdidx, cctx_T *cctx)
 
 		// optimization: turn "var = 123" from ISN_PUSHNR +
 		// ISN_STORE into ISN_STORENR
-		if (!lhs.lhs_lvar->lv_from_outer
+		if (lhs.lhs_lvar->lv_from_outer == 0
 				&& instr->ga_len == instr_count + 1
 				&& isn->isn_type == ISN_PUSHNR)
 		{
@@ -6190,9 +6233,9 @@ compile_assignment(char_u *arg, exarg_T *eap, cmdidx_T cmdidx, cctx_T *cctx)
 		    if (stack->ga_len > 0)
 			--stack->ga_len;
 		}
-		else if (lhs.lhs_lvar->lv_from_outer)
+		else if (lhs.lhs_lvar->lv_from_outer > 0)
-		    generate_STORE(cctx, ISN_STOREOUTER,
+		    generate_STOREOUTER(cctx, lhs.lhs_lvar->lv_idx,
-						   lhs.lhs_lvar->lv_idx, NULL);
+						  lhs.lhs_lvar->lv_from_outer);
 		else
 		    generate_STORE(cctx, ISN_STORE, lhs.lhs_lvar->lv_idx, NULL);
 	    }

src/vim9execute.c

@@ -60,6 +60,8 @@ struct ectx_S {
 
     garray_T	*ec_outer_stack;    // stack used for closures
     int		ec_outer_frame;	    // stack frame in ec_outer_stack
+    garray_T	*ec_outer_up_stack;   // ec_outer_stack one level up
+    int		ec_outer_up_frame;    // ec_outer_frame one level up
 
     garray_T	ec_trystack;	// stack of trycmd_T values
     int		ec_in_catch;	// when TRUE in catch or finally block
@@ -149,6 +151,8 @@ exe_newlist(int count, ectx_T *ectx)
 
 /*
  * Call compiled function "cdf_idx" from compiled code.
+ * This adds a stack frame and sets the instruction pointer to the start of the
+ * called function.
  *
  * Stack has:
  * - current arguments (already there)
@@ -248,6 +252,7 @@ call_dfunc(int cdf_idx, int argcount_arg, ectx_T *ectx)
     STACK_TV_BOT(2)->vval.v_string = (void *)ectx->ec_outer_stack;
     STACK_TV_BOT(3)->vval.v_number = ectx->ec_outer_frame;
     STACK_TV_BOT(4)->vval.v_number = ectx->ec_frame_idx;
+    // TODO: save ec_outer_up_stack as well?
     ectx->ec_frame_idx = ectx->ec_stack.ga_len;
 
     // Initialize local variables
@@ -266,6 +271,15 @@ call_dfunc(int cdf_idx, int argcount_arg, ectx_T *ectx)
     {
 	ectx->ec_outer_stack = ufunc->uf_partial->pt_ectx_stack;
 	ectx->ec_outer_frame = ufunc->uf_partial->pt_ectx_frame;
+	ectx->ec_outer_up_stack = ufunc->uf_partial->pt_outer_stack;
+	ectx->ec_outer_up_frame = ufunc->uf_partial->pt_outer_frame;
+    }
+    else if (ufunc->uf_flags & FC_CLOSURE)
+    {
+	ectx->ec_outer_stack = &ectx->ec_stack;
+	ectx->ec_outer_frame = ectx->ec_frame_idx;
+	ectx->ec_outer_up_stack = ectx->ec_outer_stack;
+	ectx->ec_outer_up_frame = ectx->ec_outer_frame;
     }
 
     // Set execution state to the start of the called function.
@@ -417,6 +431,8 @@ handle_closure_in_use(ectx_T *ectx, int free_arguments)
 		pt->pt_funcstack = funcstack;
 		pt->pt_ectx_stack = &funcstack->fs_ga;
 		pt->pt_ectx_frame = ectx->ec_frame_idx - top;
+		pt->pt_outer_stack = ectx->ec_outer_stack;
+		pt->pt_outer_frame = ectx->ec_outer_frame;
 	    }
 	}
     }
@@ -598,6 +614,9 @@ call_bfunc(int func_idx, int argcount, ectx_T *ectx)
 
 /*
  * Execute a user defined function.
+ * If the function is compiled this will add a stack frame and set the
+ * instruction pointer at the start of the function.
+ * Otherwise the function is called here.
  * "iptr" can be used to replace the instruction with a more efficient one.
  */
     static int
@@ -743,11 +762,18 @@ call_partial(typval_T *tv, int argcount_arg, ectx_T *ectx)
 
 	if (pt->pt_func != NULL)
 	{
+	    int frame_idx = ectx->ec_frame_idx;
 	    int ret = call_ufunc(pt->pt_func, argcount, ectx, NULL);
 
-	    // closure may need the function context where it was defined
+	    if (ectx->ec_frame_idx != frame_idx)
+	    {
+		// call_dfunc() added a stack frame, closure may need the
+		// function context where it was defined.
 		ectx->ec_outer_stack = pt->pt_ectx_stack;
 		ectx->ec_outer_frame = pt->pt_ectx_frame;
+		ectx->ec_outer_up_stack = pt->pt_outer_stack;
+		ectx->ec_outer_up_frame = pt->pt_outer_frame;
+	    }
 
 	    return ret;
 	}
@@ -1041,6 +1067,8 @@ fill_partial_and_closure(partial_T *pt, ufunc_T *ufunc, ectx_T *ectx)
 	// variables in the current stack.
 	pt->pt_ectx_stack = &ectx->ec_stack;
 	pt->pt_ectx_frame = ectx->ec_frame_idx;
+	pt->pt_outer_stack = ectx->ec_outer_stack;
+	pt->pt_outer_frame = ectx->ec_outer_frame;
 
 	// If this function returns and the closure is still
 	// being used, we need to make a copy of the context
@@ -1220,17 +1248,23 @@ call_def_function(
 	    // TODO: is this always the right way?
 	    ectx.ec_outer_stack = &current_ectx->ec_stack;
 	    ectx.ec_outer_frame = current_ectx->ec_frame_idx;
+	    ectx.ec_outer_up_stack = current_ectx->ec_outer_stack;
+	    ectx.ec_outer_up_frame = current_ectx->ec_outer_frame;
 	}
 	else
 	{
 	    ectx.ec_outer_stack = partial->pt_ectx_stack;
 	    ectx.ec_outer_frame = partial->pt_ectx_frame;
+	    ectx.ec_outer_up_stack = partial->pt_outer_stack;
+	    ectx.ec_outer_up_frame = partial->pt_outer_frame;
 	}
     }
     else if (ufunc->uf_partial != NULL)
     {
 	ectx.ec_outer_stack = ufunc->uf_partial->pt_ectx_stack;
 	ectx.ec_outer_frame = ufunc->uf_partial->pt_ectx_frame;
+	ectx.ec_outer_up_stack = ufunc->uf_partial->pt_outer_stack;
+	ectx.ec_outer_up_frame = ufunc->uf_partial->pt_outer_frame;
     }
 
     // dummy frame entries
@@ -1514,11 +1548,30 @@ call_def_function(
 
 	    // load variable or argument from outer scope
 	    case ISN_LOADOUTER:
+		{
+		    typval_T	*stack;
+		    int		depth = iptr->isn_arg.outer.outer_depth;
+
 		    if (GA_GROW(&ectx.ec_stack, 1) == FAIL)
 			goto failed;
-		copy_tv(STACK_OUT_TV_VAR(iptr->isn_arg.number),
+		    if (depth <= 1)
+			stack = ((typval_T *)ectx.ec_outer_stack->ga_data)
+							 + ectx.ec_outer_frame;
+		    else if (depth == 2)
+			stack = ((typval_T *)ectx.ec_outer_up_stack->ga_data)
+						      + ectx.ec_outer_up_frame;
+		    else
+		    {
+			SOURCING_LNUM = iptr->isn_lnum;
+			iemsg("LOADOUTER level > 2 not supported yet");
+			goto failed;
+		    }
+
+		    copy_tv(stack + STACK_FRAME_SIZE
+					       + iptr->isn_arg.outer.outer_idx,
 							      STACK_TV_BOT(0));
 		    ++ectx.ec_stack.ga_len;
+		}
 		break;
 
 	    // load v: variable
@@ -1719,7 +1772,8 @@ call_def_function(
 	    // store variable or argument in outer scope
 	    case ISN_STOREOUTER:
 		--ectx.ec_stack.ga_len;
-		tv = STACK_OUT_TV_VAR(iptr->isn_arg.number);
+		// TODO: use outer_depth
+		tv = STACK_OUT_TV_VAR(iptr->isn_arg.outer.outer_idx);
 		clear_tv(tv);
 		*tv = *STACK_TV_BOT(0);
 		break;
@@ -3622,19 +3676,29 @@ ex_disassemble(exarg_T *eap)
 					    (varnumber_T)(iptr->isn_arg.number));
 		break;
 	    case ISN_LOAD:
-	    case ISN_LOADOUTER:
 		{
-		    char *add = iptr->isn_type == ISN_LOAD ? "" : "OUTER";
-
 		    if (iptr->isn_arg.number < 0)
-			smsg("%4d LOAD%s arg[%lld]", current, add,
+			smsg("%4d LOAD arg[%lld]", current,
 				(varnumber_T)(iptr->isn_arg.number
 							  + STACK_FRAME_SIZE));
 		    else
-			smsg("%4d LOAD%s $%lld", current, add,
+			smsg("%4d LOAD $%lld", current,
 					  (varnumber_T)(iptr->isn_arg.number));
 		}
 		break;
+	    case ISN_LOADOUTER:
+		{
+		    if (iptr->isn_arg.number < 0)
+			smsg("%4d LOADOUTER level %d arg[%d]", current,
+				iptr->isn_arg.outer.outer_depth,
+				iptr->isn_arg.outer.outer_idx
+							  + STACK_FRAME_SIZE);
+		    else
+			smsg("%4d LOADOUTER level %d $%d", current,
+					      iptr->isn_arg.outer.outer_depth,
+					      iptr->isn_arg.outer.outer_idx);
+		}
+		break;
 	    case ISN_LOADV:
 		smsg("%4d LOADV v:%s", current,
 				       get_vim_var_name(iptr->isn_arg.number));
@@ -3699,16 +3763,22 @@ ex_disassemble(exarg_T *eap)
 		break;
 
 	    case ISN_STORE:
+		if (iptr->isn_arg.number < 0)
+		    smsg("%4d STORE arg[%lld]", current,
+				      iptr->isn_arg.number + STACK_FRAME_SIZE);
+		else
+		    smsg("%4d STORE $%lld", current, iptr->isn_arg.number);
+		break;
 	    case ISN_STOREOUTER:
 		{
-		    char *add = iptr->isn_type == ISN_STORE ? "" : "OUTER";
-
 		if (iptr->isn_arg.number < 0)
-		    smsg("%4d STORE%s arg[%lld]", current, add,
+		    smsg("%4d STOREOUTEr level %d arg[%d]", current,
-			 (varnumber_T)(iptr->isn_arg.number + STACK_FRAME_SIZE));
+			    iptr->isn_arg.outer.outer_depth,
+			    iptr->isn_arg.outer.outer_idx + STACK_FRAME_SIZE);
 		else
-		    smsg("%4d STORE%s $%lld", current, add,
+		    smsg("%4d STOREOUTER level %d $%d", current,
-					    (varnumber_T)(iptr->isn_arg.number));
+			    iptr->isn_arg.outer.outer_depth,
+			    iptr->isn_arg.outer.outer_idx);
 		}
 		break;
 	    case ISN_STOREV: