Allow public methods from non-public superclasses to bind

[headius]

The existing logic will never bind any methods declared on a non-
public superclass, but this is incorrect behavior if the subclass
is public. A public subclass will expose public methods from any
superclass, regardless of the superclass's visibility, so we need
to also bind those methods.

There are two changes here:

• Allow methods to be gathered from non-public superclasses if the
  target class is public.
• Allow those methods to be bound even if there's no definition on
  the target class (no so-called "local method")

This fixes #6197

[headius]

@enebo @kares We need to discuss whether this is the proper fix.

[headius]

One question I have about this... should we instead just allow all public methods from any class to bind on that class now? The reason we don't bind anything if there's no "local" method is because it expects to see the method on a superclass. However since the superclass is non-public we don't bind any of its methods.

The reason we don't bind public methods from non-public classes is because they can't be invoked directly on that class. For example, there are non-public implementations of Iterator within each JDK collection class; we don't want to bind the version of the method that can't be seen, so we bind the interface method which can.

[headius]

At least one JI spec fails so there's some work to do here.

[kares]

should we instead just allow all public methods from any class to bind on that class now? The reason we don't bind anything if there's no "local" method is because it expects to see the method on a superclass.

That is something I wanted in the past.
I understand the idea behind not binding them but than it's not perfect e.g. with have sub-classes you eventually bind a method to a Java class but than if a Java sub-class enters Ruby land taking the method from super-class method is still going to call the override.

The Java classes simply have Java semantics in terms of taking a reflected method and calling it.
I am obviously biased but I would consider binding everything as is present on the Java class?

Internal types (such as java.util.Iterator impls) having public methods already report all public methods, I don't see why potential internal super-classes would not show their public methods.

[headius]

I am obviously biased but I would consider binding everything as is present on the Java class?

Well let's give it a try and see how CI shakes out.

The problem I expect we'll see is when you have a non-public implementation of a public interface. Instead of only binding the interface form of the method, we'll bind the implementation that's associated with the non-public class. Since that class is not public, we can't dispatch to the implementation and things break.

I've pushed the commit so we can see everything that breaks, but I think this example illustrates it:

$ jruby -e "java.util.ArrayList.new.iterator.next"
TypeError: illegal access on 'next': class org.jruby.javasupport.JavaMethod (in module org.jruby.dist) cannot access a member of class java.util.ArrayList$Itr (in module java.base) with modifiers "public"
  <main> at -e:1

[headius]

Amazingly, that changed passed! But my example case still stands. Apparently we are not testing it!

[headius]

The spec provided by @fidothe in #6198 passes with this change, and I've fixed the one failure. That PR can be merged after this one.

[headius]

An additional two changes were needed:

• Soften the "equivalent methods" check by allowing parent method's return type to be the same or a superclass of the child method's return type (previously they had to be identical).
• Only replace the child method with its equivalent parent method if the parent method's class is public (previously visibility of parent class was not considered).

This fixes the additional failure calling StringBuilder methods that are also defined and public on the package-visible AbstractStringBuilder class. A method like append(String) was not considered equivalent because the AbstractStringBuilder versions returned AbstractStringBuilder, so the AbstractStringBuilder version was added to the list of methods and could be selected as the target. The additional visibility check ensures that the now-equivalent StringBuilder append(String) won't be replaced by the less-visible AbstractStringBuilder version.

[headius]

Looks like this is finally green. The changes have been squashed into a single commit that describes the key four changes made here.

It may still be warranted to start simply using Class#getMethods and binding everything it returns to each class, rather than doing the more complicated parent walk we do now, but I'll merge this.