patch 9.1.0999: Vim9: leaking finished exception

Problem:  leaking finished exception
          (after v9.1.0984)
Solution: use finish_exception to clean up caught exceptions
          (Yee Cheng Chin)

In Vimscript, v:exception/throwpoint/stacktrace are supposed to reflect
the currently caught exception, and be popped after the exception is
finished (via endtry, finally, or a thrown exception inside catch).
Vim9script does not handle this properly, and leaks them instead. This
is clearly visible when launching GVim with menu enabled.  A caught
exception inside the s:BMShow() in menu.vim would show up when querying
`v:stacktrace` even though the exception was already caught and handled.

To fix this, just use the same functionality as Vimscript by calling
`finish_exception` to properly restore the states. Note that this
assumes `current_exception` is always the same as `caught_stack` which
believe should be the case.

Added tests for this. Also fix up test_stacktrace to properly test the
stack restore behavior where we have nested exceptions in catch blocks
and to also test the vim9script functionality properly.

- Also, remove its dependency on explicitly checking a line number in
  runtest.vim which is a very fragile way to write tests as any minor
  change in runtest.vim (shared among all tests) would require changing
  test_stacktrace.vim. We don't actually need such granularity in the
  test.

closes: #16413

Signed-off-by: Yee Cheng Chin <ychin.git@gmail.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>

diff --git a/src/ex_eval.c b/src/ex_eval.c
index e996ce2a12b01be6f0113c18112d6830df7762e0..3c865c396511da8083de47e6d90ddee80778d3a4 100644 (file)
--- a/src/ex_eval.c
+++ b/src/ex_eval.c
@@ -718,7 +718,7 @@ catch_exception(except_T *excp)
 /*
  * Remove an exception from the caught stack.
  */
