Fix ON TIMEOUT/ON ERROR handlers not firing from inside a function (#3)#6
Open
p0dalirius wants to merge 1 commit intomainfrom
Open
Fix ON TIMEOUT/ON ERROR handlers not firing from inside a function (#3)#6p0dalirius wants to merge 1 commit intomainfrom
p0dalirius wants to merge 1 commit intomainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue
Closes #3
Root Cause
gotoLabel()refuses to run whilem_callStackis non-empty and emits"GOTO is not allowed inside functions", then stops the script. This guard is reasonable for user-writtenGOTOinside a function body (where top-level label indices would be misleading), but it was also being invoked on the internal dispatch path fromnotifyTerminalStateChanged()(forON ERROR) andendExpect(false)(forON TIMEOUT) without distinguishing the two callers. When a timeout or error-locked state occurred while execution was inside aCALLed function, the registered handler was therefore never dispatched — the script terminated with a misleading message the author never triggered themselves.Fix Description
Introduce
ScriptExecutor::unwindToTopLevel()that pops any open exec frames down to each active call frame's capturedexecStackDepthand then callsreturnFromFunction()to restore the call frame's saved parameter variables, looping until the call stack is empty. Invoke this helper at the two internal dispatch points —notifyTerminalStateChanged()(beforegotoLabel(m_onErrorLabel)) andendExpect(false)(beforegotoLabel(m_onTimeoutLabel)) — so the runtime is at top-level script context by the timegotoLabel()runs.gotoLabel()itself is unchanged: user-writtenGOTOinside a function still errors exactly as before.An alternative considered and rejected was to relax
gotoLabel()itself to unwind unconditionally. Rejected because it would silently change the meaning of userGOTOinside function bodies and mask authoring mistakes that the current diagnostic usefully surfaces.How Verified
tests/test_script_executor.cpp::onTimeoutFromInsideFunction. It installs aFakeScreenScreenInterfacestub, setsGLOBAL EXPECT_TIMEOUT 50, registersON TIMEOUT GOTO handler, then calls a function containingEXPECT TEXT "never-present". The test spies onlogMessageandexecutionErrorand asserts thatLOG "caught"(at the handler) fires,LOG "should-not-run"(after theCALL) does not, and no"GOTO is not allowed inside functions"error is emitted.unwindToTopLevel()calls and reran the test — it correctly failed atcaughtLoggedassertion, then passed again once restored. This confirms the test exercises the bug, not a tautology.test_script_lexerandtest_script_parserboth still pass.Test Coverage
Added:
tests/test_script_executor.cpp::onTimeoutFromInsideFunction— end-to-end regression via the publicScriptExecutorAPI with a stubScreenInterface. A new executor test target (test_script_executor) is added toCMakeLists.txtalongside the existing lexer and parser tests.Scope of Change
src/script_executor.cpp,include/5250script/script_executor.h,tests/test_script_executor.cpp(new),CMakeLists.txtgotoLabel()still rejects user-writtenGOTOinside a function.Risk and Rollout
Local change confined to the internal ON-handler dispatch paths. No change to public API shape. Previously, a timeout or error-locked event inside a function always ended the script with an error; now it correctly dispatches the registered handler (or, if no handler is registered, behaves exactly as before).
Notes
Establishes an executor test harness (
FakeScreen+ScriptExecutordriven viaQSignalSpy) that future executor-level tests can reuse.