feature: ExplicitResultTypes for Scala 3 #2023

tgodzik · 2024-07-25T12:13:08Z

Approach: use the stable Scala 3 presentation compiler available since 3.3.2

There is still plenty to do, and I am not 100% sure about the approach, but it's a start. Tests are passing but some outputs are different the Scala 2 ones. I also found some issues in some of the tests, I removed them until I can fix them in the compiler.

TODO:

extract common parts for both Scala 2 and 3 to a trait/abstract class
~~fix issues in the compiler~~ (non-blocking?)
download specific compiler for a specific version instead of explicitly setting dependency
~~separate tests for LTS and Next~~ with both on the latest it seem ok, but we might need some compat at some point
make sure that each patch is sequentially one after another (currently all are done one the same code, which might cause issues)

bjaglin · 2024-07-29T12:09:01Z

Exciting! I will have a look at it some time this week.

bjaglin

That looks promising, thanks for putting this together!

In terms of workflow, we could merge this quite early and mutualize code / enable more tests as we go, since nothing is published for Scala 3 anyway.

I will try to dust off #1680 in mid/late August since that remains a prerequisite. I had put it on hold as I was watching scalameta/scalameta#3347 (do you know if you or anyone else will allocate time to it by the way?), but I guess we could release experimental Scala 3 versions that would only allow rules written without quasiquotes, with the 2.13/3 sandwitch.

bjaglin · 2024-08-04T22:42:31Z

scalafix-rules/src/main/scala-3/scalafix/internal/rule/Embedded.scala

+  private def supportPresentationCompilerInDotty(scalaVersion: String) = {
+    scalaVersion.split("\\.").take(3).map(_.toInt) match {
+      case Array(3, minor, patch) => minor > 3 || minor == 3 && patch >= 4
+      case _ => false
+    }
+  }


Since we control the runtime of scalafix, and we'll first publish scala3 artifacts with 3.3.4 or later, this should always be true. If that's correct, I assume we don't need coursier and we can declare the dependency statically in scalafix-rules_3?

We could, but wouldn't we need to publish the rules for every Scala version then, not just binary? Rules are not released for every scala version, right? Potentially, we could compile the rule with 3.3.x and use them for later versions while overriding the compiler and presentation compiler versions.

Also I commented out using 2.13 compiled rules for Scala 3, but I can make it work with coursier.

tgodzik · 2024-08-08T18:26:02Z

Ok, I am getting closer to a finish. I left some TODOs I want to still address, but there is nothing major. I've seen some test failure on one of JDK versions earlier so I will also check it out (I think we need to use JDK 11 at a minimum for the Scala 3 tests, so might need to disable the tests there)

The biggest change I did is extracting most of the methods to a common base trait, but also enabled to use presentation compiler as a fallback for Scala 2. I noticed why we might be getting some failures in the tests (those removed for now), so I will also take a look.

I had put it on hold as I was watching scalameta/scalameta#3347 (do you know if you or anyone else will allocate time to it by the way?), but I guess we could release experimental Scala 3 versions that would only allow rules written without quasiquotes, with the 2.13/3 sandwitch.

I need to get back to it, I want to get something ready here and will be back to scalameta.

scalafix-rules/src/main/scala/scalafix/internal/pc/Embedded.scala

scalafix-rules/src/main/scala-3/scalafix/internal/rule/ExplicitResultTypes.scala

tgodzik · 2024-08-09T17:18:56Z

Another fun observation is that running tests with static dependency on the compiler seems much faster than the dynamic one. I am not sure what it's caused by, I was pretty sure that separate classloaders are almost zero cost.

tgodzik · 2024-08-09T17:24:06Z

Another fun observation is that running tests with static dependency on the compiler seems much faster than the dynamic one. I am not sure what it's caused by, I was pretty sure that separate classloaders are almost zero cost.

Ach, I know what is happening, we are loading a separate classloader each time for each new file 😨

bjaglin · 2024-08-09T17:36:03Z

Ach, I know what is happening, we are loading a separate classloader each time for each new file 😨

Right, we'd have to amortise the classloading cost across files, and ideally across invocations, to keep CPU/IO/RAM at a reasonable level when scalafixOnCompile := true. And shared/global state outside a build tool is scary 🙈.

tgodzik · 2024-08-09T17:47:12Z

Ok, switched to using presentation compiler statically if the minor version of Scala 3 is the same and for dynamic loading I added caching which should fix things.

bjaglin

Thanks for all the work!

I had a look at infrastructure rather than actual code of the rule, since we can iterate on that easily after.

I'd like to sort out 2 things (mentioned in comments) before we merge:

whether we want mtags or not (I think we can do without it)
move input/output files to scala instead of copying to scala-3 when possible

build.sbt

scalafix-rules/src/main/scala-3/scalafix/internal/rule/ExplicitResultTypes.scala

scalafix-rules/src/main/scala/scalafix/internal/pc/Embedded.scala

scalafix-rules/src/main/scala-2/scalafix/internal/rule/ExplicitResultTypes.scala

build.sbt

scalafix-rules/src/main/scala-3/scalafix/internal/rule/ExplicitResultTypes.scala

scalafix-rules/src/main/scala/scalafix/internal/pc/Embedded.scala

scalafix-rules/src/main/scala-2/scalafix/internal/rule/ExplicitResultTypes.scala

tgodzik · 2024-08-19T13:05:18Z

I will probably be back to this next week, sorry for the delay!

bjaglin · 2024-08-21T22:49:09Z

I will probably be back to this next week, sorry for the delay!

No worry, it gave me some time to finalize my PR about publishing cli_3.x.y #2034, in light of all discussions here. I am going to merge it very soon since it's already way too big.

Commits should be self-describing, but here are the highlights to help you rebase (I can do it if you prefer?)