-    static void
+    void
 finish_exception(except_T *excp)
 {
     if (excp != caught_stack)

diff --git a/src/proto/ex_eval.pro b/src/proto/ex_eval.pro
index 979d6fb8f499c337600b4454d404bc11718c8940..feffc8fb023f78588745ca40d2aad4e51e359104 100644 (file)
--- a/src/proto/ex_eval.pro
+++ b/src/proto/ex_eval.pro
@@ -11,6 +11,7 @@ char *get_exception_string(void *value, except_type_T type, char_u *cmdname, int
 int throw_exception(void *value, except_type_T type, char_u *cmdname);
 void discard_current_exception(void);
 void catch_exception(except_T *excp);
+void finish_exception(except_T *excp);
 void exception_state_save(exception_state_T *estate);
 void exception_state_restore(exception_state_T *estate);
 void exception_state_clear(void);

diff --git a/src/testdir/test_stacktrace.vim b/src/testdir/test_stacktrace.vim
index 5c71d5023d3524c1769d17022fed6e3f609d72dc..fc8510a2d1477c6140c74f38ed730787d1deace5 100644 (file)
--- a/src/testdir/test_stacktrace.vim
+++ b/src/testdir/test_stacktrace.vim
@@ -10,7 +10,7 @@ func Filepath(name)
 endfunc
 
 func AssertStacktrace(expect, actual)
-  call assert_equal(#{lnum: 617, filepath: Filepath('runtest.vim')}, a:actual[0])
+  call assert_equal(Filepath('runtest.vim'), a:actual[0]['filepath'])
   call assert_equal(a:expect, a:actual[-len(a:expect):])
 endfunc
 
@@ -97,6 +97,12 @@ func Test_vstacktrace()
     call Xfunc1()
   catch
     let stacktrace = v:stacktrace
+    try
+      call Xfunc1()
+    catch
+      let stacktrace_inner = v:stacktrace
+    endtry
+    let stacktrace_after = v:stacktrace " should be restored by the exception stack to the previous one
   endtry
   call assert_equal([], v:stacktrace)
   call AssertStacktrace([
@@ -104,9 +110,15 @@ func Test_vstacktrace()
        \ #{funcref: funcref('Xfunc1'), lnum: 5, filepath: Filepath('Xscript1')},
        \ #{funcref: funcref('Xfunc2'), lnum: 4, filepath: Filepath('Xscript2')},
        \ ], stacktrace)
+  call AssertStacktrace([
+       \ #{funcref: funcref('Test_vstacktrace'), lnum: 101, filepath: s:thisfile},
+       \ #{funcref: funcref('Xfunc1'), lnum: 5, filepath: Filepath('Xscript1')},
+       \ #{funcref: funcref('Xfunc2'), lnum: 4, filepath: Filepath('Xscript2')},
+       \ ], stacktrace_inner)
+  call assert_equal(stacktrace, stacktrace_after)
 endfunc
 
-func Test_zzz_stacktrace_vim9()
+func Test_stacktrace_vim9()
   let lines =<< trim [SCRIPT]
   var stacktrace = getstacktrace()
   assert_notequal([], stacktrace)
@@ -122,11 +134,9 @@ func Test_zzz_stacktrace_vim9()
       assert_true(has_key(d, 'lnum'))
     endfor
   endtry
+  call assert_equal([], v:stacktrace)
   [SCRIPT]
   call v9.CheckDefSuccess(lines)
-  " FIXME: v:stacktrace is not cleared after the exception handling, and this
-  " test has to be run as the last one because of this.
-  " call assert_equal([], v:stacktrace)
 endfunc
 
 " vim: shiftwidth=2 sts=2 expandtab

diff --git a/src/testdir/test_vim9_script.vim b/src/testdir/test_vim9_script.vim
index 550c725dd7641520de620bac34409e0d1cc29ed6..b29f3cd80c4558357a0ea489d6b77a479453bb2a 100644 (file)
--- a/src/testdir/test_vim9_script.vim
+++ b/src/testdir/test_vim9_script.vim
@@ -945,6 +945,47 @@ def Test_try_catch_throw()
       endif
   END
   v9.CheckDefAndScriptSuccess(lines)
+
+  # test that the v:exception stacks are correctly restored
+  try
+    try
+      throw 101
+    catch
+      assert_equal('101', v:exception)
+      try
+      catch
+      finally
+        assert_equal('101', v:exception) # finally shouldn't clear if it doesn't own it
+      endtry
+      assert_equal('101', v:exception)
+      throw 102 # Re-throw inside catch block
+    endtry
+  catch
+    assert_equal('102', v:exception)
+    try
+      throw 103 # throw inside nested exception stack
+    catch
+      assert_equal('103', v:exception)
+    endtry
+    assert_equal('102', v:exception) # restored stack
+  finally
+    assert_equal('', v:exception) # finally should clear if it owns the exception
+  endtry
+  try
+    try
+      throw 104
+    catch
+      try
+        exec 'nonexistent_cmd' # normal exception inside nested exception stack
+      catch
+        assert_match('E492:', v:exception)
+      endtry
+      eval [][0] # normal exception inside catch block
+    endtry
+  catch
+    assert_match('E684:', v:exception)
+  endtry
+  assert_equal('', v:exception) # All exceptions properly popped
 enddef
 
 def Test_unreachable_after()
@@ -1417,11 +1458,23 @@ def Test_throw_line_number()
     eval 2 + 2
     throw 'exception'
   enddef
+  def Func2()
+    eval 1 + 1
+    eval 2 + 2
+    eval 3 + 3
+    throw 'exception'
+  enddef
   try
     Func()
   catch /exception/
+    try
+      Func2()
+    catch /exception/
+      assert_match('line 4', v:throwpoint)
+    endtry
     assert_match('line 3', v:throwpoint)
   endtry
+  assert_match('', v:throwpoint)
 enddef
 
 

diff --git a/src/version.c b/src/version.c
index 49a3523a21b9a6e3c34e56440abfc7a1f09ed00b..46b7e355958f42671d8b942f2b635a2c96037aa5 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -704,6 +704,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    999,
 /**/
     998,
 /**/

diff --git a/src/vim9execute.c b/src/vim9execute.c
index d6962804b361d72b9a7e3874f6c65fc5f9492b19..c7f0e673b22845c56920e080c1f5828cd874426c 100644 (file)
--- a/src/vim9execute.c
+++ b/src/vim9execute.c
@@ -3281,7 +3281,15 @@ exec_instructions(ectx_T *ectx)
                trycmd_T *trycmd = ((trycmd_T *)trystack->ga_data)
                                                        + trystack->ga_len - 1;
                if (trycmd->tcd_frame_idx == ectx->ec_frame_idx)
-                   trycmd->tcd_caught = FALSE;
+               {
+                   if (trycmd->tcd_caught)
+                   {
+                       // Inside a "catch" we need to first discard the caught
+                       // exception.
+                       finish_exception(caught_stack);
+                       trycmd->tcd_caught = FALSE;
+                   }
+               }
            }
        }
 
@@ -4972,6 +4980,12 @@ exec_instructions(ectx_T *ectx)
                    // Reset the index to avoid a return statement jumps here
                    // again.
                    trycmd->tcd_finally_idx = 0;
+                   if (trycmd->tcd_caught)
+                   {
+                       // discard the exception
+                       finish_exception(caught_stack);
+                       trycmd->tcd_caught = FALSE;
+                   }
                    break;
                }
 
@@ -4986,12 +5000,10 @@ exec_instructions(ectx_T *ectx)
                    trycmd = ((trycmd_T *)trystack->ga_data) + trystack->ga_len;
                    if (trycmd->tcd_did_throw)
                        did_throw = TRUE;
-                   if (trycmd->tcd_caught && current_exception != NULL)
+                   if (trycmd->tcd_caught)
                    {
                        // discard the exception
-                       if (caught_stack == current_exception)
-                           caught_stack = caught_stack->caught;
-                       discard_current_exception();
+                       finish_exception(caught_stack);
                    }
 
                    if (trycmd->tcd_return)
@@ -5040,12 +5052,10 @@ exec_instructions(ectx_T *ectx)
                    {
                        trycmd_T    *trycmd = ((trycmd_T *)trystack->ga_data)
                                                        + trystack->ga_len - 1;
-                       if (trycmd->tcd_caught && current_exception != NULL)
+                       if (trycmd->tcd_caught)
                        {
                            // discard the exception
-                           if (caught_stack == current_exception)
-                               caught_stack = caught_stack->caught;
-                           discard_current_exception();
+                           finish_exception(caught_stack);
                            trycmd->tcd_caught = FALSE;
                        }
                    }