Conversation
An extra thing corrected here was that most returns within END blocks can be statically determined so an LJE replaces the return during build time. This was a minor part of this but I mention it to get it out of the way. The main fix was to change how we initiate a non-local return from a closure. This commit changes from primarily walking a dynamic scope stack to walking the static scope stack. There are two reasons for this: 1. visibility to scopes which we do not add variables for 2. JITing may eliminate dynamic scopes So the new mechanism uses getContainingReturnToScope. As an added bonus both checkForLJE and initiateNonLocalReturn use the same method now. Before the two methods were very very similar but one used IR and the other used dynamic scope. There is a tiny wrinkle to the new way but it does make sense. static scope is static but not all information we need is static. We cannot know if a closure will be a proc or a lambda. So when we initiate a non-local return so long as we are within closures we check dynamic scopes looking to see if we are within a lambda (getContainingLambda). My only wonderment of this whole commit is runInterpreter added a catch to for IRReturnJump. This is definitely ok to have here but I wonder if it covers all paths into execution or not? Is there another place where we should add a catch?
|
ok found some errors to fix with full spec run |
…ff work or we get another error
…e to research after this commit). I could not originally reproduce this locally, but then I ran the tests by doing ```jruby -S rake spec``` which ran core specs + other tests. Our run in CI does ```jruby spec/core_spec.rb``` which will compile the file spec/core_spec.rb. The issue found from this is CompileIRBlockBody seems to realize it has no variables and that it then makes no dynamicScope. I guard against no dynamic scope now, but could this non-dynamicscope block end up being where we want to return from? I don't think so. I have tried define_method and explicit lambda and I see no failures....
… a better way by using the IRScope. This was what handBreakAndReturnsInLambda actually wanted in the end and it allows JIT to not care about whether it will make a dynamic scope or not...win win.
| if (scope.isWithinEND()) { | ||
| // ENDs do not allow returns | ||
| addInstr(new ThrowExceptionInstr(IRException.RETURN_LocalJumpError)); | ||
| } else { |
There was a problem hiding this comment.
MRI doesn't appear to statically determine this, or else they just immediately bail out without propagating an exception. Probably doesn't matter that we actually raise? Maybe better?
[] ~/projects/jruby $ rvm ruby-2.5.3 do ruby -e 'END { begin; return; rescue Exception => e; p e; else; puts "not caught"; end }'
-e: unexpected return
[] ~/projects/jruby $ jruby -e 'END { begin; return; rescue Exception => e; p e; else; puts "not caught"; end }'
#<LocalJumpError: unexpected return>
There was a problem hiding this comment.
yeah hmm. I did not consider the ability to catch. I do actually consider how we do it to be more correct but of course we emulate them...Even if we change this I would prefer to follow up on this in another PR/commit later. This is better than our escape crash. I also am not very concerned about this behavioral difference?
| public final Object returnValue; | ||
|
|
||
| private IRReturnJump(DynamicScope scopeToReturnFrom, Object rv) { | ||
| private IRReturnJump(IRScope returnScope, IRScope scopeToReturnFrom, Object rv) { |
There was a problem hiding this comment.
Naming is a little weird here...these mean the same thing in my head.
There was a problem hiding this comment.
returnScope is the scope where the return lives and scopeToReturnFrmo is where the return will exit from. I am happy to change those though.
There was a problem hiding this comment.
Maybe sourceScope and targetScope, or returnScope and targetScope? Entirely up to you, it's a minor issue.
|
With these two reverts we pass all tests and one extra weird test I know fails...master also failed with exact same behavior. Plan after landing this is to re-land these IRScope vs using DynamicScope commits and fix the last weird bug I know about. The remaining issue known is about JITing the same define_method in a loop in the root scope. |
2 participants
My main commit explains most of the details of what changed. See #5517 for more on what was broken.