patch 8.2.2539: Vim9: return from finally block causes a hang

Problem:    Vim9: return from finally block causes a hang.
Solution:   Store both the finally and endtry indexes. (closes #7885)

Author: brammool

src/testdir/test_vim9_disassemble.vim

@@ -422,7 +422,7 @@ def Test_disassemble_try()
   var res = execute('disass s:ScriptFuncTry')
   assert_match('<SNR>\d*_ScriptFuncTry\_s*' ..
         'try\_s*' ..
-        '\d TRY catch -> \d\+, finally -> \d\+\_s*' ..
+        '\d TRY catch -> \d\+, finally -> \d\+, endtry -> \d\+\_s*' ..
         'echo "yes"\_s*' ..
         '\d PUSHS "yes"\_s*' ..
         '\d ECHO 1\_s*' ..
@@ -437,6 +437,7 @@ def Test_disassemble_try()
         '\d\+ PUSHS "no"\_s*' ..
         '\d\+ ECHO 1\_s*' ..
         'finally\_s*' ..
+        '\d\+ FINALLY\_s*' ..
         'throw "end"\_s*' ..
         '\d\+ PUSHS "end"\_s*' ..
         '\d\+ THROW\_s*' ..
@@ -1137,12 +1138,12 @@ def Test_disassemble_for_loop_continue()
         '4 FOR $0 -> 22\_s*' ..
         '5 STORE $1\_s*' ..
         'try\_s*' ..
-        '6 TRY catch -> 17, end -> 20\_s*' ..
+        '6 TRY catch -> 17, endtry -> 20\_s*' ..
         'echo "ok"\_s*' ..
         '7 PUSHS "ok"\_s*' ..
         '8 ECHO 1\_s*' ..
         'try\_s*' ..
-        '9 TRY catch -> 13, end -> 15\_s*' ..
+        '9 TRY catch -> 13, endtry -> 15\_s*' ..
         'echo "deeper"\_s*' ..
         '10 PUSHS "deeper"\_s*' ..
         '11 ECHO 1\_s*' ..

src/testdir/test_vim9_script.vim

@@ -577,6 +577,16 @@ def Test_try_catch_throw()
     counter += 1
   endfor
   assert_equal(4, counter)
+
+  # return in finally after empty catch
+  def ReturnInFinally(): number
+    try
+    finally
+      return 4
+    endtry
+    return 2
+  enddef
+  assert_equal(4, ReturnInFinally())
 enddef
 
 def Test_cnext_works_in_catch()

src/version.c

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

src/vim9.h

@@ -100,6 +100,7 @@ typedef enum {
     ISN_THROW,	    // pop value of stack, store in v:exception
     ISN_PUSHEXC,    // push v:exception
     ISN_CATCH,	    // drop v:exception
+    ISN_FINALLY,    // start of :finally block
     ISN_ENDTRY,	    // take entry off from ec_trystack
     ISN_TRYCONT,    // handle :continue inside a :try statement
 
@@ -208,10 +209,16 @@ typedef struct {
     int	    for_end;	    // position to jump to after done
 } forloop_T;
 
-// arguments to ISN_TRY
+// indirect arguments to ISN_TRY
 typedef struct {
     int	    try_catch;	    // position to jump to on throw
     int	    try_finally;    // :finally or :endtry position to jump to
+    int	    try_endtry;	    // :endtry position to jump to
+} tryref_T;
+
+// arguments to ISN_TRY
+typedef struct {
+    tryref_T *try_ref;
 } try_T;
 
 // arguments to ISN_TRYCONT

src/vim9compile.c

@@ -7518,10 +7518,17 @@ compile_try(char_u *arg, cctx_T *cctx)
 
     if (cctx->ctx_skip != SKIP_YES)
     {
-	// "catch" is set when the first ":catch" is found.
-	// "finally" is set when ":finally" or ":endtry" is found
+	isn_T	*isn;
+
+	// "try_catch" is set when the first ":catch" is found or when no catch
+	// is found and ":finally" is found.
+	// "try_finally" is set when ":finally" is found
+	// "try_endtry" is set when ":endtry" is found
 	try_scope->se_u.se_try.ts_try_label = instr->ga_len;
-	if (generate_instr(cctx, ISN_TRY) == NULL)
+	if ((isn = generate_instr(cctx, ISN_TRY)) == NULL)
+	    return NULL;
+	isn->isn_arg.try.try_ref = ALLOC_CLEAR_ONE(tryref_T);
+	if (isn->isn_arg.try.try_ref == NULL)
 	    return NULL;
     }
 
@@ -7577,8 +7584,8 @@ compile_catch(char_u *arg, cctx_T *cctx UNUSED)
 
 	// End :try or :catch scope: set value in ISN_TRY instruction
 	isn = ((isn_T *)instr->ga_data) + scope->se_u.se_try.ts_try_label;
-	if (isn->isn_arg.try.try_catch == 0)
-	    isn->isn_arg.try.try_catch = instr->ga_len;
+	if (isn->isn_arg.try.try_ref->try_catch == 0)
+	    isn->isn_arg.try.try_ref->try_catch = instr->ga_len;
 	if (scope->se_u.se_try.ts_catch_label != 0)
 	{
 	    // Previous catch without match jumps here
@@ -7670,7 +7677,7 @@ compile_finally(char_u *arg, cctx_T *cctx)
 
     // End :catch or :finally scope: set value in ISN_TRY instruction
     isn = ((isn_T *)instr->ga_data) + scope->se_u.se_try.ts_try_label;
-    if (isn->isn_arg.try.try_finally != 0)
+    if (isn->isn_arg.try.try_ref->try_finally != 0)
     {
 	emsg(_(e_finally_dup));
 	return NULL;
@@ -7688,14 +7695,19 @@ compile_finally(char_u *arg, cctx_T *cctx)
     compile_fill_jump_to_end(&scope->se_u.se_try.ts_end_label,
 							     this_instr, cctx);
 
-    isn->isn_arg.try.try_finally = this_instr;
+    // If there is no :catch then an exception jumps to :finally.
+    if (isn->isn_arg.try.try_ref->try_catch == 0)
+	isn->isn_arg.try.try_ref->try_catch = this_instr;
+    isn->isn_arg.try.try_ref->try_finally = this_instr;
     if (scope->se_u.se_try.ts_catch_label != 0)
     {
 	// Previous catch without match jumps here
 	isn = ((isn_T *)instr->ga_data) + scope->se_u.se_try.ts_catch_label;
 	isn->isn_arg.jump.jump_where = this_instr;
 	scope->se_u.se_try.ts_catch_label = 0;
     }
+    if (generate_instr(cctx, ISN_FINALLY) == NULL)
+	return NULL;
 
     // TODO: set index in ts_finally_label jumps
 
@@ -7731,8 +7743,8 @@ compile_endtry(char_u *arg, cctx_T *cctx)
     try_isn = ((isn_T *)instr->ga_data) + scope->se_u.se_try.ts_try_label;
     if (cctx->ctx_skip != SKIP_YES)
     {
-	if (try_isn->isn_arg.try.try_catch == 0
-				      && try_isn->isn_arg.try.try_finally == 0)
+	if (try_isn->isn_arg.try.try_ref->try_catch == 0
+				      && try_isn->isn_arg.try.try_ref->try_finally == 0)
 	{
 	    emsg(_(e_missing_catch_or_finally));
 	    return NULL;
@@ -7751,12 +7763,6 @@ compile_endtry(char_u *arg, cctx_T *cctx)
 	compile_fill_jump_to_end(&scope->se_u.se_try.ts_end_label,
 							  instr->ga_len, cctx);
 
-	// End :catch or :finally scope: set value in ISN_TRY instruction
-	if (try_isn->isn_arg.try.try_catch == 0)
-	    try_isn->isn_arg.try.try_catch = instr->ga_len;
-	if (try_isn->isn_arg.try.try_finally == 0)
-	    try_isn->isn_arg.try.try_finally = instr->ga_len;
-
 	if (scope->se_u.se_try.ts_catch_label != 0)
 	{
 	    // Last catch without match jumps here
@@ -7770,11 +7776,9 @@ compile_endtry(char_u *arg, cctx_T *cctx)
 
     if (cctx->ctx_skip != SKIP_YES)
     {
-	if (try_isn->isn_arg.try.try_finally == 0)
-	    // No :finally encountered, use the try_finaly field to point to
-	    // ENDTRY, so that TRYCONT can jump there.
-	    try_isn->isn_arg.try.try_finally = instr->ga_len;
-
+	// End :catch or :finally scope: set instruction index in ISN_TRY
+	// instruction
+	try_isn->isn_arg.try.try_ref->try_endtry = instr->ga_len;
 	if (cctx->ctx_skip != SKIP_YES
 				   && generate_instr(cctx, ISN_ENDTRY) == NULL)
 	    return NULL;
@@ -8867,6 +8871,10 @@ delete_instr(isn_T *isn)
 	    vim_free(isn->isn_arg.script.scriptref);
 	    break;
 
+	case ISN_TRY:
+	    vim_free(isn->isn_arg.try.try_ref);
+	    break;
+
 	case ISN_2BOOL:
 	case ISN_2STRING:
 	case ISN_2STRING_ANY:
@@ -8899,6 +8907,7 @@ delete_instr(isn_T *isn)
 	case ISN_ENDTRY:
 	case ISN_EXECCONCAT:
 	case ISN_EXECUTE:
+	case ISN_FINALLY:
 	case ISN_FOR:
 	case ISN_GETITEM:
 	case ISN_JUMP:
@@ -8943,7 +8952,6 @@ delete_instr(isn_T *isn)
 	case ISN_STRINDEX:
 	case ISN_STRSLICE:
 	case ISN_THROW:
-	case ISN_TRY:
 	case ISN_TRYCONT:
 	case ISN_UNLETINDEX:
 	case ISN_UNLETRANGE:

src/vim9execute.c

@@ -26,8 +26,9 @@
 typedef struct {
     int	    tcd_frame_idx;	// ec_frame_idx at ISN_TRY
     int	    tcd_stack_len;	// size of ectx.ec_stack at ISN_TRY
-    int	    tcd_catch_idx;	// instruction of the first catch
-    int	    tcd_finally_idx;	// instruction of the finally block or :endtry
+    int	    tcd_catch_idx;	// instruction of the first :catch or :finally
+    int	    tcd_finally_idx;	// instruction of the :finally block or zero
+    int	    tcd_endtry_idx;	// instruction of the :endtry
     int	    tcd_caught;		// catch block entered
     int	    tcd_cont;		// :continue encountered, jump here
     int	    tcd_return;		// when TRUE return from end of :finally
@@ -2517,10 +2518,9 @@ call_def_function(
 							+ trystack->ga_len - 1;
 		    if (trycmd != NULL
 				  && trycmd->tcd_frame_idx == ectx.ec_frame_idx
-				  && ectx.ec_instr[trycmd->tcd_finally_idx]
-						       .isn_type != ISN_ENDTRY)
+				  && trycmd->tcd_finally_idx != 0)
 		    {
-			// jump to ":finally"
+			// jump to ":finally" once
 			ectx.ec_iidx = trycmd->tcd_finally_idx;
 			trycmd->tcd_return = TRUE;
 		    }
@@ -2665,8 +2665,9 @@ 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_catch;
-		    trycmd->tcd_finally_idx = iptr->isn_arg.try.try_finally;
+		    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;
 
@@ -2731,13 +2732,26 @@ call_def_function(
 			trycmd = ((trycmd_T *)trystack->ga_data)
 							+ trystack->ga_len - i;
 			trycmd->tcd_cont = iidx;
-			iidx = trycmd->tcd_finally_idx;
+			iidx = trycmd->tcd_finally_idx == 0
+			    ? trycmd->tcd_endtry_idx : trycmd->tcd_finally_idx;
 		    }
 		    // jump to :finally or :endtry of current try statement
 		    ectx.ec_iidx = iidx;
 		}
 		break;
 
+	    case ISN_FINALLY:
+		{
+		    garray_T	*trystack = &ectx.ec_trystack;
+		    trycmd_T    *trycmd = ((trycmd_T *)trystack->ga_data)
+							+ trystack->ga_len - 1;
+
+		    // Reset the index to avoid a return statement jumps here
+		    // again.
+		    trycmd->tcd_finally_idx = 0;
+		    break;
+		}
+
 	    // end of ":try" block
 	    case ISN_ENDTRY:
 		{
@@ -4348,11 +4362,17 @@ ex_disassemble(exarg_T *eap)
 		{
 		    try_T *try = &iptr->isn_arg.try;
 
-		    smsg("%4d TRY catch -> %d, %s -> %d", current,
-				 try->try_catch,
-				 instr[try->try_finally].isn_type == ISN_ENDTRY
-							   ? "end" : "finally",
-				 try->try_finally);
+		    if (try->try_ref->try_finally == 0)
+			smsg("%4d TRY catch -> %d, endtry -> %d",
+				current,
+				try->try_ref->try_catch,
+				try->try_ref->try_endtry);
+		    else
+			smsg("%4d TRY catch -> %d, finally -> %d, endtry -> %d",
+				current,
+				try->try_ref->try_catch,
+				try->try_ref->try_finally,
+				try->try_ref->try_endtry);
 		}
 		break;
 	    case ISN_CATCH:
@@ -4369,6 +4389,9 @@ ex_disassemble(exarg_T *eap)
 				      trycont->tct_where);
 		}
 		break;
+	    case ISN_FINALLY:
+		smsg("%4d FINALLY", current);
+		break;
 	    case ISN_ENDTRY:
 		smsg("%4d ENDTRY", current);
 		break;