remove dialect and use scalaVersion #1392

mlachkar · 2021-05-07T14:09:16Z

Follows #1373

bjaglin

LGTM on using scalaVersion to control the internal dialect passed to scalameta for parsing. A few docs & implementation details comments.

As discussed offline, for the record:

this flag is not mandatory at the moment - it's only required for rules using the presentation compiler (ExplicitResultTypes amongst the built-in rules) to ensure compatibility at the class/pickle level
it would be redundant and confusing for the user to ask for another flag just for the dialect
sbt-scalafix only passes it for semantic rules at the moment, but it's trivial to update it
the current default is the runtime Scala version of Scalafix, so effectively 2.12 or 2.13, which does not change the default behavior for parsing sources
👍 for supporting a major version only (single int), to cover the use case of dialect selection via the CLI (or potentially in the configuration file itself) with as little provided information as possible (--scala-version 3).

bjaglin · 2021-05-10T06:13:19Z

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

-      assert(out.contains("Scala2"))
-    }
-  )
+//  checkSemantic(


scalafix-tests/unit/src/test/scala/scalafix/tests/config/ScalaVersionSuite.scala

bjaglin · 2021-05-10T06:17:19Z

scalafix-core/src/main/scala/scalafix/internal/config/ScalaVersion.scala

+          Success(Minor(major, minor.toInt))
+        }
+      case MajorPattern(major) => MajorVersion.from(major.toLong).flatMap(major => Success(Major(major)))
+      case _ => Failure(new Exception(s"$s not a valid ScalaVersion."))


Suggested change

case _ => Failure(new Exception(s"$s not a valid ScalaVersion."))

case _ => Failure(new Exception(s"$s not a valid Scala version"))

bjaglin · 2021-05-10T06:20:01Z

scalafix-core/src/main/scala/scalafix/internal/config/ScalaVersion.scala

+  def from(s: String): Try[ScalaVersion] = {
+    val version = s.split("-").head
+    version match {
+      case "Scala2" | "scala2" => Success(scala2) // not sure


not sure about supporting this syntax? Given the documentation of the flag, it seems a bit strange to me that the user would provide something else than x[.x[.x]]. Maybe we can strip the potential heading [S|s]cala before parsing?

I think we can just not support this as you said.

bjaglin · 2021-05-10T06:21:07Z

scalafix-core/src/main/scala/scalafix/internal/config/ScalaVersion.scala

+       }
+   }
+
+  private val longPattern = """\d{1,19}"""


I am guessing we can stick to 2? also you capture ints here, not longs

you're right

bjaglin · 2021-05-10T06:25:27Z

scalafix-core/src/main/scala/scalafix/internal/config/ScalaVersion.scala

+   }
+
+  private val longPattern = """\d{1,19}"""
+  private val BasicVersion = raw"""($longPattern)\.($longPattern)\.($longPattern)""".r


Suggested change

private val BasicVersion = raw"""($longPattern)\.($longPattern)\.($longPattern)""".r

private val FullVersion = raw"""($longPattern)\.($longPattern)\.($longPattern)""".r

bjaglin · 2021-05-10T06:30:26Z

scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

