JRuby (please use github issues at http://bugs.jruby.org)
  1. JRuby (please use github issues at http://bugs.jruby.org)
  2. JRUBY-5294

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

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: JRuby 1.5.6
    • Fix Version/s: JRuby 1.6.4
    • Component/s: Java Integration
    • Labels:
      None
    • Environment:
      Mac OS X 10.6.5
      Java(TM) SE Runtime Environment (build 1.6.0_22-b04-307-10M3261)
      Java HotSpot(TM) 64-Bit Server VM (build 17.1-b03-307, mixed mode)
      JRuby 1.5.6
    • Number of attachments :
      1

      Description

      Define a Java interface with a single-argument method named 'exec'. Define a Java class that defines a method that uses instances of that interface as a callback. Use JRuby to invoke the class's method using a block as its argument, attempting to take advantage of the closure conversion feature of JRuby.

      When the callback is invoked by the Java method, rather than running the code defined in the block, the system attempts to coerce the argument to a String and Kernel.exec it. The same issue happens for any Kernel.* method name I've tried.

      I'm a complete Ruby/JRuby noob, but based on a quick perusal of the code, it looks like closure conversion is implemented by defining method_missing on a fresh new RubyClass, and that any methods already defined for Ruby's Object are going to be invoked ahead of any interface methods with the same names. This behavior may be completely expected, then; if so, I apologize for the trouble.

      I've attached example code illustrating the problem.

        Activity

        Hide
        Charles Oliver Nutter added a comment -

        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

        Show
        Charles Oliver Nutter added a comment - 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
        Hide
        Jason Fager added a comment -

        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!

        Show
        Jason Fager added a comment - 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!
        Hide
        Charles Oliver Nutter added a comment -

        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.

        Show
        Charles Oliver Nutter added a comment - 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.
        Hide
        Charles Oliver Nutter added a comment -

        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.

        Show
        Charles Oliver Nutter added a comment - 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.
        Hide
        Charles Oliver Nutter added a comment -

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

        Show
        Charles Oliver Nutter added a comment - Patch to make interface impl use normal visibility-honoring dispatch: https://gist.github.com/798232
        Hide
        Jason Fager added a comment -

        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.

        Show
        Jason Fager added a comment - 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.
        Hide
        Charles Oliver Nutter added a comment -

        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).

        Show
        Charles Oliver Nutter added a comment - 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).
        Hide
        Jeremy Stephens added a comment -

        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.

        Show
        Jeremy Stephens added a comment - 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.

          People

          • Assignee:
            Unassigned
            Reporter:
            Jason Fager
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: