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

Initial Spike to support JDK 9 #1994

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@kittylyst

kittylyst commented May 13, 2017

This patch is a first step to supporting Java 9. It fixes some compile errors that the tighter Java 9 compiler insists upon, and refactors classes into the module structure. More work is still needed though - so it may be better on a branch rather than mainline.

@nfekete

This comment has been minimized.

Show comment
Hide comment
@nfekete

nfekete May 13, 2017

Member

If we compile this code, will it work with Java8? (i.e. backward compatible on bytecode level)

Member

nfekete commented May 13, 2017

If we compile this code, will it work with Java8? (i.e. backward compatible on bytecode level)

@kittylyst

This comment has been minimized.

Show comment
Hide comment
@kittylyst

kittylyst May 13, 2017

I do not believe so. What todays experiments have shown me leads me to think that there is no way that a warnings == fatal project can have a source base that compiles cleanly under both JDK 8 & 9. It seems that the javac compiler is much more strongly-conforming in 9 than 8.

kittylyst commented May 13, 2017

I do not believe so. What todays experiments have shown me leads me to think that there is no way that a warnings == fatal project can have a source base that compiles cleanly under both JDK 8 & 9. It seems that the javac compiler is much more strongly-conforming in 9 than 8.

@nfekete

This comment has been minimized.

Show comment
Hide comment
@nfekete

nfekete May 13, 2017

Member

Shouldn't then we keep our primary JDK version at 8, so that we can use it both in JRE8 and 9? I guess at least the bytecode compiled with JDK8 will be forward compatible with JDK9. Or am I wrong?

Member

nfekete commented May 13, 2017

Shouldn't then we keep our primary JDK version at 8, so that we can use it both in JRE8 and 9? I guess at least the bytecode compiled with JDK8 will be forward compatible with JDK9. Or am I wrong?

@kittylyst

This comment has been minimized.

Show comment
Hide comment
@kittylyst

kittylyst May 14, 2017

OK, here's a breakdown of what's in the PR, so you know which bits you might want:

.gitignore - just ignore vi swap files : SAFE for both 8 & 9
file renames & module-info.java : only needed if having a fork or branch that is JDK 9
pom.xml - only needed if having a fork or branch that is JDK 9. Probably a good idea to move to Scala 2.12 soon though, even for Java 8
Changes to the GwtIncompatible.java files - SAFE for both 8 & 9 : not needed for 8 but the current situation is actually incorrect and the 9 compiler exposes it.
io/vavr/Lazy.java - NOT SAFE to backport to 8. 8 is actually incorrect and the 9 compiler exposes it.
io/vavr/AssertionsExtensions.java : SAFE for both 8 & 9. 9 changes behaviour so that this method will emit a deprecation warning whereas 8 won't
io/vavr/MatchTest.java - I have no idea what the correct form for this for 9 is. The current 8 code is wrong, and relies on a type inferencing bug in javac. I could not find a correct form for 9, and exposed a javac bug in 9 when trying to forward port it. The current code is unsafe and almost certainly doesn't mean what you think it does.

I know this isn't a great story for you, but please consider how you might support 9 going forwards. Vavr is a great project, and as it has so few dependencies, it could be made into a set of explicit modules, which would be great to see. Thanks for all the work so far!

kittylyst commented May 14, 2017

OK, here's a breakdown of what's in the PR, so you know which bits you might want:

.gitignore - just ignore vi swap files : SAFE for both 8 & 9
file renames & module-info.java : only needed if having a fork or branch that is JDK 9
pom.xml - only needed if having a fork or branch that is JDK 9. Probably a good idea to move to Scala 2.12 soon though, even for Java 8
Changes to the GwtIncompatible.java files - SAFE for both 8 & 9 : not needed for 8 but the current situation is actually incorrect and the 9 compiler exposes it.
io/vavr/Lazy.java - NOT SAFE to backport to 8. 8 is actually incorrect and the 9 compiler exposes it.
io/vavr/AssertionsExtensions.java : SAFE for both 8 & 9. 9 changes behaviour so that this method will emit a deprecation warning whereas 8 won't
io/vavr/MatchTest.java - I have no idea what the correct form for this for 9 is. The current 8 code is wrong, and relies on a type inferencing bug in javac. I could not find a correct form for 9, and exposed a javac bug in 9 when trying to forward port it. The current code is unsafe and almost certainly doesn't mean what you think it does.

