[type hierarchy] Method references of public classes refer to non-visible methods of sub-classes

[danieldietrich]

@tcn reported the following:

import javaslang.collection.Array;
import javaslang.collection.HashMap;

import java.util.function.BiFunction;

public class Test {

    public static void main(String[] args) {

        // prints HashMap((a, AA), (b, BB), (c, CC))
        test((l, r) -> l.merge(r));

        // Exception in thread "main" java.lang.IllegalAccessError: tried to access class javaslang.collection.AbstractMap from class Test
        test(HashMap::merge);

    }

    static void test(BiFunction<HashMap<String, String>, HashMap<String, String>, HashMap<String, String>> merge) {
        final HashMap<String, String> reduced = Array.of("a", "b", "c")
                .map(t -> HashMap.of(t, t.concat(t).toUpperCase()))
                .reduce(merge);
        System.out.println(reduced);
    }
}

HashMap has no declared method merge. It inherits merge from AbstractMap. The method reference HashMap::merge internally points to AbstractMap::merge. Because our class AbstractMap is package private, we get a java.lang.IllegalAccessError.

We need to check sub-classes of all package private classes!

The solution which solves this bug should not make internal classes like AbstractMap visible. It is not intended to be public API, we added it to reduce code duplication. We could override the methods in the sub-classes and delegate to the methods of the package-private super-class. Maybe it is also an option not to inherit from package-private classes, so we are forced to delegate (to static methods).

[ruslansennov]

First of all we can move all tests to another package

[danieldietrich]

Mmhh, interesting idea. We could do that!

[nipafx]

Came here from Twitter, sorry for barging in.

Are you sure this is the correct analysis? It sounds very "un-Java" and I can not reproduce it in a simple setup (Gist pending).

[nipafx]

Here's the Gist. I can't see anything unusual.

[nipafx]

Gonna keep spamming... :)

This looks like the same case: IllegalAccessError when using a public method reference of a package-private class through a public subclass from another package

[danieldietrich]

Hi Nicolai, I like that kind of spam! More of that :)
Thanks for the links. I ran your code - also ok here (with the line commented out), using jdk1.8.0_74.
Seems to be a jdk bug.

Btw: I'm a big fan of your blog, great stuff!

[danieldietrich]

@ruslansennov hold your horses (moving the tests). In the case of a jdk bug we should wait for a fix.

[tcn]

As a side node: doesn't work on 25.66-b00-graal-vm-0.10 either (same for some old 1.9.0-ea-b90 I still had lying around).
(http://www.oracle.com/technetwork/oracle-labs/program-languages/downloads/index.html)

[danieldietrich]

The tests in Nicolai's gist show that

• it works for references to an instance method of a particular object containingObject::instanceMethodName
• it does not work for references to an instance method of an arbitrary object of a particular type ContainingType::methodName

(see Oracle Docs / Method References)

[ggalmazor]

Just pitching in some (maybe not useful) theory here:

Is AbstractMap a key piece in Map's type hierarchy? If not, maybe this is one of those cases where we try to apply inheritance wrongly while pursuing code reuse.

Code in AbstractMap can be reused without inheritance. Package local static methods will work and what has to be weighed here is the trade off of having some calls duplicated in classes that should use AbstractMap's methods.

[danieldietrich]

@ggalmazor yes, that's what I also thinking about - pulling the AbstractXxx classes out of the collection type hierarchy and provide static helper methods instead.

Even if it is a JDK bug and it will be (or already is?) fixed in a future JDK version, it would be more robust to make it work independent of a specific JDK version.

[ggalmazor]

Glad to hear that from you. I deal with this kind of problems in my daily work a lot and sometimes it's hard to explain this to people. 👍

[danieldietrich]

Even if we rename the test packages we can't ensure that we make exhaustive tests by checking if all methods have sufficient visibility in sub-classes when used in conjunction with method references.

Another approach to test it would be to create a test class MethodReferenceTest that

• reads all the public methods from all (public) types within all javaslang packages
• creates a method reference using reflection / invoke dynamic / low level MethodHandle API
• there are basically two different types: static method references and instance method references
• I'm sure that we have to maintain an exception list / a whitelist of methods that we do not test...

We do not need to do this for release 2.1.0 because it is a nice to have test but it would help to make Javaslang more robust regarding further development.

Towards release 2.1.0 we will look with a sharp eye if there are potential spots that may make problems. Candidates are our (new) abstract collection classes that are package private:

• AbstractBitSet(?) (update: this is compatible with method-references)
• AbstractMap
• AbstractMultiMap
• AbstractQueue

But these types should not introduce new API, I think there are public interfaces that already define the methods, so there should no IllegalAccessErrors occur. We will see...

[danieldietrich]

Related to JDK Bug 8141122

[danieldietrich]

(Found a JDK type inference bug on the way fixing the JDK bug of this issue. Details here: https://gist.github.com/danieldietrich/18902076d53ef09b5c90472b613be85b)

[l0rinc]

Not sure if this changes anything, byt judging from https://www.youtube.com/watch?v=QnMDsI2GbOc

this may be a non-issue in Java 9.

[danieldietrich]

For now we stay backward-compatible to Java 8. Even Javaslang 3.0.0 will not be lifted to Java 9 as minimum version because there are no specific features we could use to make life easier for our users. Java 10 will be different :)

[l0rinc]

I agree, it's just a FYI :)

[danieldietrich]

Yes, more fine-grained visibility features are very nice. What I still miss are sealed types, like in Scala. There are situation I need non-final, public types but want to prohibit that they are extended by types outside of the current scope.

[danieldietrich]

Needs to be re-evaluated, if it is still a problem with Java 11.

[danieldietrich]

In Vavr 1.0 we will not have non-visible abstract classes. Our inline code generator will inject shared code.