Kwargs should not need dynamic scope, since we have static handy. #3906
+22
−18
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
...unfortunately...
In the process of getting the JIT to work properly without the AddLocalVarLoadStore pass, I ran into an issue with Block getting set into DynamicScope and blowing up (caused by OptimizeDelegation, fixed in 88a8896). In fixing that I realized that this particular method was always requiring a dynamic scope, for no obvious reason:
It turned out that arity-checking of keyword arguments forced a DynamicScope so it could get to StaticScope. However, all Ruby bodies have direct access to their StaticScope now, so the DynamicScope was unnecessary.
Unfortunately, after I came up with this patch to remove that flag, I ran into another crash due to CheckLJE not reporting that it required DynamicScope.
I fixed that (on master) in 434e54e, but it just left me stranded on another mysterious failure in Rake's helpers.rb:
Error:
Method:
For some reason, variables are now getting crossed. The block param
flags
is getting mixed up withtest_task later on
, at least from the perspective of the inner block.I have not provided the IR for brevity, but
-Xir.print
should be working properly now...build this branch and try to run anything with rake.So, this is a work in progress. I expect this latest issue is another problem with scope optimizations killing a scope that should be there or vice versa...something that was masked before by the arity check's dynscope requirement. Any thoughts?
cc @subbuss @enebo