Skip to content

Commit 103d4da

Browse files
eregoneregon
committed
Copy the CRuby bug/behavior that the ruby2_keyword flag on a Hash is not reset for caller(*args) -> callee(*args) calls
* More precisely, a copy of the marked Hash is made, but the copy also has the ruby2_keywords flag set, which is unexpected. * See https://bugs.ruby-lang.org/issues/18625
1 parent 0fba93e commit 103d4da

File tree

15 files changed

+144
-64
lines changed

15 files changed

+144
-64
lines changed

‎src/main/java/org/truffleruby/cext/CExtNodes.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1093,7 +1093,7 @@ protected Object callSuper(VirtualFrame frame, Object[] args) {
10931093
final InternalMethod superMethod = superMethodLookup.getMethod();
10941094
// This C API only passes positional arguments, but maybe it should be influenced by ruby2_keywords hashes?
10951095
return callSuperMethodNode.execute(
1096-
frame, callingSelf, superMethod, EmptyArgumentsDescriptor.INSTANCE, args, nil);
1096+
frame, callingSelf, superMethod, EmptyArgumentsDescriptor.INSTANCE, args, nil, null);
10971097
}
10981098

10991099
@TruffleBoundary

‎src/main/java/org/truffleruby/core/basicobject/BasicObjectNodes.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ protected Object instanceExec(VirtualFrame frame, Object receiver, Object[] argu
448448
block.declarationContext.getRefinements());
449449
var descriptor = RubyArguments.getDescriptor(frame);
450450
return callBlockNode.executeCallBlock(
451-
declarationContext, block, receiver, block.block, descriptor, arguments);
451+
declarationContext, block, receiver, block.block, descriptor, arguments, null);
452452
}
453453

454454
@Specialization

‎src/main/java/org/truffleruby/core/method/MethodNodes.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ protected Object call(Frame callerFrame, RubyMethod method, Object[] rubyArgs, R
138138
final Object[] newArgs = RubyArguments.repack(rubyArgs, method.receiver);
139139
RubyArguments.setMethod(newArgs, internalMethod);
140140
assert RubyArguments.assertFrameArguments(newArgs);
141-
return callInternalMethodNode.execute(callerFrame, internalMethod, method.receiver, newArgs);
141+
return callInternalMethodNode.execute(callerFrame, internalMethod, method.receiver, newArgs, null);
142142
}
143143
}
144144

@@ -360,7 +360,7 @@ public Object execute(VirtualFrame frame) {
360360
final Object originalBoundMethodReceiver = RubyArguments.getSelf(RubyArguments.getDeclarationFrame(frame));
361361
Object[] rubyArgs = RubyArguments.repack(frame.getArguments(), originalBoundMethodReceiver);
362362
RubyArguments.setMethod(rubyArgs, method);
363-
return callInternalMethodNode.execute(frame, method, originalBoundMethodReceiver, rubyArgs);
363+
return callInternalMethodNode.execute(frame, method, originalBoundMethodReceiver, rubyArgs, null);
364364
}
365365
}
366366

‎src/main/java/org/truffleruby/core/method/RubyMethod.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public Object execute(Object[] arguments,
7373
final Object[] convertedArguments = foreignToRubyArgumentsNode.executeConvert(arguments);
7474
final Object[] frameArgs = RubyArguments.pack(null, null, method, null, receiver, nil,
7575
EmptyArgumentsDescriptor.INSTANCE, convertedArguments);
76-
return callInternalMethodNode.execute(null, method, receiver, frameArgs);
76+
return callInternalMethodNode.execute(null, method, receiver, frameArgs, null);
7777
}
7878
// endregion
7979

‎src/main/java/org/truffleruby/core/module/ModuleNodes.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -809,7 +809,7 @@ public Object classExec(ArgumentsDescriptor descriptor, RubyModule self, Object[
809809
new FixedDefaultDefinee(self),
810810
block.declarationContext.getRefinements());
811811

812-
return callBlockNode.executeCallBlock(declarationContext, block, self, block.block, descriptor, args);
812+
return callBlockNode.executeCallBlock(declarationContext, block, self, block.block, descriptor, args, null);
813813
}
814814
}
815815

@@ -2271,7 +2271,8 @@ protected RubyModule refine(RubyModule namespace, RubyModule moduleToRefine, Rub
22712271
refinement,
22722272
block.block,
22732273
EmptyArgumentsDescriptor.INSTANCE,
2274-
EMPTY_ARGUMENTS);
2274+
EMPTY_ARGUMENTS,
2275+
null);
22752276
return refinement;
22762277
}
22772278