I know this isn't a great story for you, but please consider how you might support 9 going forwards. Vavr is a great project, and as it has so few dependencies, it could be made into a set of explicit modules, which would be great to see. Thanks for all the work so far!

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich May 16, 2017

Member

Hi Ben,

thank you for the PR. It is great to see a JCP member here!

So far, I haven't found a reason to bump Vavr's binary code to the upcoming Java version. JDK9 doesn't offer any features that will enhance our API. If we would use modules under the hood, we would just ensure that we expose the right things to the outside. Currently we use workarounds for that purpose, like auxiliary classes and package private classes (I know, even if they are not part of the contract, they are currently discoverable via reflection).

However, it is important that Java 9 applications are able to use the Vavr binaries compiled with Java 8. This is one thing that has to be investigated. I haven't found the time to get my hands on Java 9, yet (besides jshell - I love it).

Thank you also for your summary. One thing I don't like to see is that GWT will not be able any more to find the package-private GwtIncompatible annotation, which I do not want to expose to the outside. It is also no option to pull in a 3rd party library to get this annotation, Vavr is a zero-dependency library.

It really surprises me that Lazy and MatchTest (resp. our pattern matching API) are incorrect. We put much effort in the implementation and from my point of view all makes perfectly sense. For example I tested pretty well that types are recursively inferred along the pattern cascade. Please note that the match cases are partial functions. I.e. a pattern might not match if the given sub-types do not match the actual types. Especially we did not implement the visitor pattern, we did something more general. It is like Scala but without exhaustiveness checks by the compiler.

The current code is unsafe and almost certainly doesn't mean what you think it does.

Is it the exhaustiveness you address? Could you please elaborate on this?

I could not find a correct form for 9, and exposed a javac bug in 9 when trying to forward port it.

Did you already filed a bug on Oracles bug-tracker?

I think I have to dive deeper into Java 9 in order to understand what went wrong with Lazy and Match. Your investigation makes perfectly sense. We will see how we can benefit from Java modules. At the latest we will jump to the next binary version with Java 10, there are a lot of interesting features we are waiting for.

Member

danieldietrich commented May 16, 2017

Hi Ben,

thank you for the PR. It is great to see a JCP member here!

So far, I haven't found a reason to bump Vavr's binary code to the upcoming Java version. JDK9 doesn't offer any features that will enhance our API. If we would use modules under the hood, we would just ensure that we expose the right things to the outside. Currently we use workarounds for that purpose, like auxiliary classes and package private classes (I know, even if they are not part of the contract, they are currently discoverable via reflection).

However, it is important that Java 9 applications are able to use the Vavr binaries compiled with Java 8. This is one thing that has to be investigated. I haven't found the time to get my hands on Java 9, yet (besides jshell - I love it).

Thank you also for your summary. One thing I don't like to see is that GWT will not be able any more to find the package-private GwtIncompatible annotation, which I do not want to expose to the outside. It is also no option to pull in a 3rd party library to get this annotation, Vavr is a zero-dependency library.

It really surprises me that Lazy and MatchTest (resp. our pattern matching API) are incorrect. We put much effort in the implementation and from my point of view all makes perfectly sense. For example I tested pretty well that types are recursively inferred along the pattern cascade. Please note that the match cases are partial functions. I.e. a pattern might not match if the given sub-types do not match the actual types. Especially we did not implement the visitor pattern, we did something more general. It is like Scala but without exhaustiveness checks by the compiler.

The current code is unsafe and almost certainly doesn't mean what you think it does.

Is it the exhaustiveness you address? Could you please elaborate on this?

