patch 8.2.2651: Vim9: restoring command modifiers happens after jump

Problem:    Vim9: restoring command modifiers happens after jump.
Solution:   Move the restore instruction to before the jump. (closes #8006)
            Also handle for and while.

Author: brammool

src/testdir/test_vim9_disassemble.vim

@@ -1896,7 +1896,95 @@ def Test_silent()
         '\d PUSHS "error"\_s*' ..
         '\d ECHOERR 1\_s*' ..
         '\d CMDMOD_REV\_s*' ..
-        '\d RETURN 0',
+        '\d\+ RETURN 0',
+        res)
+enddef
+
+def s:SilentIf()
+  silent if 4 == g:five
+  silent elseif 4 == g:five
+  silent endif
+enddef
+
+def Test_silent_if()
+  var res = execute('disass s:SilentIf')
+  assert_match('<SNR>\d*_SilentIf\_s*' ..
+        'silent if 4 == g:five\_s*' ..
+        '\d\+ CMDMOD silent\_s*' ..
+        '\d\+ PUSHNR 4\_s*' ..
+        '\d\+ LOADG g:five\_s*' ..
+        '\d\+ COMPAREANY ==\_s*' ..
+        '\d\+ CMDMOD_REV\_s*' ..
+        '\d\+ JUMP_IF_FALSE -> \d\+\_s*' ..
+        'silent elseif 4 == g:five\_s*' ..
+        '\d\+ JUMP -> \d\+\_s*' ..
+        '\d\+ CMDMOD silent\_s*' ..
+        '\d\+ PUSHNR 4\_s*' ..
+        '\d\+ LOADG g:five\_s*' ..
+        '\d\+ COMPAREANY ==\_s*' ..
+        '\d\+ CMDMOD_REV\_s*' ..
+        '\d\+ JUMP_IF_FALSE -> \d\+\_s*' ..
+        'silent endif\_s*' ..
+        '\d\+ RETURN 0',
+        res)
+enddef
+
+def s:SilentFor()
+  silent for i in [0]
+  silent endfor
+enddef
+
+def Test_silent_for()
+  var res = execute('disass s:SilentFor')
+  assert_match('<SNR>\d*_SilentFor\_s*' ..
+        'silent for i in \[0\]\_s*' ..
+        '\d CMDMOD silent\_s*' ..
+        '\d STORE -1 in $0\_s*' ..
+        '\d PUSHNR 0\_s*' ..
+        '\d NEWLIST size 1\_s*' ..
+        '\d CMDMOD_REV\_s*' ..
+        '5 FOR $0 -> 8\_s*' ..
+        '\d STORE $1\_s*' ..
+        'silent endfor\_s*' ..
+        '\d JUMP -> 5\_s*' ..
+        '8 DROP\_s*' ..
+        '\d RETURN 0\_s*',
+        res)
+enddef
+
+def s:SilentWhile()
+  silent while g:not
+  silent endwhile
+enddef
+
+def Test_silent_while()
+  var res = execute('disass s:SilentWhile')
+  assert_match('<SNR>\d*_SilentWhile\_s*' ..
+        'silent while g:not\_s*' ..
+        '0 CMDMOD silent\_s*' ..
+        '\d LOADG g:not\_s*' ..
+        '\d COND2BOOL\_s*' ..
+        '\d CMDMOD_REV\_s*' ..
+        '\d JUMP_IF_FALSE -> 6\_s*' ..
+
+        'silent endwhile\_s*' ..
+        '\d JUMP -> 0\_s*' ..
+        '6 RETURN 0\_s*',
+         res)
+enddef
+
+def s:SilentReturn(): string
+  silent return "done"
+enddef
+
+def Test_silent_return()
+  var res = execute('disass s:SilentReturn')
+  assert_match('<SNR>\d*_SilentReturn\_s*' ..
+        'silent return "done"\_s*' ..
+        '\d CMDMOD silent\_s*' ..
+        '\d PUSHS "done"\_s*' ..
+        '\d CMDMOD_REV\_s*' ..
+        '\d RETURN',
         res)
 enddef
 
@@ -1924,19 +2012,5 @@ def Test_profiled()
         res)
 enddef
 
-def s:SilentReturn(): string
-  silent return "done"
-enddef
-
-def Test_silent_return()
-  var res = execute('disass s:SilentReturn')
-  assert_match('<SNR>\d*_SilentReturn\_s*' ..
-        'silent return "done"\_s*' ..
-        '\d CMDMOD silent\_s*' ..
-        '\d PUSHS "done"\_s*' ..
-        '\d CMDMOD_REV\_s*' ..
-        '\d RETURN',
-        res)
-enddef
 
 " vim: ts=8 sw=2 sts=2 expandtab tw=80 fdm=marker

src/version.c

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

src/vim9compile.c

@@ -2172,6 +2172,45 @@ generate_undo_cmdmods(cctx_T *cctx)
     return OK;
 }
 
