Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bind only public interface methods for private classes #5984

Merged
merged 2 commits into from Dec 2, 2019

Conversation

headius
Copy link
Member

@headius headius commented Nov 26, 2019

This includes two fixes introduced (at least in part) by #5832:

  • Private classes were considered for method binding even though
    we no longer set them accessible on Java 9+ when they have not
    already been opened. This resulted in Java 9+ environments
    producing method errors when they attempted to dispatch to those
    methods on private classes.
  • In order to fix the above for private classes with public
    interfaces, this patch also fixes how we traverse a class's
    implemented interfaces. Possibly from changes in #5832, we were
    not walking an interface's superinterfaces, causing us to only
    walk one level in such cases.

In my local tests, this caused failures in spec:ji for specs accessing the following classes; I do not believe that these methods should be callable, since they are on private inner classes without any public interface method:

public static CapsInnerInterface localMethodClass2() {
class CapsImpl implements CapsInnerInterface, Serializable {
private final int counter;
CapsImpl(int counter) { this.counter = counter; }
public String capsMethod() { return "CapsImpl2" + counter; }
}
return new CapsImpl(++capsImplCounter);
}
private static int capsAnonCounter;
public static CapsInnerInterface anonymousMethodClass() {
return new CapsInnerSerial() {
private final int counter = ++capsAnonCounter;
public String capsMethod() { return "CapsAnon" + counter; }
};
}

* Private classes were considered for method binding even though
  we no longer set them accessible on Java 9+ when they have not
  already been opened. This resulted in Java 9+ environments
  producing method errors when they attempted to dispatch to those
  methods on private classes.
* In order to fix the above for private classes with public
  interfaces, this patch also fixes how we traverse a class's
  implemented interfaces. Possibly from changes in jruby#5832, we were
  not walking an interface's superinterfaces, causing us to only
  walk one level in such cases.
@headius headius added this to the JRuby 9.2.10.0 milestone Nov 26, 2019
@headius
Copy link
Member Author

@headius headius commented Nov 26, 2019

@kares @enebo I do not think the failing spec should pass. I have not looked into its lineage yet, but the capsMethod in question is only defined on those private inner classes. It likely passed previously because we were aggressively setting the methods accessible.

Java would not be able to compile against or invoke these methods for a normal invocation.

enebo
enebo approved these changes Nov 27, 2019
@headius
Copy link
Member Author

@headius headius commented Dec 2, 2019

Confirmed in both cases that Java cannot call the capsMethod in question, and so we should not be binding it. The general rule of thumb is that we don't bind methods on non-visible classes.

[] ~/projects/jruby $ javac -cp test/target/test-classes/ CapsTester.java
CapsTester.java:5: error: cannot find symbol
    InnerClasses.localMethodClass().capsMethod();
                                   ^
  symbol:   method capsMethod()
  location: interface CapsInnerInterface
1 error

[] ~/projects/jruby $ javac -cp test/target/test-classes/ CapsTester.java
CapsTester.java:5: error: cannot find symbol
    InnerClasses.anonymousMethodClass().capsMethod();
                                       ^
  symbol:   method capsMethod()
  location: interface CapsInnerInterface
1 error

These were born pending and only the attempt to call `capsMethod`
started to fail with jruby#5984. That method only exists on private
class definitions, and is not callable from normal Java, so we do
not bind it.
@headius headius merged commit 3e8b5d1 into jruby:master Dec 2, 2019
5 checks passed
@headius headius deleted the private_class_method_binding branch Dec 2, 2019
@kares
Copy link
Member

@kares kares commented Dec 3, 2019

makes sense but what about compatibility for users on 8,
they might expect private methods to be callable, isn't this a breaking change?

@enebo
Copy link
Member

@enebo enebo commented Dec 3, 2019

@kares I will make an argument that binding to private without an explicit setAccessible in their code was never intended behavior. In this case we can maybe just point out this was unintended behavior (e.g. a bug). I guess the bigger question with that answer is how common this is in reality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants