Problem: When an if condition is constant true, the else block is skipped during compilation. However, any elseif condition within that skipped block was still compiled. This caused errors when the condition referenced variables only declared in the skipped block or when it checked for missing features (like has('clipboard')).
Solution: In compile_elseif(), check if ctx_skip is already set to SKIP_YES. If so, skip compiling the entire condition expression using skip_expr_cctx() and avoid processing it further.
fixes: #19160
https://github.com/vim/vim/pull/20021
(2 files)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@chrisbra commented on this pull request.
In src/testdir/test_vim9_script.vim:
> @@ -5933,4 +5933,20 @@ def Test_g_variable_shadow_multi_context() unlet g:F enddef +" Test that dead elseif conditions are skipped without errors +def Test_skip_elseif_dead_branch() + if true + echo 'ok' + else + var x = 0 + if x > 0 + echo 'pos' + # This condition should never be compiled + elseif x < 0 + echo 'neg' + endif + endif +enddef +defcompile
Please follow the existing pattern in this file here:
see the first function:
def Test_vim9script_feature() # example from the help, here the feature is always present var lines =<< trim END " old style comment if !has('vim9script') " legacy commands would go here finish endif vim9script # Vim9 script commands go here g:didit = true END v9.CheckScriptSuccess(lines) assert_equal(true, g:didit) unlet g:didit enddef
That is, use the here document to write your test scripts and then use the v9.CheckScriptSuccess() functions to check the result on sourcing.
Please also add a test for the opposite issue, to make sure the else if is executed after the false expression.
def Test_skip_elseif_after_dead_then() var lines =<< trim END vim9script g:reached = 0 if false echo 'never' elseif true g:reached = 1 endif END v9.CheckScriptSuccess(lines) assert_equal(1, g:reached) unlet g:reached
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@sahinf pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@sahinf commented on this pull request.
In src/testdir/test_vim9_script.vim:
> @@ -5933,4 +5933,20 @@ def Test_g_variable_shadow_multi_context() unlet g:F enddef +" Test that dead elseif conditions are skipped without errors +def Test_skip_elseif_dead_branch() + if true + echo 'ok' + else + var x = 0 + if x > 0 + echo 'pos' + # This condition should never be compiled + elseif x < 0 + echo 'neg' + endif + endif +enddef +defcompile
Oops done! I verified that the first test fails without the patch; though the 2nd test passes without it. Also we have to be inside a def block in the heredoc to hit the bug.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Request changes. The fix introduces a regression: if false / elseif true
no longer enters the elseif block. The condition guard
(cctx->ctx_skip == SKIP_YES) is too broad; it also triggers for the normal
case where the preceding if branch was statically false, which is exactly
when the elseif condition must be compiled and evaluated.
A one-line change to check the saved outer-scope skip state
(scope->se_skip_save == SKIP_YES) fixes both issue #19160 and avoids the
regression.
The original reproduction from #19160 requires a Vim built with -clipboard.
I reproduced the essence of the bug using a constant-true outer if and a
nested if/elseif inside the dead else branch. Without the PR the compiler
still tries to compile the elseif condition inside the dead branch.
With the PR applied (master patch only) the original case no longer errors,
confirming the fix resolves the reported issue.
Test case:
vim9script var result = '' def Bug() if false result = 'A' elseif true result = 'B (expected)' else result = 'C (bug)' endif enddef Bug() writefile([result], '/tmp/bug.out')
master: writes B (expected) (correct).dead-branch): writes C (bug) — the elseif branch is neverelse.In compile_elseif() (src/vim9cmds.c), ctx_skip == SKIP_YES arises in two
distinct situations:
if condition was a compile-time constant false. In thisctx_skip to SKIP_UNKNOWN andif/else scope is dead (e.g. we are inside theelse branch of a statically-true outer if). In this case theThe PR conflates situations 1 and 2 by returning early whenever
ctx_skip == SKIP_YES. This breaks situation 1: the elseif condition is
skipped and no JUMP_IF_FALSE / end-label jumps are emitted, so the elseif
body effectively disappears from the generated code.
Test_if_elseif_else only uses dynamic conditions
(if what == 1 / elseif what == 2 / ...), and Test_if_const_expr doesn't
exercise the if false / elseif <...> pattern. That's why the PR's
make test_vim9_script run appears green despite the regression.
The compiler already records the outer ctx_skip on the if-scope via
scope->se_skip_save (see compile_if() in src/vim9cmds.c and existing
checks at lines 709, 793, 858 that already use this field to detect a dead
outer scope). Using the same signal in compile_elseif() cleanly
distinguishes situation 2 from situation 1:
- if (cctx->ctx_skip == SKIP_YES) + if (scope->se_skip_save == SKIP_YES) { - // Already skipped a block so skip this condition without compiling + // The enclosing outer block is dead, so skip this elseif condition + // without compiling it. skip_expr_cctx(&p, cctx); return p; }
I verified this locally:
if false / elseif true / else regression: returns B (expected).make test_vim9_script: 162 tests pass, including the PR's new testsTest_skip_elseif_dead_branch and Test_skip_elseif_with_has_in_dead_branch.To prevent future breakage, please add a test that covers the non-dead
if false / elseif true / else pattern as well:
def Test_if_false_elseif_true_still_takes_elseif()
var lines =<< trim END vim9script
var result = ''
def F()
if false
result = 'A'
elseif true
result = 'B'
else
result = 'C'
endif
enddef
F()
assert_equal('B', result)
END
v9.CheckScriptSuccess(lines)
enddefThe test comment says "inside a :def function" but the test body actually
puts everything inside a script with a nested :def. Consider adjusting
the comment (or dropping it) for accuracy. Not blocking.
Request changes. The current patch fixes #19160 but breaks a common
existing pattern (if false / elseif true). Switching the guard to
scope->se_skip_save == SKIP_YES resolves both concerns with a minimal
change.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@sahinf pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
thanks both
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()