Clojure

Clojure master fails on JDK 11 EA builds due to overloaded toArray in gvec.clj

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.10
  • Component/s: None
  • Labels:
  • Environment:
    java version "11-ea" 2018-09-25
    Java(TM) SE Runtime Environment 18.9 (build 11-ea+21)
    Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11-ea+21, mixed mode)
    OS : Ubuntu
  • Patch:
    Code
  • Approval:
    Ok

Description

The java.util.Collection interface has long had the methods:

Object[] toArray()
<T> T[] toArray​(T[] a)

JDK 11 adds a new method with default impl:

default <T> T[] toArray​(IntFunction<T[]> generator)

For cases of a deftype implementing java.util.Collection, this makes existing implementations of the 1-arity toArray ambiguous. In Java, static typing means that existing implementors are not broken, but in our dynamic typing world, we now have ambiguity. *shakes fist at Java*

In Clojure itself, this comes up in gvec in the primitive vector implementation, which is done with deftype. This breaks the compilation of Clojure on JDK 11. This has also come up in some external projects that do the same thing (like core.rrbvector). See CRRBV-18.

Proposed: Add a type hint to disambiguate in JDK 11+.

This breaks our compilation of deftype implementation of java.util.Collection, which includes primitive vector in gvec, but also many external Clojure implementors.

Patch: clj-2374-2.patch

  1. clj-2374.patch
    21/Sep/18 8:33 AM
    1 kB
    Alex Miller
  2. clj-2374-2.patch
    01/Oct/18 9:30 AM
    1.0 kB
    Alex Miller

Activity

Hide
Alex Miller added a comment -

We added JDK 11 EA to our test build matrix yesterday but I had not had the chance to file this yet (https://build.clojure.org/job/clojure-test-matrix/), so thanks! I think it's worth looking into Compiler/Reflector changes to see if this is fixable at a different level.

I don't see that this means that this or other versions of Clojure would not work on Java 11. The bytecode produced is still compatible; this is an issue caused by a JDK lib change.

Show
Alex Miller added a comment - We added JDK 11 EA to our test build matrix yesterday but I had not had the chance to file this yet (https://build.clojure.org/job/clojure-test-matrix/), so thanks! I think it's worth looking into Compiler/Reflector changes to see if this is fixable at a different level. I don't see that this means that this or other versions of Clojure would not work on Java 11. The bytecode produced is still compatible; this is an issue caused by a JDK lib change.
Hide
Ghadi Shayban added a comment -

That's right, AOT'ed bytecode will still work on JDK11, but you can't build gvec on JDK11 because there is a new overload to toArray that requires a type hint. We could add the disambiguation hint now in anticipation of the change.

Show
Ghadi Shayban added a comment - That's right, AOT'ed bytecode will still work on JDK11, but you can't build gvec on JDK11 because there is a new overload to toArray that requires a type hint. We could add the disambiguation hint now in anticipation of the change.
Hide
Alex Miller added a comment -

I don't think it's a new overload - the arity existed, but now has a default implementation, right?

Show
Alex Miller added a comment - I don't think it's a new overload - the arity existed, but now has a default implementation, right?
Hide
Ghadi Shayban added a comment -

That's right, it's a new method with the same name and arity, but different arg types. Users can override default interface methods, so from our perspective we need to have a type hint to disambiguate the new method when reifying:

toArray(T[] a)
toArray(IntFunction<T[]> generator) # added

(This method could have been added with a new name!)

Show
Ghadi Shayban added a comment - That's right, it's a new method with the same name and arity, but different arg types. Users can override default interface methods, so from our perspective we need to have a type hint to disambiguate the new method when reifying:
toArray(T[] a)
toArray(IntFunction<T[]> generator) # added
(This method could have been added with a new name!)
Hide
Peter Monks added a comment -

Will a fix to this issue be back-ported to older versions of Clojure? If so, which ones?

Show
Peter Monks added a comment - Will a fix to this issue be back-ported to older versions of Clojure? If so, which ones?
Hide
Ghadi Shayban added a comment -

No backport necessary as it's only a bug when compiling Clojure itself on JDK 11

Show
Ghadi Shayban added a comment - No backport necessary as it's only a bug when compiling Clojure itself on JDK 11
Hide
Peter Monks added a comment - - edited

@Ghadi hmmm.........I seem to be seeing it with (at least) Clojure 1.7.0 through 1.10.0-snapshot on JDK 11: https://travis-ci.com/pmonks/multigrep/jobs/148464731, and not when compiling Clojure. This link is for a basic CI build of a very simple (zero dependency, beyond Clojure itself) Clojure library that simply consumes the official, published Clojure JARs for these versions.

Any ideas why I might be seeing that?

I'll admit that I've never bothered to find out exactly which parts of core Clojure are AOTed vs compiled-at-runtime, and so one possible explanation is that this issue is tickled by some of the compiled-at-runtime portions of core Clojure.

Show
Peter Monks added a comment - - edited @Ghadi hmmm.........I seem to be seeing it with (at least) Clojure 1.7.0 through 1.10.0-snapshot on JDK 11: https://travis-ci.com/pmonks/multigrep/jobs/148464731, and not when compiling Clojure. This link is for a basic CI build of a very simple (zero dependency, beyond Clojure itself) Clojure library that simply consumes the official, published Clojure JARs for these versions. Any ideas why I might be seeing that? I'll admit that I've never bothered to find out exactly which parts of core Clojure are AOTed vs compiled-at-runtime, and so one possible explanation is that this issue is tickled by some of the compiled-at-runtime portions of core Clojure.
Hide
Andy Fingerhut added a comment -

I think Ghadi's comment may be intended to mean: "there are JARs for earlier Clojure releases published already, and most people use those rather than compiling Clojure from source code themselves". The only people that will ever see this error are a few developers wanting to build old versions of Clojure from source code.

That said, for those few it might be convenient for them if they didn't have to hunt down this patch and apply it themselves, I guess, but I would understand if the Clojure core team would rather document this for developers, rather than put out ".1" releases of older Clojure versions for a Clojure-build-time-with-latest-JDK issue.

Show
Andy Fingerhut added a comment - I think Ghadi's comment may be intended to mean: "there are JARs for earlier Clojure releases published already, and most people use those rather than compiling Clojure from source code themselves". The only people that will ever see this error are a few developers wanting to build old versions of Clojure from source code. That said, for those few it might be convenient for them if they didn't have to hunt down this patch and apply it themselves, I guess, but I would understand if the Clojure core team would rather document this for developers, rather than put out ".1" releases of older Clojure versions for a Clojure-build-time-with-latest-JDK issue.
Hide
Peter Monks added a comment - - edited

Andy the build I referenced above is using those published JARs - I am not attempting to compile Clojure from source - and yet I'm seeing this exception (and hence believe I'm seeing the issue described here). Any ideas why?

Show
Peter Monks added a comment - - edited Andy the build I referenced above is using those published JARs - I am not attempting to compile Clojure from source - and yet I'm seeing this exception (and hence believe I'm seeing the issue described here). Any ideas why?
Hide
Andy Fingerhut added a comment - - edited

