Add scalafix-interfaces with Java APIs for reflective invocation #783

olafurpg · 2018-08-07T15:29:06Z

This commit adds a new 8kb module scalafix-interfaces with Java-only
interfaces exposing a public API to invoke Scalafix via reflection.
This API is intended for build tools such as sbt, gradle or IDEs like
IntelliJ.

This API is inspired by how zinc Java APIs and Dotty Java interfaces
are designed. A document describing alternative approaches can be
seen here
https://docs.google.com/document/d/1Y1MVMjVQ8P25YEI3uvh86gg3n61WZng0hr1qRUS_S6I/edit#heading=h.1fe4wp3hxrrj

This PR is large for two reasons

Several large files were split into multiple files with almost no code changes
This commit makes several improvements to the linter and reporting pipeline.

The old reporting pipeline had several problems:

it was difficult to get the rule name and category id separately
because they were merged into a single string.
based on my personal observation, the old LintCategory and LintMessage
were not intuitive and several contributed linter rules used them
in weird ways. I hope the new LintMessage interface and LintID
case class makes it more intuitive what the role of category IDs and
explanations is. Rule authors are able to create custom classes
that extend LintMessage. This design is similar to how Dotty
error messages are designed and expensive values such as explanations
can be computed on-demand.
the internal ScalafixReporter trait contains a lot of complex methods with
questionable value. Now it contains a single method matching
the Java interface API.
the cli did not have an extensible way to hook into the reporting
of linter messages. Now it's possible to collect linter messages
programmatically the same way compiler diagnostics are reported
via zinc.

This commit introduces almost no breaking changes to the public API.
Existing rules and tests in this repo are unchanged. Users that
previously called LintMessage.copy (should be rare) must migrate to
use LintMessage.EagerLintMessage.copy instead.

olafurpg · 2018-08-07T15:32:36Z

scalafix-tests/unit/src/test/scala-2.12/scalafix/tests/cli/ScalafixImplSuite.scala

+  }
+
+  test("runMain") {
+    // This is a full integration test that stresses the full breadth of the scalafix-interfaces API


Here is a full integration test for the new reflective API. Gives an idea how it will be used.

olafurpg · 2018-08-07T15:33:03Z

scalafix-interfaces/src/main/java/scalafix/interfaces/Scalafix.java

+     * @throws ScalafixException in case of errors during classloading, most likely caused
+     * by an incorrect classloader argument.
+     */
+    static Scalafix classloadInstance(ClassLoader classLoader) throws ScalafixException {


This is the entrypoint for reflectively loading a Scalafix instance.

olafurpg · 2018-08-07T15:33:37Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixMainArgs.java

+ * @implNote This interface is not intended for extension, the only implementation of this interface
+ * should live in the Scalafix repository.
+ */
+public interface ScalafixMainArgs {


Builder interface to construct arguments for the scalafix cli.

olafurpg · 2018-08-07T15:35:17Z

scalafix-core/src/main/scala/scalafix/lint/LintMessage.scala

-      else ""
-
-    s"[${category.id}] $message$explanation"
+trait LintMessage {


LintMessage became a trait so that it's possible (but not necessary) to implement custom message types

class UnusedCode(val position: Position) extends LintMessage { ... }

Question whether basic values should have default implementations 🤔 For example categoryID and explanation could be empty string and severity could default to error. I'll go ahead and update that ASAP.

This commit adds a new 8kb module scalafix-interfaces with Java-only interfaces exposing a public API to invoke Scalafix via reflection. This API is intended for build tools such as sbt, gradle or IDEs like IntelliJ. This API is inspired by how zinc Java APIs and Dotty Java interfaces are designed. A document describing alternative approaches can be seen here https://docs.google.com/document/d/1Y1MVMjVQ8P25YEI3uvh86gg3n61WZng0hr1qRUS_S6I/edit#heading=h.1fe4wp3hxrrj This commit is large for two reasons 1. Several large files were split into multiple files with limited changes 2. This commit makes several improvements to the linter and reporting pipeline. The old reporting pipeline had several problems: - it was difficult to get the rule name and category id separately because they were merged into a single string. - based on my personal observation, the old `LintCategory` and `LintMessage` were not intuitive and several contributed linter rules used them in weird ways. I hope the new `LintMessage` interface and `LintID` case class makes it more intuitive what the role of category IDs and explanations is. Rule authors are able to create custom classes that extend `LintMessage`. This design is similar to how Dotty error messages are designed and expensive values such as explanations can be computed on-demand. - the ScalafixReporter trait contains a lot of complex methods with questionable value. Now it contains a single method matching the Java interface API. - the cli did not have an extensible way to hook into the reporting of linter messages. Now it's possible to collect linter messages programmatically the same way compiler diagnostics are reported via zinc. This commit introduces *almost* no breaking changes to the public API. All existing rules and tests in this repo are unchanged. Users that previously called `LintMessage.copy` (should be rare) must migrate to use `LintMessage.EagerLintMessage.copy` instead.

MasseGuillaume · 2018-08-07T17:50:58Z

scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixDiagnosticImpl.scala

@@ -0,0 +1,66 @@
+package scalafix.internal.interfaces
+import java.util.Optional


newline between 1 and 2 plz.

Re-opened scalameta/scalafmt#1069 This started happening more frequently now because intellij-scala uses Scalafmt after refactorings.

MasseGuillaume · 2018-08-07T18:03:13Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixLintID.java

+    /**
+     * @return The name of the rule that produced this diagnostic.
+     */
+    String ruleName();


For example: RemoveUnusedImport

MasseGuillaume · 2018-08-07T18:03:28Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixLintID.java

+     * Empty if the rule only reports diagnostics of a single
+     * category.
+     */
+    String categoryID();


I guess in Disable.get, get is the categoryID.

MasseGuillaume · 2018-08-07T18:06:06Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixMainArgs.java

+     * @param paths Files and directories to run Scalafix on. Directories are recursively expanded
+     *              for files matching the patterns <code>*.scala</code> and <code>*.sbt</code>.
+     */
+    ScalafixMainArgs withPaths(List<Path> paths);


Hum, how is it expanded? How do you exclude directories?

I added withExcludedPaths and updated the docstring to mention it's recommended to pass in only files with this programmatic API. Directory expansion is only supported to make cli invocations ergonomic.

MasseGuillaume · 2018-08-07T18:11:32Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixMainCallback.java

+     *
+     * @param diagnostic the reported diagnostic.
+     */
+    void reportDiagnostic(ScalafixDiagnostic diagnostic);


This is a bit limited compared to what scalafix can do.

It would be useful to have a list of files that were modified. Ideally, it would be useful to have interactive rewrites where the user can make a choice. For example for collection.Seq in the new collection design.

I agree, I want to expand this interface with handlers for fixing files but I decided to leave it for the future since currently I only need to hook into linter messages from sbt-scalafix. I opened #784 to continue the discussion

MasseGuillaume · 2018-08-07T18:32:10Z

scalafix-tests/unit/src/test/scala/scalafix/tests/core/PositionSuite.scala

+
+  def check(name: String, original: String, expected: String): Unit = {
+    test(name) {
+      val marker = "@@".r


Can we reuse this: https://github.com/scalameta/scalameta/blob/master/tests/shared/src/test/scala/scala/meta/tests/tokenizers/BaseTokenizerCoverageSuite.scala ?

It looks really neat:
https://github.com/scalameta/scalameta/blob/master/tests/shared/src/test/scala/scala/meta/tests/tokenizers/TokenizerCoverageSuite.scala

I personally dislike unicode characters since they're inconvenient to write. However, I refactored the test to use

val startMarker = '→' val stopMarker = '←'

for consistency with the Scalameta tests. The extraction implementation stayed unchanged.

MasseGuillaume · 2018-08-07T18:33:41Z

scalafix-tests/unit/src/test/scala/scalafix/tests/cli/CliSemanticSuite.scala

@@ -44,15 +44,22 @@ class CliSemanticSuite extends BaseCliSuite {
    }
  )

-  checkSemantic(
-    name = "MissingSemanticDB",
-    args = Array(), // no --classpath


can we keep this comment?

MasseGuillaume · 2018-08-07T18:36:52Z

scalafix-tests/unit/src/test/scala-2.12/scalafix/tests/cli/ScalafixImplSuite.scala

+    Files.createDirectories(d)
+    Files.createDirectories(src)
+    val semicolon = src.resolve("Semicolon.scala")
+    // This rule is published to Maven Central to simplify testing --tool-classpath.


I would prefer to see this in the build.

I would: add example-scalafix-rule project and make the test task depends on publishLocal of example-scalafix-rule

I agree it would be good to bake this into the build but I opted for this because it's closer to the environment end-users will use this functionality (I've been bitten by different in custom build environments and end-user environments).

I want this integration test to be as self-contained as possible so that readers don't have to read build.sbt to understand what's going on. I can imagine this test case will be a good reference point for people who want to write unit tests when integrating with Scalafix.

MasseGuillaume · 2018-08-07T18:38:47Z

scalafix-tests/unit/src/test/scala-2.12/scalafix/tests/cli/ScalafixImplSuite.scala

+import scalafix.testkit.DiffAssertions
+
+class ScalafixImplSuite extends FunSuite with DiffAssertions {
+  def semanticdbPluginPath(): String = {


Can we get this from BuildInfo?

BuildInfo doesn't robustly support task generators, we could make it a resource generator but that would make the test harder to read IMO. I inlined it here since that makes the test more self-contained (instead of magically pulling in info from the build) and avoids the need to regenerate this information on every compile/run instead of only when we run ScalafixImplSuite.

olafurpg

Thank you for the review @MasseGuillaume !

olafurpg · 2018-08-08T08:33:59Z

scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixDiagnosticImpl.scala

@@ -0,0 +1,66 @@
+package scalafix.internal.interfaces
+import java.util.Optional


Re-opened scalameta/scalafmt#1069 This started happening more frequently now because intellij-scala uses Scalafmt after refactorings.

olafurpg · 2018-08-08T09:54:00Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixMainArgs.java

+     * @param paths Files and directories to run Scalafix on. Directories are recursively expanded
+     *              for files matching the patterns <code>*.scala</code> and <code>*.sbt</code>.
+     */
+    ScalafixMainArgs withPaths(List<Path> paths);


I added withExcludedPaths and updated the docstring to mention it's recommended to pass in only files with this programmatic API. Directory expansion is only supported to make cli invocations ergonomic.

olafurpg · 2018-08-08T09:57:41Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixMainCallback.java

+     *
+     * @param diagnostic the reported diagnostic.
+     */
+    void reportDiagnostic(ScalafixDiagnostic diagnostic);


I agree, I want to expand this interface with handlers for fixing files but I decided to leave it for the future since currently I only need to hook into linter messages from sbt-scalafix. I opened #784 to continue the discussion

olafurpg · 2018-08-08T09:58:48Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixLintID.java

+    /**
+     * @return The name of the rule that produced this diagnostic.
+     */
+    String ruleName();


olafurpg · 2018-08-08T09:58:53Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixLintID.java

+     * Empty if the rule only reports diagnostics of a single
+     * category.
+     */
+    String categoryID();


olafurpg · 2018-08-08T10:00:51Z

scalafix-tests/unit/src/test/scala-2.12/scalafix/tests/cli/ScalafixImplSuite.scala

+import scalafix.testkit.DiffAssertions
+
+class ScalafixImplSuite extends FunSuite with DiffAssertions {
+  def semanticdbPluginPath(): String = {


BuildInfo doesn't robustly support task generators, we could make it a resource generator but that would make the test harder to read IMO. I inlined it here since that makes the test more self-contained (instead of magically pulling in info from the build) and avoids the need to regenerate this information on every compile/run instead of only when we run ScalafixImplSuite.

olafurpg · 2018-08-08T10:33:52Z

scalafix-tests/unit/src/test/scala-2.12/scalafix/tests/cli/ScalafixImplSuite.scala

+    Files.createDirectories(d)
+    Files.createDirectories(src)
+    val semicolon = src.resolve("Semicolon.scala")
+    // This rule is published to Maven Central to simplify testing --tool-classpath.


I agree it would be good to bake this into the build but I opted for this because it's closer to the environment end-users will use this functionality (I've been bitten by different in custom build environments and end-user environments).

I want this integration test to be as self-contained as possible so that readers don't have to read build.sbt to understand what's going on. I can imagine this test case will be a good reference point for people who want to write unit tests when integrating with Scalafix.

olafurpg · 2018-08-08T10:44:06Z

scalafix-tests/unit/src/test/scala/scalafix/tests/cli/CliSemanticSuite.scala

@@ -44,15 +44,22 @@ class CliSemanticSuite extends BaseCliSuite {
    }
  )

-  checkSemantic(
-    name = "MissingSemanticDB",
-    args = Array(), // no --classpath


olafurpg · 2018-08-08T10:48:27Z

scalafix-tests/unit/src/test/scala/scalafix/tests/core/PositionSuite.scala

+
+  def check(name: String, original: String, expected: String): Unit = {
+    test(name) {
+      val marker = "@@".r


I personally dislike unicode characters since they're inconvenient to write. However, I refactored the test to use

val startMarker = '→' val stopMarker = '←'

for consistency with the Scalameta tests. The extraction implementation stayed unchanged.

olafurpg · 2018-08-08T10:54:43Z

@ShaneDelmore @xeno-by I am curious to hear your thoughts about the new public Java interface API. The internal changes for ScalafixReporter are less interesting and require less scrutiny, most of the work there was to synchronize the internal implementation with the public Java interface API.

olafurpg · 2018-08-09T05:42:06Z

Gonna merge to unblock a new milestone release, happy to address feedback in followup PRs.

This upgrade introduced a new Java API in a module scalafix-interfaces that exposes a public API for build tools and IDEs scalacenter/scalafix#783. This means sbt-scalafix no longer needs to invoke the fragile `main(args: Array[String]): Unit` interface. This also means it's possible to hook into the linter reporting pipeline and use the sbt logger for diagnostics.

olafurpg commented Aug 7, 2018

View reviewed changes

olafurpg force-pushed the interfaces branch 2 times, most recently from 0386f17 to 33a7b3e Compare August 7, 2018 16:06

olafurpg force-pushed the interfaces branch from 33a7b3e to 5713a8f Compare August 7, 2018 16:17

MasseGuillaume approved these changes Aug 7, 2018

View reviewed changes

Add withExcludedPaths combinator to ScalafixMainArgs.

a03d9cc

olafurpg mentioned this pull request Aug 8, 2018

Expose more handlers in ScalafixMainCallback #784

Closed

Address review feedback

db5f87d

olafurpg commented Aug 8, 2018

View reviewed changes

olafurpg requested review from xeno-by and ShaneDelmore August 8, 2018 10:51

olafurpg merged commit d1b7ff1 into scalacenter:master Aug 9, 2018

olafurpg deleted the interfaces branch August 9, 2018 05:42

olafurpg mentioned this pull request Aug 9, 2018

Upgrade to Scalafix v0.6.0-M14 scalacenter/sbt-scalafix#11

Merged

		@@ -0,0 +1,66 @@
		package scalafix.internal.interfaces
		import java.util.Optional

Add scalafix-interfaces with Java APIs for reflective invocation #783

Add scalafix-interfaces with Java APIs for reflective invocation #783

Conversation

olafurpg commented Aug 7, 2018 • 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

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

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

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

olafurpg commented Aug 8, 2018

olafurpg commented Aug 9, 2018

olafurpg commented Aug 7, 2018 •

edited

Loading