I could not find a correct form for 9, and exposed a javac bug in 9 when trying to forward port it.

Did you already filed a bug on Oracles bug-tracker?

I think I have to dive deeper into Java 9 in order to understand what went wrong with Lazy and Match. Your investigation makes perfectly sense. We will see how we can benefit from Java modules. At the latest we will jump to the next binary version with Java 10, there are a lot of interesting features we are waiting for.

@kittylyst

This comment has been minimized.

Show comment
Hide comment
@kittylyst

kittylyst May 22, 2017

Hi Daniel - thanks for the comments on the PR. We are making progress on this - including having collaborated to help get a version of maven-compiler-plugin (I think it will be 3.6.2) that works with 9. Currently, having problems with maven-bundle-plugin. The problem seems to be in bndtools - and some folk from the OSGi community are helping us look at the issue.

I think the current issue we're facing is related to this thread - #1225 - is there a workaround that doesn't involve using the maven-bundle-plugin ? - as apparently it will be a while before it gets updated to handle JDK 9.

I started a thread about this here: https://groups.google.com/forum/#!forum/bndtools-users - maybe you could come & join?

Will respond to the API and other points in a separate comment - getting the build working is my top priority because of the time-sensitivity.

kittylyst commented May 22, 2017

Hi Daniel - thanks for the comments on the PR. We are making progress on this - including having collaborated to help get a version of maven-compiler-plugin (I think it will be 3.6.2) that works with 9. Currently, having problems with maven-bundle-plugin. The problem seems to be in bndtools - and some folk from the OSGi community are helping us look at the issue.

I think the current issue we're facing is related to this thread - #1225 - is there a workaround that doesn't involve using the maven-bundle-plugin ? - as apparently it will be a while before it gets updated to handle JDK 9.

I started a thread about this here: https://groups.google.com/forum/#!forum/bndtools-users - maybe you could come & join?

Will respond to the API and other points in a separate comment - getting the build working is my top priority because of the time-sensitivity.

kittylyst and others added some commits May 22, 2017

@kittylyst

This comment has been minimized.

Show comment
Hide comment
@kittylyst

kittylyst May 25, 2017

Just to follow up on this:

There are now fixes in place for maven-compiler-plugin and BndTools that have progressed the build. However, the test cases are still failing to compile. The issue appears to be within Vavr itself, to do with the generated test cases.

As I'm not too familiar with the source generation that Vavr does yet - if someone has some cycles to help me look at this, it would be greatly appreciated.

kittylyst commented May 25, 2017

Just to follow up on this:

There are now fixes in place for maven-compiler-plugin and BndTools that have progressed the build. However, the test cases are still failing to compile. The issue appears to be within Vavr itself, to do with the generated test cases.

As I'm not too familiar with the source generation that Vavr does yet - if someone has some cycles to help me look at this, it would be greatly appreciated.

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Jun 13, 2017

Member

@kittylyst thank you for all your effort! Currently I'm on vacation (until end of June). Then I will help you on this issue. Currently I have to catch up with all the pending comments and pull requests...

I have some questions:

  • You said "getting the build working is my top priority because of the time-sensitivity". Why is it time-sensitive? (JDK 9 GA is deferred to 21 Sep '17)
  • I do not expect (enterprise) users to early adopt Java 9 when it is released. Too many open tooling questions, library incompatibilities etc. My goal is to keep Vavr Java 8 compatible. Do I understand correctly, that this PR isn't Java 8 compatible any more?

However, I think I need to get my hands dirty in order to understand the impact :)

Member

danieldietrich commented Jun 13, 2017

@kittylyst thank you for all your effort! Currently I'm on vacation (until end of June). Then I will help you on this issue. Currently I have to catch up with all the pending comments and pull requests...

I have some questions:

  • You said "getting the build working is my top priority because of the time-sensitivity". Why is it time-sensitive? (JDK 9 GA is deferred to 21 Sep '17)
  • I do not expect (enterprise) users to early adopt Java 9 when it is released. Too many open tooling questions, library incompatibilities etc. My goal is to keep Vavr Java 8 compatible. Do I understand correctly, that this PR isn't Java 8 compatible any more?

However, I think I need to get my hands dirty in order to understand the impact :)

