planck-repl / planck

Stand-alone ClojureScript REPL
https://planck-repl.org
Eclipse Public License 1.0
1.03k stars 68 forks source link

Use Clojure-produced version of GCL JAR #1014

Open pyrmont opened 4 years ago

pyrmont commented 4 years ago

As discussed in #1013, Planck should perhaps switch to using the Clojure-produced version of GCL on Maven Central.

My memory is that this was how Planck worked previously but that we switched to the current approach because there had developed a large gap between the 'current' version of GCL that was documented online and the Clojure-produced one.

Is this an unavoidable problem because of the coming type inference stuff?

mfikes commented 4 years ago

@pyrmont Fundamentally, the difference between what the JVM ClojureScript compiler had on its classpath and our scheme for replacing it (by downloading a zip, and treating them as :libs) appeared to be causing subtle JS dependency indexing issues that were behind #1011.

At its core, this is the result of the fact that the code ClojureScript compiler master now more deeply analyzes all Closure code being processed (in the name of getting type inference information, along with other useful metadata).

With #1013 I attempted to force the issue by excluding what the JVM was using, and by putting those lib directories on the JVM ClojureScript compiler's classpath, but that was leading to other odd behavior (things derailing), which, while probably resolvable with sufficient effort, was possible to work around by simply instead telling it to use a newer version of GCL (very close to what Planck ships with).

With the type inference stuff, there is a small possibility that code might be generated for core functions by the JVM compiler that is correct considering the GCL JAR it is using, but incorrect if things change in the version of GCL that we are using in Planck.

A small example might be a function that is inferred to return a Boolean value being used in a test, thus eliminating a checked if. If that function is later revised in GCL to no longer return a Boolean, and a checked if is then needed, we will effectively have bad code gen for those spots... missing checked ifs where they are needed. (This is a stretch and highly unlikely, but is the best example I can come up with.)

So... with all of this, you can argue that since ClojureScript master's code gen depends on GCL, it should probably match the one that Planck is going to ship with.

Unfortunately, Planck is currently shipping a GCL that is newer than any version that is currently available in a JAR, so we can't just go back to using the most recent JAR without it potentially being a breaking change (this would effectively involve switching to shipping an older version of GCL in Planck relative to what it currently ships).

So, long term, my thought is that some day this should be rectified when newer GCL JARs are shipped, and we could have Planck just put such a JAR on the classpath, and still inspect its contents in order to generate gcl.cljs, but keep things simple by using and only using that JAR dep, and no longer put things in a lib directory that we point to with :libs.

I know of nothing broken right now, but this ticket can serve to move to that newer pattern at some point in the future. :)

pyrmont commented 4 years ago

Thanks for the explanation :) That makes sense. I feel bad because I think I originally proposed the change to using the newer GCL and now that's gone and made more work! I'm glad you were able to figure out the problem and hopefully there'll be a new JAR of GCL soon :)

mfikes commented 4 years ago

Note that updated copies were deployed recently with Alex Miller indicating:

google-closure-library 0.0-20191016-6ae1f72f and org/clojure/google-closure-library-third-party 0.0-20191016-6ae1f72f are out there now the third party jar is ~50% the size of the last release. not sure if that's weird

mfikes commented 4 years ago

WIP in this branch https://github.com/planck-repl/planck/tree/issue-1014-gcl-jar

Things fail to build and I haven't discovered the cause.

mfikes commented 4 years ago

Hrm. Simply changing to 0.0-20191016-6ae1f72f in deps.edn causes things to fail. It also causes issues for ClojureScript master if you revise it to use that version.