@@ -129,7 +127,7 @@ case class Args(
    @Description(
      "The Scala compiler version that was used to compile this project."


Since we are extending the use of this flag for syntactic rules, I think we should relax a bit the constraint of "prior compilation". Also, the reference to project is a bit ambiguous, considering the other flags, I think files would be more consistent.

Suggested change

"The Scala compiler version that was used to compile this project."

"The Scala compiler version that the provided files are targeting, or the actual version that was used to compile them when a classpath is provided."

bjaglin · 2021-05-10T06:40:01Z

scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

@@ -371,7 +368,7 @@ case class Args(
      settingInScala2: String,
      settingInScala3Opt: Option[String]
  ): Option[String] = {
-    if (scalaVersion.isEmpty || scalaVersion.startsWith("2")) {
+    if (scalaVersion.value.isEmpty || scalaVersion.value.startsWith("2")) {


add helper on ScalaVersion? Or Ordering?

yes definitely. It was just to make everything compiles

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

bjaglin · 2021-05-10T11:58:08Z

scalafix-core/src/main/scala/scalafix/v1/SyntacticDocument.scala

+  @deprecated(
+    "use fromInput(input: Input, scalaVersion: ScalaVersion) instead",
+    "0.9.28"
+  )
+  def fromInput(inout: Input, dialect: Dialect): SyntacticDocument = {
+    val tree = parse(inout, dialect).get: Tree
+    fromTree(tree)
+  }


you can remove this since it was never published, no?

This method was there already.

bjaglin · 2021-05-10T11:59:03Z

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

@@ -169,6 +164,7 @@ ScalafixArguments withToolClasspath(
     *                this must match the binary version available in the classloader of
     *                this instance, as requested/provided in the static factory methods
     *                of {@link Scalafix}.
+     * @throws ScalafixException In case of an error parsing the scalaVersion.


the javadoc could be updated in the same way the CLI doc was updated

the cli sentence is finally not that clear for me!

@Description( "The Scala compiler version that the provided files are targeting, " + "or the actual version that was used to compile them when a classpath is provided." )

what we want is

the full scalaverion that was used to compile the code source

in the absence of the fullVersion, we require either the scala binary version of the code source, or the major scala Version

we need at least the major, to know with which dialect to parse the code source, but some rules may require more..

Something like that?

The major or binary Scala version that the provided files are targeting, or the full version that was used to compile them when a classpath is provided.

bjaglin · 2021-05-11T18:12:52Z

FTR, I just ran into scalameta/scalameta#2323, so I think whatever the outcome of that discussion is, it's good that we hide the dialect and infer it based on scalaVersion (and potentially scalacOptions).

mlachkar · 2021-05-11T23:39:42Z

I agree with you!

bjaglin approved these changes May 10, 2021

View reviewed changes

mlachkar force-pushed the scalaVersion branch 3 times, most recently from f765918 to f0be46b Compare May 10, 2021 10:55

mlachkar marked this pull request as ready for review May 10, 2021 10:56

mlachkar requested a review from bjaglin May 10, 2021 11:43

bjaglin approved these changes May 10, 2021

View reviewed changes

remove dialect and use scalaVersion

4199f64

mlachkar force-pushed the scalaVersion branch from dbf4ddf to 4199f64 Compare May 10, 2021 13:09

mlachkar merged commit 6c9eeec into scalacenter:main May 10, 2021

mlachkar deleted the scalaVersion branch May 10, 2021 17:35

bjaglin mentioned this pull request May 10, 2021

initial support for running rules on Scala 3 sources scalacenter/sbt-scalafix#210

Merged

bjaglin mentioned this pull request May 11, 2021

Scala 3 support #1316

Closed

5 tasks

mlachkar mentioned this pull request Dec 17, 2021

Scala 3 support cosmicsilence/gradle-scalafix#60

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove dialect and use scalaVersion #1392

remove dialect and use scalaVersion #1392

mlachkar commented May 7, 2021 •

edited by bjaglin

Loading

bjaglin left a comment

bjaglin May 10, 2021

bjaglin May 10, 2021

bjaglin May 10, 2021

mlachkar May 10, 2021 •

edited

Loading

bjaglin May 10, 2021

mlachkar May 10, 2021

bjaglin May 10, 2021

bjaglin May 10, 2021

bjaglin May 10, 2021

mlachkar May 10, 2021

bjaglin May 10, 2021

mlachkar May 10, 2021 •

edited

Loading

bjaglin May 10, 2021

mlachkar May 10, 2021

mlachkar May 10, 2021

mlachkar May 10, 2021

bjaglin May 10, 2021 •

edited

Loading

mlachkar May 10, 2021

bjaglin commented May 11, 2021

mlachkar commented May 11, 2021

	case _ => Failure(new Exception(s"$s not a valid ScalaVersion."))
	case _ => Failure(new Exception(s"$s not a valid Scala version"))

	private val BasicVersion = raw"""($longPattern)\.($longPattern)\.($longPattern)""".r
	private val FullVersion = raw"""($longPattern)\.($longPattern)\.($longPattern)""".r

		@@ -129,7 +127,7 @@ case class Args(
		@Description(
		"The Scala compiler version that was used to compile this project."

	"The Scala compiler version that was used to compile this project."
	"The Scala compiler version that the provided files are targeting, or the actual version that was used to compile them when a classpath is provided."

remove dialect and use scalaVersion #1392

remove dialect and use scalaVersion #1392

Conversation

mlachkar commented May 7, 2021 • edited by bjaglin Loading

bjaglin 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

mlachkar May 10, 2021 • 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

mlachkar May 10, 2021 • 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

bjaglin May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjaglin commented May 11, 2021

mlachkar commented May 11, 2021

mlachkar commented May 7, 2021 •

edited by bjaglin

Loading

mlachkar May 10, 2021 •

edited

Loading

mlachkar May 10, 2021 •

edited

Loading

bjaglin May 10, 2021 •

edited

Loading