Decouple the Scala version used by Ammonite from the one exposed to users #1135

alexarchambault · 2020-12-04T10:06:54Z

This currently includes #1133, so it actually looks slightly lengthier than it is.

~~Checking what the CI says in a first time, I'll describe it in more details right after.~~

That makes it easier later on to extract a Java interface for CompilerLifecycleManager

Either has less cases to handle, which makes the extraction of a Java interface for CompilerLifecycleManager more straightforward.

Having it rely on Frame from ammonite.repl.api, rather than a concrete Frame implementation, makes it easier to extract an interface for CompilerLifecycleManager later on.

This change doesn't change the semantic of this logic. It makes easier to adapt this method to the upcoming Java-based Preprocessor API.

…and remove Compiler.parse This allows to strip the Compiler public API of references to scala.tools.nsc. Later on, makes it easier to extract a Java interface for Compiler.

This makes it more straightforward to extract a Java API for parsing later on.

Just like the previous commit, this facilitates extracting a Java API for parsing later on.

colors is only useful from the REPL, not from scripts. It makes more sense that it lives in ReplAPI.

util will be a shared dependency of runtime / interp on the one hand, and the upcoming compiler module on the other. Moving Classpath here allows it to be used by both.

The clipboard stuff can live entirely in user space, there's no need to have its calls cross the userspace <-> Ammonite internals barrier. Later on, this is helpful to Java-ize ReplAPI.

That makes it easier to Java-ize InterAPI. It should be do-able to add back os-lib stuff later on.

util should need to be cross-compiled for all supported Scala versions, including the upcoming Scala 3. Dropping the ops dependency allows not to have to cross-compile ops.

It seems it's unused.

Just like for util <-> ops, repl-api is meant to be cross-compiled to all supported Scala versions. Removing the dependency towards ops allows not to cross-compile ops.

That makes it more straightforward to Java-ize InterpAPI later on.

That makes it easier to Java-ize ReplAPI later on.

When creating a compiler instance, Ammonite passes it two class paths: the "initial" one, and the main one. The former is subject to whitelisting when --thin is passed, not the latter. Later on, when adding --scala-version, there can be JARs used internally by Ammonite that are in both paths. Their classes need to be blacklisted, but these JARs should still be loaded via the main class path, as any other JAR added by users. Before this commit, we were naively removing from the main class path any JAR already in the initial class path, which was a problem for the JARs in both paths mentioned above.

ForkClassLoader injects resources from another ClassLoader. When --thin is passed, it could have injected resources that should have been blacklisted. Alternatively, maybe we could have adde a whitelisting layer on top of it too…

A few things are not whitelisted anymore after removing the ops dependency here or there. Whitelisting in these tests will be added back when decoupling the Ammonite and user-facing Scala versions.

It gets thrown by users, with their own Scala version, and later caught by Ammonite with its own Scala version. So it needs to be put in a shared Java API, so that AmmoniteExit is loaded by the same ClassLoader for both users and Ammonite alike.

The Java module is meant to be used by users and Ammonite at the same time via the same class loader. The Scala module is meant to be loaded on the fly in the user ClassLoader, depending on which Scala versions users want to use.

This simplifies the conversion of this class to a Java interface (single method instead of two, and no explicit collection).

This changes InterpAPI from a Scala class to a Java class. For this to work, some of the methods of InterpAPI get more Java-compatible signatures. In particular, mutable collections become java.util.List. In order to recover the ease of use of the Scala signatures and collections, we add extension methods, defined in InterpBridge.InterpAPIExtensions, so that most former Scala methods can still be used with InterpAPI.

TestBridge lives in "user space" in some tests. It needs to be compiled with the user space Scala version, rather than the Ammonite Scala version. It's put in a dedicated module, and its class path is passed along the one of amm.compiler and amm.repl.api.full to the cross tests.

Just in case

The Ammonite and user Scala classes are not loaded by the same class loader in cross tests, so some comparisons have to be tweaked.

It seems these are needed for scala-xml to work in particular. Loading compiler plugins (which involves XML stuff) fails without this.

