Scala 3 support #466

keynmol · 2021-03-21T22:56:50Z

After lots of printlns I realised there was a custom scalameta-like parser for Scala 3.

All of the positions in sections had Input.None, so slicing wasn't working - making all tests fail.

I've added a few things on top of #443 (didn't realise it was in a fork), and now I seem to be getting

[error] Failed: Total 170, Failed 54, Errors 0, Passed 107, Skipped 9

On the 3.0.0-M3 tests. Some of them are super easy to fix (different wording error messages in Scala3), I'll take a look at the rest which might be more complex.

Makes it easier to work in the build.

Previously, mdoc didn't support Scala 3 when processing markdown files, it only supports Scala 3 worksheets. This commit adds support to additionally process markdown files with Scala 3. This is still a work-in-progress. Scala 2 tests are still passing and the Scala 3 build *compiles* but tests fail.

fail proper this time.

tgodzik · 2021-03-22T15:36:53Z

After lots of printlns I realised there was a custom scalameta-like parser for Scala 3.

All of the positions in sections had Input.None, so slicing wasn't working - making all tests fail.

The positions coming from Scala 3 parser are empty. We used it before Scala 3 dialect for scalameta parser was finished. Do let me know if you need any help with Scala 3 part, that was mostly done by be 😅

keynmol · 2021-03-25T18:51:33Z

Thanks @tgodzik I sort of took a stab at it almost mechanically, now it's the hard part.

I still need to figure out @olafurpg's changes to cli module (whether they're supposed to go out?), as they introduce metaconfig's scala 2 macros and don't work for me locally. What I'm trying to figure out is if I'm missing something :D

So just to clarify:

We can now use semanticdb for latest scala
We are missing the dotty position <-> semanticdb position conversion, right?
And the custom implementation of markdown processing using dotty compiler directly can go, right?

I should've clarified those before starting with this :)

tgodzik · 2021-03-26T16:17:31Z

Thanks @tgodzik I sort of took a stab at it almost mechanically, now it's the hard part.

I still need to figure out @olafurpg's changes to cli module (whether they're supposed to go out?), as they introduce metaconfig's scala 2 macros and don't work for me locally. What I'm trying to figure out is if I'm missing something :D

@olafurpg might know more.

So just to clarify:

We can now use semanticdb for latest scala

Do you mean scalameta parser? Semanticdb is not used here as far as I know.

We are missing the dotty position <-> semanticdb position conversion, right?

We would most likely need to add that to use scalameta parser, but we do have those conversions for Scala 2, so this should be something similar.

And the custom implementation of markdown processing using dotty compiler directly can go, right?

Yeah, if we use scalameta parser the only thing that needs to stay are separated MarkdownCompiler for SCala 2 and 3.

olafurpg · 2021-03-26T17:48:54Z

The cli module will need to stay on 2.13 because it uses macros.

Correct. That’s great news!

keynmol · 2021-03-26T17:50:53Z

Ok awesome, I'll then take another stab at this over the weekend to get it into a working state hopefully, unless I hit another blocker.

keynmol · 2021-03-27T13:04:33Z

Ok @olafurpg @tgodzik I had to make everything worse to make it better :D

cli module cross-builds for all versions with the exception of using scala 2 macros

I've separated the macros part so it can be addressed separately
Fixed compilation in a few places that were broken
Separated js modifier tests into a separate module - allowed me to delete some files but also keep it on Scala 2 (so it can be fixed separately)
Updated the branch with all the latest changes
Fixed dependency versions (fs2 specifically)

So in the current state, the build and cross-sources must be configured correctly to move on to just fixing the tests and potential bugs.

Therefore my proposal is:

Reviewing, cleaning up the machinery
Making sure 2.11 - 2.13 is working as before
Before merging, disable scala3 tests again
In the next PR focus on just fixing the tests

keynmol · 2021-03-27T13:24:18Z

build.sbt

+lazy val fs2Version = Def.setting {
+  if (scalaVersion.value.startsWith("2.11")) "2.1.0"
+  else if (scalaVersion.value == "3.0.0-M2") "2.5.0"
+  else "2.5.3"
+}
+
+lazy val munitVersion = Def.setting {
+  if (scalaVersion.value == "3.0.0-M2")
+    "0.7.21"
+  else V.munit
+}
+


Previously tests weren't running on Scala 3, so we didn't have to cherry pick versions.

olafurpg · 2021-03-28T22:38:29Z

The proposed plan sounds great. It’s good to separate the structural changes from fixing the tests.

keynmol · 2021-03-29T20:21:03Z

Another yak that I've just realised:

CLI tests have no chance of passing on Scala 3 unless some form of args processing takes place - at the moment it's just returning default settings (because no derivation), and the CliSuite tries to process the entire docs folder.

So my questions are:

What's the status of metaconfig and Scala 3? I don't have the bandwidth to create Scala 3 macros for it for all the versions mdoc needs (meta api changed a lot between M2 -> M3 -> RC1)
Alternatively, is manual composition of settings an option? This would remove the scala 2 macros from here, making it more verbose but also potentially less effort across this many Scala 3 versions.

tgodzik · 2021-03-29T20:59:53Z

@keynmol can't we leave cli on 2.13 only just as @olafurpg said?

The cli module will need to stay on 2.13 because it uses macros.

Edit: We would have to move that into a separate module.

keynmol · 2021-03-29T21:04:29Z

@tgodzik

I'm probably confused then - in Olaf's original PR, mdoc module dependsOn(cli) - which in this case means mdoc has to be cross-built for same versions as cli

Are you proposing an inverse relation? cli depends on mdoc and stays on 2.13 only? And moving CliSuite to cli module?

tgodzik · 2021-03-29T21:18:49Z

I'm probably confused then - in Olaf's original PR, mdoc module dependsOn(cli) - which in this case means mdoc has to be cross-built for same versions as cli

Are you proposing an inverse relation? cli depends on mdoc and stays on 2.13 only? And moving CliSuite to cli module?

I haven't looked closely at the PR 😓 , but cli seems to be build only for scala 2 versions. Was that working? I think the plan was not to build it for Scala 3. If it's impossible to reverse the relation (cli do depend on mdoc), we could put everything cli related into scala-2 directory. I can take a look closer tomorrow and see what's going on.

tgodzik · 2021-03-29T21:24:04Z

For sure we could CLI tests into scala-2 directory and run it only for Scala 2.

keynmol · 2021-03-29T21:26:51Z

I hope Olaf can shed light :D

Will take a closer look tomorrow at what interface is passed through between cli and mdoc (MainSettings?) and perhaps I just did something silly and misunderstood how the two are separated.

tgodzik · 2021-03-30T13:32:44Z

I hope Olaf can shed light :D

Will take a closer look tomorrow at what interface is passed through between cli and mdoc (MainSettings?) and perhaps I just did something silly and misunderstood how the two are separated.

Overall, I think MainSettings will just not work for Scala 3. we either need to migrate metaconfig to Scala 3 (in that case only 3.0.0-RC2 would be enough) or switch to a separate configuration library (something like sconfig for example). The same problem exists in scalafix scalacenter/scalafix#1316

olafurpg · 2021-03-31T07:26:15Z

I would oppose switching metaconfig with sconfig. My long-term plan is to replace metaconfig with Scalameta/moped but neither of those projects currently support Scala 3. If there’s any effort to upgrade the macros for Scala 3 then it should ideally happen in Moped, but it’s not something I expect will happen in the near future.

The cli module can compile with 2.13 and be used from 3. This was working in my original mdoc PR making the build compile with Scala 3. There are maybe some issues with this approach if users have both sourcecode 2.13/3 on their classpath but it should be fixable.

keynmol · 2021-03-31T19:26:02Z

UPDATE: it was necessary to set 2.13 on cli explicitly 🤦 that was some days lost :D

According to compatbility guide it should work, yep.

I got thrown off because locally when I run sbt '++3.0.0-M3 test' (like CI does) I get:

...
[warn] There may be incompatibilities among your library dependencies; run 'evicted' to see detailed eviction warnings.
[info] compiling 1 Java source to /home/velvetbaldmime/projects/mdoc/mdoc-interfaces/target/scala-3.0.0-M3/classes ...
[info] compiling 1 Scala source to /home/velvetbaldmime/projects/mdoc/tests/tests/target/scala-3.0.0-M3/classes ...
[warn]
[warn]  Note: Unresolved dependencies path:
[error] sbt.librarymanagement.ResolveException: Error downloading org.scalameta:mdoc-cli_3.0.0-M3:2.2.18+26-36c4b783+20210331-2011-SNAPSH

And I got the same on your branch..

I've removed scala 3 from cli and pushed, will see if it behaves differently on CI.

olafurpg · 2021-04-12T08:16:14Z

tests/unit/src/test/scala/tests/cli/MdocPropertiesSuite.scala


 class MdocPropertiesSuite extends FunSuite {
  test("default") {
    val obtained = MdocProperties.default(PathIO.workingDirectory).scalacOptions

-    if (BuildInfo.scalaVersion.startsWith("3.0"))
+    if (mdoc.internal.BuildInfo.scalaVersion.startsWith("3.0"))


FYI, I've found it very helpful to create custom MUnit tags to skip tests based on the Scala version. See https://github.com/scalameta/munit/blob/35d868403aca0e71b31b8a794472ddcbee370334/tests/shared/src/test/scala/munit/BaseSuite.scala for an example of how it's used in the MUnit codebase

We have similar tags in the cross tests in Metals also.

olafurpg

This has been a lot of work! Thank you @keynmol for this contribution. It's fantastic to see that mdoc will soon be able to support Scala 3.

The magic import tests in FileSuite aren't that important to support in Scala 3. Internally, mdoc kind of has ammonite-style scripting support excluding incremental compilation and several other caveats. I never got around to fully polish that functionality into a publicly documented feature. It's totally fine to run those tests only in Scala 2.

I would recommend using MUnit tags to skip failing tests in Scala 3 and then merge this PR before the diff gets any bigger!

build.sbt

cli/src/main/scala-3/mdoc/internal/cli/SettingsGeneric.scala

mdoc/src/main/scala-3/mdoc/internal/markdown/Instrumenter.scala

tgodzik

Great work! I think it's almost finished, I left some comments/questions.

build.sbt

tgodzik · 2021-04-12T13:15:38Z

cli/src/main/scala/mdoc/PreModifier.scala

@@ -10,7 +10,7 @@ import scala.meta.inputs.Input
 import scala.meta.io.AbsolutePath
 import scala.collection.JavaConverters._
 import scala.meta.io.RelativePath
-import mdoc.internal.pos.PositionSyntax._
+// import mdoc.internal.pos.PositionSyntax._


Let's remove this one if it compiles

cli/src/main/scala/mdoc/internal/cli/InputFile.scala

cli/src/main/scala/mdoc/internal/cli/Settings.scala

tgodzik · 2021-04-12T13:36:19Z

mdoc/src/main/scala/mdoc/internal/markdown/Processor.scala

+
+  val scala =
+    if (BuildInfo.scalaBinaryVersion.startsWith("3.0"))
+      Scala3.withAllowToplevelTerms(true).withAllowToplevelStatements(true)


withAllowToplevelStatements is true by default.

mdoc/src/main/scala/mdoc/internal/markdown/Processor.scala

tgodzik · 2021-04-12T13:39:12Z

tests/unit/src/test/scala/tests/cli/MdocPropertiesSuite.scala


 class MdocPropertiesSuite extends FunSuite {
  test("default") {
    val obtained = MdocProperties.default(PathIO.workingDirectory).scalacOptions

-    if (BuildInfo.scalaVersion.startsWith("3.0"))
+    if (mdoc.internal.BuildInfo.scalaVersion.startsWith("3.0"))


We have similar tags in the cross tests in Metals also.

tests/unit/src/test/scala/tests/cli/ScalacOptionsSuite.scala

tgodzik · 2021-04-12T14:10:45Z

tests/worksheets/src/test/scala/tests/worksheets/WorksheetSuite.scala

       |val xx = fn
-       |         ^^
+       |           ^


We worked around the issue, because this was causing issues for edit distance. If it's on a non-existing position we cannot match it to any real token. #386

mdoc/src/main/scala-3/mdoc/internal/markdown/MarkdownCompiler.scala

tgodzik · 2021-04-16T15:26:52Z

.github/workflows/ci.yml

@@ -18,7 +18,7 @@ jobs:
    steps:
      - uses: actions/checkout@v2
      - uses: olafurpg/setup-scala@v10
-      - run: sbt docs/mdoc
+      - run: sbt '++2.12.13 docs/mdoc'


Isn't this the default version?

It no longer is - I had to set 2.13 to be the default for the "Scala 3 module depending on 2.13 module to work"

Here's the failure: https://github.com/scalameta/mdoc/runs/2331363147#step:4:256

cli module is a dependency of mdoc, which is a dependency of docs and it needs to be on 2.12 for I guess reusing the SBT plugin code

Is this still necessary after #493? Evilplot has 2.13 releases on Sonatype

tgodzik · 2021-04-16T15:28:21Z

tests/worksheets/src/test/scala/tests/worksheets/WorksheetSuite.scala

       |val xx = fn
-       |         ^^
+       |           ^


Could you open up an issue to figure it out later on?

keynmol · 2021-04-16T17:03:01Z

Issue opened for the token matching issue: #485

tgodzik

I left some more comments, but I think we are close to finishing. @keynmol do you need any help with it? We can always merge as is and try to improve things later on as long as Scala 2 functionality remain. What do you think?

tgodzik · 2021-04-30T16:48:34Z

tests/unit/src/test/scala/tests/cli/ScalacOptionsSuite.scala

@@ -99,8 +110,9 @@ class ScalacOptionsSuite extends BaseCliSuite {
    onStdout = { out => assert(!out.contains("discarded non-Unit value")) }
  )

+  // TODO: fix for Scala 3


Does the flag even work for Scala 3? We can create a follow up issue.

It does exist but it doesn't seem to do anything in REPL - is it the TPrint that prints out the type?

For Scala 3 it's based on macros. It's not the TPrint as in case of Scala 2

It's OK to remove the TODO comment. It's already explicitly documented that this test is skipped in Scala 3. If it's important to fix this test then it's better to open an issue and link to the issue from here

build.sbt

tgodzik · 2021-04-30T16:56:45Z

mdoc/src/main/scala-3/mdoc/internal/markdown/MarkdownCompiler.scala

@@ -111,6 +116,14 @@ class MarkdownCompiler(
    report(vreporter, input, fileImports, run.runContext, edit)
  }

+  class CollectionReporter extends dotty.tools.dotc.reporting.Reporter {


Can we use FilterStoreReporter ?

Looking at it - it seems to be implemented in terms of Scala 2 models, do you mean this one? https://github.com/scalameta/mdoc/blob/main/mdoc/src/main/scala-2/mdoc/internal/markdown/FilterStoreReporter.scala (and its version specific variations)

I had to create this one because (trying to reconstruct from memory) because I was getting NPE from the built-in one and for warning tests I needed all the diagnostics, and by default only errors are surfaced

mdoc/src/main/scala-3/mdoc/internal/markdown/MarkdownCompiler.scala

keynmol · 2021-05-01T08:45:25Z

We can always merge as is and try to improve things later on as long as Scala 2 functionality remain

Yep - that's why I didn't unify things like instrumenter - to avoid breaking the Scala 2 versions, and combine them in a later issue.

tgodzik

I think we should merge it and fix the rest of the issues in separate PRs. I don't see anything really blocking here. @olafurpg what do you think?

olafurpg

LGTM 👍 This PR represents significant work. It's a fantastic milestone to be able to support Scala 3 and Scala 2 with mdoc. Thank you @keynmol and @tgodzik for making this happen 🙏

olafurpg · 2021-05-07T13:49:14Z

.github/workflows/ci.yml

@@ -18,7 +18,7 @@ jobs:
    steps:
      - uses: actions/checkout@v2
      - uses: olafurpg/setup-scala@v10
-      - run: sbt docs/mdoc
+      - run: sbt '++2.12.13 docs/mdoc'


Is this still necessary after #493? Evilplot has 2.13 releases on Sonatype

olafurpg · 2021-05-07T13:49:37Z

.github/workflows/mdoc.yml

@@ -12,6 +12,6 @@ jobs:
          fetch-depth: 0
      - uses: olafurpg/setup-scala@v10
      - uses: olafurpg/setup-gpg@v3
-      - run: sbt docs/docusaurusPublishGhpages
+      - run: sbt '++2.12.13 docs/docusaurusPublishGhpages'


Ditto, evilplot

Ah, I think I got it now.

We need 2.13 to be the default version in the build for the Scala 3 module -> dependsOn Scala 2.13 module to work

2.12 is needed in docs specifically because it depends on the plugin which cannot be built for 2.13 (SBT)

Hmm, why does the docs project depend on the sbt plugin? The plugin is enabled in the docs project but there shouldn’t be a project dependency there 🤔

either way, feel free to ignore this comment

olafurpg · 2021-05-07T13:51:02Z

mdoc/src/main/scala-3/mdoc/internal/markdown/CodePrinter.scala

+import java.io.PrintStream
+
+class CodePrinter(ps: PrintStream, baseIndent: Int = 0, baseNest: Int = 0) {
+  private def indent = "  " * (baseIndent + nestCount)


What happens if the user has 3-space indent?

Pushed the test in 2198002

Looks good. Always worth having tests like these

olafurpg · 2021-05-07T13:55:27Z

tests/unit/src/test/scala/tests/cli/ScalacOptionsSuite.scala

@@ -99,8 +110,9 @@ class ScalacOptionsSuite extends BaseCliSuite {
    onStdout = { out => assert(!out.contains("discarded non-Unit value")) }
  )

+  // TODO: fix for Scala 3


It's OK to remove the TODO comment. It's already explicitly documented that this test is skipped in Scala 3. If it's important to fix this test then it's better to open an issue and link to the issue from here

olafurpg · 2021-05-07T13:58:36Z

tests/unit/src/test/scala/tests/markdown/NestSuite.scala

@@ -231,7 +355,7 @@ class NestSuite extends BaseMarkdownSuite {
  )

  checkError(
-    "implicit-nok",
+    "implicit-nok".tag(SkipScala3),


This could be worth commenting next to the test.

keynmol · 2021-05-09T07:39:20Z

olafurpg and others added 3 commits January 6, 2021 11:48

Remove Scala 3 compiler warnings.

82c7488

Makes it easier to work in the build.

Return Scalameta for Scala3 and make tests

e069cec

fail proper this time.

keynmol added 4 commits March 27, 2021 11:23

Make cli module compile (with macro caveats) for Scala 3

267fc55

fix cross-building

968ff35

Merge branch 'master' into start-fixing-scala3-tests

ce3ddb2

Separate JS unit tests to enable Scala3 cross-compilation

d771fe6

keynmol added 2 commits March 27, 2021 13:12

Fix worksheets suite on 2.11, remove noise

d76448f

Run scalafmt

92b93fa

keynmol commented Mar 27, 2021

View reviewed changes

Fix scripted tests

36c4b78

keynmol mentioned this pull request Mar 29, 2021

mdoc run fails in dotty 3.0.0-M1 #414

Closed

Merge branch 'master' into start-fixing-scala3-tests

69891d2

Try not compiling cli for scala 3

b958a91

Fix BuildInfo

4f49476

olafurpg reviewed Apr 12, 2021

View reviewed changes

olafurpg requested changes Apr 12, 2021

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

cli/src/main/scala-3/mdoc/internal/cli/SettingsGeneric.scala Outdated Show resolved Hide resolved

mdoc/src/main/scala-3/mdoc/internal/markdown/Instrumenter.scala Outdated Show resolved Hide resolved

tgodzik requested changes Apr 12, 2021

View reviewed changes

keynmol added 6 commits April 13, 2021 08:56

PR comments: Remove unused code

9a6812d

PR comments: Move unit test helpers/base specs

ac2861f

PR comments: move code, remove deprecated methods

9f24835

PR comments: Revert position changes and ignore FileSuite

68da375

scalafmt

2c15733

Force 2.12 on website publishing

06e17e0

keynmol commented Apr 14, 2021

View reviewed changes

mdoc/src/main/scala-3/mdoc/internal/markdown/MarkdownCompiler.scala Show resolved Hide resolved

keynmol added 2 commits April 16, 2021 08:24

Ignore/revert some scala 3 tests

bc77dcc

formatting

921c316

tgodzik reviewed Apr 16, 2021

View reviewed changes

tgodzik reviewed Apr 30, 2021

View reviewed changes

keynmol added 2 commits May 1, 2021 09:51

Merge main, remove bloopGenerate settings

4d55f8f

Remove dotty compat from cli module

11c946a

tgodzik approved these changes May 5, 2021

View reviewed changes

Merge branch 'main' into start-fixing-scala3-tests

d931ab3

tgodzik mentioned this pull request May 6, 2021

Add option to specify modifier in the worksheet API #492

Merged

olafurpg approved these changes May 7, 2021

View reviewed changes

PR comments + Scala 3 specific three space indent test

2198002

keynmol changed the title ~~Start fixing scala3 tests~~ Scala 3 support May 8, 2021

keynmol merged commit 824523e into main May 9, 2021

keynmol deleted the start-fixing-scala3-tests branch May 9, 2021 07:38

This was referenced May 10, 2021

Support for dotty #347

Closed

Unify code printers for Scala 2 and Scala 3 #498

Open

Scala 3 support #466

Scala 3 support #466

Conversation

keynmol commented Mar 21, 2021

tgodzik commented Mar 22, 2021

keynmol commented Mar 25, 2021

tgodzik commented Mar 26, 2021

olafurpg commented Mar 26, 2021

keynmol commented Mar 26, 2021

keynmol commented Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

olafurpg commented Mar 28, 2021

keynmol commented Mar 29, 2021

tgodzik commented Mar 29, 2021 • edited Loading

keynmol commented Mar 29, 2021

tgodzik commented Mar 29, 2021

tgodzik commented Mar 29, 2021

keynmol commented Mar 29, 2021

tgodzik commented Mar 30, 2021

olafurpg commented Mar 31, 2021

keynmol commented Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

tgodzik 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

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

keynmol commented Apr 16, 2021

tgodzik 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keynmol commented May 1, 2021

tgodzik left a comment

Choose a reason for hiding this comment

olafurpg 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

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

keynmol commented May 9, 2021

keynmol commented Mar 27, 2021 •

edited

Loading

tgodzik commented Mar 29, 2021 •

edited

Loading

keynmol commented Mar 31, 2021 •

edited

Loading