‎src/main/java/org/truffleruby/core/proc/ProcNodes.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,8 @@ protected Object call(Frame callerFrame, RubyProc proc, Object[] rubyArgs, RootC
204204
ProcOperations.getSelf(proc),
205205
RubyArguments.getBlock(rubyArgs),
206206
RubyArguments.getDescriptor(rubyArgs),
207-
RubyArguments.getRawArguments(rubyArgs));
207+
RubyArguments.getRawArguments(rubyArgs),
208+
null);
208209
}
209210
}
210211

‎src/main/java/org/truffleruby/language/dispatch/DispatchNode.java

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -206,12 +206,17 @@ public Object callWithBlock(Object receiver, String method, Object block, Object
206206

207207
public Object callWithDescriptor(Object receiver, String method, Object block, ArgumentsDescriptor descriptor,
208208
Object[] arguments) {
209+
return callWithDescriptor(receiver, method, block, descriptor, arguments, null);
210+
}
211+
212+
public Object callWithDescriptor(Object receiver, String method, Object block, ArgumentsDescriptor descriptor,
213+
Object[] arguments, LiteralCallNode literalCallNode) {
209214
final Object[] rubyArgs = RubyArguments.allocate(arguments.length);
210215
RubyArguments.setSelf(rubyArgs, receiver);
211216
RubyArguments.setBlock(rubyArgs, block);
212217
RubyArguments.setDescriptor(rubyArgs, descriptor);
213218
RubyArguments.setArguments(rubyArgs, arguments);
214-
return dispatch(null, receiver, method, rubyArgs);
219+
return dispatch(null, receiver, method, rubyArgs, literalCallNode);
215220
}
216221

217222
public Object callWithFrame(Frame frame, Object receiver, String method) {
@@ -272,6 +277,11 @@ public final Object callWithFrameAndBlock(Frame frame, Object receiver, String m
272277
}
273278

274279
public final Object dispatch(Frame frame, Object receiver, String methodName, Object[] rubyArgs) {
280+
return dispatch(frame, receiver, methodName, rubyArgs, null);
281+
}
282+
283+
public final Object dispatch(Frame frame, Object receiver, String methodName, Object[] rubyArgs,
284+
LiteralCallNode literalCallNode) {
275285
assert RubyArguments.getSelf(rubyArgs) == receiver;
276286

277287
final RubyClass metaclass = metaclassNode.execute(receiver);
@@ -286,7 +296,7 @@ public final Object dispatch(Frame frame, Object receiver, String methodName, Ob
286296
if (RubyGuards.isForeignObject(receiver)) { // TODO (eregon, 16 Aug 2021) maybe use a final boolean on the class to know if foreign
287297
return callForeign(receiver, methodName, rubyArgs);
288298
} else {
289-
return callMethodMissing(frame, receiver, methodName, rubyArgs);
299+
return callMethodMissing(frame, receiver, methodName, rubyArgs, literalCallNode);
290300
}
291301
}
292302
}
@@ -295,17 +305,19 @@ public final Object dispatch(Frame frame, Object receiver, String methodName, Ob
295305
RubyArguments.setCallerData(rubyArgs, getFrameOrStorageIfRequired(frame));
296306

297307
assert RubyArguments.assertFrameArguments(rubyArgs);
298-
return callNode.execute(frame, method, receiver, rubyArgs);
308+
return callNode.execute(frame, method, receiver, rubyArgs, literalCallNode);
299309
}
300310

301-
private Object callMethodMissing(Frame frame, Object receiver, String methodName, Object[] rubyArgs) {
311+
private Object callMethodMissing(Frame frame, Object receiver, String methodName, Object[] rubyArgs,
312+
LiteralCallNode literalCallNode) {
302313
// profiles through lazy node creation
303314
final RubySymbol symbolName = nameToSymbol(methodName);
304315

305316
final Object[] newArgs = RubyArguments.repack(rubyArgs, receiver, 0, 1);
306317

307318
RubyArguments.setArgument(newArgs, 0, symbolName);
308-
final Object result = getMethodMissingNode().dispatch(frame, receiver, "method_missing", newArgs);
319+
final Object result = getMethodMissingNode().dispatch(frame, receiver, "method_missing", newArgs,
320+
literalCallNode);
309321

310322
if (result == MISSING) {
311323
methodMissingMissing.enter();

‎src/main/java/org/truffleruby/language/dispatch/LiteralCallNode.java

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
2020
import org.truffleruby.language.arguments.EmptyArgumentsDescriptor;
2121
import org.truffleruby.language.arguments.KeywordArgumentsDescriptor;
22-
import org.truffleruby.language.arguments.KeywordArgumentsDescriptorManager;
22+
import org.truffleruby.language.methods.SharedMethodInfo;
2323

2424
/** A literal call site in Ruby code: one of foo(), super or yield. */
2525
public abstract class LiteralCallNode extends RubyContextSourceNode {
@@ -28,16 +28,16 @@ public abstract class LiteralCallNode extends RubyContextSourceNode {
2828
@Child private CopyHashAndSetRuby2KeywordsNode copyHashAndSetRuby2KeywordsNode;
2929

3030
protected final boolean isSplatted;
31-
@CompilationFinal private boolean lastArgIsNotHashProfile, notRuby2KeywordsHashProfile, emptyKeywordsProfile,
32-
notEmptyKeywordsProfile;
31+
@CompilationFinal private boolean lastArgIsNotHashProfile, ruby2KeywordsHashProfile, notRuby2KeywordsHashProfile,
32+
emptyKeywordsProfile, notEmptyKeywordsProfile;
3333

3434
protected LiteralCallNode(boolean isSplatted, ArgumentsDescriptor descriptor) {
3535
this.isSplatted = isSplatted;
3636
this.descriptor = descriptor;
3737
}
3838

3939
// NOTE: args is either frame args or user args
40-
protected ArgumentsDescriptor getArgumentsDescriptorAndCheckRuby2KeywordsHash(Object[] args, int userArgsCount) {
40+
protected boolean isRuby2KeywordsHash(Object[] args, int userArgsCount) {
4141
assert isSplatted : "this is only needed if isSplatted";
4242

4343
if (descriptor == EmptyArgumentsDescriptor.INSTANCE) { // *rest and no kwargs passed explicitly (k: v/k => v/**kw)
@@ -51,18 +51,16 @@ protected ArgumentsDescriptor getArgumentsDescriptorAndCheckRuby2KeywordsHash(Ob
5151
lastArgIsNotHashProfile = true;
5252
}
5353

54-
return descriptor;
54+
return false;
5555
}
5656

5757
if (((RubyHash) lastArgument).ruby2_keywords) { // both branches profiled
58-
if (copyHashAndSetRuby2KeywordsNode == null) {
58+
if (!ruby2KeywordsHashProfile) {
5959
CompilerDirectives.transferToInterpreterAndInvalidate();
60-
copyHashAndSetRuby2KeywordsNode = insert(CopyHashAndSetRuby2KeywordsNode.create());
60+
ruby2KeywordsHashProfile = true;
6161
}
6262

63-
RubyHash unmarked = copyHashAndSetRuby2KeywordsNode.execute((RubyHash) lastArgument, false);
64-
ArrayUtils.setLast(args, unmarked);
65-
return KeywordArgumentsDescriptorManager.EMPTY;
63+
return true;
6664
} else {
6765
if (!notRuby2KeywordsHashProfile) {
6866
CompilerDirectives.transferToInterpreterAndInvalidate();
@@ -72,7 +70,7 @@ protected ArgumentsDescriptor getArgumentsDescriptorAndCheckRuby2KeywordsHash(Ob
7270
}
7371
}
7472

75-
return descriptor;
73+
return false;
7674
}
7775

7876
// NOTE: args is either frame args or user args
@@ -99,4 +97,17 @@ public static Object[] removeEmptyKeywordArguments(Object[] args) {
9997
return ArrayUtils.extractRange(args, 0, args.length - 1);
10098
}
10199

100+
// NOTE: args is either frame args or user args
101+
public void copyRuby2KeywordsHash(Object[] args, SharedMethodInfo info) {
102+
if (!info.getArity().hasRest()) { // https://bugs.ruby-lang.org/issues/18625
103+
if (copyHashAndSetRuby2KeywordsNode == null) {
104+
CompilerDirectives.transferToInterpreterAndInvalidate();
105+
copyHashAndSetRuby2KeywordsNode = insert(CopyHashAndSetRuby2KeywordsNode.create());
106+
}
107+
108+
final RubyHash lastArgument = (RubyHash) ArrayUtils.getLast(args);
109+
ArrayUtils.setLast(args, copyHashAndSetRuby2KeywordsNode.execute(lastArgument, false));
110+
}
111+
}
112+
102113
}

‎src/main/java/org/truffleruby/language/dispatch/RubyCallNode.java

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.truffleruby.language.arguments.ArgumentsDescriptor;
2626
import org.truffleruby.language.arguments.EmptyArgumentsDescriptor;
2727
import org.truffleruby.language.arguments.KeywordArgumentsDescriptor;
28+
import org.truffleruby.language.arguments.KeywordArgumentsDescriptorManager;
2829
import org.truffleruby.language.arguments.RubyArguments;
2930
import org.truffleruby.language.arguments.SplatToArgsNode;
3031
import org.truffleruby.language.literal.NilLiteralNode;
@@ -92,19 +93,20 @@ public Object execute(VirtualFrame frame) {
9293
Object[] rubyArgs = RubyArguments.allocate(arguments.length);
9394
RubyArguments.setSelf(rubyArgs, receiverObject);
9495

95-
final ArgumentsDescriptor descriptor;
96+
ArgumentsDescriptor descriptor = this.descriptor;
97+
boolean ruby2KeywordsHash = false;
9698
executeArguments(frame, rubyArgs);
9799
if (isSplatted) {
98100
rubyArgs = splatArgs(receiverObject, rubyArgs);
99-
descriptor = getArgumentsDescriptorAndCheckRuby2KeywordsHash(rubyArgs,
100-
RubyArguments.getRawArgumentsCount(rubyArgs));
101-
} else {
102-
descriptor = this.descriptor;
101+
ruby2KeywordsHash = isRuby2KeywordsHash(rubyArgs, RubyArguments.getRawArgumentsCount(rubyArgs));
102+
if (ruby2KeywordsHash) {
103+
descriptor = KeywordArgumentsDescriptorManager.EMPTY;
104+
}
103105
}
104106

105107
RubyArguments.setBlock(rubyArgs, executeBlock(frame));
106108

107-
return doCall(frame, receiverObject, descriptor, rubyArgs);
109+
return doCall(frame, receiverObject, descriptor, rubyArgs, ruby2KeywordsHash);
108110
}
109111

110112
@Override
@@ -130,24 +132,25 @@ public void assign(VirtualFrame frame, Object value) {
130132
RubyArguments.setBlock(rubyArgs, executeBlock(frame));
131133

132134
// no ruby2_keywords behavior for assign
133-
doCall(frame, receiverObject, descriptor, rubyArgs);
135+
doCall(frame, receiverObject, descriptor, rubyArgs, false);
134136
}
135137

136-
public Object doCall(VirtualFrame frame, Object receiverObject, ArgumentsDescriptor descriptor, Object[] rubyArgs) {
138+
public Object doCall(VirtualFrame frame, Object receiverObject, ArgumentsDescriptor descriptor, Object[] rubyArgs,
139+
boolean ruby2KeywordsHash) {
137140
// Remove empty kwargs in the caller, so the callee does not need to care about this special case
138141
if (descriptor instanceof KeywordArgumentsDescriptor && emptyKeywordArguments(rubyArgs)) {
139142
rubyArgs = removeEmptyKeywordArguments(rubyArgs);
140-
RubyArguments.setDescriptor(rubyArgs, EmptyArgumentsDescriptor.INSTANCE);
141-
} else {
142-
RubyArguments.setDescriptor(rubyArgs, descriptor);
143+
descriptor = EmptyArgumentsDescriptor.INSTANCE;
143144
}
145+
RubyArguments.setDescriptor(rubyArgs, descriptor);
144146

145147
if (dispatch == null) {
146148
CompilerDirectives.transferToInterpreterAndInvalidate();
147149
dispatch = insert(DispatchNode.create(dispatchConfig));
148150
}
149151

150-
final Object returnValue = dispatch.dispatch(frame, receiverObject, methodName, rubyArgs);
152+
final Object returnValue = dispatch.dispatch(frame, receiverObject, methodName, rubyArgs,
153+
ruby2KeywordsHash ? this : null);
151154
if (isAttrAssign) {
152155
final Object value = rubyArgs[rubyArgs.length - 1];
153156
assert RubyGuards.assertIsValidRubyValue(value);
@@ -165,7 +168,7 @@ public Object executeWithArgumentsEvaluated(VirtualFrame frame, Object receiverO
165168
RubyArguments.setSelf(rubyArgs, receiverObject);
166169
RubyArguments.setBlock(rubyArgs, blockObject);
167170
RubyArguments.setArguments(rubyArgs, argumentsObjects);
168-
return doCall(frame, receiverObject, descriptor, rubyArgs);
171+
return doCall(frame, receiverObject, descriptor, rubyArgs, false);
169172
}
170173

171174
private Object executeBlock(VirtualFrame frame) {

0 commit comments

Comments
 (0)