@Peter That looks like an issue when compiling the core.rrb-vector library, which despite its name is a library separate from Clojure, and has an open ticket for its own independent issue that has the same JDK 11 based root cause as this ticket: https://dev.clojure.org/jira/browse/CRRBV-18

It looks like in your multigrep project, you have a test dependency on midje, which through one or two other deps (including fipp) depends upon core.rrb-vector. I did not dig into your travis build to check, but if that step is not doing any testing, but only building multigrep code, there should be no need to pull in midje in that step. For any midge-based testing, it will have that problem until core.rrb-vector, then fipp, etc. then midje have their dependencies updated and released to avoid this problem.

Show
Andy Fingerhut added a comment - - edited @Peter That looks like an issue when compiling the core.rrb-vector library, which despite its name is a library separate from Clojure, and has an open ticket for its own independent issue that has the same JDK 11 based root cause as this ticket: https://dev.clojure.org/jira/browse/CRRBV-18 It looks like in your multigrep project, you have a test dependency on midje, which through one or two other deps (including fipp) depends upon core.rrb-vector. I did not dig into your travis build to check, but if that step is not doing any testing, but only building multigrep code, there should be no need to pull in midje in that step. For any midge-based testing, it will have that problem until core.rrb-vector, then fipp, etc. then midje have their dependencies updated and released to avoid this problem.
Hide
Peter Monks added a comment - - edited

Andy ah ok that makes more sense. It looks like midje (which I'm using for unit testing) has a dependency on core.rrb-vector.

[edit following your edit] Yes I can confirm that the issue is with midje (see https://github.com/marick/lein-midje/issues/66), and yes I am using midje for unit testing as part of the CI process on Travis, so it's not as trivial as simply removing midje from the dependencies and/or Travis script. I'll probably just bite the bullet and port to clojure.test.

Show
Peter Monks added a comment - - edited Andy ah ok that makes more sense. It looks like midje (which I'm using for unit testing) has a dependency on core.rrb-vector. [edit following your edit] Yes I can confirm that the issue is with midje (see https://github.com/marick/lein-midje/issues/66), and yes I am using midje for unit testing as part of the CI process on Travis, so it's not as trivial as simply removing midje from the dependencies and/or Travis script. I'll probably just bite the bullet and port to clojure.test.
Hide
Ghadi Shayban added a comment -

RRB vector has a new release you can pull and override your transitive dependency.

Show
Ghadi Shayban added a comment - RRB vector has a new release you can pull and override your transitive dependency.

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: