Skip to content

Commit

Permalink
[Truffle] Proper implementation of Method#to_proc
Browse files Browse the repository at this point in the history
  • Loading branch information
@chrisseaton
chrisseaton committed Apr 18, 2015
1 parent 79ed302 commit 06c5d29
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 17 deletions.
3 changes: 1 addition & 2 deletions spec/truffle/tags/core/method/to_proc_tags.txt
View file
@@ -1,3 +1,2 @@
fails:Method#to_proc returns a Proc object with the correct arity
fails:Method#to_proc returns a proc that can be used by define_method
fails:Method#to_proc returns a proc that can receive a block
fails:Method#to_proc returns a Proc which does not depends on the value of self
28 changes: 28 additions & 0 deletions truffle/src/main/java/org/jruby/truffle/nodes/core/MethodNodes.java
View file
Expand Up @@ -233,4 +233,32 @@ public RubyUnboundMethod unbind(RubyMethod method) {

}

@CoreMethod(names = "to_proc")
public abstract static class ToProcNode extends CoreMethodNode {

public ToProcNode(RubyContext context, SourceSection sourceSection) {
super(context, sourceSection);
}

public ToProcNode(ToProcNode prev) {
super(prev);
}

@Specialization
public RubyProc toProc(RubyMethod method) {
return new RubyProc(
getContext().getCoreLibrary().getProcClass(),
RubyProc.Type.LAMBDA,
method.getMethod().getSharedMethodInfo(),
method.getMethod().getCallTarget(),
method.getMethod().getCallTarget(),
method.getMethod().getCallTarget(),
method.getMethod().getDeclarationFrame(),
method.getMethod(),
method.getReceiver(),
null);
}

}

}
17 changes: 11 additions & 6 deletions truffle/src/main/java/org/jruby/truffle/nodes/yield/YieldDispatchHeadNode.java
View file
Expand Up @@ -42,15 +42,20 @@ public Object dispatchWithModifiedSelf(VirtualFrame currentFrame, RubyProc block
// TODO: assumes this also changes the default definee.

Frame frame = block.getDeclarationFrame();
FrameSlot slot = frame.getFrameDescriptor().findOrAddFrameSlot(RubyModule.VISIBILITY_FRAME_SLOT_ID, "dynamic visibility for def", FrameSlotKind.Object);
Object oldVisibility = frame.getValue(slot);

try {
frame.setObject(slot, Visibility.PUBLIC);
if (frame != null) {

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Apr 20, 2015

Member

How could frame be null there?

This comment has been minimized.

Sign in to view

Copy link
@chrisseaton

chrisseaton Apr 20, 2015

Author Contributor

Methods don't have declaration frames - only blocks do. This patch is about turning methods into procs, and so we get procs without declaration frames.

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Apr 20, 2015

Member

Would creating a mostly empty frame be a good alternative?
FrameSlot slot = frame.getFrameDescriptor().findOrAddFrameSlot(RubyModule.VISIBILITY_FRAME_SLOT_ID, "dynamic visibility for def", FrameSlotKind.Object);
Object oldVisibility = frame.getValue(slot);

try {
frame.setObject(slot, Visibility.PUBLIC);

return dispatch.dispatchWithSelfAndBlock(currentFrame, block, self, block.getBlockCapturedInScope(), argumentsObjects);
} finally {
frame.setObject(slot, oldVisibility);
}
} else {
return dispatch.dispatchWithSelfAndBlock(currentFrame, block, self, block.getBlockCapturedInScope(), argumentsObjects);
} finally {
frame.setObject(slot, oldVisibility);
}
}

Expand Down
9 changes: 0 additions & 9 deletions truffle/src/main/ruby/core/shims.rb
View file
Expand Up @@ -114,15 +114,6 @@ class Rational

ENV['TZ'] = 'UTC'

class Method
def to_proc
meth = self
proc { |*args|
meth.call(*args)
}
end
end

class MatchData
def full
@cached_full ||= begin
Expand Down

6 comments on commit 06c5d29

@nirvdrum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the Ruby wrong or just suboptimal? It was taken from the MRI documentation.

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the proc is called with a block the shim ignored it.

@eregon
Copy link
Member

@eregon eregon commented on 06c5d29 Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shim could just take a block and pass it I suppose.
The general issue is to fake the parameters to match the ones of the original methods, which is not possible in Ruby, so it might be needed to define it in Java.

How can we solve the problem of being able to call on non-self?

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think the parameter handling thing may also be important - maybe you can query the arity of a block - and in this case it would tell you it was anything, and maybe it should tell you the arity of the original method.

Rubinius uses a primitive rather than Ruby, so it seemed to make sense to do the same.

Not sure what you mean by being able to call on non-self?

@eregon
Copy link
Member

@eregon eregon commented on 06c5d29 Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by being able to call on non-self?

The failing spec.

@eregon
Copy link
Member

@eregon eregon commented on 06c5d29 Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubinius uses a primitive rather than Ruby, so it seemed to make sense to do the same.

But it does explicitly check if the proc is associated with a method, which is not ideal IMHO, while our native conversion should be able to copy parameters metadata between proc and block. So this commit is good for that.

def parameters
if @ruby_method
return @ruby_method.parameters
elsif @bound_method
return @bound_method.parameters
end

Please sign in to comment.