alexarchambault · 2020-12-04T15:24:01Z

The changes introduced by this PR are quite large… There are some breaking changes in the early commits (those changing os.Path to java.nio.file.Path in the APIs), but I'm thinking of reverting those, so that I could break this PR in smaller chunks, that could be discussed and merged one after the other.

The overall goal is to make Ammonite interact with the user session and scalac only via Java APIs, and this is achieved by adding Java interfaces, in new pure Java modules:

ammonite.interp.api.InterAPI becomes a Java interface, and its module interp-api is now pure Java,
ammonite.repl.api.ReplAPI becomes a Java interface, and its module repl-api is now pure Java,
new interfaces ammonite.compiler.iface.{ CompilerLifeCycleManager, CompilerBuilder, Parser } are introduced in a new compiler-interface module.

Extension methods, such as these or these, help make these interfaces more usable from Scala code.

APIs

The original interp-api and repl-api modules actually get split in two: interp-api / interp-api-full, and repl-api / repl-api-full. interp-api and repl-api are pure Java. interp-api-full and repl-api-full are Scala modules. The APIHolders and extension methods for the Java APIs live in the latter modules.

`--scala`

The actual decoupling of Scala versions is enabled when --scala X.Y.Z is passed to the Ammonite launcher. This option takes over the former --thin option. --scala X.Y.Z re-uses the whitelisting from --thin, but only leaves pure Java classes in the whitelist (those of interp-api and repl-api in particular). This way, the initial interpreter ClassLoader only contains pure Java classes. To load a Scala version, and recover the former Scala methods of the APIs, we fetch interp-api-full and repl-api-full for Scala X.Y.Z upon startup, and add them to the interpreter ClassLoader just like libraries users would have added themselves. This way, these additional libraries are effectively isolated from Ammonite itself.

Compiler

For the compiler, a new compiler Scala module has implementations for the interfaces of the pure Java compiler-interface. Just like for interp-api-full and repl-api-full, upon startup, we fetch compiler for Scala X.Y.Z, and add it to the interpreter ClassLoader. We then create instances of some of its classes by reflection, and cast them as one of ammonite.compiler.iface.{CompilerLifeCycleManager, CompilerBuilder, Parser}. We then pass those instances to Interpreter in particular, which uses them to parse and compile things.

Tests

About the tests, I only enabled this feature in REPL tests for now (but manual tests for scripts work).

To test that, a new amm.repl.cross-tests module is added. Its sources are those of the tests of amm.repl as is (but for a boolean variable that enables cross testing). This module is defined as a CrossModule, but doesn't use the "cross-variable" as its Scala version. Instead, it assumes it's the Scala version that should be exposed to users in the tests. So it sticks to the main Scala version (currently, 2.13.3), but pulls the class path of interp-api-full / repl-api-full / compiler for the cross Scala version, and passes it to the tests via a Java property. The tests then just add those to the interpreter class loader, just like how --scala X.Y.Z does. So

$ ./mill amm.repl.cross-tests[2.12.12].test

runs the test sessions code in 2.12.12, but the logic running the sessions runs itself in 2.13.3.

alexarchambault · 2020-12-04T15:34:08Z

To test --scala X.Y.Z locally, the following command should do:

$ ./mill '__[2.12.12].publishLocal' &&\
    ./mill 'amm.interp.api.publishLocal' &&\
    ./mill 'amm.compiler.interface.publishLocal' &&\
    ./mill 'amm.repl.api.publishLocal' &&\
    ./mill -i 'amm[2.13.3].run' --scala 2.12.12

This publishes the modules that are loaded upon startup first (interp-api-full, repl-api-full, compiler, and their dependencies), along with the pure Java modules. --scala X.Y.Z can then pick those from the local Ivy2 repository.

alexarchambault · 2020-12-04T15:47:06Z

To enable --scala 3 support later on, only the modules that get fetched upon startup need to be cross-compiled for Scala 3. These are

interp-api-full
repl-api-full
compiler
util

Their Scala dependencies are:

$ cs resolve com.lihaoyi:ammonite-repl-api-full_2.13.3:2.3.8-71-f113c5050 | grep '2\.13' | grep -v '^com\.lihaoyi:ammonite' | grep -v '^org.scala-lang:'
com.lihaoyi:fansi_2.13:0.2.9:default
com.lihaoyi:fastparse_2.13:2.3.0:default
com.lihaoyi:geny_2.13:0.6.2:default
com.lihaoyi:mainargs_2.13:0.1.4:default
com.lihaoyi:os-lib_2.13:0.7.1:default
com.lihaoyi:pprint_2.13:0.5.9:default
com.lihaoyi:scalaparse_2.13:2.3.0:default
com.lihaoyi:sourcecode_2.13:0.2.1:default
org.scala-lang.modules:scala-collection-compat_2.13:2.1.4:default
org.scala-lang.modules:scala-xml_2.13:1.2.0:default

All of them but fastparse / scalaparse / mainargs are already cross-published for 3.0.0-M2. fastparse / scalaparse are pulled by the compiler module, we should work around them by using the dotty parser.

mainargs for 3.0.0-M2 is missing. It's only needed for @main entrypoints, so I'm thinking about disabling those for Scala 3 in a first time, until we can get mainargs for it.

alexarchambault · 2020-12-04T15:52:16Z

I'm hesitating discussing the various commits of this PR right now, or split it instead… @lihaoyi Just tell me if you're ok with the general direction, and feel like reviewing this (+4000 -2000 / 74 commits) PR or if you prefer me to split it in smaller PRs.

lihaoyi · 2020-12-04T23:45:36Z

Thanks Alex! I won't be able to get to this in the next few days, but hopefully have time coming up next week to give this a proper review

alexarchambault · 2020-12-17T11:34:15Z

I've come to realize that this PR actually deals with several successive concerns, even though things are not done in this order in the commit history:

extracting compiler- and parsing-related code (mostly from interp) to a new distinct compiler module
"Java-ize" the interface between the interp and compiler modules (no more direct dependency between both, instead interp expects an instance of a Java interface, that users can find an implementation of in compiler)
"Java-ize" the interface between interp and interp-api / repl-api (no direct dependency to / from interp, instead interp creates instances of Java interfaces, that interp-api / repl-api pick at runtime later on)
wire things so that e.g. Ammonite running in 2.13 can run 2.12 code (from the CLI, and in the tests)

It might be possible to add Scala 3 support with only the first step, even though I'd be in favor of keeping the others too (they would allow to stop cross-compiling most modules to 2.12 while retaining 2.12 support for some time, and would even allow later on to compile most Ammonite modules with Scala 3 only while retaining Scala 2 support).

So to help reviewing this, I could rebase things, and open a first PR only adding the compiler module, that should be the less controversial change here. And we can wait before deciding to Java-ize things. I've been working on the core of Scala 3 support lately, and it should be possible to rebase it on an Ammonite code base without the "Java-ize" stuff.

alexarchambault · 2020-12-17T11:35:09Z

Also, after the changes here, I feel the graph of modules in Ammonite becomes somehow messy… (ops, util, runtime, compiler-interface, compiler, interp-api-full, interp-api, interp, repl-api-full, repl-api, repl, main, …). I think it could be simplified, by merging some modules:

ops, interp-api-full, repl-api-full into a single api module
runtime merged into interp
repl merged into main
interp-api and repl-api into a single api-interface module.

So that we would end up with just:

util: shared utility between interp and compiler
compiler: code interfacing with scalac / dotc
compiler-interface: interfaces implemented in compiler
api: user-facing code
api-interface: interfaces passed to api to interact with the interpreter
interp: module allowing to create / embed a full-fledged interpreter
main: the CLI app of Ammonnite, wrapping an interpreter from interp in a CLI application.

It could also be beneficial before the changes in this PR. In that case, we would end up with just:

util
api: user-facing code
interp: module allowing to create / embed a full-fledged interpreter
main: the CLI app of Ammonnite, wrapping an interpreter from interp in a CLI application.

(No more runtime, interp-api / repl-api, repl modules.)

