Skip to content

Commit 0b3843d

Browse files
andrykonchinandrykonchin
committed
Don't use ReadCallerFrameNode in ModuleNodes.DefineMethodNode class
1 parent 66059ef commit 0b3843d

File tree

3 files changed

+132
-48
lines changed

3 files changed

+132
-48
lines changed

‎spec/ruby/core/module/define_method_spec.rb

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,18 +219,55 @@ class DefineMethodSpecClass
219219
o.block_test2.should == o
220220
end
221221

222+
it "raises TypeError if name cannot converted to String" do
223+
-> {
224+
Class.new { define_method(1001, -> {}) }
225+
}.should raise_error(TypeError, /is not a symbol nor a string/)
226+
227+
-> {
228+
Class.new { define_method([], -> {}) }
229+
}.should raise_error(TypeError, /is not a symbol nor a string/)
230+
end
231+
232+
it "converts non-String name to String with #to_str" do
233+
obj = Object.new
234+
def obj.to_str() "foo" end
235+
236+
new_class = Class.new { define_method(obj, -> { :called }) }
237+
new_class.new.foo.should == :called
238+
end
239+
240+
it "raises TypeError when #to_str called on non-String name returns non-String value" do
241+
obj = Object.new
242+
def obj.to_str() [] end
243+
244+
-> {
245+
Class.new { define_method(obj, -> {}) }
246+
}.should raise_error(TypeError, /can't convert Object to String/)
247+
end
248+
222249
it "raises a TypeError when the given method is no Method/Proc" do
223250
-> {
224251
Class.new { define_method(:test, "self") }
225-
}.should raise_error(TypeError)
252+
}.should raise_error(TypeError, "wrong argument type String (expected Proc/Method/UnboundMethod)")
226253

227254
-> {
228255
Class.new { define_method(:test, 1234) }
229-
}.should raise_error(TypeError)
256+
}.should raise_error(TypeError, "wrong argument type Integer (expected Proc/Method/UnboundMethod)")
230257

231258
-> {
232259
Class.new { define_method(:test, nil) }
233-
}.should raise_error(TypeError)
260+
}.should raise_error(TypeError, "wrong argument type NilClass (expected Proc/Method/UnboundMethod)")
261+
end
262+
263+
it "uses provided Method/Proc even if block is specified" do
264+
new_class = Class.new do
265+
define_method(:test, -> { :method_is_called }) do
266+
:block_is_called
267+
end
268+
end
269+
270+
new_class.new.test.should == :method_is_called
234271
end
235272

236273
it "raises an ArgumentError when no block is given" do

‎src/main/java/org/truffleruby/core/exception/CoreExceptions.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,14 @@ public RubyException typeErrorRescueInvalidClause(Node currentNode) {
682682
return typeError("class or module required for rescue clause", currentNode);
683683
}
684684

