Allow configuring the dialect in Scalafix #1373

mlachkar · 2021-04-16T10:53:02Z

Scala3 dialect is a superset of Scala213 dialect:

implicit val Scala3 = Scala213
    .withAllowAndTypes(true)
    // there 3 different ways to specify vargs, some will be removed in future Scala 3 versions
    .withAllowAtForExtractorVarargs(true) // both @ and : work currently for Scala 3
    .withAllowColonForExtractorVarargs(true) // both @ and : work currently for Scala 3
    .withAllowPostfixStarVarargSplices(true)
    .withAllowEnums(true)
    .withAllowImplicitByNameParameters(true)
    .withAllowInlineMods(true)
    .withAllowLiteralTypes(true)
    .withAllowMethodTypes(false)
    .withAllowOrTypes(true) // Scala 3: `val a: A | B`
    .withAllowTrailingCommas(true)
    .withAllowTraitParameters(true)
    .withAllowTypeLambdas(true)
    .withAllowViewBounds(false) // View bounds have been removed in Dotty
    .withAllowXmlLiterals(false) // Scala 3: parser doesn't support xml
    .withAllowGivenUsing(true)
    .withAllowExtensionMethods(true)
    .withAllowOpenClass(true)
    .withAllowSpliceAndQuote(true)
    .withAllowToplevelStatements(true)
    .withAllowOpaqueTypes(true)
    .withAllowExportClause(true)
    .withAllowCommaSeparatedExtend(true)
    .withAllowEndMarker(true)
    .withAllowInterpolationDolarQuoteEscape(true)
    .withAllowSignificantIndentation(true)
    .withAllowQuestionMarkPlaceholder(true)
    .withAllowTypeParamUnderscore(false)
    .withAllowByNameRepeatedParameters(true)
    .withAllowLazyValAbstractValues(true)
    .withAllowUpperCasePatternVarBinding(true)
    .withAllowDerives(true)
    .withAllowTypeInBlock(true)
    .withAllowPolymorphicFunctions(true)
    .withAllowMatchAsOperator(true)
    .withAllowTypeMatch(true)
    .withAllowInfixMods(true)
    .withAllowSymbolLiterals(false)
    .withAllowDependentFunctionTypes(true)
    .withAllowAllTypedPatterns(true)
    .withAllowAsForImportRename(true)
    .withAllowStarWildcardImport(true)
    ```

    Maybe we don't need a new parameter to choose which dialect.

mlachkar · 2021-04-16T10:58:16Z

scalafix-core/src/main/scala/scala/meta/internal/scalafix/ScalafixScalametaHacks.scala

@@ -8,8 +8,8 @@ import scala.meta.internal.trees.Origin

 object ScalafixScalametaHacks {
  def dialect(language: String): Dialect =
-    if (language == "Scala") dialects.Scala212
-    else if (language.isEmpty) dialects.Scala212
+    if (language == "Scala") dialects.Scala3


this method is only used to add an extension method never used in our code base! Maybe we can remove this code and the extension method ?

Sounds right, it's under internal anyway

mlachkar · 2021-04-16T11:29:39Z

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

@@ -13,16 +13,10 @@ case class ParserConfig(
    inlineKeyword: Boolean = false
 ) {

-  private val dialect = scala.meta.dialects.Scala213.copy(


I don't see the point of writing our own definition of dialect. this one is not correct.
implicit val Scala213 = Scala212
.withAllowImplicitByNameParameters(true)
.withAllowLiteralTypes(true)
.withAllowNumericLiteralUnderscoreSeparators(true)
.withAllowTryWithAnyExpr(true)

I am not sure either - does it make sense to keep ParserConfig at all?

IIRC, the motivation for the custom dialect was to allow as much as possible of Scala 3 syntax without introducing regressions for Scala 2 users. Some Scala 3 syntax can be enabled safely while other syntax like significant indent may change the parsed structure of Scala 2 programs.

Ah, thanks! I understand better! Clever idea.

mlachkar · 2021-04-16T11:29:54Z

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

@@ -13,16 +13,10 @@ case class ParserConfig(
    inlineKeyword: Boolean = false
 ) {

-  private val dialect = scala.meta.dialects.Scala213.copy(
-    allowLiteralTypes = literalTypes,


already true in 2.12

mlachkar · 2021-04-16T11:30:39Z

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

@@ -13,16 +13,10 @@ case class ParserConfig(
    inlineKeyword: Boolean = false
 ) {

-  private val dialect = scala.meta.dialects.Scala213.copy(
-    allowLiteralTypes = literalTypes,
-    allowTrailingCommas = trailingCommas,


already true in 2.12

mlachkar · 2021-04-16T11:33:01Z

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

-  private val dialect = scala.meta.dialects.Scala213.copy(
-    allowLiteralTypes = literalTypes,
-    allowTrailingCommas = trailingCommas,
-    allowInlineMods = inlineKeyword,


we set allowInlineMods to false, and it's true in scala3

mlachkar · 2021-04-16T11:33:14Z

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

-    allowLiteralTypes = literalTypes,
-    allowTrailingCommas = trailingCommas,
-    allowInlineMods = inlineKeyword,
-    allowInlineIdents = !inlineKeyword


allowInlineIdent is true since 2.10

tgodzik · 2021-04-16T15:00:48Z

Hmm... this is true in most cases and I was also thinking about maybe doing this, but it might not be best to use it by default. Especially later on after some syntax will be deprecated then we will for sure need a new dialect. And some things are already unsupported for example:

  .withAllowViewBounds(false) // View bounds have been removed in Dotty

I would rather add another option to .scalafix.conf.

bjaglin

Scala3 dialect is a superset of Scala213 dialect:

I guess this is a wrong assumption, so scalafix needs to know whether it's dealing with 2.x or 3 sources.

I would rather add another option to .scalafix.conf.

I initially thought we should follow the scalafmt way (optional runner.dialect key), but scalafix is a bit different, because for semantic rules, the scala version is already mandatory, as a CLI/invocation argument.

So instead of adding a configuration key, we could make that argument mandatory also for syntactic rules. For build tool clients, it's an acceptable breaking change, but for CLI users like https://eed3si9n.com/syntactic-scalafix-rule-for-unified-slash-syntax (maybe a bad example as in this case we look at the extension to infer the dialect), it's quite annoying.

What about keeping the scala version optional for syntactic rules, but when missing try to pars with scala2.x dialect and fall back to scala3 on error (once per invocation no matter how many files we have)? Is there a case where both parsers would succeed but produce different trees?

bjaglin · 2021-04-18T22:04:31Z

scalafix-core/src/main/scala/scala/meta/internal/scalafix/ScalafixScalametaHacks.scala

@@ -8,8 +8,8 @@ import scala.meta.internal.trees.Origin

 object ScalafixScalametaHacks {
  def dialect(language: String): Dialect =
-    if (language == "Scala") dialects.Scala212
-    else if (language.isEmpty) dialects.Scala212
+    if (language == "Scala") dialects.Scala3


Sounds right, it's under internal anyway

bjaglin · 2021-04-18T22:17:54Z

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

@@ -13,16 +13,10 @@ case class ParserConfig(
    inlineKeyword: Boolean = false
 ) {

-  private val dialect = scala.meta.dialects.Scala213.copy(


I am not sure either - does it make sense to keep ParserConfig at all?

bjaglin · 2021-04-18T22:18:30Z

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

@@ -38,14 +38,12 @@ trait ScalafixMetaconfigReaders {
    import ScalafixConfig.{DefaultDialect => Default}
    import scala.meta.dialects._
    ReaderUtil.oneOf[Dialect](


where is this ConfDecoder used?

tgodzik · 2021-04-19T15:09:10Z

What about keeping the scala version optional for syntactic rules, but when missing try to pars with scala2.x dialect and fall back to scala3 on error (once per invocation no matter how many files we have)? Is there a case where both parsers would succeed but produce different trees?

There are a couple, especially related to optional braces, but mostly if a code was weirdly formatted to start with.

object A{
  def b =
   12
   23
}

I think in case of Scala 3, 12 and 23 will belong to b, while for Scala 2 only 12 will.

tgodzik · 2021-04-19T15:11:03Z

If possible I would try to declare the Scala version explicitly, but if it turns out to be more problematic for different clients, we could try with the fallback, but maybe issue a warning?

bjaglin · 2021-04-21T20:18:30Z

Any thought on this correctness vs simplicity dilemma @olafurpg?

olafurpg · 2021-04-21T21:12:14Z

I would lean towards requiring users to explicitly declare that the Scala 3 dialect should be used. As Tomasz pointed out, there are subtle risks from enabling full Scala 3 syntax by default. I would avoid heuristics and attempting an automatic fallback. One thing we could do is to customize syntax error messages to recommend enabling the Scala 3 dialect to fix the problem.

…iles

mlachkar · 2021-04-26T13:49:05Z

in scalafix.conf, we can already provide a Dialect. In this commit, I just make sure that it's used to parse files.
This commit dosn't add the possibilit to configure the dialect through the CLI.

bjaglin

Getting there 👍

in scalafix.conf, we can already provide a Dialect. In this commit, I just make sure that it's used to parse files.

Ah great if that's already available. Could you add a unit test to demonstrate this?

This commit dosn't add the possibilit to configure the dialect through the CLI.

For semantic rules, the scala version is mandatory in the CLI args - it should have priority since it means class files exist with that version, so if it does not match with the dialect in the configuration, should we proceed with that one but issue a warning?

bjaglin · 2021-04-26T16:08:38Z

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

-    allowXmlLiterals = true
-  )
-
+  val DefaultDialect = scala.meta.dialects.Scala213


Following up on the previous discussion above: this is a breaking change since currently some scala3 syntax can be parsed without tweaking the default. That's what we want but we should document that.

I can keep the same if needed. I just need to check which one was actually used.

so this default is based on 212 and it was only used in old rules (none of the ones we have in scalafix).
the real dialect that was used is this one, where all the additional configuration is already part of scala213.

private val dialect = scala.meta.dialects.Scala213.copy( allowLiteralTypes = true, allowTrailingCommas = true, allowInlineMods = flase, allowInlineIdents = true )

I don't know if it's useful to allow scala 3 syntax in scala213, but we could allow some of it.

bjaglin · 2021-04-26T16:11:46Z

build.sbt

@@ -63,6 +62,7 @@ lazy val core = project
  .settings(
    moduleName := "scalafix-core",
    buildInfoSettingsForCore,
+    scalacOptions ++= syntheticsOn.value,


instead of adding it to every scala2 project, can't you keep it in ThisBuild and remove it for the test-input? that would reduce the noise

project/ScalafixBuild.scala

bjaglin · 2021-04-26T16:14:57Z

scalafix-core/src/main/scala/scalafix/config/CustomMessage.scala

+    decoder[Symbol.Global](field = "symbol")(
+      ScalafixMetaconfigReaders.symbolGlobalReader
+    )


is that a no-op?

the goal was just to know which implicit is used from ScalafixMetaconfigReaders

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

bjaglin · 2021-04-26T16:16:23Z

scalafix-tests/output/src/main/scala-3/test/HelloWorld.scala

+object HelloWorld:
+
+  @main def hello = println("Hello, world!")
+// This comment is added by Scalafix


… scala 3

mlachkar · 2021-04-27T11:17:41Z

Still missing: the CLI.
We can deduce the dialect using the scalaVersion if present, which would override the dialect configured in .scalafix.conf.
or we can add the argument dialect in cli, and take it into account.

I don't know what's the best solution.

mlachkar · 2021-04-29T09:53:27Z

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

+    @Description(
+      "default Scala213" +
+        "Possibilities: Scala3, Scala213"
+    )


Need to be improved

bjaglin

A few minor comments, great work!

bjaglin · 2021-04-29T11:20:01Z

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

-      Paradise211,
-      Paradise212
+      Scala213,
+      Scala3


what happened to Sbt1 ? I guess it could be useful if someone has a sbt-type file with a different extension than .sbt (very far-fetched)

even the old code don't really take sbt dialect into account.
sbt dialect is always 213 (which is the default) with allowTopLevelItems.
and now sbtDialct is
val DefaultSbtDialect: Dialect = scala.meta.dialects.Sbt1 [Now]

private val sbtDialect = dialect.copy(allowToplevelTerms = true) [Previously]

// This the code which choose which dialect following the extension of the file

def dialectForFile(path: String): Dialect = if (path.endsWith(".sbt")) sbtDialect else dialect

So users are supposed to choose either scala213 or scala3 in dialect. Do you think we should restrict to those two values?

implicit lazy val dialectReader: ConfDecoder[Dialect] = { import ScalafixConfig.{DefaultDialect => Default} import scala.meta.dialects._ ReaderUtil.oneOf[Dialect]( Scala213, Scala3, //Scala211 //Scala212 ) }

By the way, I thought sbt1 is just sbt with a higher version 1.x.x
Isn't that the case?

Do you think we should restrict to those two values?

If we do, then it would be Scala2 and Scala3, no?

By the way, I thought sbt1 is just sbt with a higher version 1.x.x
Isn't that the case?

It is, the format and binary backward compatibility is frozen in 1.x

even the old code don't really take sbt dialect into account.
sbt dialect is always 213 (which is the default) with allowTopLevelItems.

Interesting! I think we should rather use the dialect defined in scalameta than defning our own variant here, no?
https://github.com/scalameta/scalameta/blob/6c0a370cc278d473588f973c436d8378a691d2b1/scalameta/dialects/shared/src/main/scala/scala/meta/dialects/package.scala#L70-L72

yes! It's what I did. I Like the idea of having only scala2 and Scala3 dialect !!

bjaglin · 2021-04-29T11:20:32Z

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

@@ -27,19 +29,15 @@ final class SyntacticDocument private[scalafix] (
 }

 object SyntacticDocument {
+  @deprecated("use fromInput(input: Input, dialect: Dialect) instead", "0.9.27")


Suggested change

@deprecated("use fromInput(input: Input, dialect: Dialect) instead", "0.9.27")

@deprecated("use fromInput(input: Input, dialect: Dialect) instead", "0.9.28")

bjaglin · 2021-04-29T11:28:24Z

build.sbt


 inThisBuild(
  List(
    onLoadMessage := s"Welcome to scalafix ${version.value}",
    scalaVersion := scala213,
    crossScalaVersions := List(scala213, scala212, scala211),
    fork := true,
+    scalacOptions ++= syntheticsOn.value,


I guess you can now revert this change and get rid of syntheticsOn since the scala3 customization is only applied where scala3 cross-building is enabled.

bjaglin · 2021-04-29T11:40:15Z

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

@@ -121,6 +123,11 @@ case class Args(
      "The Scala compiler version that was used to compile this project."
    )
    scalaVersion: String = scala.util.Properties.versionNumberString,
+    @Description(
+      "default Scala213" +


Suggested change

"default Scala213" +

"Defaults to Scala213 for `.scala` files and Sbt1 for `.sbt` files" +

bjaglin · 2021-04-29T11:41:00Z

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

@@ -121,6 +123,11 @@ case class Args(
      "The Scala compiler version that was used to compile this project."
    )
    scalaVersion: String = scala.util.Properties.versionNumberString,
+    @Description(
+      "default Scala213" +
+        "Possibilities: Scala3, Scala213"


doesn't metaconfig generate this out of the box?

Ah, I don't know

bjaglin · 2021-04-29T11:47:02Z

project/ScalafixBuild.scala

@@ -108,6 +115,12 @@ object ScalafixBuild extends AutoPlugin with GhpagesKeys {
      "unit/test:runMain scalafix.tests.util.SaveExpect" ::
        s
    },
+    // just to launch first test in scala 3


this is unlikely to be updated when we have more than one test

Suggested change

// just to launch first test in scala 3

mlachkar commented Apr 16, 2021

View reviewed changes

mlachkar requested a review from bjaglin April 16, 2021 11:44

bjaglin reviewed Apr 18, 2021

View reviewed changes

Clean ScalafixMetaconfigReaders and add scala3 to DialectReader

4f93c8e

mlachkar force-pushed the dialect branch from 1cca6c7 to 3cd98e9 Compare April 26, 2021 13:38

Make sure that we use the dialect configured in .scalaconf to parse f…

0b530b8

…iles

mlachkar force-pushed the dialect branch from 3cd98e9 to 0b530b8 Compare April 26, 2021 13:46

mlachkar requested a review from bjaglin April 26, 2021 13:50

mlachkar force-pushed the dialect branch 2 times, most recently from 8dbaacc to c275892 Compare April 26, 2021 15:33

bjaglin reviewed Apr 26, 2021

View reviewed changes

mlachkar force-pushed the dialect branch 2 times, most recently from dd3add7 to b505cd6 Compare April 27, 2021 08:19

Add one HelloWorld test to make sure the parsing is working well with…

0fa1b89

… scala 3

mlachkar force-pushed the dialect branch from b505cd6 to 0fa1b89 Compare April 27, 2021 09:41

mlachkar requested a review from bjaglin April 27, 2021 12:57

mlachkar force-pushed the dialect branch from 1795a63 to 29f65f0 Compare April 28, 2021 11:27

Fix mima

cf30a3f

mlachkar force-pushed the dialect branch from 29f65f0 to cf30a3f Compare April 28, 2021 12:02

revert the change that make the implicits readers explicits

f1d4472

mlachkar commented Apr 29, 2021

View reviewed changes

bjaglin approved these changes Apr 29, 2021

View reviewed changes

mlachkar changed the title ~~Make scala3 dialect the default one~~ Allow configuring the dialect in Scalafix Apr 29, 2021

Add dialect to the list of Args for Cli usage

b56ec7f

mlachkar force-pushed the dialect branch from 3cb7a37 to b56ec7f Compare April 29, 2021 16:08

mlachkar merged commit bf0df22 into scalacenter:main Apr 29, 2021

bjaglin mentioned this pull request May 11, 2021

remove dialect and use scalaVersion #1392

Merged

	@deprecated("use fromInput(input: Input, dialect: Dialect) instead", "0.9.27")
	@deprecated("use fromInput(input: Input, dialect: Dialect) instead", "0.9.28")

	"default Scala213" +
	"Defaults to Scala213 for `.scala` files and Sbt1 for `.sbt` files" +

Allow configuring the dialect in Scalafix #1373

Allow configuring the dialect in Scalafix #1373

Conversation

mlachkar commented Apr 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Apr 16, 2021

bjaglin left a comment • 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

tgodzik commented Apr 19, 2021

tgodzik commented Apr 19, 2021

bjaglin commented Apr 21, 2021

olafurpg commented Apr 21, 2021

mlachkar commented Apr 26, 2021 • edited 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

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 commented Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

bjaglin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlachkar Apr 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlachkar commented Apr 16, 2021 •

edited

Loading

bjaglin left a comment •

edited

Loading

mlachkar commented Apr 26, 2021 •

edited

Loading

mlachkar commented Apr 27, 2021 •

edited

Loading

mlachkar Apr 29, 2021 •

edited

Loading