alexarchambault · 2020-12-17T11:37:57Z

@lihaoyi If needed, I can be up for a call, if there are things you think would be more convenient to discuss this way.

To clarify some of the stuff I said, it's probably too early for a thorough review… But I can split this PR in several ways, that can be reviewed / discussed successively, either by adding the compiler module first (that shouldn't be controversial), or by Java-izing stuff head-on.

alexarchambault · 2020-12-18T14:06:05Z

FYI, I pushed the early Scala 3 support on this branch. Its README gives more details about what works / what doesn't work yet, and how to run Ammonite with Scala 3 support from the branch sources.

lihaoyi · 2020-12-19T01:25:59Z

Hey @alexarchambault! Sorry I just got to this.

I think at a high level, I want to preserve the runtime/interp and repl/main splits. I think they have meaningful semantics distinctions (e.g. runtime doesn't need compiler, interp does) and helps us keep things organized and e.g. avoid accidentally classloading the scala compiler on the cached-script code path.

Regarding your earlier question splitting up the PR, I think limiting the first step to extracting a compiler module that differs between Scala2 and Scala3 would be my preference. AFAIK you should only need to Javaize the interface between compiler and the rest of the code, and wouldn't need to Javaize the other inter-module interfaces since those will remain on Scala2. This seems like it would also help avoid the sprawl of -api and -api-full modules.

In general, let's aim to get Scala 3 in with minimal churn, and then we can consider further cleanups like dropping cross-compilation across Scala 2 versions later

smarter · 2020-12-20T00:50:08Z

I think there's some overlap between the Java compiler interface you're trying to define and the one that metals uses: https://github.com/scalameta/metals/tree/main/mtags-interfaces/src/main/java/scala/meta/pc, maybe there's an opportunity for defining a common standard here? In Dotty we also have https://github.com/lampepfl/dotty/tree/master/interfaces/src/dotty/tools/dotc/interfaces which is very minimal but could be extended.

alexarchambault · 2020-12-20T22:59:02Z

I think at a high level, I want to preserve the runtime/interp and repl/main splits. I think they have meaningful semantics distinctions (e.g. runtime doesn't need compiler, interp does) and helps us keep things organized and e.g. avoid accidentally classloading the scala compiler on the cached-script code path.

Right. About runtime/interp though, with the changes here, interp doesn't depend anymore on scala-compiler, only on a new pure Java compiler-interface module. So runtime makes less sense in that case I guess.

Regarding your earlier question splitting up the PR, I think limiting the first step to extracting a compiler module that differs between Scala2 and Scala3 would be my preference.

Yeah, that's what I'm going to do. We may even not need the Java API for compiler either. Scala 2.13 and Scala 3 code should be able to work alongside each other, both at compile-time and runtime. My plan now would be to try to keep interp/repl/main/interp-api/repl-api in Scala 2.13, and only compiler in Scala 3, if that can work.

alexarchambault · 2020-12-20T23:32:02Z

@smarter Thanks for those links! It might be possible to extend those for Ammonite… The interfaces from metals seem quite specific to it though (with many lsp4j classes involved). But some definitions from the dotty interfaces could have been used here (like Diagnostic).

That said, for now, the level at which we abstract things in this PR involves things quite specific to Ammonite, like class whitelist stuff, the dependency completer, or even the handling of "Frames". But maybe with slightly lower level abstractions, we could get interfaces that could be useful in other contexts, and having them in dotty itself would be interesting then.

alexarchambault · 2020-12-20T23:35:28Z

Just so you know, I'll probably open a new PR adding just the new compiler module during the first week of January I think (so somewhere between Jan 4th and 8th - while still working on Scala 3 stuff based on this branch in a first time).

alexarchambault · 2021-01-13T17:35:59Z

Closing this. Going to open new PRs with less changes.

alexarchambault added 23 commits December 4, 2020 10:55

Unused imports

1f35b28

Remove dead code

c86cac9

Remove unused method in build.sc

68e4f26

Remove unused param in build.sc

ff75cec

Don't pass Storage to CompilerLifecycleManager

8028dda

That makes it easier later on to extract a Java interface for CompilerLifecycleManager

Get rid of Res in CompilerLifecycleManager

0e0817e

Either has less cases to handle, which makes the extraction of a Java interface for CompilerLifecycleManager more straightforward.

Have CompilerLifecycleManager rely on ammonite.repl.api.Frame

37dd520

Having it rely on Frame from ammonite.repl.api, rather than a concrete Frame implementation, makes it easier to extract an interface for CompilerLifecycleManager later on.

Simplify Res stuff

b6477d1

This change doesn't change the semantic of this logic. It makes easier to adapt this method to the upcoming Java-based Preprocessor API.

Move Preprocessor instantiation to Compiler

55bbbdb

…and remove Compiler.parse This allows to strip the Compiler public API of references to scala.tools.nsc. Later on, makes it easier to extract a Java interface for Compiler.

Mark non-public Parsers methods as private

7006e34

This makes it more straightforward to extract a Java API for parsing later on.

Add return types on Parsers public methods

57bd314

Just like the previous commit, this facilitates extracting a Java API for parsing later on.

Move InterpAPI.colors to ReplAPI

e02dabc

colors is only useful from the REPL, not from scripts. It makes more sense that it lives in ReplAPI.

Move Classpath to util module

1a50c49

util will be a shared dependency of runtime / interp on the one hand, and the upcoming compiler module on the other. Moving Classpath here allows it to be used by both.

Leave clipboard stuff in user-facing module

82346f9

The clipboard stuff can live entirely in user space, there's no need to have its calls cross the userspace <-> Ammonite internals barrier. Later on, this is helpful to Java-ize ReplAPI.

Remove interp-api dependency towards ops

dfcecf6

That makes it easier to Java-ize InterAPI. It should be do-able to add back os-lib stuff later on.

Remove util dependency towards ops

45c9c84

util should need to be cross-compiled for all supported Scala versions, including the upcoming Scala 3. Dropping the ops dependency allows not to have to cross-compile ops.

Remove runtime dependency towards ops

7230735

It seems it's unused.

Remove repl-api dependency towards ops

9e0b3f6

Just like for util <-> ops, repl-api is meant to be cross-compiled to all supported Scala versions. Removing the dependency towards ops allows not to cross-compile ops.

Remove interp-api dependency towards util

84a7e34

That makes it more straightforward to Java-ize InterpAPI later on.

Use NIO rather than os-lib in repl-api

85c2c7e

That makes it easier to Java-ize ReplAPI later on.

Disable ForkClassLoader when --thin is passed

6f51936

ForkClassLoader injects resources from another ClassLoader. When --thin is passed, it could have injected resources that should have been blacklisted. Alternatively, maybe we could have adde a whitelisting layer on top of it too…

Disable whitelisting in repl and integration tests

68c4fb7

A few things are not whitelisted anymore after removing the ops dependency here or there. Whitelisting in these tests will be added back when decoupling the Ammonite and user-facing Scala versions.

alexarchambault force-pushed the scala-version-decoupling branch from 8269d06 to 87e7341 Compare December 4, 2020 10:49

Disable --thin in integration tests until work on it is done

5e81a3f

alexarchambault force-pushed the scala-version-decoupling branch from 87e7341 to 26ddbcf Compare December 4, 2020 11:29

alexarchambault added 4 commits December 4, 2020 12:29

Java-ize AmmoniteExit

552d088

It gets thrown by users, with their own Scala version, and later caught by Ammonite with its own Scala version. So it needs to be put in a shared Java API, so that AmmoniteExit is loaded by the same ClassLoader for both users and Ammonite alike.

Split interp-api in a Java module + a Scala module

a9dc194

The Java module is meant to be used by users and Ammonite at the same time via the same class loader. The Scala module is meant to be loaded on the fly in the user ClassLoader, depending on which Scala versions users want to use.

Merge LoadJar.cp methods

f4966b7

This simplifies the conversion of this class to a Java interface (single method instead of two, and no explicit collection).

alexarchambault added 7 commits December 4, 2020 12:29

Check user Scala version in cross tests

999ab84

Just in case

Tweak tests for cross-Scala version testing

51c569c

The Ammonite and user Scala classes are not loaded by the same class loader in cross tests, so some comparisons have to be tweaked.

Whitelist more JDK classes

da8ee3f

It seems these are needed for scala-xml to work in particular. Loading compiler plugins (which involves XML stuff) fails without this.

Tweak caching test (investigate why this is needed?)

bfcc46e

Ensure publishing stuff is fine with pure Java modules

42b5d61

NO

9e402f2

alexarchambault force-pushed the scala-version-decoupling branch from 26ddbcf to 9e402f2 Compare December 4, 2020 11:30

alexarchambault added 2 commits December 4, 2020 15:09

Add helper InterpAPI and ReplAPI traits for the website

a4205fd

Add extra cross job on CI

3ade62a

SethTisue mentioned this pull request Dec 20, 2020

support in scalac for ignoring hash-bang line in script files scala/scala3#10858

Closed

alexarchambault mentioned this pull request Jan 13, 2021

Scala 3 compatibility ? almond-sh/almond#718

Open

alexarchambault closed this Jan 13, 2021

alexarchambault deleted the scala-version-decoupling branch January 13, 2021 17:36

alexarchambault mentioned this pull request Jan 14, 2021

Move compiler-related stuff to a new compiler module #1144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple the Scala version used by Ammonite from the one exposed to users #1135

Decouple the Scala version used by Ammonite from the one exposed to users #1135

alexarchambault commented Dec 4, 2020 •

edited

Loading

alexarchambault commented Dec 4, 2020 •

edited

Loading

alexarchambault commented Dec 4, 2020

alexarchambault commented Dec 4, 2020 •

edited

Loading

alexarchambault commented Dec 4, 2020 •

edited

Loading

lihaoyi commented Dec 4, 2020

alexarchambault commented Dec 17, 2020

alexarchambault commented Dec 17, 2020 •

edited

Loading

alexarchambault commented Dec 17, 2020 •

edited

Loading

alexarchambault commented Dec 18, 2020

lihaoyi commented Dec 19, 2020 •

edited

Loading

smarter commented Dec 20, 2020

alexarchambault commented Dec 20, 2020

alexarchambault commented Dec 20, 2020

alexarchambault commented Dec 20, 2020 •

edited

Loading

alexarchambault commented Jan 13, 2021

Decouple the Scala version used by Ammonite from the one exposed to users #1135

Decouple the Scala version used by Ammonite from the one exposed to users #1135

Conversation

alexarchambault commented Dec 4, 2020 • edited Loading

alexarchambault commented Dec 4, 2020 • edited Loading

APIs

--scala

Compiler

Tests

alexarchambault commented Dec 4, 2020

alexarchambault commented Dec 4, 2020 • edited Loading

alexarchambault commented Dec 4, 2020 • edited Loading

lihaoyi commented Dec 4, 2020

alexarchambault commented Dec 17, 2020

alexarchambault commented Dec 17, 2020 • edited Loading

alexarchambault commented Dec 17, 2020 • edited Loading

alexarchambault commented Dec 18, 2020

lihaoyi commented Dec 19, 2020 • edited Loading

smarter commented Dec 20, 2020

alexarchambault commented Dec 20, 2020

alexarchambault commented Dec 20, 2020

alexarchambault commented Dec 20, 2020 • edited Loading

alexarchambault commented Jan 13, 2021

alexarchambault commented Dec 4, 2020 •

edited

Loading

alexarchambault commented Dec 4, 2020 •

edited

Loading

`--scala`

alexarchambault commented Dec 4, 2020 •

edited

Loading

alexarchambault commented Dec 4, 2020 •

edited

Loading

alexarchambault commented Dec 17, 2020 •

edited

Loading

alexarchambault commented Dec 17, 2020 •

edited

Loading

lihaoyi commented Dec 19, 2020 •

edited

Loading

alexarchambault commented Dec 20, 2020 •

edited

Loading