Scalafix command for scala-cli with basic optionns and tests #2968

Vigorge · 2024-06-17T10:10:04Z

Resolves Issue #647

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

Gedochao · 2024-06-18T13:37:54Z

Hey, thanks for the contribution!
I'll make sure to review this as soon as I am available, but given the volume of code included, I might need a couple days to get to it.

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

modules/integration/src/test/scala/scala/cli/integration/ScalafixTests.scala

Gedochao · 2024-06-25T10:00:49Z

NOTE: After this is cleaned up and merged, we'll want to merge the scalafix sub-command with the fix sub-command (there's no point to keep them separate).
Individual fix features would probably be enabled/disabled by flags.
We'll do that as a follow-up, no need to worry about it in this PR.

bjaglin

Thanks for starting this 🤩

I am not familiar with scala-cli at all, but as the maintainer of sbt-scalafix and scalafix, here is some feedback FWIW!

modules/cli/src/main/scala/scala/cli/commands/scalafix/ScalafixOptions.scala

project/deps.sc

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

modules/integration/src/test/scala/scala/cli/integration/ScalafixTests.scala

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

modules/cli/src/main/scala/scala/cli/commands/scalafix/ScalafixOptions.scala

tgodzik · 2024-06-26T12:21:50Z

Thanks @bjaglin!

@Vigorge if you are at any point overwhelmed by amount of comments, we are here to help. You already did a lot of great work 😍

Vigorge · 2024-07-04T13:35:16Z

Thanks @bjaglin!

@Vigorge if you are at any point overwhelmed by amount of comments, we are here to help. You already did a lot of great work 😍

Thanks a lot for such an impactful feedback! These comments are really helpful and I'll surely take them into account. I really hope that I will resolve them in a few days and add some more features. Unfortunately, I had to take a little break, but now am fully committed to work more on this PR now

modules/cli/src/main/scala/scala/cli/commands/scalafix/ScalafixOptions.scala

Gedochao · 2024-08-08T09:18:50Z

Not sure why didn't the CI run, but a rebase from main seems to be necessary, anyway.

SethTisue · 2024-08-08T17:12:02Z

Great to see activity on this — I think this could be big for Scalafix, in terms of people being able to use it more easily.

modules/integration/src/test/scala/scala/cli/integration/ScalafixTests.scala

Vigorge · 2024-08-12T13:45:24Z

Hi, sorry for taking such a long time to resolve all those threads and thanks a lot again for such feedback.

The PR is ready for review.

bjaglin · 2024-08-12T14:50:03Z

Thanks for the follow-ups @Vigorge 👍 I will take a look at the PR tomorrow.

bjaglin

Hey @Vigorge, thanks for all the follow-ups/additions. Having a semantic rule tested against all Scala versions is a great achievement!

I went through a thorough second review to avoid any surprise in the third/final round. From my very own perspective (again, I am not familiar with scala-cli), here are the outstanding comments to be addressed before this could be released

workspace / current directory injection seems broken - I did not manage to use the CLI
diagnostics/linters are ignored under --check
using scalafix-interfaces with native-image is impossible and raises a cryptic error - a more actionnable error message should be added until this is sorted out in a further PR

bjaglin · 2024-08-13T07:10:13Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

+    case CommandLineError =>
+      "A command-line argument was parsed incorrectly"
+    case MissingSemanticdbError =>
+      "A semantic rewrite was run on a source file that has no associated META-INF/semanticdb/.../*.semanticdb"


nit: all scalafix rules are not rewrites

Suggested change

"A semantic rewrite was run on a source file that has no associated META-INF/semanticdb/.../*.semanticdb"

"A semantic rule was run on a source file that has no associated META-INF/semanticdb/.../*.semanticdb"

bjaglin · 2024-08-13T08:03:45Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

+        evaluation.getErrorMessage.toScala.toList
+    }
+    else
+      customScalafixInstance.run().map(prepareErrorMessage).toList


Scalafix already outputs human readable messages to stdout by default, so users will get duplicate error messages.

$ launcher --power scalafix Test.scala The `scalafix` sub-command is experimental Please bear in mind that non-ideal user experience should be expected. If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at https://github.com/VirtusLab/scala-cli No rules requested to run No rules were provided to Scalafix so nothing happened

The scalafix framework output (not to be mixed up with the scalafix rules effective output, see comment above) can be silenced or re-routed via withPrintStream(). As the internal log entries can carry more information than the status code, I would recommend removing the custom error messages, and either mapping a PrintStream delegating to the scala-cli logger (that's what sbt-scalafix does) or actually simply keeping the default PrintStream (stdout), for the same reason as in the comment above.

bjaglin · 2024-08-13T08:16:18Z

modules/integration/src/test/scala/scala/cli/integration/ScalafixTests213.scala

+  test("external rule") {
+    val unnamedParamsInputsContent: String =
+      """//> using options -P:semanticdb:synthetics:on
+        |//> using compileOnly.dep "com.github.jatcwang::scalafix-named-params:0.2.4"


Will possibility of loading custom rules via compileOnly.dep be obvious to a scala-cli user? It wasn't to me, so a mention of it in the docs may be useful.

bjaglin · 2024-08-13T08:25:59Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/ScalafixOptions.scala

+
+  @Group(HelpGroup.Format.toString)
+  @Tag(tags.experimental)
+  @HelpMessage("Fail the invocation instead of applying fixes")


nit: some rules won't emit rewrites (they are just linters), in which case --check does not change anything, so I would be more specific here

Suggested change

@HelpMessage("Fail the invocation instead of applying fixes")

@HelpMessage("Fail the invocation if rewrites are needed")

bjaglin · 2024-08-13T08:30:07Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/ScalafixOptions.scala

+  implicit lazy val help: Help[ScalafixOptions]     = Help.derive
+
+  val cmdName             = "scalafix"
+  private val helpHeader  = "Fixes Scala code according to scalafix rules."


nit: adding the linter aspect of scalafix to the description

Suggested change

private val helpHeader = "Fixes Scala code according to scalafix rules."

private val helpHeader = "Run Scalafix rules to lint or rewrite Scala code."

bjaglin · 2024-08-13T09:54:44Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

+      val externalDeps =
+        options.shared.dependencies.compileOnlyDependency ++ successfulBuildOpt.map(
+          _.options.classPathOptions.extraCompileOnlyDependencies.values.flatten.map(_.value.render)
+        ).getOrElse(Seq.empty)


Not blocking for this PR as the experimental flag allows iterating

Even if that's likely the most harmless way to get scala-cli to resolve an artifact (either via a directive or the corresponding CLI flag I assume), this approach has several limitations

it might impact the build by evicting some transient dependencies

the fact that this is not in the base class probably shows, but it won't allow loading custom rules when running against Scala 3 sources (unless there is some kind of fallback to 2.13 artifacts built-in?), since for now Scalafix is not cross-published to Scala 3, as the mapping above shows

scala-cli/modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

Line 90 in 3e08eb2

case v if v.startsWith("3.") => "2.13"

bjaglin · 2024-08-13T10:02:51Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

+    val scalafix = ScalafixInterface
+      .fetchAndClassloadInstance(scalaBinaryVersion)
+      .newArguments()
+      .withWorkingDirectory(workspace.toNIO)


workspace seems to reference an ephemeral directory that does not contain (or no longer contains?) the source file when I run the output of ./mill -i show 'cli[].launcher' against a source in a random directory

➜ /tmp cat Test.scala //> using options -Wunused:all package foo object Hello { def main(args: Array[String]): Unit = { val name = "John" println("Hello") } }

➜ /tmp /Users/brice.jaglin/git/scala/scala-cli/out/cli/3.3.3/launcher.dest/launcher --power scalafix --rules=RemoveUnused ./Test.scala Some utilized features are marked as experimental: - `scalafix` sub-command - `--rules=RemoveUnused` option Please bear in mind that non-ideal user experience should be expected. If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at https://github.com/VirtusLab/scala-cli [warn] ./Test.scala:7:9 [warn] unused local definition [warn] val name = "John" [warn] ^^^^ error: /Users/brice.jaglin/Library/Caches/ScalaCli/virtual-projects/a7/project-c4a6da38/Test.scala is not a file Something unexpected happened running Scalafix

bjaglin · 2024-08-13T10:13:21Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

+        val s = inputs.sourceFiles().collect {
+          case sc: Script          => sc.path
+          case sc: SourceScalaFile => sc.path
+        }


nit: scalafix is doing the same check (I think), so I'd remove this, in case we add support for other extensions upstream

bjaglin · 2024-08-13T11:04:00Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

+    val relPaths          = sourcePaths.map(_.toNIO.getFileName)
+
+    val scalafix = ScalafixInterface
+      .fetchAndClassloadInstance(scalaBinaryVersion)


Unsurprisingly, this fails when using the native-image bundle because of the dynamic nature of scalafix-interfaces.

➜ /tmp /Users/brice.jaglin/git/scala/scala-cli/out/cli/3.3.3/base-image/nativeImage.dest/scala-cli --power scalafix Test.scala The `scalafix` sub-command is experimental Please bear in mind that non-ideal user experience should be expected. If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at https://github.com/VirtusLab/scala-cli Error: scalafix.interfaces.ScalafixException: Failed to load 'scalafix-interfaces.properties' to lookup versions For more details, please see '/private/tmp/.scala-build/stacktraces/1723547209-8851074228942378765.log'

➜ /tmp cat /private/tmp/.scala-build/stacktraces/1723546008-8191378505818715998.log scalafix.interfaces.ScalafixException: Failed to load 'scalafix-interfaces.properties' to lookup versions scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:126) scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:85) scala.cli.commands.scalafix.Scalafix$.runCommand(Scalafix.scala:100) scala.cli.commands.scalafix.Scalafix$.runCommand(Scalafix.scala:56) scala.cli.commands.ScalaCommand.run(ScalaCommand.scala:388) scala.cli.commands.ScalaCommand.run(ScalaCommand.scala:369) caseapp.core.app.CaseApp.main(CaseApp.scala:157) scala.cli.commands.ScalaCommand.main(ScalaCommand.scala:354) caseapp.core.app.CommandsEntryPoint.main(CommandsEntryPoint.scala:169) scala.cli.ScalaCliCommands.main(ScalaCliCommands.scala:126) scala.cli.ScalaCli$.main0(ScalaCli.scala:295) scala.cli.ScalaCli$.main(ScalaCli.scala:119) scala.cli.ScalaCli.main(ScalaCli.scala) java.lang.NullPointerException: inStream parameter is null java.base@17.0.6/java.util.Objects.requireNonNull(Objects.java:233) java.base@17.0.6/java.util.Properties.load(Properties.java:407) scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:124) scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:85) scala.cli.commands.scalafix.Scalafix$.runCommand(Scalafix.scala:100) scala.cli.commands.scalafix.Scalafix$.runCommand(Scalafix.scala:56) scala.cli.commands.ScalaCommand.run(ScalaCommand.scala:388) scala.cli.commands.ScalaCommand.run(ScalaCommand.scala:369) caseapp.core.app.CaseApp.main(CaseApp.scala:157) scala.cli.commands.ScalaCommand.main(ScalaCommand.scala:354) caseapp.core.app.CommandsEntryPoint.main(CommandsEntryPoint.scala:169) scala.cli.ScalaCliCommands.main(ScalaCliCommands.scala:126) scala.cli.ScalaCli$.main0(ScalaCli.scala:295) scala.cli.ScalaCli$.main(ScalaCli.scala:119) scala.cli.ScalaCli.main(ScalaCli.scala)

We could probably fix these property lookups by telling GraalVM about it, but the classloading just after would fail anyway.

I am afraid we will need to run Scalafix through the scalafix-cli launcher (like scalafmt apparently?) instead of scalafix-interfaces. It would be a bit more cumbersome to pass CLI args, and a couple of advanced features wouldn't be available, but it should work overall. Or maybe there is a way to classload Scalafix within bloop somehow @tgodzik ? It would have the benefit of keeping classloaders warm across invocations.

Anyway, to unlock this PR, I guess we could add an early return if the native-image runtime is detected, and handle native-image compatibility in a later PR? I assume the native-image bundle is the most popular distribution channel though... 😢

Ouch! You're right, it seems that it won't make a lot of sence if it doens't work in native image.
I'll try to replace interfaces by scalafix-cli launcher and see how it is going to turn out.

bjaglin · 2024-08-13T11:29:37Z

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala

+      val evaluation = customScalafixInstance.evaluate()
+      if (evaluation.isSuccessful)
+        evaluation.getFileEvaluations.foldLeft(List.empty[String]) {
+          case (errors, fileEvaluation) =>
+            val problemMessage = fileEvaluation.getErrorMessage.toScala.orElse(
+              fileEvaluation.previewPatchesAsUnifiedDiff.toScala
+            )
+            errors ++ problemMessage
+        }


With this implementation, --check effectively ignores diagnostics (linters)

➜ /tmp cat Test.scala package foo final object Hello { def main(args: Array[String]): Unit = { val name = null } }

Expected failure (no --check)

➜ /tmp /Users/brice.jaglin/git/scala/scala-cli/out/cli/3.3.3/launcher.dest/launcher --power scalafix --rules=DisableSyntax --scalafix-arg=--settings.DisableSyntax.noNulls=true Test.scala && echo OK Some utilized features are marked as experimental: - `scalafix` sub-command - `--rules=DisableSyntax` option - `--scalafix-arg=--settings.DisableSyntax.noNulls=true` option Please bear in mind that non-ideal user experience should be expected. If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at https://github.com/VirtusLab/scala-cli /private/tmp/Test.scala:5:16: error: [DisableSyntax.null] null should be avoided, consider using Option instead val name = null ^^^^ A Scalafix linter error was reported

Unexpected success (with --check)

➜ /tmp /Users/brice.jaglin/git/scala/scala-cli/out/cli/3.3.3/launcher.dest/launcher --power scalafix --rules=DisableSyntax --scalafix-arg=--settings.DisableSyntax.noNulls=true Test.scala --check && echo OK Some utilized features are marked as experimental: - `scalafix` sub-command - `--rules=DisableSyntax` option - `--scalafix-arg=--settings.DisableSyntax.noNulls=true` option - `--check` option Please bear in mind that non-ideal user experience should be expected. If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at https://github.com/VirtusLab/scala-cli OK

Instead of evaluating manually and reconstructing logs from lints/rewrites in each file (which might result in missing upcoming features), you should call run() with the --check flag. To capture output, you can redirect the ScalafixMainCallback (effectively the output of the scalafix rules) to the scala-cli logger (that's what sbt-scalafix does), or probably more pragmatic, just keep the default which is dumping to stdout, since moving to scalafix-cli for native-image (see other comment) won't allow this level of integration in the future anyway.

This shows that invocation output (either with or without --check for a linter rule such as DisableSyntax, and with --check for a rewrite rule such as RedundantSyntax) should be asserted in the tests.

Gedochao · 2024-09-11T08:50:40Z

@Vigorge how's the progress on this? Can we unblock you somehow?

Vigorge · 2024-09-17T15:56:39Z

@Vigorge how's the progress on this? Can we unblock you somehow?

HI, still am working on the native image solution and gonna add feature in few days.

Thanks for support! I will comment here if some help will be needed.

dos65 · 2024-10-16T13:58:54Z

@Gedochao Hi, we did changes in MR to make scalafix work both in jvm and native builds. Could you approve CI run?

Scalafix command native

dos65 · 2024-10-18T08:49:12Z

Update. I need one additionial iteration to go trough comments from @bjaglin
(add a bit more docs and tests) but in general I hope the PR is close to completion.

Vigorge marked this pull request as draft June 17, 2024 10:12

tgodzik reviewed Jun 17, 2024

View reviewed changes

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala Outdated Show resolved Hide resolved

Gedochao reviewed Jun 17, 2024

View reviewed changes

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala Outdated Show resolved Hide resolved

Gedochao reviewed Jun 25, 2024

View reviewed changes

modules/cli/src/main/scala/scala/cli/commands/scalafix/Scalafix.scala Outdated Show resolved Hide resolved

Gedochao reviewed Jun 25, 2024

View reviewed changes

modules/integration/src/test/scala/scala/cli/integration/ScalafixTests.scala Outdated Show resolved Hide resolved

bishabosha mentioned this pull request Jun 25, 2024

scalafix command to run library migration & hygiene rules #647

Open

Gedochao linked an issue Jun 25, 2024 that may be closed by this pull request

scalafix command to run library migration & hygiene rules #647

Open

bjaglin reviewed Jun 26, 2024

View reviewed changes

Gedochao reviewed Aug 8, 2024

View reviewed changes

Vigorge added 6 commits August 12, 2024 10:07

Scalafix command from scala-cli with basic optionns and tests

de3068b

Refactor documentation and minor inconveniences

3309101

Make scalac options and scala version configurable

acde158

Remove fixed scalac options

f70d9be

Correct ScalafixOptions tags to experimental

bf73c8c

Add more tests and fix external rule dependencies forwarding to scalafix

9da5f2a

Vigorge force-pushed the scalafix-command branch from 2848d16 to 9da5f2a Compare August 12, 2024 12:23

Vigorge marked this pull request as ready for review August 12, 2024 12:27

dos65 reviewed Aug 12, 2024

View reviewed changes

modules/integration/src/test/scala/scala/cli/integration/ScalafixTests.scala Outdated Show resolved Hide resolved

Vigorge added 2 commits August 12, 2024 16:33

Differentiate tests for 2.12, 2.13 and 3+ scala versions

61b18f7

Fix undeleted printlns

f1bc403

Vigorge requested review from Gedochao and bjaglin August 12, 2024 13:42

Fmt and fix

3e08eb2

bjaglin reviewed Aug 13, 2024

View reviewed changes

Native image support

9fa372d

dos65 and others added 2 commits October 16, 2024 17:16

scalafix: fix native image support

669af1e

Merge pull request #1 from dos65/scalafix-command-native

52f6591

Scalafix command native

dos65 force-pushed the scalafix-command branch from a409057 to 52f6591 Compare October 16, 2024 14:16

scalafix: run fix + fix tests

bb38e9c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scalafix command for scala-cli with basic optionns and tests #2968

Scalafix command for scala-cli with basic optionns and tests #2968

Vigorge commented Jun 17, 2024

Gedochao commented Jun 18, 2024

Gedochao commented Jun 25, 2024 •

edited

Loading

bjaglin left a comment

tgodzik commented Jun 26, 2024

Vigorge commented Jul 4, 2024

Gedochao commented Aug 8, 2024

SethTisue commented Aug 8, 2024

Vigorge commented Aug 12, 2024 •

edited

Loading

bjaglin commented Aug 12, 2024

bjaglin left a comment •

edited

Loading

bjaglin Aug 13, 2024

bjaglin Aug 13, 2024 •

edited

Loading

bjaglin Aug 13, 2024

bjaglin Aug 13, 2024

bjaglin Aug 13, 2024

bjaglin Aug 13, 2024

bjaglin Aug 13, 2024

bjaglin Aug 13, 2024

bjaglin Aug 13, 2024

Vigorge Aug 13, 2024

bjaglin Aug 13, 2024 •

edited

Loading

bjaglin Aug 13, 2024

Gedochao commented Sep 11, 2024

Vigorge commented Sep 17, 2024

dos65 commented Oct 16, 2024

dos65 commented Oct 18, 2024

	"A semantic rewrite was run on a source file that has no associated META-INF/semanticdb/.../*.semanticdb"
	"A semantic rule was run on a source file that has no associated META-INF/semanticdb/.../*.semanticdb"

	@HelpMessage("Fail the invocation instead of applying fixes")
	@HelpMessage("Fail the invocation if rewrites are needed")

	private val helpHeader = "Fixes Scala code according to scalafix rules."
	private val helpHeader = "Run Scalafix rules to lint or rewrite Scala code."

Scalafix command for scala-cli with basic optionns and tests #2968

Are you sure you want to change the base?

Scalafix command for scala-cli with basic optionns and tests #2968

Conversation

Vigorge commented Jun 17, 2024

Gedochao commented Jun 18, 2024

Gedochao commented Jun 25, 2024 • edited Loading

bjaglin left a comment

Choose a reason for hiding this comment

tgodzik commented Jun 26, 2024

Vigorge commented Jul 4, 2024

Gedochao commented Aug 8, 2024

SethTisue commented Aug 8, 2024

Vigorge commented Aug 12, 2024 • edited Loading

bjaglin commented Aug 12, 2024

bjaglin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjaglin Aug 13, 2024 • edited Loading

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

bjaglin Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gedochao commented Sep 11, 2024

Vigorge commented Sep 17, 2024

dos65 commented Oct 16, 2024

dos65 commented Oct 18, 2024

Gedochao commented Jun 25, 2024 •

edited

Loading

Vigorge commented Aug 12, 2024 •

edited

Loading

bjaglin left a comment •

edited

Loading

bjaglin Aug 13, 2024 •

edited

Loading

bjaglin Aug 13, 2024 •

edited

Loading