<jmh.version>1.18</jmh.version>
<junit.version>4.12</junit.version>
<gwt.version>2.8.0</gwt.version>
<maven.build-helper.version>1.12</maven.build-helper.version>
<maven.bundle.version>3.2.0</maven.bundle.version>
<maven.bundle.version>3.3.0</maven.bundle.version>

This comment has been minimized.

@paplorinc

paplorinc Jul 14, 2017

Contributor

If you separate these in another PR - so that it stays Java 8 & 9 compatible - I'm sure that will be merged :)

@paplorinc

paplorinc Jul 14, 2017

Contributor

If you separate these in another PR - so that it stays Java 8 & 9 compatible - I'm sure that will be merged :)

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Jul 15, 2017

Member

This PR is important! Currently it is not priority 1 but it will flow into the master at some point...

Member

danieldietrich commented Jul 15, 2017

This PR is important! Currently it is not priority 1 but it will flow into the master at some point...

@paplorinc

Good job, though I would definitely separate it into multiple pull requests, as it obviously cannot be merged in this way (e.g. it's not Java 8 compatible).

@@ -87,8 +87,26 @@ Note: The maven build currently needs to be started from the vavr root dir
<maven.exec.version>1.5.0</maven.exec.version>
<maven.gwt.plugin>1.0-rc-6</maven.gwt.plugin>
<scala.maven.version>3.2.2</scala.maven.version>
<scala.version>2.11.7</scala.version>
</properties>
<scala.version>2.12.2</scala.version>

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

👍 for Scala update
(code generation crashed otherwise anyway)

@paplorinc

paplorinc Sep 20, 2017

Contributor

👍 for Scala update
(code generation crashed otherwise anyway)

@@ -101,7 +101,7 @@ private Lazy(Supplier<? extends T> supplier) {
*/
public static <T> Lazy<Seq<T>> sequence(Iterable<? extends Lazy<? extends T>> values) {
Objects.requireNonNull(values, "values is null");
return Lazy.of(() -> Vector.ofAll(values).map(Lazy::get));
return Lazy.of(() -> Vector.ofAll(values).map(Lazy<? extends T>::get));

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

i think this is a bug in both the JDK and in Idea, they report different things for every variation of the code

@paplorinc

paplorinc Sep 20, 2017

Contributor

i think this is a bug in both the JDK and in Idea, they report different things for every variation of the code

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

An alternative suggestion that Idea accepts also:

@SuppressWarnings("Convert2MethodRef") // TODO fixme on Java 9
public static <T> Lazy<Seq<T>> sequence(Iterable<? extends Lazy<? extends T>> values) {
    Objects.requireNonNull(values, "values is null");
    return Lazy.of(() -> Vector.ofAll(values).map(v -> v.get()));
}
@paplorinc

paplorinc Sep 20, 2017

Contributor

An alternative suggestion that Idea accepts also:

@SuppressWarnings("Convert2MethodRef") // TODO fixme on Java 9
public static <T> Lazy<Seq<T>> sequence(Iterable<? extends Lazy<? extends T>> values) {
    Objects.requireNonNull(values, "values is null");
    return Lazy.of(() -> Vector.ofAll(values).map(v -> v.get()));
}
@@ -27,6 +27,7 @@ public static ClassAssert assertThat(Class<?> clazz) {
this.clazz = clazz;
}
@SuppressWarnings("deprecation")

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

instead of this I would recommend using the new canAccess method, something like:

public void isNotInstantiable() {
    try {
        Assertions.assertThat(clazz.getDeclaredConstructor().canAccess(null)).isFalse();
    } catch (NoSuchMethodException e) {
        throw new AssertionError("no default constructor found");
    }
}

Edit: This isn't java 8 compatible, my mistake

@paplorinc

paplorinc Sep 20, 2017

Contributor

instead of this I would recommend using the new canAccess method, something like:

public void isNotInstantiable() {
    try {
        Assertions.assertThat(clazz.getDeclaredConstructor().canAccess(null)).isFalse();
    } catch (NoSuchMethodException e) {
        throw new AssertionError("no default constructor found");
    }
}

Edit: This isn't java 8 compatible, my mistake

assertThat(actual).isEqualTo("Some(1)::List(Some(2.0))");
}
// @SuppressWarnings("UnnecessaryLocalVariable")

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

@danieldietrich, tomorrow is the release date of JDK 9, this commit solves most of the compile problems (I basically recreated everything to validate it, good job, guys (there are two commiters, apparently).
If you could fix these (it's weird generics magic that I didn't have the courage to understand), we could get on the Java 9 wave(r).

@paplorinc

paplorinc Sep 20, 2017

Contributor

@danieldietrich, tomorrow is the release date of JDK 9, this commit solves most of the compile problems (I basically recreated everything to validate it, good job, guys (there are two commiters, apparently).
If you could fix these (it's weird generics magic that I didn't have the courage to understand), we could get on the Java 9 wave(r).

@@ -29,3 +29,6 @@ hs_err_pid*
# -- Emacs
.#*

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

seems unrelated

@paplorinc

paplorinc Sep 20, 2017

Contributor

seems unrelated

@@ -66,15 +66,15 @@ Note: The maven build currently needs to be started from the vavr root dir
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<assertj.core.version>3.6.2</assertj.core.version>
<eclipse.lifecycle.mapping.version>1.0.0</eclipse.lifecycle.mapping.version>
<java.version>1.8</java.version>
<java.version>9</java.version>

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

It won't be merged this way, since it has to be Java 8 compatible also.

@paplorinc

paplorinc Sep 20, 2017

Contributor

It won't be merged this way, since it has to be Java 8 compatible also.

@@ -0,0 +1,7 @@
module io.vavr {

This comment has been minimized.

@paplorinc

paplorinc Sep 20, 2017

Contributor

Is this still Java 8 compatible?
@danieldietrich do we need a separate, modularized Java 9 release also?

@paplorinc

paplorinc Sep 20, 2017

Contributor

Is this still Java 8 compatible?
@danieldietrich do we need a separate, modularized Java 9 release also?

<maven.clean.version>3.0.0</maven.clean.version>
<maven.install.version>2.5.2</maven.install.version>
<maven.compiler.version>3.6.0</maven.compiler.version>
<maven.compiler.version>3.6.2-SNAPSHOT</maven.compiler.version>

This comment has been minimized.

@paplorinc

paplorinc Sep 23, 2017

Contributor

3.7.0 is already available and it solves the compiler exception also

@paplorinc

paplorinc Sep 23, 2017

Contributor

3.7.0 is already available and it solves the compiler exception also

// public void shouldDecomposeListWithNonEmptyTail() {
// final List<Option<Number>> intOptionList = List.of(Option.some(1), Option.some(2.0));
// final String actual = Match(intOptionList).of(
// Case($Cons($Some($(1)), $Cons($Some($(2.0)), $())), (x, xs) -> {

This comment has been minimized.

@paplorinc

paplorinc Sep 23, 2017

Contributor

The following seems to work:

Case($Cons($Some($(1)), $Cons($Some($(2.0)), $())),
    (Some<Number> x, List<Option<Number>> xs) -> x + "::" + xs)
@paplorinc

paplorinc Sep 23, 2017

Contributor

The following seems to work:

Case($Cons($Some($(1)), $Cons($Some($(2.0)), $())),
    (Some<Number> x, List<Option<Number>> xs) -> x + "::" + xs)
@@ -279,6 +297,13 @@ Note: The maven build currently needs to be started from the vavr root dir
</goals>
</execution>
</executions>
<dependencies>

This comment has been minimized.

@paplorinc

paplorinc Sep 23, 2017

Contributor

why was this needed?

@paplorinc

paplorinc Sep 23, 2017

Contributor

why was this needed?

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