Feed to whole classpath to scalafix and fix `ExplicitResultTypes` #172

lefou · 2023-12-14T12:35:57Z

Feed the whole classpath to scalafix, which is needed for some scalafix rules like ExplicitReusltTypes to work correctly.

Added the ExplicitResultTypes rule to the integration test to reproduces the initial issue discovered in a Mill PR (github.com/com-lihaoyi/mill/pull/2922) and make sure we correctly handle it.

Fix #174

This PR also raises the minimal supported Mill version to 0.10.15, as this is the fist release which makes the ScalaModule.semanticDbEnablePluginScalacOptions task accessible for inheriting modules.

lefou · 2024-01-06T10:21:29Z

I tried to fix the issue by applying the findings of @bjaglin in #174 (comment) and the details of the Javadoc comments for ScalafixArguments.withClasspath:

     /**
     * @param classpath Full Java classpath of the module being fixed. Required for running
     *                  semantic rewrites such as ExpliticResultTypes. Source files that
     *                  are to be fixed must be compiled with the semanticdb-scalac compiler
     *                  plugin and must have corresponding <code>META-INF/semanticdb/../*.semanticdb</code>
     *                  payloads. The dependency classpath must be included as well but dependency
     *                  sources do not have to be compiled with semanticdb-scalac.
     */
    ScalafixArguments withClasspath(List<Path> classpath);

This indeed makes the ExplicitResultTypes rule work, but now another integration test custom fails with the following error:

[info] compiling 1 Scala source to /home/runner/work/mill-scalafix/mill-scalafix/out/itest/0.10.12/test.dest/custom-rule/out/project/compile.dest/classes ...
Error:  bad option: -P:semanticdb:synthetics:on
Error:  one error found
1 targets failed
project.compile Compilation failed

This reproduces an issue discovered in a Mill PR * com-lihaoyi/mill#2922

lefou · 2024-01-06T10:50:14Z

but now another integration test custom fails with the following error:

[info] compiling 1 Scala source to /home/runner/work/mill-scalafix/mill-scalafix/out/itest/0.10.12/test.dest/custom-rule/out/project/compile.dest/classes ...
Error:  bad option: -P:semanticdb:synthetics:on
Error:  one error found
1 targets failed
project.compile Compilation failed

Although, I have now idea, why this didn't fail before, I think the issue is, that we can't feed extra semanticDb options via scalacOptions, as the semanticDb plugin isn't loaded for normal compilation.

Instead, we need to customize the options specific for the semanticDbData task. Turns out, in Mill we never anticipated such a need an haven't provided a dedicated semanticDbOptions target.

But I think we can override the semanticDbEnablePluginScalacOptions target to get the same result.

lefou · 2024-01-06T11:17:21Z

Looks like the semanticDbEnablePluginScalacOptions target is private in Mill 0.10, so I can't make the custom-rules feature work out-of-the-box.

Here are our options:

Don't support custom options for semanticdb for Mill 0.10
No longer support Mill 0.10 in mill-scalafix
Fix Mill 0.10 and make semanticDbEnablePluginScalacOptions protected as in Mill 0.11
Introduce a new semanticDbOptions target in Mill 0.11 and backport it to Mill 0.10.

I think option 1. is the most straight forward actionable options. Additionally, we might strive for option 4.

Another workaround for the custom rules features in Mill 0.10 might be to explicitly add the semanticDb plugin to the project, such that it is available in the default compilation and can be customized via scalacOptions.

This makes it accessible for downstream tooling. * Use case: joan38/mill-scalafix#172 Essentially, this is a backport of what we have in Mill 0.11, so this PR is in the spirit of the previous pull requests we merged. Pull request: #2951

bjaglin · 2024-01-06T22:57:20Z

My understanding is that on master, it's possible but cumbersome to run rules that require -P:semanticdb:synthetics:on (since adding the option prevents regular compiling, so you can only do it for one-shot rule execution, outside CI). Of course, on Mill 0.11, it's better as it's possible without compromising the compile task just like shown in 9a72ab9, but it was probably unknown because undocumented until now (and unrequested anyway?).
-P:semanticdb:synthetics:on turns out to be needed by the custom rule tested here (which is was a popular one, granted), but it's not a very commonly needed flag from what I know, and I can't think of any other important semanticdb-scalac flag for scalafix rules.

As a result, from my outsider Scalafix perspective (not taking into account Mill 0.10 vs 0.11 usage), I'd say that (1) is the most pragmatic choice as downside is very limited for a big upside: better enable the built-in ExplicitResultTypes with the downside of preventing a community rule to run with Mill 0.10 than waiting to find a regression-less solution.

bjaglin · 2024-01-06T23:49:29Z

Looking at the fix, I was wondering how the incremental compiler handles a regular compilation concurrent to the semantidcb-output one, as introduced by this PR. A quick look at out/itest/0.11.1/test.dest/fix/out/chrome-profile.json seems to indicate that they are indeed 2 separate processes as the task graph shows, most likely serialized by the zinc worker.

How much do they benefit from each other and how does their order impact the incremental gain? Considering the overhead of semanticdb-scalac, I assume the design rationale behind the 2 separate compilations was that it is beneficial to postpone the semantidcb-output one (potentially to "never"). However, for cold scalafix invocations, the extra compilation may become a significant overhead on large projects.

Another workaround for the custom rules features in Mill 0.10 might be to explicitly add the semanticDb plugin to the project, such that it is available in the default compilation and can be customized via scalacOptions.

This functional workaround would also address the performance consideration above I believe. I assume mill-scalafix could then override semanticDbData to rely on compile so that BSP or other clients of that task benefit from the eager semanticdb generation? Sorry if this is slightly off topic, I went down the rabbit hole of Mill internals with this issue 😄

lefou · 2024-01-07T09:44:04Z

In Mill, semanticDbData is a complete separate zinc invocation, with the only purpose to produce semanticDb files. The idea is to be able to make semanticDb available in any modules, without the need to change and impact the regular compile workflow. There are various cons with having semanticDb enabled with the default compilation, like compilation slowdown or potentially invalid compiler options (-Werror). On the other side, semanticDb is needed in Metals or other toolchains like scalafix, hence I'm rather proud to found a way to produce it without affecting the normal compilation at all. There's a even longer story. If you interested, you can read the comments of PR com-lihaoyi/mill#1977, which initially introduced it in Mill, and follow the pointers.

lefou · 2024-01-07T09:56:40Z

How much do they benefit from each other and how does their order impact the incremental gain?

Both tasks are independent but take profit from previous runs (they are incremental) as well as the upstream zinc analysis data.

Considering the overhead of semanticdb-scalac, I assume the design rationale behind the 2 separate compilations was that it is beneficial to postpone the semantidcb-output one (potentially to "never"). However, for cold scalafix invocations, the extra compilation may become a significant overhead on large projects.

The semanticDb compiler invocation just stops after the phase that generates/writes the semanticDb data, so the overhead is lower then you might think. And in a large project with multiple modules, the compiler is already hot after a small amount of module compilations, so it's not that bad.

Another workaround for the custom rules features in Mill 0.10 might be to explicitly add the semanticDb plugin to the project, such that it is available in the default compilation and can be customized via scalacOptions.

This functional workaround would also address the performance consideration above I believe. I assume mill-scalafix could then override semanticDbData to rely on compile so that BSP or other clients of that task benefit from the eager semanticdb generation?

Mill could be more clever in semanticDbData task and detect, whether the compile task already produced all of it. But that's not that simple and probably not worth the work and resulting code complexity. (At least, there is no open request for it.)

Sorry if this is slightly off topic, I went down the rabbit hole of Mill internals with this issue 😄

No problem and Kudos for doing that. I hope you enjoyed it and try out Mill someday.

lefou · 2024-01-07T10:25:56Z

Here are our options:

Don't support custom options for semanticdb for Mill 0.10

No longer support Mill 0.10 in mill-scalafix

