-
-
Notifications
You must be signed in to change notification settings - Fork 372
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
Add Scala 3 support via dotty compatibility #1150
Add Scala 3 support via dotty compatibility #1150
Conversation
7312742
to
9d05da9
Compare
9d05da9
to
a4ee402
Compare
fa18ef7
to
17c078e
Compare
87fae0c
to
4839d55
Compare
f2c678b
to
f6d58ed
Compare
@lihaoyi This is ready for review, if you wish. Don't fear the number of commits or the number of changed lines… All commits but one touch less than or around 100 lines. The only sizeable commit is the one adding the Scala 3 source of the That said, I can always split that to make it easier to review it:
The tests now pass in #1151, that switches to Lastly, some things could be cleaned-up in the biggest commit (Scala 3 code in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexarchambault I skimmed this, I think it looks good to me. I trust both that you know your way around the codebase, and also our test suite to catch any regressions in the old code paths.
Since the bulk of the code here is in Scala 3, could it be worth getting someone more familiar with Scala3 to review those sections? Even if you need to explain what the code is meant to do, a second perspective from the Scala3 side of things may be valuable to make sure what we're doing sounds reasonable to them
f6d58ed
to
3c252f5
Compare
(Last push force: folded #1151 here.) |
3c252f5
to
88d7a83
Compare
I agree it would be nice to have more Scala 3 expert eyes have a look, yes. @smarter @anatoliykmetyuk Don't know if you guys would be interested in having a look at the commit named In more detail, it adds the following classes:
(I might push force some clean-ups of that commit, as I just found some duplication or formatting related issues…) |
Looks like there's quite a lot of code adapted from existing code in dotty, my advice would be to instead change dotty to make the existing code reusable for your purposes when possible. |
I agree. That's mostly syntax-highlighting and completion-related code. I'll look into submitting a PR to dotty. |
88d7a83
to
d425697
Compare
Hi @alexarchambault. I was just working with Spark SQL in Jupyter thanks to Almond, and I'm curious how you're handling the removal of Cheers. |
@mallman In the Ammonite codebase, most symbol literals (maybe even all of them, I think) are related to os-lib paths, in code like I'm not sure how spark-sql will handle the end of symbol literals, but IIRC, it also accepts string literals in most places, right? Using string literals would be the way to go then. (By the way, this kind of question might be more suited to the Gitter channel, although I have to admit I should check my Gitter notifications more often 😬) |
Some tests are disabled, as it seems the Scala 3 typer doesn't discriminate between `class Foo` and `object Foo`, which prevents imports of a class and an object with the same name, but different namespaces, to work as expected.
Without the added braces, indentation-based syntax assumes the def-s / lazy val-s are not complete, and expects more input.
There's a candidate deep completion implementation for Scala 3 already, but it's waaaaay too slow as of now.
Some logic for import $ivy completion is there already, but it needs a bit more tinkering (in the way it handles backticks in particular).
Seems these are related to root packages…
`desugar` support needs to be added back in Scala 3.
Support for URL-based class path needs to be added back (this needs a Scala 3 equivalent to CustomURLZipArchive).
No -Yrangepos option to tinker with in Scala 3 I think.
Line numbers from Scala 3 are currently wrong.
Turns out what we do from interp.configureCompiler(…) doesn't matter in this test, so we do something that compiles with both Scala 2 and 3.
9c5ef03
to
d989f44
Compare
So this PR adds Scala 3 support, as a follow up of #1144.
Its changes successively consist in:
All commits but one don't touch more than approx. 100 lines each, which should make it easy to review those. The only sizable commit is the one adding the Scala 3 implementation of the
compiler
module, underamm/compiler/src/main/scala-3
(8c22bfa). This commit mainly:amm/compiler/src/main/scala-3
,build.sc
so that using just3
as cross version builds and runs Ammonite as a mix of Scala 2.13 / 3 modules (that run fine together thanks to dotty compatibility)Many of the sources this commit adds under
amm/compiler/src/main/scala-3
are adapted either from dotty sources, or from Ammonite Scala 2 things.Some features don't work yet in Scala 3, or don't work as fine as in Scala 2 yet. The commits updating the tests should make clear what has been disabled for Scala 3. Most notably:
LineNumberModifier
plugin for Scala 3 would fix both at the same time, elseammonite.interp.script.PositionOffsetConversion
could be used to fix at least the formeramm/compiler/src/main/scala-3/dotty/ammonite/compiler/Completion.scala
), but it's awfully slow, so it's disabled by default (look for the commented out line callingaddDeepCompletions
)Completion.scala
), but it has issues with backticks. This required tinkering in Scala 2, which seemed less straightforward to do in Scala 3 (IIRC, dotty doesn't easily hand us over backticks around what's being completed)source
anddesugar
macros: they need to be ported over to Scala 3 (using Tasty reflection-based macros I guess)There are also limitations in the Scala 3 specific code, most notably:
@
) is regex based for now (I didn't try to have the dotty parser handle those yet)