685+
@TruffleBoundary
686+
public RubyException typeErrorExpectedProcOrMethodOrUnboundMethod(Object object, Node currentNode) {
687+
String badClassName = LogicalClassNode.getUncached().execute(object).fields.getName();
688+
return typeError(
689+
StringUtils.format("wrong argument type %s (expected Proc/Method/UnboundMethod)", badClassName),
690+
currentNode);
691+
}
692+
685693
@TruffleBoundary
686694
public RubyException typeError(String message, Node currentNode, Throwable javaThrowable) {
687695
RubyClass exceptionClass = context.getCoreLibrary().typeErrorClass;

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

Lines changed: 84 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@
7676
import org.truffleruby.interop.ToJavaStringNode;
7777
import org.truffleruby.language.LexicalScope;
7878
import org.truffleruby.language.Nil;
79-
import org.truffleruby.language.NotProvided;
8079
import org.truffleruby.language.RubyBaseNode;
8180
import org.truffleruby.language.RubyBaseNodeWithExecute;
8281
import org.truffleruby.language.RubyConstant;
@@ -1225,52 +1224,90 @@ private void warnAlreadyInitializedConstant(RubyModule module, String name,
12251224

12261225
}
12271226

1227+
@GenerateUncached
12281228
@CoreMethod(
12291229
names = "define_method",
12301230
needsBlock = true,
12311231
required = 1,
12321232
optional = 1,
1233-
split = Split.NEVER,
1234-
argumentNames = { "name", "proc_or_method", "block" })
1235-
@NodeChild(value = "module", type = RubyNode.class)
1236-
@NodeChild(value = "name", type = RubyBaseNodeWithExecute.class)
1237-
@NodeChild(value = "proc", type = RubyNode.class)
1238-
@NodeChild(value = "block", type = RubyNode.class)
1239-
public abstract static class DefineMethodNode extends CoreMethodNode {
1233+
argumentNames = { "name", "proc_or_method", "block" },
1234+
alwaysInlined = true)
1235+
@ImportStatic(RubyArguments.class)
1236+
public abstract static class DefineMethodNode extends AlwaysInlinedMethodNode {
1237+
1238+
//Checkstyle: stop
1239+
@Specialization(guards = { "isMethodParameterProvided(rubyArgs)", "isRubyMethod(getArgument(rubyArgs, 1))" })
1240+
//Checkstyle: resume
1241+
protected RubySymbol defineMethodWithMethod(
1242+
Frame callerFrame, RubyModule module, Object[] rubyArgs, RootCallTarget target,
1243+
@Cached NameToJavaStringNode nameToJavaStringNode,
1244+
@Cached(allowUncached = true) CanBindMethodToModuleNode canBindMethodToModuleNode) {
1245+
final String name = nameToJavaStringNode.execute(RubyArguments.getArgument(rubyArgs, 0));
1246+
final Object method = RubyArguments.getArgument(rubyArgs, 1);
1247+
1248+
return addMethod(module, name, (RubyMethod) method, canBindMethodToModuleNode);
1249+
}
1250+
1251+
//Checkstyle: stop
1252+
@Specialization(guards = { "isMethodParameterProvided(rubyArgs)", "isRubyProc(getArgument(rubyArgs, 1))" })
1253+
//Checkstyle: resume
1254+
protected RubySymbol defineMethodWithProc(
1255+
Frame callerFrame, RubyModule module, Object[] rubyArgs, RootCallTarget target,
1256+
@Cached NameToJavaStringNode nameToJavaStringNode) {
1257+
final String name = nameToJavaStringNode.execute(RubyArguments.getArgument(rubyArgs, 0));
1258+
final Object method = RubyArguments.getArgument(rubyArgs, 1);
12401259

1241-
@Child private ReadCallerFrameNode readCallerFrame = ReadCallerFrameNode.create();
1260+
needCallerFrame(callerFrame, target);
1261+
return addProc(module, name, (RubyProc) method, callerFrame.materialize());
1262+
}
12421263

1243-
@CreateCast("name")
1244-
protected RubyBaseNodeWithExecute coerceToString(RubyBaseNodeWithExecute name) {
1245-
return NameToJavaStringNode.create(name);
1264+
//Checkstyle: stop
1265+
@Specialization(
1266+
guards = { "isMethodParameterProvided(rubyArgs)", "isRubyUnboundMethod(getArgument(rubyArgs, 1))" })
1267+
//Checkstyle: resume
1268+
protected RubySymbol defineMethodWithUnboundMethod(
1269+
Frame callerFrame, RubyModule module, Object[] rubyArgs, RootCallTarget target,
1270+
@Cached NameToJavaStringNode nameToJavaStringNode) {
1271+
final String name = nameToJavaStringNode.execute(RubyArguments.getArgument(rubyArgs, 0));
1272+
final Object method = RubyArguments.getArgument(rubyArgs, 1);
1273+
1274+
needCallerFrame(callerFrame, target);
1275+
return addUnboundMethod(module, name, (RubyUnboundMethod) method, callerFrame.materialize());
12461276
}
12471277

1248-
@TruffleBoundary
1249-
@Specialization
1250-
protected RubySymbol defineMethod(RubyModule module, String name, NotProvided proc, Nil block) {
1251-
throw new RaiseException(getContext(), coreExceptions().argumentError("needs either proc or block", this));
1278+
@Specialization(guards = {
1279+
"isMethodParameterProvided(rubyArgs)",
1280+
"!isExpectedMethodParameterType(getArgument(rubyArgs, 1))" })
1281+
protected RubySymbol defineMethodWithUnexpectedMethodParameterType(
1282+
Frame callerFrame, RubyModule module, Object[] rubyArgs, RootCallTarget target) {
1283+
final Object method = RubyArguments.getArgument(rubyArgs, 1);
1284+
throw new RaiseException(getContext(),
1285+
coreExceptions().typeErrorExpectedProcOrMethodOrUnboundMethod(method, this));
12521286
}
12531287

1254-
@Specialization
1255-
protected RubySymbol defineMethodBlock(
1256-
VirtualFrame frame, RubyModule module, String name, NotProvided proc, RubyProc block) {
1257-
return defineMethodProc(frame, module, name, block, nil);
1288+
@Specialization(guards = { "!isMethodParameterProvided(rubyArgs)", "isBlockProvided(rubyArgs)" })
1289+
protected RubySymbol defineMethodWithBlock(
1290+
Frame callerFrame, RubyModule module, Object[] rubyArgs, RootCallTarget target,
1291+
@Cached NameToJavaStringNode nameToJavaStringNode) {
1292+
final String name = nameToJavaStringNode.execute(RubyArguments.getArgument(rubyArgs, 0));
1293+
final Object block = RubyArguments.getBlock(rubyArgs);
1294+
needCallerFrame(callerFrame, target);
1295+
return addProc(module, name, (RubyProc) block, callerFrame.materialize());
12581296
}
12591297

1260-
@Specialization
1261-
protected RubySymbol defineMethodProc(
1262-
VirtualFrame frame, RubyModule module, String name, RubyProc proc, Nil block) {
1263-
return defineMethod(module, name, proc, readCallerFrame.execute(frame));
1298+
@Specialization(guards = { "!isMethodParameterProvided(rubyArgs)", "!isBlockProvided(rubyArgs)" })
1299+
protected RubySymbol defineMethodWithoutMethodAndBlock(
1300+
Frame callerFrame, RubyModule nodule, Object[] rubyArgs, RootCallTarget target) {
1301+
throw new RaiseException(getContext(), coreExceptions().argumentErrorProcWithoutBlock(this));
12641302
}
12651303

12661304
@TruffleBoundary
1267-
@Specialization
1268-
protected RubySymbol defineMethodMethod(RubyModule module, String name, RubyMethod methodObject, Nil block,
1305+
private RubySymbol addMethod(RubyModule module, String name, RubyMethod method,
12691306
@Cached CanBindMethodToModuleNode canBindMethodToModuleNode) {
1270-
final InternalMethod method = methodObject.method;
1307+
final InternalMethod internalMethod = method.method;
12711308

1272-
if (!canBindMethodToModuleNode.executeCanBindMethodToModule(method, module)) {
1273-
final RubyModule declaringModule = method.getDeclaringModule();
1309+
if (!canBindMethodToModuleNode.executeCanBindMethodToModule(internalMethod, module)) {
1310+
final RubyModule declaringModule = internalMethod.getDeclaringModule();
12741311
if (RubyGuards.isSingletonClass(declaringModule)) {
12751312
throw new RaiseException(getContext(), coreExceptions().typeError(
12761313
"can't bind singleton method to a different class",
@@ -1282,20 +1319,13 @@ protected RubySymbol defineMethodMethod(RubyModule module, String name, RubyMeth
12821319
}
12831320
}
12841321

1285-
module.fields.addMethod(getContext(), this, method.withName(name));
1322+
module.fields.addMethod(getContext(), this, internalMethod.withName(name));
12861323
return getSymbol(name);
12871324
}
12881325

1289-
@Specialization
1290-
protected RubySymbol defineMethod(
1291-
VirtualFrame frame, RubyModule module, String name, RubyUnboundMethod method, Nil block) {
1292-
final MaterializedFrame callerFrame = readCallerFrame.execute(frame);
1293-
return defineMethodInternal(module, name, method, callerFrame);
1294-
}
1295-
12961326
@TruffleBoundary
1297-
private RubySymbol defineMethodInternal(RubyModule module, String name, RubyUnboundMethod method,
1298-
final MaterializedFrame callerFrame) {
1327+
private RubySymbol addUnboundMethod(RubyModule module, String name, RubyUnboundMethod method,
1328+
MaterializedFrame callerFrame) {
12991329
final InternalMethod internalMethod = method.method;
13001330
if (!ModuleOperations.canBindMethodTo(internalMethod, module)) {
13011331
final RubyModule declaringModule = internalMethod.getDeclaringModule();
@@ -1313,12 +1343,11 @@ private RubySymbol defineMethodInternal(RubyModule module, String name, RubyUnbo
13131343
}
13141344
}
13151345

1316-
return addMethod(module, name, internalMethod, callerFrame);
1346+
return addInternalMethod(module, name, internalMethod, callerFrame);
13171347
}
13181348

13191349
@TruffleBoundary
1320-
private RubySymbol defineMethod(RubyModule module, String name, RubyProc proc,
1321-
MaterializedFrame callerFrame) {
1350+
private RubySymbol addProc(RubyModule module, String name, RubyProc proc, MaterializedFrame callerFrame) {
13221351
final RootCallTarget callTargetForLambda = proc.callTargets.getCallTargetForLambda();
13231352
final RubyLambdaRootNode rootNode = RubyLambdaRootNode.of(callTargetForLambda);
13241353
final SharedMethodInfo info = proc.getSharedMethodInfo().forDefineMethod(module, name, proc);
@@ -1329,7 +1358,7 @@ private RubySymbol defineMethod(RubyModule module, String name, RubyProc proc,
13291358
final RubyLambdaRootNode newRootNode = rootNode.copyRootNode(info, newBody);
13301359
final RootCallTarget newCallTarget = newRootNode.getCallTarget();
13311360

1332-
final InternalMethod method = InternalMethod.fromProc(
1361+
final InternalMethod internalMethod = InternalMethod.fromProc(
13331362
getContext(),
13341363
info,
13351364
proc.declarationContext,
@@ -1338,7 +1367,7 @@ private RubySymbol defineMethod(RubyModule module, String name, RubyProc proc,
13381367
Visibility.PUBLIC,
13391368
proc,
13401369
newCallTarget);
1341-
return addMethod(module, name, method, callerFrame);
1370+
return addInternalMethod(module, name, internalMethod, callerFrame);
13421371
}
13431372

13441373
private static class CallMethodWithLambdaBody extends RubyContextSourceNode {
@@ -1371,7 +1400,7 @@ public Object execute(VirtualFrame frame) {
13711400
}
13721401

13731402
@TruffleBoundary
1374-
private RubySymbol addMethod(RubyModule module, String name, InternalMethod method,
1403+
private RubySymbol addInternalMethod(RubyModule module, String name, InternalMethod method,
13751404
MaterializedFrame callerFrame) {
13761405
method = method.withName(name);
13771406

@@ -1381,6 +1410,16 @@ private RubySymbol addMethod(RubyModule module, String name, InternalMethod meth
13811410
return getSymbol(method.getName());
13821411
}
13831412

1413+
protected boolean isMethodParameterProvided(Object[] rubyArgs) {
1414+
final int count = RubyArguments.getPositionalArgumentsCount(rubyArgs, false);
1415+
return count >= 2;
1416+
}
1417+
1418+
protected boolean isExpectedMethodParameterType(Object method) {
1419+
return RubyGuards.isRubyMethod(method) || RubyGuards.isRubyUnboundMethod(method) ||
1420+
RubyGuards.isRubyProc(method);
1421+
}
1422+
13841423
}
13851424

13861425
@CoreMethod(names = "extend_object", required = 1, visibility = Visibility.PRIVATE)

0 commit comments

Comments
 (0)