+/*
+ * If an ISN_CMDMOD was just generated drop it.
+ */
+    static void
+drop_cmdmod(cctx_T *cctx)
+{
+    garray_T	*instr = &cctx->ctx_instr;
+
+    // Drop any CMDMOD instruction
+    if (cctx->ctx_has_cmdmod
+	    && ((isn_T *)instr->ga_data)[instr->ga_len - 1].isn_type
+								 == ISN_CMDMOD)
+    {
+	--instr->ga_len;
+	cctx->ctx_has_cmdmod = FALSE;
+    }
+}
+
+/*
+ * Get the index of the current instruction.
+ * This compenstates for a preceding ISN_CMDMOD and ISN_PROF_START.
+ */
+    static int
+current_instr_idx(cctx_T *cctx)
+{
+    garray_T	*instr = &cctx->ctx_instr;
+    int		idx = instr->ga_len;
+
+    if (cctx->ctx_has_cmdmod && ((isn_T *)instr->ga_data)[idx - 1]
+						       .isn_type == ISN_CMDMOD)
+	--idx;
+#ifdef FEAT_PROFILE
+    if (cctx->ctx_profiling && ((isn_T *)instr->ga_data)[idx - 1]
+						   .isn_type == ISN_PROF_START)
+	--idx;
+#endif
+    return idx;
+}
+
 #ifdef FEAT_PROFILE
     static void
 may_generate_prof_end(cctx_T *cctx, int prof_lnum)
@@ -6877,6 +6916,9 @@ compile_if(char_u *arg, cctx_T *cctx)
 	    return NULL;
     }
 
+    // CMDMOD_REV must come before the jump
+    generate_undo_cmdmods(cctx);
+
     scope = new_scope(cctx, IF_SCOPE);
     if (scope == NULL)
 	return NULL;
@@ -6937,24 +6979,36 @@ compile_elseif(char_u *arg, cctx_T *cctx)
     if (scope->se_u.se_if.is_seen_skip_not)
     {
 	// A previous block was executed, skip over expression and bail out.
-	// Do not count the "elseif" for profiling.
-#ifdef FEAT_PROFILE
-	if (cctx->ctx_profiling && ((isn_T *)instr->ga_data)[instr->ga_len - 1]
-						   .isn_type == ISN_PROF_START)
-	    --instr->ga_len;
-#endif
+	// Do not count the "elseif" for profiling and cmdmod
+	instr->ga_len = current_instr_idx(cctx);
+
 	skip_expr_cctx(&p, cctx);
 	return p;
     }
 
     if (cctx->ctx_skip == SKIP_UNKNOWN)
     {
+	int moved_cmdmod = FALSE;
+
+	// Move any CMDMOD instruction to after the jump
+	if (((isn_T *)instr->ga_data)[instr->ga_len - 1].isn_type == ISN_CMDMOD)
+	{
+	    if (ga_grow(instr, 1) == FAIL)
+		return NULL;
+	    ((isn_T *)instr->ga_data)[instr->ga_len] =
+				  ((isn_T *)instr->ga_data)[instr->ga_len - 1];
+	    --instr->ga_len;
+	    moved_cmdmod = TRUE;
+	}
+
 	if (compile_jump_to_end(&scope->se_u.se_if.is_end_label,
 						    JUMP_ALWAYS, cctx) == FAIL)
 	    return NULL;
 	// 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;
+	if (moved_cmdmod)
+	    ++instr->ga_len;
     }
 
     // compile "expr"; if we know it evaluates to FALSE skip the block
@@ -7007,6 +7061,9 @@ compile_elseif(char_u *arg, cctx_T *cctx)
 	if (bool_on_stack(cctx) == FAIL)
 	    return NULL;
 
+	// CMDMOD_REV must come before the jump
+	generate_undo_cmdmods(cctx);
+
 	// "where" is set when ":elseif", "else" or ":endif" is found
 	scope->se_u.se_if.is_if_label = instr->ga_len;
 	generate_JUMP(cctx, JUMP_IF_FALSE, 0);
@@ -7090,6 +7147,7 @@ compile_endif(char_u *arg, cctx_T *cctx)
     garray_T	*instr = &cctx->ctx_instr;
     isn_T	*isn;
 
+    drop_cmdmod(cctx);
     if (scope == NULL || scope->se_type != IF_SCOPE)
     {
 	emsg(_(e_endif_without_if));
@@ -7160,7 +7218,6 @@ compile_for(char_u *arg_start, cctx_T *cctx)
     int		var_count = 0;
     int		semicolon = FALSE;
     size_t	varlen;
-    garray_T	*instr = &cctx->ctx_instr;
     garray_T	*stack = &cctx->ctx_type_stack;
     scope_T	*scope;
     lvar_T	*loop_lvar;	// loop iteration variable
@@ -7230,8 +7287,11 @@ compile_for(char_u *arg_start, cctx_T *cctx)
 	    item_type = vartype->tt_member->tt_member;
     }
 
+    // CMDMOD_REV must come before the FOR instruction
+    generate_undo_cmdmods(cctx);
+
     // "for_end" is set when ":endfor" is found
-    scope->se_u.se_for.fs_top_label = instr->ga_len;
+    scope->se_u.se_for.fs_top_label = current_instr_idx(cctx);
     generate_FOR(cctx, loop_lvar->lv_idx);
 
     arg = arg_start;
