Make `VAVr` Java 8 & 9 compatible #2105

l0rinc · 2017-09-23T13:03:48Z

related to:

codecov-io · 2017-09-23T14:12:12Z

Codecov Report

Merging #2105 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #2105   +/-   ##
========================================
  Coverage      97.5%   97.5%           
  Complexity     5202    5202           
========================================
  Files            92      92           
  Lines         11944   11944           
  Branches       1576    1576           
========================================
  Hits          11646   11646           
  Misses          148     148           
  Partials        150     150

Impacted Files	Coverage Δ	Complexity Δ
vavr/src/main/java/io/vavr/Lazy.java	`100% <100%> (ø)`	`34 <1> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8cb9ca...3aa7ad0. Read the comment docs.

danieldietrich

Hi, great - many thanks!
It would be cool to have jdk8 & jdk9 travis-ci builds. We could accomplish it by adding jdk9 to the .travis.yml.

See https://docs.travis-ci.com/user/languages/java/#Testing-Against-Multiple-JDKs

danieldietrich · 2017-09-23T15:52:47Z

pom.xml

@@ -32,6 +32,7 @@ We use these goals frequently to keep the dependencies and plugins up-to-date:
    <description>Vavr (formerly called Javaslang) is an object-functional language extension to Java 8+.</description>
    <modules>
        <module>vavr</module>
+        <module>vavr-benchmark</module>


Does a Java 9 build require now this module? Just asking...
I ask myself if now a jar is released for vavr-benchmark, too (i.e. to maven central).
Is the benchmark profile then still needed?

It's in a separate commit, because it's obviously not strictly related :)
It should be part of the project so that it is compile-time checked. I will disable the deployment, it's a valid concern.

Checked it, since it has tests only, there's nothing to deploy.
The benchmark profile has a run config also, so it still has its purpose - though we can remove the duplicate modules part, thanks for the note! :)

(I had to test this module for Java 9 compatibility also, that's why I modified this part also)

The benchmarks should not be part of the main build, even if it is a minimal benchmark.
If there are assertions/checks, they should be moved to the vavr core unit tests.
Benchmarks are about measuring. The only valid case to run them as part of the tests would be that we break the build if the performance decreases. But that is (currently) not possible.

The benchmarks aren't run during the build. Not even the assertions. just the compilation.
They are just part of the project, to make sure any refactoring is propagated there also.

I see, that's cool.

danieldietrich · 2017-09-23T15:55:07Z

vavr/src/main/java/io/vavr/Lazy.java

    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 -> lazy.get()));


Does a java 9 build not compile with the method reference?

Yeah, it's weird :/
We should probably open a bug for the JDK and for Idea

danieldietrich · 2017-09-23T15:55:22Z

vavr/src/test/java/io/vavr/AssertionsExtensions.java

            try {
-                cons = clazz.getDeclaredConstructor();
+                final Constructor<?> cons = clazz.getDeclaredConstructor();


danieldietrich · 2017-09-23T15:57:01Z

vavr/src/test/java/io/vavr/MatchTest.java

-                    // types are inferred correctly!
-                    final Some<Number> head = x;
-                    final List<Option<Number>> tail = xs;
-                    return head + "::" + tail;


I would keep the block. It was intended to make the types explicitly visible (even if vars are not used).
The difference to (Some<Number> x, List<Option<Number>> xs) -> x + "::" + xs) is that we have List<Option<Number>> tail = xs. I.e. we show which general type is inferred by Match.

Yeah, except that it's not working in Java 9, different types are inferred for some reason.
If I specify it explicitly, as done here, it's working.
Not sure how to fix it otherwise.

l0rinc · 2017-09-24T08:31:27Z

The JDK 9 additional build is a very good idea, let me do that quickly :)

l0rinc · 2017-09-24T09:11:33Z

vavr/src/test/java/io/vavr/MatchTest.java

@@ -315,17 +315,12 @@ public void shouldDecomposeListOfTuple3() {
        assertThat(actual).isEqualTo("(begin, 10, 4.5)::List((middle, 11, 0.0), (end, 12, 1.2))");
    }

-    @SuppressWarnings("UnnecessaryLocalVariable")
    @Test
    public void shouldDecomposeListWithNonEmptyTail() {
        final List<Option<Number>> intOptionList = List.of(Option.some(1), Option.some(2.0));


The problem is, this isn't an intOptionList, it's a numberOptionList

danieldietrich · 2017-09-24T09:51:23Z

vavr/src/test/java/io/vavr/MatchTest.java

@@ -318,12 +319,12 @@ public void shouldDecomposeListOfTuple3() {
    @SuppressWarnings("UnnecessaryLocalVariable")
    @Test
    public void shouldDecomposeListWithNonEmptyTail() {
-        final List<Option<Number>> intOptionList = List.of(Option.some(1), Option.some(2.0));
-        final String actual = Match(intOptionList).of(
+        final List<Option<? extends Number>> numberOptionList = List.of(Option.some(1), Option.some(2.0));


I think because of the added ? extends the build currently breaks. Does it not compile with jdk9 otherwise?
Another fix could be List.<Option<Number>> of(Option.some(1), Option.some(2.0));

Dunno, man, this seems to randomly pass or fail on JDK 9. There's a wedding I should attend, instead I'm debugging this :p

Go to the medding, wan - eh I mean wedding, man. We can do this later :)

darn ... could you take a look, it's hopeless (it just passed locally ...)?

Yes, will do that later today! Currently I'm in the middle of splitting #2093 in to smaller PRs. After that (and after going to german election)...

danieldietrich · 2017-09-24T10:01:20Z

pom.xml

@@ -253,6 +254,8 @@ We use these goals frequently to keep the dependencies and plugins up-to-date:
                            <arg>-Xlint:all</arg>
                            <!-- Workaround for JDK bug https://bugs.openjdk.java.net/browse/JDK-6999068 -->
                            <arg>-Xlint:-processing</arg>
+                            <!-- Enable Java 9 compilation with 1.8 compatibility -->
+                            <arg>-Xlint:-options</arg>


Where do we use the options? Is it the implicit <target>${java.version}</target>?

this is needed because java.version is always 8, even if the selected JDK is 9. The JDK cannot compile to another bytecode version, giving a warning. This disables the warning, since we're only checking the build, not producing any artifacts.

<java.version>1.8</java.version>

l0rinc · 2017-09-25T07:51:58Z

vavr/src/test/java/io/vavr/MatchTest.java

-                Case($Cons($Some($(1)), $Cons($Some($(2.0)), $())), (x, xs) -> {
+        final List<Option<Number>> numberOptionList = List.of(Option.some(1), Option.some(2.0));
+        final String actual = Match(numberOptionList).of(
+                Case($Cons($Some($(1)), $Cons($Some($(2.0)), $())), (Some<Number> x, List<Option<Number>> xs) -> {


@danieldietrich, it only compiles on Java 9 if we explicitly specify the types here. Is that still ok?

danieldietrich

Many thanks, this change looks great!

danieldietrich · 2017-09-25T22:58:39Z

Just realized that the build is broken for jdk9 (you told me already). It does not matter - I will fix that tomorrow. Thx!

* Fix Java 9 warnings * Enable code generation on Java 9 * Update the rest of the build dependencies * Enable the `vavr-benchmark` module * Add Java 9 as an alternative Travis build

l0rinc changed the title ~~Make VAVr compilable under both Java 8 & 9~~ Make VAVr Java 8 & 9 compatible Sep 23, 2017

danieldietrich suggested changes Sep 23, 2017

View reviewed changes

l0rinc commented Sep 24, 2017

View reviewed changes

danieldietrich reviewed Sep 24, 2017

View reviewed changes

danieldietrich added build/infrastructure/ide «cross cutting» labels Sep 24, 2017

danieldietrich modified the milestones: vavr-1.0.0, vavr-0.9.2 Sep 24, 2017

Pap Lőrinc added 5 commits September 25, 2017 10:36

Fix Java 9 warnings

e357ac2

Enable code generation on Java 9

29c7ac5

Update the rest of the build dependencies

f275dd9

Enable the vavr-benchmark module

354965f

Add Java 9 as an alternative Travis build

357ed2f

l0rinc commented Sep 25, 2017

View reviewed changes

danieldietrich approved these changes Sep 25, 2017

View reviewed changes

danieldietrich merged commit 44a23e7 into vavr-io:master Sep 25, 2017

l0rinc deleted the Java9 branch September 26, 2017 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `VAVr` Java 8 & 9 compatible #2105

Make `VAVr` Java 8 & 9 compatible #2105

l0rinc commented Sep 23, 2017

codecov-io commented Sep 23, 2017 •

edited

Loading

danieldietrich left a comment

danieldietrich Sep 23, 2017

l0rinc Sep 24, 2017

l0rinc Sep 24, 2017

l0rinc Sep 24, 2017

danieldietrich Sep 24, 2017 •

edited

Loading

l0rinc Sep 24, 2017

danieldietrich Sep 24, 2017

danieldietrich Sep 23, 2017

l0rinc Sep 24, 2017

danieldietrich Sep 23, 2017

danieldietrich Sep 23, 2017

l0rinc Sep 24, 2017

l0rinc commented Sep 24, 2017

l0rinc Sep 24, 2017

danieldietrich Sep 24, 2017

l0rinc Sep 24, 2017

danieldietrich Sep 24, 2017

l0rinc Sep 24, 2017

danieldietrich Sep 24, 2017

danieldietrich Sep 24, 2017

l0rinc Sep 24, 2017

l0rinc Sep 25, 2017

danieldietrich left a comment

danieldietrich commented Sep 25, 2017

Make VAVr Java 8 & 9 compatible #2105

Make VAVr Java 8 & 9 compatible #2105

Conversation

l0rinc commented Sep 23, 2017

codecov-io commented Sep 23, 2017 • edited Loading

Codecov Report

danieldietrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietrich Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rinc commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietrich left a comment

Choose a reason for hiding this comment

danieldietrich commented Sep 25, 2017

Make `VAVr` Java 8 & 9 compatible #2105

Make `VAVr` Java 8 & 9 compatible #2105

codecov-io commented Sep 23, 2017 •

edited

Loading

danieldietrich Sep 24, 2017 •

edited

Loading