Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vm: rework the exception handling #1061

Merged
merged 13 commits into from
Feb 5, 2024
172 changes: 172 additions & 0 deletions tests/exception/tfinally6.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
discard """
description: '''
Multiple tests regarding ``finally`` interaction with exception handlers
and raised exceptions.
'''
knownIssue.c js: "The current exception is not properly cleared"
"""

var steps: seq[int]

template test(call: untyped, expect: untyped) =
steps = @[] # reset the step list
{.line.}:
call
doAssert steps == expect, $steps
doAssert getCurrentException().isNil, "current exception wasn't cleared"

# ------ the tests follow ------

proc simpleFinally() =
try:
try:
raise ValueError.newException("a")
finally:
steps.add 1
steps.add 2
except ValueError as e:
steps.add 3
doAssert e.msg == "a"

test simpleFinally(), [1, 3]

proc raiseFromFinally() =
try:
try:
raise ValueError.newException("a")
finally:
steps.add 1
raise ValueError.newException("b")
doAssert false, "unreachable"
except ValueError as e:
# the exception raised in the finally clause overrides the one raised
# earlier
steps.add 2
doAssert e.msg == "b"
doAssert getCurrentException() == e

steps.add 3

test raiseFromFinally(), [1, 2, 3]

proc reraiseFromFinally() =
try:
try:
raise ValueError.newException("a")
finally:
steps.add 1
# abort the exception but immediately re-raise it
raise
doAssert false, "unreachable"
except ValueError as e:
steps.add 2
doAssert e.msg == "a"
doAssert getCurrentException() == e

steps.add 3

test reraiseFromFinally(), [1, 2, 3]

proc exceptionInFinally() =
## Raise, and fully handle, an exception within a finally clause that was
## entered through exceptionl control-flow.
zerbina marked this conversation as resolved.
Show resolved Hide resolved
try:
try:
raise ValueError.newException("a")
finally:
steps.add 1
try:
raise ValueError.newException("b")
except ValueError as e:
steps.add 2
doAssert e.msg == "b"
doAssert getCurrentException() == e

steps.add 3
# the current exception must be the one with which the finally section
# was entered
doAssert getCurrentException().msg == "a"

doAssert false, "unreachable"
except ValueError as e:
steps.add 4
doAssert e.msg == "a"

steps.add 5

test exceptionInFinally(), [1, 2, 3, 4, 5]

proc leaveFinally1() =
## Ensure that exiting a finally clause entered through exceptional control-
## flow via unstructured control-flow (break) works and properly clears the
## current exception.
block exit:
try:
raise ValueError.newException("a")
finally:
steps.add 1
doAssert getCurrentException().msg == "a"
break exit
doAssert false, "unreachable"

steps.add 2

test leaveFinally1(), [1, 2]

proc leaveFinally2() =
## Ensure that aborting an exception raised within a finally clause entered
## through exceptional control-flow doesn't interfere with the original
## exception.
try:
try:
raise ValueError.newException("a")
finally:
block exit:
steps.add 1
try:
raise ValueError.newException("b")
finally:
steps.add 2
# discards the in-flight exception 'b'
break exit
doAssert false, "unreachable"

steps.add 3
# the current exception must be the one the finally was entered with:
doAssert getCurrentException().msg == "a"
# unwinding continues as usual
doAssert false, "unreachable"
except ValueError as e:
steps.add 4
doAssert e.msg == "a"
doAssert getCurrentException() == e

test leaveFinally2(), [1, 2, 3, 4]

proc leaveFinally3(doExit: bool) =
## Ensure that aborting an exception in a finally clause still visits all
## enclosing finally clauses, and that the finally clauses observe the
## correct current exception.
block exit:
try:
try:
raise ValueError.newException("a")
finally:
steps.add 1
try:
if doExit: # obfuscate the break
break exit
finally:
steps.add 2
# the current exception must still be set
doAssert getCurrentException().msg == "a"
doAssert false, "unreachable"
doAssert false, "unreachable"
finally:
steps.add 5
# the finally section is not part of the inner try statement, so the
# current exception is nil
doAssert getCurrentException() == nil
doAssert false, "unreachable"

test leaveFinally3(true), [1, 2, 5]
33 changes: 33 additions & 0 deletions tests/exception/thandle_in_finally.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
discard """
description: '''
Ensure that a finally clause can fully handle an exception by re-raising
and catching it
'''
knownIssue.c js vm: '''
Not implemented properly by the code generator and/or runtime
'''
"""

# XXX: it's questionable whether this really should work / be allowed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking this through myself:

  • on line 17 we decide to 'cancel the current computation', targeting the handler on line 29
  • on line 20 we decide to 'retarget the cancellation handler', now on line 21
  • upon leaving the line 21 cancellation handler, we decide to continue the rest of the computation

Reading through my characterization, I think the 'precise' question is, should a finally be allowed re-parent an existing exception (cancellation)? I believe the answer is "yes, it should" because if not then, the property we'd be after is that finally cannot raise, which seems infeasible/not very useful. I consider this a variant of raising an entirely new exception that overrides the old one, except in this case our 'new exception' 'adopts' the data of the old one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure I completely follow. A finally could be allowed to raise without having to allow re-targeting an existing exception to a handler within the finally (e.g., by disallowing re-raise outside of except branches.

In your model, would line 28 be executed?

Copy link
Collaborator

@saem saem Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your model, would line 28 be executed?

That's a great question, I want to say yes. But that would mean we entered the finally through exceptional means, it handled the exception internally, then the treatment would have to be flipped to non-exceptional.

Which seems to defer the determination of how to leave a finally that much more. Where each control flow branch within a finally can change the handling... ooph.

Perhaps a static restriction is a better bet, because we can always lift the restriction later when/if we're willing to do that extra analysis and think through all the implications (extra accumulated experience in the future will also be beneficial).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which seems to defer the determination of how to leave a finally that much more. Where each control flow branch within a finally can change the handling... ooph.

Yeah, this is the reason why I'd prefer not supporting the re-targeting.

Perhaps a static restriction is a better bet, because we can always lift the restriction later when/if we're willing to do that extra analysis and think through all the implications (extra accumulated experience in the future will also be beneficial).

Okay, in that case I would make a follow up that statically disallows a re-raise outside of except branches.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, and thanks for working through the reasoning with me.


var steps: seq[int]

try:
try:
raise CatchableError.newException("a")
finally:
try:
raise # re-raise the active exception
except CatchableError:
# catch the exception
steps.add 1
# leaving this except handler means that the exception is handled
steps.add 2
doAssert getCurrentException() == nil
# unwinding doesn't continue when leaving the finally
steps.add 3
except CatchableError:
# never reached, since the excepti
zerbina marked this conversation as resolved.
Show resolved Hide resolved
doAssert false, "unreachable"

doAssert steps == [1, 2]
30 changes: 30 additions & 0 deletions tests/exception/tleave_except.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
discard """
description: '''
Ensure that exiting an exception handler via unstructured control-flow
(``break``) works and properly clears the current exception.
'''
output: "done"
knownIssue.js: "The current exception is not properly cleared"
"""

var steps: seq[int]

block exit:
try:
raise ValueError.newException("a")
except ValueError as e:
steps.add 1
if e.msg == "a":
# an unstructured exit of the except block needs to pop the current
# exception
break exit
else:
doAssert false, "unreachable"
doAssert false, "unreachable"

steps.add 2

doAssert getCurrentException() == nil, "current exception was cleared"
doAssert steps == [1, 2]

echo "done" # ensures that the assertion weren't jumped over
zerbina marked this conversation as resolved.
Show resolved Hide resolved
31 changes: 31 additions & 0 deletions tests/exception/traise_and_handle_in_except.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
discard """
description: '''
Ensure that raising and fully handling an exception within an ``except``
branch works.
'''
output: "done"
knownIssue.js: "The current exception is not properly cleared"
"""

var steps: seq[int]

try:
raise ValueError.newException("a")
except ValueError as e:
steps.add 1
try:
raise ValueError.newException("b")
except ValueError as e:
steps.add 2
doAssert e.msg == "b"
doAssert getCurrentException() == e

steps.add 3
# make sure the current exception is still the correct one
doAssert getCurrentException() == e
zerbina marked this conversation as resolved.
Show resolved Hide resolved

steps.add 4
doAssert getCurrentException() == nil, "current exception wasn't cleared"
doAssert steps == [1, 2, 3, 4]

echo "done" # make sure the assertions weren't skipped over
26 changes: 26 additions & 0 deletions tests/exception/twrong_handler.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
discard """
description: '''
Ensure that the correct except branch is jumped to after exiting a try
block through unstructured, but non-exceptional, control-flow.
'''
"""

proc test(doExit: bool): bool =
try:
block exit:
try:
if doExit:
break exit
else:
discard "fall through"
except ValueError:
doAssert false, "unreachable"

# the above except branch previously caught the exception
raise ValueError.newException("a")
except ValueError as e:
doAssert e.msg == "a"
result = true

doAssert test(true)
doAssert test(false)