core_3 is not published, let's stick to core_2.13 until scalameta_3 is out
- this allows to classload source & published 2.13 community rules from cli_3.x.y, providing a completely transparent upgrade from cli_2.x.y to cli_3.x.y when Scala 3 sources are targeted, in the next patch release of scalafix/sbt-scalafix
- I hacked something to fix metaconfig macros in cli_3.x.y (and removed cosmetic usage of sourcecode internally as it was causing even more headaches) - it's not pretty, but I think we can live with that as artifacts that use it are not meant to be used directly and binary compatibility will be broken anyway as soon as scalameta_3 is out
rules_3.x.y & cli_3.x.y are built & published with both LTS & Next
- their sbt projects, as well as all tests projects that depend on them, are now suffixed with the full version (just like their full cross-version publishing strategy, so it's a good move anyway)
- scalafix-interfaces will load cli_LTS if 3.3.y or earlier is requested, or cli_Next otherwise - clients willing to support ExplicitResultTypes on Scala 3 will need to be updated to (1) stop falling back to 2.z and (2) provide the full version information to maximize chances for the statically-packaged PC to be used
  - there is no reason to actively test cli_2.x.y against Scala 3 sources any longer since upcoming versions of sbt-scalafix & other clients should load cli_3.x.y whenever Scala 3 sources are targeted
- since cli_Next will become the default for cs install scalafix, I made sure all rules were available/tested against Scala 2 sources via expect3_5_0Target2_13_14 (at the moment, there is not reason that it shouldn't work, but it's more of a precaution for any upcoming changes)
  - I added a dirty hack to ignore ExplicitResultTypes expect tests when running in cli_3.x.y - we should be able to remove it in this PR (using the flag controlling usage of dynamic PC loading via coursier)
  - with this PR, the service loader catalog scalafix.v1.Rule should now be the same for all Scala versions so no need to have separate instances

bjaglin · 2024-08-28T23:15:03Z

@tgodzik let me know if you don't have bandwidth to handle it this week, I can try to find time for this during the weekend, ahead of the upcoming Scalafix release next week (for the new Scala 2.x and 3 LTS releases).

tgodzik · 2024-08-29T07:27:27Z

I was thinking of finishing scalameta/scalameta#3939 first, so probably I would get back to it later. If you have time and the will you are welcome to try and finish this PR 👍

Otherwise, I will probably get back to it next week.

bjaglin · 2024-08-29T07:51:14Z

Ok, ni problem, I'll invest (some) time by Monday (most likely tonight and Saturday), we'll see how far I can get!

I was thinking of finishing scalameta/scalameta#3939 first

This would mean scalameta would be published for Scala 3 but quasiquotes wouldn't be available right? I think for scalafix it wouldn't change anything (switching to these new artifacts would make backward compatibility with community rules in 2.13 harder actually). I'd like to provide ExplicitResultTypes first, and then look at #2041 in the coming week/months since it's much more tricky to have a smooth transition for rule authors.

tgodzik · 2024-08-29T12:41:26Z

This would mean scalameta would be published for Scala 3 but quasiquotes wouldn't be available right?

We have quasiquotes partly ready, so I would want to unblock that

bjaglin · 2024-09-10T21:06:28Z

I'll try to address outstanding comments and merge this PR this week-end since I have some bandwidth

bjaglin · 2024-09-25T17:16:52Z

I'll try to address outstanding comments and merge this PR this week-end since I have some bandwidth

Well, it clearly didn't happen (life happened), but I have a 15h train ride this Friday 27th, so I will take care of this before cutting a release.

tgodzik · 2024-09-26T09:31:42Z

I'll try to address outstanding comments and merge this PR this week-end since I have some bandwidth

Well, it clearly didn't happen (life happened), but I have a 15h train ride this Friday 27th, so I will take care of this before cutting a release.

No worries. I also planned to get back to it, but dealing with other bugs proved to be more urgent :/

bjaglin · 2024-09-27T11:02:09Z

2c9e4f9 -> 61dc47b

spinned off allow to use RC versions in arguments #2084
squashed everything else
consolidated as much as I could into scala/test/explicitResultTypes to make it easier to see what's missing/different
rebased against 7f471d0

I'll address comments as separate commits this afternoon. I am targeting to merge this tonight and cut a release now that 2.12.20, 2.13.15, and 3.3.4 are out.

project/ScalafixBuild.scala

They would have been useful for 2 things. 1. Support early Scala 3 versions (mtags) Considering RemoveUnused already requires a minimum Scala 3 version, and that we have presentation compiler for LTS patch releases, I don't think it is a big deal. 2. Support Scala 3 from cli_2 (mtags-interfaces + PC) Since Scalafix (build) clients have been selecting the cli version based on the target for years, and we found a backward-compatible way to let them ask for a given Scala 3 minor version, the migration path to cli_3 is easy. In both cases, dynamic loading was needed, causing potential resolution issues and more classloader isolation overhead.

tgodzik · 2024-09-27T21:15:03Z

Thanks @bjaglin for your work on this! I can do a follow ups with any issues that show up. Or especially getting all the tests working the same or similar enough like in Scala 2

tgodzik force-pushed the add-inferred-type branch 4 times, most recently from 25384fa to 69c827f Compare July 25, 2024 17:00

bjaglin reviewed Aug 4, 2024

View reviewed changes

tgodzik force-pushed the add-inferred-type branch 2 times, most recently from 385b1b1 to 1cbaae7 Compare August 9, 2024 11:42

bjaglin reviewed Aug 9, 2024

View reviewed changes

tgodzik force-pushed the add-inferred-type branch from 1cbaae7 to f91b2c4 Compare August 9, 2024 12:15

tgodzik force-pushed the add-inferred-type branch 2 times, most recently from a5aae38 to 2c9e4f9 Compare August 9, 2024 17:46

bjaglin mentioned this pull request Aug 11, 2024

Publish cli_3 & rules_3, built against core_2.13 #1680

Closed

14 tasks

bjaglin reviewed Aug 11, 2024

View reviewed changes

scalafix-rules/src/main/scala-2/scalafix/internal/rule/ExplicitResultTypes.scala Outdated Show resolved Hide resolved

bjaglin mentioned this pull request Aug 22, 2024

leverage cli_3.x.y scalacenter/sbt-scalafix#430

Merged

bjaglin mentioned this pull request Sep 27, 2024

allow to use RC versions in arguments #2084

Merged

bjaglin force-pushed the add-inferred-type branch from 2c9e4f9 to 1153521 Compare September 27, 2024 10:53

feature: Inferred type for Scala 3

61dc47b

bjaglin force-pushed the add-inferred-type branch from 1153521 to 61dc47b Compare September 27, 2024 11:03

bjaglin mentioned this pull request Sep 27, 2024

drop support for JDK8 #2085

Closed

run ExplicitResultTypes on Scala 3 projects

7c2268c

bjaglin force-pushed the add-inferred-type branch from cb79adc to eeb0235 Compare September 27, 2024 17:54

bjaglin reviewed Sep 27, 2024

View reviewed changes

project/ScalafixBuild.scala Show resolved Hide resolved

bjaglin force-pushed the add-inferred-type branch 4 times, most recently from 1ee6bf8 to ed29190 Compare September 27, 2024 19:52

bjaglin added 11 commits September 27, 2024 22:15

make it clear that we also use minor version to check compatibility

71714ac

lift more common stuff to ExplicitResultTypesBase

c8a266d

improve comment

954cdf7

hide the underlying presentation compiler

466d5a1

use blocking cache to prevent duplicates

89f9e0b

all rules are now supported on all versions

5ff95bb

s/PcExplicitResultTypes/PresentationCompilerTypeInferrer/

154d1a5

on version mismatch, protect PC dynamic loading behind a flag

c2afbcc

ensure atomicity when global imports are added

5f262d8

update docs

fe41df2

bjaglin force-pushed the add-inferred-type branch from ed29190 to fe41df2 Compare September 27, 2024 20:18

bjaglin marked this pull request as ready for review September 27, 2024 20:38

bjaglin merged commit aadace9 into scalacenter:main Sep 27, 2024
9 checks passed

tgodzik deleted the add-inferred-type branch September 27, 2024 21:13

bjaglin mentioned this pull request Sep 27, 2024

tweak JVM options to better handle project cardinality #2086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: ExplicitResultTypes for Scala 3 #2023

feature: ExplicitResultTypes for Scala 3 #2023

tgodzik commented Jul 25, 2024 •

edited by bjaglin

Loading

bjaglin commented Jul 29, 2024

bjaglin left a comment

bjaglin Aug 4, 2024

tgodzik Aug 5, 2024

tgodzik commented Aug 8, 2024

tgodzik commented Aug 9, 2024

tgodzik commented Aug 9, 2024

bjaglin commented Aug 9, 2024

tgodzik commented Aug 9, 2024

bjaglin left a comment

tgodzik commented Aug 19, 2024

bjaglin commented Aug 21, 2024 •

edited

Loading

bjaglin commented Aug 28, 2024

tgodzik commented Aug 29, 2024 •

edited

Loading

bjaglin commented Aug 29, 2024

tgodzik commented Aug 29, 2024

bjaglin commented Sep 10, 2024

bjaglin commented Sep 25, 2024

tgodzik commented Sep 26, 2024

bjaglin commented Sep 27, 2024 •

edited

Loading

tgodzik commented Sep 27, 2024

feature: ExplicitResultTypes for Scala 3 #2023

feature: ExplicitResultTypes for Scala 3 #2023

Conversation

tgodzik commented Jul 25, 2024 • edited by bjaglin Loading

bjaglin commented Jul 29, 2024

bjaglin left a comment

Choose a reason for hiding this comment

bjaglin Aug 4, 2024

Choose a reason for hiding this comment

tgodzik Aug 5, 2024

Choose a reason for hiding this comment

tgodzik commented Aug 8, 2024

tgodzik commented Aug 9, 2024

tgodzik commented Aug 9, 2024

bjaglin commented Aug 9, 2024

tgodzik commented Aug 9, 2024

bjaglin left a comment

Choose a reason for hiding this comment

tgodzik commented Aug 19, 2024

bjaglin commented Aug 21, 2024 • edited Loading

bjaglin commented Aug 28, 2024

tgodzik commented Aug 29, 2024 • edited Loading

bjaglin commented Aug 29, 2024

tgodzik commented Aug 29, 2024

bjaglin commented Sep 10, 2024

bjaglin commented Sep 25, 2024

tgodzik commented Sep 26, 2024

bjaglin commented Sep 27, 2024 • edited Loading

tgodzik commented Sep 27, 2024

tgodzik commented Jul 25, 2024 •

edited by bjaglin

Loading

bjaglin commented Aug 21, 2024 •

edited

Loading

tgodzik commented Aug 29, 2024 •

edited

Loading

bjaglin commented Sep 27, 2024 •

edited

Loading