Ensure uniform keywords flags in native method annotations#9544
Merged
Conversation
Because of how indy logic determines if a target method accepts keywords (see discussion in jruby#9531), we must make sure that all annotated signatures set this flag uniformly. If this is not done, the call logic may incorrectly assume that all signatures do not accept keywords, breaking keyword-passing behavior. Fixes jruby#9531
As per jruby#9531, due to limitations in how we bind methods for indy call sites, we need the keywords flag to be uniform across all bound signatures. These additional nonuniformities were found by adding a check to DescriptorInfo and fixing the resulting errors. The check will not be pushed for JRuby 10.0, given the risk of turning this soft requirement into a hard requirement, but I will push the check for JRuby 10.1 as we work toward improving the propagation of binding metdata into the method table.
Tests behavior found broken in jruby#9531. This is not a comprehensive test in that it does not check all initialize methods that accept keywords, but a hard check will be pushed for JRuby 10.1 that makes this soft requirement impossible to ignore.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The keywords flag is used by invokedynamic call sites to determine whether kwarg logic should be added to the bound method handle chain. The flag is accessed by inspecting the one
NativeCallshipped with the targetDynamicMethodto see if its annotation has the flag set.If the one signature propagated as
NativeCallis not uniform with the other bindings, it will inaccurately reflect the keyword needs of those other signatures. As a short-term workaround for this limitation, we make sure all multi-dispatch native methods set the keywords flag uniformly.This set of fixes was determined by adding a hard check to the
DescriptorInfomethod, used at binding time to gather method bindings. I'm not comfortable pushing a hard check into JRuby 10.0.x, since this was previously a soft requirement, so only the fixed flags are pushed here. TheDescriptorInfohard check will be pushed for JRuby 10.1.Fixes #9531 and #9543.