From db6c67d33d62998a0e3a8eb794df0fc7be5d96d3 Mon Sep 17 00:00:00 2001 From: Tim Morgan Date: Mon, 4 Sep 2023 08:30:28 -0500 Subject: [PATCH 1/6] Add failing test for rescue/else with LocalJumpError --- test/natalie/rescue_test.rb | 4 ++++ test/support/rescue_else_bug.rb | 14 ++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 test/support/rescue_else_bug.rb diff --git a/test/natalie/rescue_test.rb b/test/natalie/rescue_test.rb index 9ed63d7a4..8d2b6cd87 100644 --- a/test/natalie/rescue_test.rb +++ b/test/natalie/rescue_test.rb @@ -124,6 +124,10 @@ def test_until2 $!.message.should == 'foo' end end + + it 'does not get confused by a LocalJumpError' do + `bin/natalie test/support/rescue_else_bug.rb`.strip.should == 'good' + end end describe 'raise' do diff --git a/test/support/rescue_else_bug.rb b/test/support/rescue_else_bug.rb new file mode 100644 index 000000000..4363dbc5e --- /dev/null +++ b/test/support/rescue_else_bug.rb @@ -0,0 +1,14 @@ +l = -> { + begin + raise 'foo' + rescue => e + # trigger LocalJumpError that is not used + nil.tap { return :foo if false } + 'good' + else + # this code should not run + 'bad' + end +} + +puts l.call From f874b9e94dc0207501e2a618892387b8fa697459 Mon Sep 17 00:00:00 2001 From: Tim Morgan Date: Mon, 4 Sep 2023 11:02:05 -0500 Subject: [PATCH 2/6] Construct better specs for the rescue/else bug Turns out this bug can be triggered without a LocalJumpError scenario, just by nesting a rescue/else that is unused. --- test/natalie/rescue_test.rb | 31 +++++++++++++++++++++++++++++-- test/support/rescue_else_bug.rb | 14 -------------- 2 files changed, 29 insertions(+), 16 deletions(-) delete mode 100644 test/support/rescue_else_bug.rb diff --git a/test/natalie/rescue_test.rb b/test/natalie/rescue_test.rb index 8d2b6cd87..03b0b2e50 100644 --- a/test/natalie/rescue_test.rb +++ b/test/natalie/rescue_test.rb @@ -125,8 +125,35 @@ def test_until2 end end - it 'does not get confused by a LocalJumpError' do - `bin/natalie test/support/rescue_else_bug.rb`.strip.should == 'good' + it 'does not get confused by nested rescues' do + ran = [] + begin + raise 'foo' + rescue + ran << :rescue + begin + rescue + end + else + ran << :else + end + ran.should == [:rescue] + end + + it 'does not get confused by an unused LocalJumpError' do + ran = [] + l = -> { + begin + raise 'foo' + rescue + ran << :rescue + nil.tap { return 'bar' if false } # return here could raise a LocalJumpError, but doesn't + else + ran << :else + end + } + l.call + ran.should == [:rescue] end end diff --git a/test/support/rescue_else_bug.rb b/test/support/rescue_else_bug.rb deleted file mode 100644 index 4363dbc5e..000000000 --- a/test/support/rescue_else_bug.rb +++ /dev/null @@ -1,14 +0,0 @@ -l = -> { - begin - raise 'foo' - rescue => e - # trigger LocalJumpError that is not used - nil.tap { return :foo if false } - 'good' - else - # this code should not run - 'bad' - end -} - -puts l.call From 3c4956a5d73c9d752bd2d57b4bb8030659b36c42 Mon Sep 17 00:00:00 2001 From: Tim Morgan Date: Mon, 4 Sep 2023 14:06:31 -0500 Subject: [PATCH 3/6] Fix rescue/else by setting global rescued at end --- lib/natalie/compiler/instructions/try_instruction.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/natalie/compiler/instructions/try_instruction.rb b/lib/natalie/compiler/instructions/try_instruction.rb index a2b82bfc4..74949451e 100644 --- a/lib/natalie/compiler/instructions/try_instruction.rb +++ b/lib/natalie/compiler/instructions/try_instruction.rb @@ -49,17 +49,18 @@ def generate(transform) code << '} catch(ExceptionObject *exception) {' code << 'auto exception_was = env->exception()' - - code << 'GlobalEnv::the()->set_rescued(true)' code << 'env->set_exception(exception)' transform.with_same_scope(catch_body) do |t| code << t.transform(@discard_catch_result ? nil : "#{result} =") end + # FIXME: can't we just call set_exception without the clear_exception() call? code << 'if (exception_was) env->set_exception(exception_was)' code << 'else env->clear_exception()' + code << 'GlobalEnv::the()->set_rescued(true)' + code << '}' end From 13b425792eb50da925b425e70a08aa79b6031ce3 Mon Sep 17 00:00:00 2001 From: Tim Morgan Date: Mon, 4 Sep 2023 14:06:43 -0500 Subject: [PATCH 4/6] Add another test for $! just to be sure --- test/natalie/rescue_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/natalie/rescue_test.rb b/test/natalie/rescue_test.rb index 03b0b2e50..2b3249c70 100644 --- a/test/natalie/rescue_test.rb +++ b/test/natalie/rescue_test.rb @@ -123,6 +123,18 @@ def test_until2 end $!.message.should == 'foo' end + $!.should be_nil + + begin + raise 'foo' + rescue + begin + rescue + else + $!.message.should == 'foo' + end + $!.message.should == 'foo' + end end it 'does not get confused by nested rescues' do From a9bea860b100607baea64190f87f5c21ac584345 Mon Sep 17 00:00:00 2001 From: Tim Morgan Date: Mon, 4 Sep 2023 14:25:02 -0500 Subject: [PATCH 5/6] Simplify resetting of current env exception --- lib/natalie/compiler/instructions/try_instruction.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/natalie/compiler/instructions/try_instruction.rb b/lib/natalie/compiler/instructions/try_instruction.rb index 74949451e..aec4cb266 100644 --- a/lib/natalie/compiler/instructions/try_instruction.rb +++ b/lib/natalie/compiler/instructions/try_instruction.rb @@ -55,10 +55,7 @@ def generate(transform) code << t.transform(@discard_catch_result ? nil : "#{result} =") end - # FIXME: can't we just call set_exception without the clear_exception() call? - code << 'if (exception_was) env->set_exception(exception_was)' - code << 'else env->clear_exception()' - + code << 'env->set_exception(exception_was)' code << 'GlobalEnv::the()->set_rescued(true)' code << '}' From cd5975eed81083f456a49f81244af8f5e1b6f99e Mon Sep 17 00:00:00 2001 From: Tim Morgan Date: Mon, 4 Sep 2023 14:26:39 -0500 Subject: [PATCH 6/6] Refactor code for exception and raise --- src/env.cpp | 4 +--- src/kernel_module.cpp | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index cc0682cb7..4fe41aba0 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -264,11 +264,9 @@ Value Env::exception_object() { ExceptionObject *Env::exception() { auto e = this; - for (;;) { + while (e) { if (e->m_exception) return e->m_exception; - if (!e->m_caller) - break; e = e->m_caller; } return nullptr; diff --git a/src/kernel_module.cpp b/src/kernel_module.cpp index a634b0a3d..f5d0386fd 100644 --- a/src/kernel_module.cpp +++ b/src/kernel_module.cpp @@ -473,9 +473,8 @@ Value KernelModule::puts(Env *env, Args args) { Value KernelModule::raise(Env *env, Value klass, Value message) { if (!klass) { - if (env->exception()) - klass = env->exception(); - else { + klass = env->exception(); + if (!klass) { klass = find_top_level_const(env, "RuntimeError"_s); message = new StringObject { "" }; }