Fix break not turning into LocalJumpError soon enough#4694
Conversation
Fixes #4686. Fixes #4577. The logic here adds finally wrappers around all call paths that receive a block. When the call exits, the block is marked "escaped" since it no longer has a method activation to go with it. This indicates that non-local flow, like break, should immediately trigger a LocalJumpError. This passes specs but has not been tested extensively with other types of call forms that receive blocks.
enebo
left a comment
enebo
left a comment
There was a problem hiding this comment.
I think I would like the comments I made in interpreter addressed but can easily be talked out of wanting them. The null check is not used by all instrs there and duplication of block + blockType looks icky...It is a minor comment so don't let it hold us up. Otherwise it all makes sense.
| // pushed so that we can get to its static scope. For-loops now always have | ||
| // a dyn-scope pushed onto stack which makes this work in all scenarios. | ||
| return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, blockType); | ||
| return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, block, blockType); |
There was a problem hiding this comment.
Looking at this code I wonder if we shouldn't just pass block. I see also we pass blockType for nonlocal returns but at top of this method we endlessly do block == null check and we may as well push that into initiateBreak and handleNonlocalReturn.
There was a problem hiding this comment.
Turns out blockType was only used by the two cases that always have a non-null block (as we discussed on IRC, non-local return and break should only be emitted for block bodies) so I just removed that null check and made each method look at block.type directly. Addresses this and other comment.
|
|
||
| DynamicScope parentScope = dynScope.getParentScope(); | ||
|
|
||
| if (block.isEscaped()) { |
There was a problem hiding this comment.
Based on my previous comment I now wonder if block can be null here ever? Perhaps this is only called in cases where block is guaranteed non-null?
| pushFloat(flote); | ||
| if (callType == CallType.NORMAL) { | ||
| invokeOther(file, line, name, 1, false, false); | ||
| invokeOther(file, line, name, 1, false, false, false); |
There was a problem hiding this comment.
Not a review comment but just a lament that Java has no keyword arguments :P
There was a problem hiding this comment.
Yeah I don't stacking booleans like this. I might combine the two into an enum representing what exactly was passed for block argument.
There was a problem hiding this comment.
Hmmm I wonder how many people static final booleans for this sort of issue? I don't think I have ever seen that done but it is probably a pattern?
* Add literal closure method to IR closure-accepting interface * Replace double boolean in JIT code with enum * Check for null in non-local return logic
|
I've made all suggested tweaks and I'm happy with it. Note that I had to re-add a null check in IRRuntimeHelpers.initiateNonLocalReturn because it appears that that instruction is also used to return from a singleton class body. I saw an error in the JRuby suite, running without that null check, that looked like this: We may want to see if this really needs to be a non-local return (@enebo). I will open a separate issue for the known-unfixed super behavior. |
2 participants
For #4686 and #4577.
The strategy here is as it was in 1.7: mark literal blocks as "escaped" when the call they were originally passed to returns. Break operations can then immediately trigger a LocalJumpError and be rescued in all the various ways reported in #4686 and #4577.
This does not fix super calls because the additional work is nontrivial (pushing literal-closure param through a lot of code in interpreter and JIT) and the exposure surface is pretty small for super PLUS literal closure PLUS break PLUS escaping of that block.