jruby 9.0.0.0 Java::JavaLang::Character.name #3262

Closed
ggongaware opened this Issue Aug 18, 2015 · 18 comments

Comments

Projects
None yet
5 participants
@ggongaware

Java::JavaLang::Character.name
ArgumentError: wrong number of arguments (0 for 1)
from (irb):47:in <eval>' from org/jruby/RubyKernel.java:979:ineval'
from org/jruby/RubyKernel.java:1292:in loop' from org/jruby/RubyKernel.java:1099:incatch'
from org/jruby/RubyKernel.java:1099:in `catch'

@rtyler

This comment has been minimized.

Show comment
Hide comment
@rtyler

rtyler Aug 18, 2015

@ggongaware I do not believe this to be a bug since Character.getName(int codePoint) in Java takes a single argument, the code point.

This is the expected exception when a call is made without the necessary arguments, and the behavior is consistent with JRuby 1.7.21

rtyler commented Aug 18, 2015

@ggongaware I do not believe this to be a bug since Character.getName(int codePoint) in Java takes a single argument, the code point.

This is the expected exception when a call is made without the necessary arguments, and the behavior is consistent with JRuby 1.7.21

@rtyler rtyler closed this Aug 18, 2015

@ggongaware

This comment has been minimized.

Show comment
Hide comment
@ggongaware

ggongaware Aug 18, 2015

When calling get name on other random java classes, example:

Java::JavaIo::ByteArrayInputStream.name

you get a string with the class name 'Java::JavaIo::ByteArrayInputStream'

shouldn't Java::JavaLang::Character.name with zero arguments do that same?

When calling get name on other random java classes, example:

Java::JavaIo::ByteArrayInputStream.name

you get a string with the class name 'Java::JavaIo::ByteArrayInputStream'

shouldn't Java::JavaLang::Character.name with zero arguments do that same?

@rtyler

This comment has been minimized.

Show comment
Hide comment
@rtyler

rtyler Aug 18, 2015

@ggongaware AH, I see the confusion! Sorry for misunderstanding what method you were trying to invoke previously.

Since getName() is overloaded it's a bit weird to invoke the right method properly. We do have the #java_send method (this blog post covers pretty well) but that still doesn't work quite right when we try to use it on Java::JavaLang::Character since this appears to be a wrapped class object.

To get the expected result I executed:

[20] pry(main)> Java::JavaLang::Character.to_java.java_send(:getName)
=> "Java::JavaLang::Character"
[21] pry(main)> 

I'm not sure if there's a better way around it, it's awkward behavior that's for sure

rtyler commented Aug 18, 2015

@ggongaware AH, I see the confusion! Sorry for misunderstanding what method you were trying to invoke previously.

Since getName() is overloaded it's a bit weird to invoke the right method properly. We do have the #java_send method (this blog post covers pretty well) but that still doesn't work quite right when we try to use it on Java::JavaLang::Character since this appears to be a wrapped class object.

To get the expected result I executed:

[20] pry(main)> Java::JavaLang::Character.to_java.java_send(:getName)
=> "Java::JavaLang::Character"
[21] pry(main)> 

I'm not sure if there's a better way around it, it's awkward behavior that's for sure

@ggongaware

This comment has been minimized.

Show comment
Hide comment
@ggongaware

ggongaware Aug 18, 2015

Here's a monkey patch I'm using to keep an older library happy:

    Java::JavaLang::Character.class_eval do
       def self.name(*args)
         if args.length > 0
           getName(*args)
         else
          'Java::JavaLang::Character'
         end
       end
    end

Here's a monkey patch I'm using to keep an older library happy:

    Java::JavaLang::Character.class_eval do
       def self.name(*args)
         if args.length > 0
           getName(*args)
         else
          'Java::JavaLang::Character'
         end
       end
    end
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 19, 2015

Member

re-opening as I think this one should be reconsidered for fixing with a slight JI layer refactoring. in this case the mapping of name -> getName is confusing (and a little weird name(xxx)) I believe getName should only be mapped if it takes zero arguments - normal getter.

Member

kares commented Aug 19, 2015

re-opening as I think this one should be reconsidered for fixing with a slight JI layer refactoring. in this case the mapping of name -> getName is confusing (and a little weird name(xxx)) I believe getName should only be mapped if it takes zero arguments - normal getter.

@kares kares reopened this Aug 19, 2015

@kares kares added the JRuby 9000 label Aug 19, 2015

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 19, 2015

Member

@kares the original intent was to only shortcut javabean conventions and not to when there was a conflict. I find it weird that name(a) was ever created

Member

enebo commented Aug 19, 2015

@kares the original intent was to only shortcut javabean conventions and not to when there was a conflict. I find it weird that name(a) was ever created

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 19, 2015

Member

@enebo agreed, already do have some bigger WiP changes - one of which is making name work for packages (by default) - still needs more work before ready for feedback whether to proceed there.

but this one should be simple to fix, is it ok to make such change on 9K now or should it be on hold ?

Member

kares commented Aug 19, 2015

@enebo agreed, already do have some bigger WiP changes - one of which is making name work for packages (by default) - still needs more work before ready for feedback whether to proceed there.

but this one should be simple to fix, is it ok to make such change on 9K now or should it be on hold ?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 19, 2015

Member

@kares I think this can be fixed now on 9k.

Member

enebo commented Aug 19, 2015

@kares I think this can be fixed now on 9k.

@headius headius added this to the JRuby 9.0.2.0 milestone Sep 4, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 4, 2015

Member

I don't think there's a good fix here. The shortcutting of getName to name would break a key method on the proxy classes. There are many libraries like rspec that expect to be able to call name to get the name of the class.

We don't bind the shortcut because of the priority logic that favors methods already defined at the Ruby level over aliases of the Java methods. I think it's just better to tell folks to call getName here.

Member

headius commented Sep 4, 2015

I don't think there's a good fix here. The shortcutting of getName to name would break a key method on the proxy classes. There are many libraries like rspec that expect to be able to call name to get the name of the class.

We don't bind the shortcut because of the priority logic that favors methods already defined at the Ruby level over aliases of the Java methods. I think it's just better to tell folks to call getName here.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 4, 2015

Member

@headius @kares I am really confused what the problem is now. We never made shorthand methods for methods which did not follow Javabean conventions. ...but the original reporter seemingly implies we were shorthanding a getName which took a single argument? If so then this is a bug right? Something which can and should be fixed? Or @headius are you saying we need a mandatory 'name' method on all proxies so we should never short-hand getName ever. Either way something should change right?

Adding 'name' to java packages is a completely different issue and not related to this per se?

Member

enebo commented Sep 4, 2015

@headius @kares I am really confused what the problem is now. We never made shorthand methods for methods which did not follow Javabean conventions. ...but the original reporter seemingly implies we were shorthanding a getName which took a single argument? If so then this is a bug right? Something which can and should be fixed? Or @headius are you saying we need a mandatory 'name' method on all proxies so we should never short-hand getName ever. Either way something should change right?

Adding 'name' to java packages is a completely different issue and not related to this per se?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Sep 4, 2015

Member

@headius me too ... let me try to explain a little more: currently the static Character.getName(String) is aliased by JI to self.name which obviously conflicts a Ruby method - we agreed that the solution in this case touches a 'bigger' problem since it's kind of ugly to map non-true getters (those that accept args) to a ruby reader (self.name(str)) - which is currently happening. only "true" bean-style getters/setters should end up being mapped as readers/writers.

Member

kares commented Sep 4, 2015

@headius me too ... let me try to explain a little more: currently the static Character.getName(String) is aliased by JI to self.name which obviously conflicts a Ruby method - we agreed that the solution in this case touches a 'bigger' problem since it's kind of ugly to map non-true getters (those that accept args) to a ruby reader (self.name(str)) - which is currently happening. only "true" bean-style getters/setters should end up being mapped as readers/writers.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Sep 15, 2015

Member

turns out I was a little wrong - the code already checks if get/set methods are bean-style and only adds Ruby reader/writer methods in such cases. here its Character.getName(int codepoint) which actually is a match for the (bean-style) indexed property convention ... getFoo(int) --> foo(i) that is handled.

the easy fix requires removing that support - people shouldn't be using that much bar.foo(0) looks as a weird alias for bar.getFoo(0) + setters are not mapped - only getters (they would look even weirder) and it wasn't ever documented

Member

kares commented Sep 15, 2015

turns out I was a little wrong - the code already checks if get/set methods are bean-style and only adds Ruby reader/writer methods in such cases. here its Character.getName(int codepoint) which actually is a match for the (bean-style) indexed property convention ... getFoo(int) --> foo(i) that is handled.

the easy fix requires removing that support - people shouldn't be using that much bar.foo(0) looks as a weird alias for bar.getFoo(0) + setters are not mapped - only getters (they would look even weirder) and it wasn't ever documented

kares added a commit that referenced this issue Sep 15, 2015

remove support for aliasing Java indexed getters as Ruby readers (fixes
#3262)

a rarely (if ever used) feature that looks a bit weird and was "not-complete" 
(indexed setters do not receive a special treat) and never documented
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 7, 2015

Member

So what's the verdict here? What still needs to be done, @kares?

Member

headius commented Oct 7, 2015

So what's the verdict here? What still needs to be done, @kares?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 8, 2015

Member

@headius there's a PR open for review as I wanted your or @enebo's approval #3328

Member

kares commented Oct 8, 2015

@headius there's a PR open for review as I wanted your or @enebo's approval #3328

@enebo enebo modified the milestones: JRuby 9.0.3.0, JRuby 9.0.2.0 Oct 15, 2015

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 15, 2015

Member

If no one else objects to @kares PR I will land this right after 9.0.2.0 is out just in case we figure out something does use this afterall (the java beans indexed getter shorthand). I highly doubt anyone is using this because they knew it existed as a feature. It might be someone stumbled on it so I would like this to bake for a full point release in hopes of discovering any users (I doubt there are any though).

Member

enebo commented Oct 15, 2015

If no one else objects to @kares PR I will land this right after 9.0.2.0 is out just in case we figure out something does use this afterall (the java beans indexed getter shorthand). I highly doubt anyone is using this because they knew it existed as a feature. It might be someone stumbled on it so I would like this to bake for a full point release in hopes of discovering any users (I doubt there are any though).

@rtyler

This comment has been minimized.

Show comment
Hide comment

rtyler commented Oct 15, 2015

👍

@enebo enebo modified the milestones: JRuby 9.0.3.0, JRuby 9.0.4.0 Oct 21, 2015

@enebo enebo modified the milestones: JRuby 9.0.5.0, JRuby 9.0.4.0 Nov 12, 2015

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 12, 2015

Member

The PR is good to go but I do not want to land it immediately before 9.0.4.0 so we should land right after release. (@kares you can merge your own PR for this once 9.0.4 is out).

Member

enebo commented Nov 12, 2015

The PR is good to go but I do not want to land it immediately before 9.0.4.0 so we should land right after release. (@kares you can merge your own PR for this once 9.0.4 is out).

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Nov 13, 2015

Member

thx, will do one once I see the pom versions updated to 9.0.5.0-SNAPSHOT on master. ..

Member

kares commented Nov 13, 2015

thx, will do one once I see the pom versions updated to 9.0.5.0-SNAPSHOT on master. ..

@kares kares closed this in #3328 Nov 13, 2015

kares added a commit to kares/jruby that referenced this issue Jan 11, 2017

[ji] deal with conflicting get/is Java method alias (for real)
... unfortunately there's no ordering for the methods thus we have
to also deal with foo being aliased for isFoo while processing getFoo

foo -> getFoo method shall always win, since foo? is there for isFoo
in case both getFoo and isFoo are specified

the update should align nicely with expectations in #3470

due compatibility we can not fix #4432
isAnnotation() + getAnnotation(param) case is different since the get
method isn't really a getter (and we're avoding a clash due #3262)

kares added a commit to kares/jruby that referenced this issue Jan 11, 2017

[ji] deal with conflicting get/is Java method alias (for real)
currently we're nondeterministic and depend on reflected method order

foo -> getFoo method shall always win, since foo? is there for isFoo
in case both getFoo and isFoo are specified

the update should align nicely with expectations in #3470

due compatibility we can not fix #4432
isAnnotation() + getAnnotation(param) case is different since the get
method isn't a (real) getter (and we're avoding a clash due #3262)

kares added a commit to kares/jruby that referenced this issue Jan 11, 2017

[ji] deal with conflicting get/is Java method alias (for real)
currently we're nondeterministic and depend on reflected method order

foo -> getFoo method shall always win, since foo? is there for isFoo
in case both getFoo and isFoo are specified

the update should align nicely with expectations in #3470

due compatibility we can not fix #4432
isAnnotation() + getAnnotation(param) case is different since the get
method isn't a (real) getter (and we're avoding a clash due #3262)

kares added a commit to kares/jruby that referenced this issue Jan 11, 2017

[ji] deal with conflicting get/is Java method alias (for real)
currently we're nondeterministic and depend on reflected method order

foo -> getFoo method shall always win, since foo? is there for isFoo
in case both getFoo and isFoo are specified

the update should align nicely with expectations in #3470

due compatibility we can not fix #4432
isAnnotation() + getAnnotation(param) case is different since the get
method isn't a (real) getter (and we're avoding a clash due #3262)

kares added a commit to kares/jruby that referenced this issue Jan 11, 2017

[ji] deal with conflicting get/is Java method alias (for real)
currently we're nondeterministic and depend on reflected method order

foo -> getFoo method shall always win, since foo? is there for isFoo
in case both getFoo and isFoo are specified

the update should align nicely with expectations in #3470

due compatibility we can not fix #4432
isAnnotation() + getAnnotation(param) case is different since the get
method isn't a (real) getter (and we're avoding a clash due #3262)

kares added a commit to kares/jruby that referenced this issue Jan 11, 2017

[ji] deal with conflicting get/is Java method alias (for real)
currently we're nondeterministic and depend on reflected method order

foo -> getFoo method shall always win, since foo? is there for isFoo
in case both getFoo and isFoo are specified

the update should align nicely with expectations in #3470

due compatibility we can not fix #4432
isAnnotation() + getAnnotation(param) case is different since the get
method isn't a (real) getter (and we're avoding a clash due #3262)

kares added a commit to kares/jruby that referenced this issue Jan 12, 2017

[ji] deal with conflicting get/is Java method alias (for real)
currently we're nondeterministic and depend on reflected method order

foo -> getFoo method shall always win, since foo? is there for isFoo
in case both getFoo and isFoo are specified

the update should align nicely with expectations in #3470

due compatibility we can not fix #4432
isAnnotation() + getAnnotation(param) case is different since the get
method isn't a (real) getter (and we're avoding a clash due #3262)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment