-
Notifications
You must be signed in to change notification settings - Fork 323
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
Panics can be caught from TCO loops #9705
Changes from all commits
4553f8e
ff5f913
f91512b
f0d488b
cce3c03
c515188
8539997
d86c7b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,14 @@ foreign js throw_js_str = """ | |
foreign js throw_js_arr = """ | ||
throw [1,2,3]; | ||
|
||
fn_with_tco x = | ||
if x == 1 then Panic.throw "fn_with_tco" else | ||
@Tail_Call fn_with_tco x+1 | ||
|
||
global_wrapper_fn = | ||
fn_with_tco 0 | ||
|
||
|
||
add_specs suite_builder = | ||
suite_builder.group "No Method Errors" group_builder-> | ||
group_builder.specify "should be recoverable" <| | ||
|
@@ -361,6 +369,41 @@ add_specs suite_builder = | |
c2 . should_equal "finalizer" | ||
v2.to_vector . should_equal [1, 2] | ||
|
||
group_builder.specify "should be caught in a loop" <| | ||
fn_without_tco x = | ||
if x == 1 then Panic.throw "fn_without_tco" else | ||
fn_without_tco x+1 | ||
|
||
p = Panic.catch Any handler=(.payload) <| fn_without_tco 0 | ||
p . should_equal "fn_without_tco" | ||
|
||
group_builder.specify "should be caught in a TCO loop" <| | ||
fn_with_tco x = | ||
if x == 1 then Panic.throw "fn_with_tco" else | ||
@Tail_Call fn_with_tco x+1 | ||
|
||
p = Panic.catch Any handler=(.payload) <| fn_with_tco 0 | ||
p . should_equal "fn_with_tco" | ||
|
||
# The same test as the one before, but it does not use the `<|` function. | ||
group_builder.specify "should be caught in a TCO loop with a wrapper fn" <| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As suggested by @JaroslavTulach, I have added this test. And as expected, this test succeeds, but the former ("should be caught in a TCO loop") fails. It seems that your suspicion, @JaroslavTulach, was right - this is probably caused by the fact that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JaroslavTulach EDIT: The test added in cce3c03 suggests otherwise. In that commit, I have introduced a test that doesn't use the |
||
fn_with_tco x = | ||
if x == 1 then Panic.throw "fn_with_tco" else | ||
@Tail_Call fn_with_tco x+1 | ||
|
||
wrapper_fn = | ||
fn_with_tco 0 | ||
|
||
p = Panic.catch Any handler=(.payload) wrapper_fn | ||
p . should_equal "fn_with_tco" | ||
|
||
# The same test as the above one, but calls a global wrapper_fn that is declared | ||
# in top scope. | ||
group_builder.specify "should be caught in a TCO loop with a global wrapper fn" <| | ||
p = Panic.catch Any handler=(.payload) global_wrapper_fn | ||
p . should_equal "fn_with_tco" | ||
|
||
|
||
suite_builder.group "Type Errors" group_builder-> | ||
my_func x y = | ||
x + y | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Panic.catch
cannot be in tail call position. I believe it has to beNOT_TAIL
. All the tests inError_Spec
are fixed. Let's see what other collateral damage this change may have created. Btw. I have a feeling @hubertp was fighting with some similar issue a year back or so...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a good observation! At the end, it seems that we will not have to dig in the IR phases. I have just added some comments and made sure that even the
handler
callback is not in a tail call position, just to be sure - in commit 8539997.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On contrary to the
action
invocation, handler can probably be in tail position. There is nothing in theCatchPanicNode
that needs to run after handler...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good note. Add a comment about that in d86c7b5 .
handler
is invoked withTAIL_DIRECT
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while so I can't remember but maybe you are referring to #7116