@@ -7333,6 +7393,8 @@ compile_endfor(char_u *arg, cctx_T *cctx)
     forscope_T	*forscope;
     isn_T	*isn;
 
+    drop_cmdmod(cctx);
+
     if (scope == NULL || scope->se_type != FOR_SCOPE)
     {
 	emsg(_(e_for));
@@ -7376,20 +7438,14 @@ compile_endfor(char_u *arg, cctx_T *cctx)
 compile_while(char_u *arg, cctx_T *cctx)
 {
     char_u	*p = arg;
-    garray_T	*instr = &cctx->ctx_instr;
     scope_T	*scope;
 
     scope = new_scope(cctx, WHILE_SCOPE);
     if (scope == NULL)
 	return NULL;
 
-    // "endwhile" jumps back here, one before when profiling
-    scope->se_u.se_while.ws_top_label = instr->ga_len;
-#ifdef FEAT_PROFILE
-    if (cctx->ctx_profiling && ((isn_T *)instr->ga_data)[instr->ga_len - 1]
-						   .isn_type == ISN_PROF_START)
-	--scope->se_u.se_while.ws_top_label;
-#endif
+    // "endwhile" jumps back here, one before when profiling or using cmdmods
+    scope->se_u.se_while.ws_top_label = current_instr_idx(cctx);
 
     // compile "expr"
     if (compile_expr0(&p, cctx) == FAIL)
@@ -7403,6 +7459,9 @@ compile_while(char_u *arg, cctx_T *cctx)
     if (bool_on_stack(cctx) == FAIL)
 	return FAIL;
 
+    // CMDMOD_REV must come before the jump
+    generate_undo_cmdmods(cctx);
+
     // "while_end" is set when ":endwhile" is found
     if (compile_jump_to_end(&scope->se_u.se_while.ws_end_label,
 						  JUMP_IF_FALSE, cctx) == FAIL)
@@ -7420,6 +7479,7 @@ compile_endwhile(char_u *arg, cctx_T *cctx)
     scope_T	*scope = cctx->ctx_scope;
     garray_T	*instr = &cctx->ctx_instr;
 
+    drop_cmdmod(cctx);
     if (scope == NULL || scope->se_type != WHILE_SCOPE)
     {
 	emsg(_(e_while));

src/vim9execute.c

@@ -795,6 +795,21 @@ call_ufunc(
     return OK;
 }
 
+/*
+ * If command modifiers were applied restore them.
+ */
+    static void
+may_restore_cmdmod(funclocal_T *funclocal)
+{
+    if (funclocal->floc_restore_cmdmod)
+    {
+	cmdmod.cmod_filter_regmatch.regprog = NULL;
+	undo_cmdmod(&cmdmod);
+	cmdmod = funclocal->floc_save_cmdmod;
+	funclocal->floc_restore_cmdmod = FALSE;
+    }
+}
+
 /*
  * Return TRUE if an error was given or CTRL-C was pressed.
  */
@@ -2719,8 +2734,11 @@ call_def_function(
 			goto failed;
 		    ++idxtv->vval.v_number;
 		    if (list == NULL || idxtv->vval.v_number >= list->lv_len)
+		    {
 			// past the end of the list, jump to "endfor"
 			ectx.ec_iidx = iptr->isn_arg.forloop.for_end;
+			may_restore_cmdmod(&funclocal);
+		    }
 		    else if (list->lv_first == &range_list_item)
 		    {
 			// non-materialized range() list
@@ -2755,9 +2773,12 @@ call_def_function(
 		    CLEAR_POINTER(trycmd);
 		    trycmd->tcd_frame_idx = ectx.ec_frame_idx;
 		    trycmd->tcd_stack_len = ectx.ec_stack.ga_len;
-		    trycmd->tcd_catch_idx = iptr->isn_arg.try.try_ref->try_catch;
-		    trycmd->tcd_finally_idx = iptr->isn_arg.try.try_ref->try_finally;
-		    trycmd->tcd_endtry_idx = iptr->isn_arg.try.try_ref->try_endtry;
+		    trycmd->tcd_catch_idx =
+					  iptr->isn_arg.try.try_ref->try_catch;
+		    trycmd->tcd_finally_idx =
+					iptr->isn_arg.try.try_ref->try_finally;
+		    trycmd->tcd_endtry_idx =
+					 iptr->isn_arg.try.try_ref->try_endtry;
 		}
 		break;
 
@@ -2782,13 +2803,7 @@ call_def_function(
 		{
 		    garray_T	*trystack = &ectx.ec_trystack;
 
-		    if (funclocal.floc_restore_cmdmod)
-		    {
-			cmdmod.cmod_filter_regmatch.regprog = NULL;
-			undo_cmdmod(&cmdmod);
-			cmdmod = funclocal.floc_save_cmdmod;
-			funclocal.floc_restore_cmdmod = FALSE;
-		    }
+		    may_restore_cmdmod(&funclocal);
 		    if (trystack->ga_len > 0)
 		    {
 			trycmd_T    *trycmd = ((trycmd_T *)trystack->ga_data)