Fix Mill 0.10 and make semanticDbEnablePluginScalacOptions protected as in Mill 0.11

Introduce a new semanticDbOptions target in Mill 0.11 and backport it to Mill 0.10.

I went with option 3. I tagged Mill 0.10.14 which will contain com-lihaoyi/mill#2951. Once, it is released, I will update this PR.

lefou · 2024-01-07T14:30:57Z

build.sc

@@ -12,7 +12,7 @@ import mill.scalalib.api.ZincWorkerUtil.scalaNativeBinaryVersion
 import mill.scalalib.publish.{Developer, License, PomSettings, VersionControl}
 import scalalib._

-val millVersions                           = Seq("0.10.12", "0.11.1")
+val millVersions                           = Seq("0.10.15", "0.11.0")


We depend on new API since Mill 0.10.15.

lefou · 2024-01-07T14:31:18Z

itest/src/check/build.sc

@@ -3,6 +3,6 @@ import com.goyeau.mill.scalafix.ScalafixModule
 import mill.scalalib._

 object project extends ScalaModule with ScalafixModule {
-  def scalaVersion  = "2.13.6"
+  def scalaVersion  = "2.13.10"


Necessary to make test succeed.

bjaglin · 2024-01-07T15:15:43Z

I'm rather proud to found a way to produce it without affecting the normal compilation at all. There's a even longer story. If you interested, you can read the comments of PR com-lihaoyi/mill#1977, which initially introduced it in Mill, and follow the pointers.

Pretty cool indeed! I'll definitely keep an eye on whether you manage to pull a -Ystop-after-like trick for dotty, and consider following the same pattern in sbt-scalafix (I wouldn't change anything at this time for Scala 2.x).

And in a large project with multiple modules, the compiler is already hot after a small amount of module compilations, so it's not that bad.

Makes sense. #175 is a lower-hanging fruit anyway if performance ever becomes a concern.

I hope you enjoyed it and try out Mill someday.

I definitely did enjoy the design. At the very least I'm gonna watch the mill-scalafix repo and chime-in when I can help. Thanks for your answers.

bjaglin

Looks good from a Scalafix perspective 👍

bjaglin · 2024-01-07T15:23:15Z

mill-scalafix/src/com/goyeau/mill/scalafix/ScalafixModule.scala

@@ -25,7 +25,7 @@ trait ScalafixModule extends ScalaModule {
        T.ctx().log,
        repositoriesTask(),
        filesToFix(sources()).map(_.path),
-        Seq(semanticDbData().path),
+        classpath = (compileClasspath() ++ localClasspath() ++ Seq(semanticDbData())).iterator.toSeq.map(_.path),


A potential further improvement to reduce the added overhead would be to avoid triggering any compilation if none of the rules is semantic (rulesThatWillRun.exists(_.kind.isSemantic)) like it's done in sbt-scalafix. Not sure how/if this (dynamic tasks in sbt) can be achieved in Mill.

Could be tricky, as the task graph can't be changed once it has started. But we have some options at construction time, depending on how lightweight (fast) a check can be.

Anyways, this should be done in a separate PR.

lefou · 2024-01-07T20:44:17Z

I tested this PR on com-lihaoyi/mill#2922 and it now properly changes the code with missing type annotations.

joan38 · 2024-01-07T21:23:17Z

Thanks @lefou and @bjaglin!
I will make a release.

joan38 · 2024-01-08T15:29:07Z

I released 0.3.2

lefou mentioned this pull request Dec 14, 2023

Check for explicit result types in public API com-lihaoyi/mill#2922

Merged

lefou mentioned this pull request Jan 5, 2024

ExlicitResultTypes fix is not working #174

Closed

lefou changed the title ~~Add test for ExlicitResultTypes fix~~ Add (failing) test for ExlicitResultTypes fix Jan 5, 2024

lefou changed the title ~~Add (failing) test for ExlicitResultTypes fix~~ Feed to whole classpath to scalafix and fix ExlicitResultTypes Jan 6, 2024

lefou changed the title ~~Feed to whole classpath to scalafix and fix ExlicitResultTypes~~ Feed to whole classpath to scalafix and fix ExplicitResultTypes Jan 6, 2024

lefou added 3 commits January 6, 2024 11:25

Add test for ExlicitResultTypes fix

f941cbc

This reproduces an issue discovered in a Mill PR * com-lihaoyi/mill#2922

Fix expectation

b62056a

Feed the whole classpath to scalafix

2aa70ec

lefou force-pushed the explicit-result-type branch from b840857 to 2aa70ec Compare January 6, 2024 10:25

Moved semanticDb specific configuration out of scalacOptions.

9a72ab9

lefou mentioned this pull request Jan 6, 2024

Make semanticDbEnablePluginScalacOptions protected com-lihaoyi/mill#2951

Merged

lefou added 3 commits January 7, 2024 13:50

Update minimal supported Mill version to 0.10.14

406717c

Update minimal Mill version to 0.10.15

482b79c

Bump scala version in test

0f72b94

lefou marked this pull request as ready for review January 7, 2024 14:30

lefou commented Jan 7, 2024

View reviewed changes

bjaglin approved these changes Jan 7, 2024

View reviewed changes

joan38 merged commit 3662d3e into joan38:main Jan 7, 2024
1 check passed

lefou deleted the explicit-result-type branch January 7, 2024 21:26

lefou mentioned this pull request Jan 7, 2024

support older mill 0.10.x 0.11.y patch versions #166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feed to whole classpath to scalafix and fix `ExplicitResultTypes` #172

Feed to whole classpath to scalafix and fix `ExplicitResultTypes` #172

lefou commented Dec 14, 2023 •

edited

Loading

lefou commented Jan 6, 2024 •

edited

Loading

lefou commented Jan 6, 2024

lefou commented Jan 6, 2024 •

edited

Loading

bjaglin commented Jan 6, 2024

bjaglin commented Jan 6, 2024

lefou commented Jan 7, 2024 •

edited

Loading

lefou commented Jan 7, 2024 •

edited

Loading

lefou commented Jan 7, 2024 •

edited

Loading

lefou Jan 7, 2024

lefou Jan 7, 2024

bjaglin commented Jan 7, 2024

bjaglin left a comment

bjaglin Jan 7, 2024

lefou Jan 7, 2024

lefou commented Jan 7, 2024

joan38 commented Jan 7, 2024

joan38 commented Jan 8, 2024

Feed to whole classpath to scalafix and fix ExplicitResultTypes #172

Feed to whole classpath to scalafix and fix ExplicitResultTypes #172

Conversation

lefou commented Dec 14, 2023 • edited Loading

lefou commented Jan 6, 2024 • edited Loading

lefou commented Jan 6, 2024

lefou commented Jan 6, 2024 • edited Loading

bjaglin commented Jan 6, 2024

bjaglin commented Jan 6, 2024

lefou commented Jan 7, 2024 • edited Loading

lefou commented Jan 7, 2024 • edited Loading

lefou commented Jan 7, 2024 • edited Loading

lefou Jan 7, 2024

Choose a reason for hiding this comment

lefou Jan 7, 2024

Choose a reason for hiding this comment

bjaglin commented Jan 7, 2024

bjaglin left a comment

Choose a reason for hiding this comment

bjaglin Jan 7, 2024

Choose a reason for hiding this comment

lefou Jan 7, 2024

Choose a reason for hiding this comment

lefou commented Jan 7, 2024

joan38 commented Jan 7, 2024

joan38 commented Jan 8, 2024

Feed to whole classpath to scalafix and fix `ExplicitResultTypes` #172

Feed to whole classpath to scalafix and fix `ExplicitResultTypes` #172

lefou commented Dec 14, 2023 •

edited

Loading

lefou commented Jan 6, 2024 •

edited

Loading

lefou commented Jan 6, 2024 •

edited

Loading

lefou commented Jan 7, 2024 •

edited

Loading

lefou commented Jan 7, 2024 •

edited

Loading

lefou commented Jan 7, 2024 •

edited

Loading