Skip to content

Commit

Permalink
Fix vim9script 9 not restoring v:exception/etc after catching exceptions
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ychin committed Jan 9, 2025
1 parent c97e869 commit 4525553
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 15 deletions.
2 changes: 1 addition & 1 deletion src/ex_eval.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions src/proto/ex_eval.pro
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
20 changes: 15 additions & 5 deletions src/testdir/test_stacktrace.vim
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -97,16 +97,28 @@ 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([
\ #{funcref: funcref('Test_vstacktrace'), lnum: 97, filepath: s:thisfile},
\ #{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)
Expand All @@ -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
53 changes: 53 additions & 0 deletions src/testdir/test_vim9_script.vim
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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


Expand Down
28 changes: 19 additions & 9 deletions src/vim9execute.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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)
Expand Down Expand Up @@ -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;
}
}
Expand Down

0 comments on commit 4525553

Please sign in to comment.