From 45255533b184d650aff2297d00269b3af60e13c5 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Wed, 8 Jan 2025 21:42:22 -0800 Subject: [PATCH] Fix vim9script 9 not restoring v:exception/etc after catching exceptions 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. --- src/ex_eval.c | 2 +- src/proto/ex_eval.pro | 1 + src/testdir/test_stacktrace.vim | 20 +++++++++--- src/testdir/test_vim9_script.vim | 53 ++++++++++++++++++++++++++++++++ src/vim9execute.c | 28 +++++++++++------ 5 files changed, 89 insertions(+), 15 deletions(-) diff --git a/src/ex_eval.c b/src/ex_eval.c index e996ce2a12b01b..3c865c396511da 100644 --- 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 979d6fb8f499c3..feffc8fb023f78 100644 --- 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 5c71d5023d3524..b8473697e861e1 100644 --- 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(a:actual[0]['filepath'], Filepath('runtest.vim')) 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 82f808862db174..a2aee1572fa6a7 100644 --- a/src/testdir/test_vim9_script.vim +++ b/src/testdir/test_vim9_script.vim @@ -924,6 +924,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() @@ -1396,11 +1437,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 + try + Func2() + catch /exception/ + assert_match('line 4', v:throwpoint) + endtry Func() catch /exception/ assert_match('line 3', v:throwpoint) endtry + assert_match('', v:throwpoint) enddef diff --git a/src/vim9execute.c b/src/vim9execute.c index d6962804b361d7..c7f0e673b22845 100644 --- 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; } }