Skip to content

Commit d9e08e1

Browse files
eregoneregon
committed
Keyword rest arguments (**kwargs) accept non-Symbols keys in 2.7
1 parent a2ea063 commit d9e08e1

File tree

7 files changed

+40
-28
lines changed

7 files changed

+40
-28
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ Compatibility:
2929
* Updated `Dir.{glob,[]}` to raise `ArgumentError` for nul-separated strings.
3030
* `Kernel#lambda` with no block in a method called with a block raises an exception (#2004, @ssnickolay).
3131
* Implemented `BigDecimal` as C extension to improve compatibility.
32+
* `**kwargs` now accept non-Symbol keys like Ruby 2.7.
3233

3334
Performance:
3435

‎spec/tags/language/method_tags.txt

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,7 @@
11
fails:A method assigns local variables from method parameters for definition 'def m() end'
22
fails:A method assigns local variables from method parameters for definition 'def m(*a) a end'
3-
fails:A method assigns local variables from method parameters for definition 'def m(a=1, **) a end'
4-
fails:A method assigns local variables from method parameters for definition 'def m(*a, **) a end'
5-
fails:A method assigns local variables from method parameters for definition 'def m(*, **k) k end'
63
fails:A method assigns local variables from method parameters for definition 'def m(a = nil, **k) [a, k] end'
74
fails:A method assigns local variables from method parameters for definition 'def m(*a, **k) [a, k] end'
8-
fails:A method assigns local variables from method parameters for definition 'def m(**k); k end;'
9-
fails:A method assigns local variables from method parameters for definition 'def m(a:, **) a end'
10-
fails:A method assigns local variables from method parameters for definition 'def m(a:, **k) [a, k] end'
115
fails:A method assigns local variables from method parameters for definition 'def m(a, **nil); a end;'
126
fails:A method when passing an empty keyword splat to a method that does not accept keywords for definition 'def m(*a); a; end'
137
fails:A method when passing an empty keyword splat to a method that does not accept keywords for definition 'def m(a); a; end'

‎src/main/java/org/truffleruby/language/arguments/CheckKeywordArityNode.java

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
*/
1010
package org.truffleruby.language.arguments;
1111

12+
import com.oracle.truffle.api.CompilerDirectives;
1213
import org.truffleruby.RubyLanguage;
1314
import org.truffleruby.collections.BiConsumerNode;
1415
import org.truffleruby.core.hash.RubyHash;
@@ -42,9 +43,6 @@ public CheckKeywordArityNode(RubyLanguage language, Arity arity, RubyNode body)
4243
this.arity = arity;
4344
this.body = body;
4445
this.readUserKeywordsHashNode = new ReadUserKeywordsHashNode(arity.getRequired());
45-
this.checkKeywordArgumentsNode = new CheckKeywordArgumentsNode(language, arity);
46-
this.eachKeyNode = EachKeyValueNode.create();
47-
4846
}
4947

5048
@Override
@@ -74,47 +72,59 @@ private void checkArity(VirtualFrame frame) {
7472
throw new RaiseException(getContext(), coreExceptions().argumentError(given, arity.getRequired(), this));
7573
}
7674

77-
if (keywordArguments != null) {
78-
receivedKeywordsProfile.enter();
79-
eachKeyNode.executeEachKeyValue(frame, keywordArguments, checkKeywordArgumentsNode, null);
75+
if (!arity.hasKeywordsRest() && keywordArguments != null) {
76+
checkKeywordArguments(frame, keywordArguments);
77+
}
78+
}
79+
80+
void checkKeywordArguments(VirtualFrame frame, RubyHash keywordArguments) {
81+
if (eachKeyNode == null) {
82+
CompilerDirectives.transferToInterpreterAndInvalidate();
83+
eachKeyNode = insert(EachKeyValueNode.create());
8084
}
85+
if (checkKeywordArgumentsNode == null) {
86+
CompilerDirectives.transferToInterpreterAndInvalidate();
87+
checkKeywordArgumentsNode = insert(new CheckKeywordArgumentsNode(getLanguage(), arity));
88+
}
89+
eachKeyNode.executeEachKeyValue(frame, keywordArguments, checkKeywordArgumentsNode, null);
8190
}
8291

8392
private static class CheckKeywordArgumentsNode extends RubyContextNode implements BiConsumerNode {
8493

85-
private final boolean checkAllowedKeywords;
8694
private final boolean doesNotAcceptExtraArguments;
8795
private final int required;
8896
@CompilationFinal(dimensions = 1) private final RubySymbol[] allowedKeywords;
8997

9098
private final ConditionProfile isSymbolProfile = ConditionProfile.create();
9199
private final BranchProfile tooManyKeywordsProfile = BranchProfile.create();
92-
private final BranchProfile unknownKeywordProfile;
100+
private final BranchProfile unknownKeywordProfile = BranchProfile.create();
93101

94102
public CheckKeywordArgumentsNode(RubyLanguage language, Arity arity) {
95-
checkAllowedKeywords = !arity.hasKeywordsRest();
103+
assert !arity.hasKeywordsRest();
96104
doesNotAcceptExtraArguments = !arity.hasRest() && arity.getOptional() == 0;
97105
required = arity.getRequired();
98-
allowedKeywords = checkAllowedKeywords ? keywordsAsSymbols(language, arity) : null;
99-
unknownKeywordProfile = checkAllowedKeywords ? BranchProfile.create() : null;
106+
allowedKeywords = keywordsAsSymbols(language, arity);
100107
}
101108

102109
@Override
103110
public void accept(VirtualFrame frame, Object key, Object value, Object state) {
104111
if (isSymbolProfile.profile(key instanceof RubySymbol)) {
105-
if (checkAllowedKeywords && !keywordAllowed(key)) {
112+
if (!keywordAllowed(key)) {
106113
unknownKeywordProfile.enter();
107114
throw new RaiseException(
108115
getContext(),
109116
coreExceptions().argumentErrorUnknownKeyword((RubySymbol) key, this));
110117
}
111118
} else {
112-
final int given = RubyArguments.getArgumentsCount(frame); // -1 for keyword hash, +1 for reject Hash with non-Symbol key
119+
// the Hash would be split and a reject Hash be created to hold non-Symbols when there is no **kwrest parameter,
120+
// so we need to check if an extra argument is allowed
121+
final int given = RubyArguments.getArgumentsCount(frame); // -1 for keyword hash, +1 for reject Hash with non-Symbol keys
113122
if (doesNotAcceptExtraArguments && given > required) {
114123
tooManyKeywordsProfile.enter();
115124
throw new RaiseException(getContext(), coreExceptions().argumentError(given, required, this));
116125
}
117126
}
127+
118128
}
119129

120130
@ExplodeLoop

‎src/main/java/org/truffleruby/language/arguments/ReadKeywordRestArgumentNode.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ public class ReadKeywordRestArgumentNode extends RubyContextSourceNode implement
3333
@Child private SetNode setNode = SetNode.create();
3434

3535
private final ConditionProfile noHash = ConditionProfile.create();
36-
private final ConditionProfile isSymbolProfile = ConditionProfile.create();
3736

3837
public ReadKeywordRestArgumentNode(RubyLanguage language, int minimum, Arity arity) {
3938
this.excludedKeywords = CheckKeywordArityNode.keywordsAsSymbols(language, arity);
@@ -58,7 +57,7 @@ private Object lookupRestKeywordArgumentHash(VirtualFrame frame) {
5857

5958
@Override
6059
public void accept(VirtualFrame frame, Object key, Object value, Object kwRest) {
61-
if (isSymbolProfile.profile(key instanceof RubySymbol) && !keywordExcluded(key)) {
60+
if (!keywordExcluded(key)) {
6261
setNode.executeSet((RubyHash) kwRest, key, value, false);
6362
}
6463
}

‎src/main/java/org/truffleruby/language/arguments/ReadRejectedKeywordArgumentsNode.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222

2323
public class ReadRejectedKeywordArgumentsNode extends RubyContextNode implements BiConsumerNode {
2424

25-
@Child private ReadUserKeywordsHashNode readUserKeywordsHashNode;
2625
@Child private EachKeyValueNode eachKeyNode = EachKeyValueNode.create();
2726
@Child private SetNode setNode = SetNode.create();
2827

‎src/main/java/org/truffleruby/language/arguments/ReadRestArgumentNode.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ public class ReadRestArgumentNode extends RubyContextSourceNode {
2727
private final int startIndex;
2828
private final int indexFromCount;
2929
private final boolean keywordArguments;
30+
private final boolean considerRejectedKWArgs;
3031

3132
private final BranchProfile noArgumentsLeftProfile = BranchProfile.create();
3233
private final BranchProfile subsetOfArgumentsProfile = BranchProfile.create();
@@ -41,17 +42,19 @@ public ReadRestArgumentNode(
4142
int startIndex,
4243
int indexFromCount,
4344
boolean keywordArguments,
45+
boolean considerRejectedKWArgs,
4446
int minimumForKWargs) {
4547
this.startIndex = startIndex;
4648
this.indexFromCount = indexFromCount;
4749
this.keywordArguments = keywordArguments;
50+
this.considerRejectedKWArgs = considerRejectedKWArgs;
4851

49-
if (keywordArguments) {
52+
if (considerRejectedKWArgs) {
5053
this.readUserKeywordsHashNode = new ReadUserKeywordsHashNode(minimumForKWargs);
5154
}
5255

53-
this.hasKeywordsProfile = keywordArguments ? ConditionProfile.create() : null;
54-
this.hasRejectedKwargs = keywordArguments ? ConditionProfile.create() : null;
56+
this.hasKeywordsProfile = considerRejectedKWArgs ? ConditionProfile.create() : null;
57+
this.hasRejectedKwargs = considerRejectedKWArgs ? ConditionProfile.create() : null;
5558
}
5659

5760
@Override
@@ -91,7 +94,7 @@ public Object execute(VirtualFrame frame) {
9194

9295
final RubyArray rest = createArray(resultStore, resultLength);
9396

94-
if (keywordArguments) {
97+
if (considerRejectedKWArgs) {
9598
final RubyHash kwargsHash = readUserKeywordsHashNode.execute(frame);
9699

97100
if (hasKeywordsProfile.profile(kwargsHash != null)) {

‎src/main/java/org/truffleruby/parser/LoadArgumentsTranslator.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,8 @@ public RubyNode visitRestArgNode(RestArgParseNode node) {
348348
if (useArray()) {
349349
readNode = ArraySliceNodeGen.create(from, to, loadArray(sourceSection));
350350
} else {
351-
readNode = new ReadRestArgumentNode(from, -to, hasKeywordArguments, required);
351+
boolean considerRejectedKWArgs = considerRejectedKWArgs();
352+
readNode = new ReadRestArgumentNode(from, -to, hasKeywordArguments, considerRejectedKWArgs, required);
352353
}
353354

354355
final FrameSlot slot = methodBodyTranslator.getEnvironment().getFrameDescriptor().findFrameSlot(node.getName());
@@ -441,7 +442,7 @@ private RubyNode translateLocalAssignment(SourceIndexLength sourcePosition, Stri
441442
minimum += 1;
442443
}
443444

444-
final boolean considerRejectedKWArgs = firstOpt && hasKeywordArguments;
445+
final boolean considerRejectedKWArgs = firstOpt && considerRejectedKWArgs();
445446
readNode = new ReadOptionalArgumentNode(
446447
index,
447448
minimum,
@@ -644,4 +645,9 @@ protected RubyNode loadArray(SourceIndexLength sourceSection) {
644645
return node;
645646
}
646647

648+
private boolean considerRejectedKWArgs() {
649+
// If there is **kwrest, there never are rejected kwargs
650+
return argsNode.getKeywordCount() > 0 && !argsNode.hasKeyRest();
651+
}
652+
647653
}

0 commit comments

Comments
 (0)