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

Return from block broken in 9.2.8.0 #5852

Closed
davishmcclurg opened this issue Aug 28, 2019 · 20 comments
Closed

Return from block broken in 9.2.8.0 #5852

davishmcclurg opened this issue Aug 28, 2019 · 20 comments
Milestone

Comments

@davishmcclurg
Copy link
Contributor

@davishmcclurg davishmcclurg commented Aug 28, 2019

In 9.2.8.0, returning from blocks isn't working correctly in some cases:

$ RBENV_VERSION="jruby-9.2.8.0" irb --simple-prompt
>> JRUBY_VERSION
=> "9.2.8.0"
>> def test
>>   if block_given?
>>     yield
>>   else
>>     test { return }
>>     raise 'no!'
>>   end
>> end
=> :test
>> test
Traceback (most recent call last):
        7: from /Users/dharsha/.rbenv/versions/jruby-9.2.8.0/bin/irb:13:in `<main>'
        6: from org/jruby/RubyKernel.java:1193:in `catch'
        5: from org/jruby/RubyKernel.java:1193:in `catch'
        4: from org/jruby/RubyKernel.java:1425:in `loop'
        3: from org/jruby/RubyKernel.java:1061:in `eval'
        2: from (irb):10:in `evaluate'
        1: from (irb):7:in `test'
RuntimeError (no!)

Looks like it works ok in 9.2.5.0 (MRI 2.5.5 looks the same):

$ RBENV_VERSION="jruby-9.2.5.0" irb --simple-prompt
>> JRUBY_VERSION
=> "9.2.5.0"
>> def test
>>   if block_given?
>>     yield
>>   else
>>     test { return }
>>     raise 'no!'
>>   end
>> end
=> :test
>> test
=> nil

Operating system:

$ uname -a
Darwin dharsha-work.local 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64
@davishmcclurg
Copy link
Contributor Author

@davishmcclurg davishmcclurg commented Aug 28, 2019

The non-recursive version looks ok:

$ RBENV_VERSION="jruby-9.2.8.0" irb --simple-prompt
>> def test1
>>   yield
>> end
=> :test1
>>
>> def test2
>>   test1 { return }
>>   raise 'no!'
>> end
=> :test2
>> test2
=> nil

Loading

@headius
Copy link
Member

@headius headius commented Aug 28, 2019

Peculiar thing to regress! I'd expect a lot more code to fail.

Loading

@headius
Copy link
Member

@headius headius commented Aug 28, 2019

Regressed some time in 9.2.6.0 cycle.

Loading

@headius
Copy link
Member

@headius headius commented Aug 28, 2019

Looks like this regressed due to work by @enebo in a55e357. I'm looking into exactly why.

Loading

@headius
Copy link
Member

@headius headius commented Aug 28, 2019

Ok this makes sense...

It appears that the changes in a55e357 will propagate the return up the stack until the current scope is the same as the block's return target scope. I think this is the line:

if (rj.methodToReturnFrom == dynScope.getStaticScope().getIRScope()) {

However this does not work for the case you've illustrated here, where there's an intervening call from the same method that is not the frame to return to.

I believe we need to return to a frame-level comparison for this to work; the return needs to go back to the frame where the block was instantiated.

Loading

@headius headius added this to the JRuby 9.2.9.0 milestone Aug 28, 2019
@headius
Copy link
Member

@headius headius commented Aug 28, 2019

FWIW this appears to be broken in TruffleRuby as well:

$ rvm truffleruby do ruby test.rb
test.rb:6:in `foo':  (RuntimeError)
	from test.rb:10:in `<main>'

There's obviously a big spec gap here.

Loading

@enebo
Copy link
Member

@enebo enebo commented Aug 29, 2019

Bleh. good old recursive calls help to humble...

Yeah this cannot work as-is. Just to paraphrase why the old solution worked: DynamicScope is unique per activation so it is more of a object identity but my commit converted this to a class identity (all activations of the same method share the same StaticScope).

Originally, I had tried to eliminate DynamicScope because some methods do not have them. I am foggy but I think this was why this commit was made in the first place (to prevent NPE because it could not find the dynscope).

The notion of "frame" here maybe is broader than DynamicScope or Frame. It could possibly be the interpretercontext (and something within the JITTd method) as the jump off point?

The obvious solution would be to force DynamicScope on the scope which contains the closure which contains an explicit return. Not ideal in that it probably will have some scenario where we would have liked to eliminate that DynamicScope and therefore would make that method a bit slower.

Loading

@headius
Copy link
Member

@headius headius commented Aug 29, 2019

@enebo I'm pretty sure all scopes that contain a closure already force a DynamicScope, so using that for frame identity should be just fine:

// literal closures can be used to capture surrounding binding
if (hasLiteralClosure()) {
modifiedScope = true;
flags.addAll(IRFlags.REQUIRE_ALL_FRAME_FIELDS);
}

We also force a frame for such cases.

Of course this will complicate removing DynamicScope or Frame in the future, but we could narrow the requirement to only happen when we know there's a non-local return targeting this frame.

Loading

@enebo
Copy link
Member

@enebo enebo commented Aug 29, 2019

@headius ah ok well that should be fine then. At some point I would like to revisit why we force. I know some reasons why we would force (like captured vars) but ideally if we can limit what needs dynamic scope it can help in the future.

Loading

@headius
Copy link
Member

@headius headius commented Aug 29, 2019

We do it because we can't detect Proc#binding calls right now. The only way we could eliminate it is if we could look into the caller method and know it does not reify the block into a Proc object. That could be done at runtime, with a fallback to full scoping, but it's not something we have the infrastructure for right now.

Something like this at a literal block + call site:

  • If call is to a method that does not reify block:
    • Proceed with no scope/frame
  • Else
    • Move all local variables to a dynamic scope, set a bit on the method to always scope, and pass block with a full binding

Loading

@enebo
Copy link
Member

@enebo enebo commented Aug 29, 2019

@headius ah I forgot about binding and no point in trying to accommodate some later runtime optimization since that is a long ways away and we can revisit this if we ever decide we can support it.

Loading

@jessicaspeir
Copy link

@jessicaspeir jessicaspeir commented Sep 17, 2019

I'm having a potentially-related issue with the graphql-ruby gem and JRuby 9.2.8.0. A "yield called outside of block" error is getting thrown even though there is a block. The graphql-ruby issue has more details: rmosolgo/graphql-ruby#2451

Should I open a new issue, or is it sensible that the same root cause is causing the original issue described here, and the one I'm running into?

Loading

@enebo
Copy link
Member

@enebo enebo commented Sep 17, 2019

@jessicaspeir I do see resolve_fields is called twice in the call chain so this could easily be the same issue. If there is a simple enough script for me to run to see this I can verify it when I fix what is reported here.

Loading

@jessicaspeir
Copy link

@jessicaspeir jessicaspeir commented Sep 17, 2019

I think those two resolve_fields calls are two different methods with the same name, but I don't know enough about the code to be sure, or to create a small reproduction. I can always try the fix when you have it ready and open a new issue if the error persists.

Loading

@enebo
Copy link
Member

@enebo enebo commented Sep 17, 2019

@jessicaspeir ok I will try and get to this soon.

Loading

@enebo enebo closed this in 2e8958e Sep 25, 2019
@enebo
Copy link
Member

@enebo enebo commented Sep 25, 2019

@jessicaspeir can you try out graphql-ruby and see if this resolved things or we have a different issue to work through?

Loading

@gapthemind
Copy link

@gapthemind gapthemind commented Nov 11, 2019

@enebo we see the "yield called outside of block" error message when running GraphQL with version 9.2.9.0 and working properly when running 9.2.6.0.

Loading

@enebo
Copy link
Member

@enebo enebo commented Nov 11, 2019

@gapthemind bummer. Is there a simple way for me to reproduce this. I don't use graphql so a simple repro will help me nail this.

Loading

@kovyrin
Copy link

@kovyrin kovyrin commented Nov 11, 2019

Just wanted to confirm, that 9.2.9.0 did solve our issue (reported by @davishmcclurg):

$ RBENV_VERSION="jruby-9.2.9.0" irb --simple-prompt
>> JRUBY_VERSION
=> "9.2.9.0"
>> def test
>>   if block_given?
>>     yield
>>   else
>>     test { return }
>>     raise 'no!'
>>   end
>> end
=> :test
>> test
=> nil

Loading

@enebo
Copy link
Member

@enebo enebo commented Nov 11, 2019

@kovyrin HAHAHA you almost gave me a heart attack...until I read that it solved your issue :)

So I suspect graphql bug was fallout from my original changes but a different bug. I largely reinstated the old logic for 9.2.9.0 but I did not specifically revert back to old code. It is tough trying to clean up internals. :(

We will for sure get this for 9.2.10.0 assuming someone can get me a reproduction since we really really hate language level incompatibilities. Especially when they are regressions.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants