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

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.