Skip to content

Bind only public interface methods for private classes#5984

Merged
headius merged 2 commits intojruby:masterfrom
headius:private_class_method_binding
Dec 2, 2019
Merged

Bind only public interface methods for private classes#5984
headius merged 2 commits intojruby:masterfrom
headius:private_class_method_binding

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 Reduce overhead of binding Java classes and methods #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:

jruby/spec/java_integration/fixtures/InnerClasses.java

Lines 78 to 95 in b35397b

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; }
};
}

@headius
Fix two issues with binding methods.
d764ffe 
* 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 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.

@headius
Copy link
Member Author

headius commented Dec 2, 2019 •
edited
Loading

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

@headius
Remove these tests that don't pass with proper method binding.
cd72560 
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
@headius headius deleted the private_class_method_binding branch December 2, 2019 21:05
@headius headius mentioned this pull request Dec 3, 2019
Closed
@kares
Copy link
Member

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

Reviewers

@enebo enebo enebo approved these changes

Assignees

No one assigned

Labels

java integration

Projects

None yet

Milestone

JRuby 9.2.10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@headius @kares @enebo