Closure conversion doesn't play nice with some interface method names

Your theory is probably correct; it's the first thought that came to mind for me as well. It does present a conundrum; obviously we'd like any interface to be convertible, regardless of whether the methods it defines already exist on converted object. So one approach would be to proactively define them to dispatch to the proc; However...we'd have to special-case this logic just for procs/blocks, since other converted objects would want their already-existing methods to be used.

That special-casing may not be a big deal, though. If a block passed to an interface-receiving method were re-wrapped in an additional layer that forces all calls through the block, it would be possible to implement any interface regardless of the methods it might define. And this would not really be unexpected; a literal block is not outwardly a proc "yet", and so there's no normal way a user would have actively defined methods on it expecting them to receive the interface calls.

Marking for 1.6. Time is short for the 1.6 release, but there are a couple possible courses of action:

• Fix it as above, with the block being wrapped in an additional layer that forces the interface methods through the block body. There are potential side effects if the converted block (as a Proc) is expected to be the same type of object it is now (namely, a Proc with method_missing defined).
• Provide a warning for cases where the interface method(s) in question is (are) already implemented by what would be a Proc object. This would not fix it, but would avoid folks spinning their wheels when peculiar errors arise. It would also, however, incur a hit for closure-converting calls since they would need to scan Proc for the N interface methods.
• Do nothing. This is the first report of this behavior causing problems in the wild in the 2-3 years we have had closure conversion in place. Deal with it in a future release after more careful thought.

Sorry for the confusion on this...I can appreciate that it must have been very confusing. And to be honest, I'm surprised we had not though of it before

A couple of thoughts:

• While the biggest pain point working through this was the lack of a usable error message from JRuby (making option 2 sound nice), I would have saved almost the whole time I spent on this if the behavior had been documented on the wiki or in another Google-juiced-for-"jruby closure conversion" location. For me, at least, noting this somewhere visible would have 99% of the benefit of a warning without the tradeoff you mention (and we might be accomplishing this right now

• As a point of comparison, Groovy's implementation of closure conversion relies on its 'as' keyword, where you explicitly mention the interface you want the closure to implement. Obviously JRuby wouldn't want to add a keyword, but the point is that it might be okay to expose the additional layer you mentioned to the programmer rather than making it happen automatically behind the scenes. A helper function w/ the signature as(interface, &block) wouldn't add much typing and might make a fix easier. Incidentally, this approach might also help with a second small issue I'm having, where JRuby will sometimes pick the wrong overloaded Java method when passed a closure-converted arg.

• The current solution isn't particularly burdensome (just rename the interface method, or if that's not an option, explicitly implement the interface with a Ruby class), so doing nothing sounds reasonable to me if a good solution is hard to settle on.

Thanks for taking a look!

Reproduced using Mirah:

~/projects/jruby ➔ mirahc -e "interface Exec do; def exec(a:String); puts a; end; end"

~/projects/jruby ➔ mirahc -e "class Execer; def doExec(x:Exec); x.exec('foo'); end; end"

~/projects/jruby ➔ jruby -rjava -e "Java::Execer.new.doExec {}"
org/jruby/RubyKernel.java:1729:in `exec': No such file or directory - foo (Errno::ENOENT)
	from -e:1:in `(root)'

I'm pondering solutions.

Wow, I thought of a blindingly simple solution that makes perfect sense: make interface dispatch into its contained object obey visibility rules.

Many of the methods we'd like to avoid accidentally dispatching to, like exec, are defined as private methods on all objects. Here's the full list of private methods for a Proc instance:

~/projects/jruby ➔ jruby -e "Proc.new{}.private_methods.sort.each_slice(5) {|a| p a}"
["Array", "Float", "Integer", "String", "__callee__"]
["__method__", "`", "abort", "at_exit", "autoload"]
["autoload?", "block_given?", "callcc", "caller", "catch"]
["chomp", "chomp!", "chop", "chop!", "eval"]
["exec", "exit", "exit!", "fail", "fork"]
["format", "getc", "gets", "global_variables", "gsub"]
["gsub!", "initialize", "initialize_copy", "iterator?", "lambda"]
["load", "local_variables", "loop", "method_missing", "open"]
["p", "print", "printf", "proc", "putc"]
["puts", "raise", "rand", "readline", "readlines"]
["remove_instance_variable", "require", "scan", "select", "set_trace_func"]
["singleton_method_added", "singleton_method_removed", "singleton_method_undefined", "sleep", "split"]
["sprintf", "srand", "sub", "sub!", "syscall"]
["system", "test", "throw", "trace_var", "trap"]
["untrace_var", "warn"]

If we make the interface dispatching obey visibility, none of these methods will be callable from outside the proc and interface impl logic. Instead, they'll dispatch through method_missing and end up in the body of the proc. Here's confirmation that this is the case:

~/projects/jruby ➔ jruby -rjava -e "Java::Execer.new.doExec {puts 'here'}"
here

Now this isn't a perfect solution; Proc still has a large set of methods that could potentially conflict. But it fixes the method you reported for this bug and removes a large number of other possible conflicts. Here's the list of public Proc methods that would still not be eligible for single-method interface impl with a block:

~/projects/jruby ➔ jruby -e "Proc.new{}.public_methods.each_slice(5) {|a| p a}"
["to_s", "to_proc", "dup", "clone", "=="]
["arity", "binding", "call", "[]", "methods"]
["freeze", "extend", "nil?", "object_id", "tainted?"]
["method", "is_a?", "instance_variable_defined?", "instance_variable_get", "instance_variable_set"]
["hash", "display", "send", "private_methods", "enum_for"]
["equal?", "type", "instance_of?", "id", "taint"]
["class", "instance_variables", "to_a", "__send__", "=~"]
["protected_methods", "inspect", "__id__", "tap", "frozen?"]
["respond_to?", "instance_eval", "===", "untaint", "to_enum"]
["singleton_methods", "eql?", "instance_exec", "kind_of?", "public_methods"]

Most of these are very Ruby-specific. A few might be concerns like "clone" or "send". But I think this may be a good first fix and likely safe and simple enough to get into 1.6.RC2.

Patch to make interface impl use normal visibility-honoring dispatch: https://gist.github.com/798232

Just for the sake of completeness, a corner case that comes to mind is recursive invocation - if I know that the interface method I'm trying to implement through conversion is named 'exec', within the body of the closure I may try to call 'exec' expecting recursive dispatch back to the closure. Since the Proc's private exec method is visible at that point, though, wouldn't it be invoked instead?

If the original issue was so rare, I can't imagine that this scenario would crop up often (if ever), and even independent of the original issue, respecting visibility sounds like the right thing to do anyways. +1, thanks again for taking a look.

Bump to 1.6.1 for further consideration. Still tricky, since needs better error or default case, but noncritical (i.e. not much will visibly change for most users with a fix).

I ran into this bug today while working on SWT when trying to implement OpenWindowListener (http://help.eclipse.org/indigo/topic/org.eclipse.platform.doc.isv/reference/api/org/eclipse/swt/browser/OpenWindowListener.html). The Kernel#open method is being called instead.

I'm working around it by creating an anonymous Ruby class.