scalameta / metals

Scala language server with rich IDE features 🚀
https://scalameta.org/metals/
Apache License 2.0
2.08k stars 330 forks source link

Formatting with Scala 3 can break your build when triggered in a `project/*.scala` file #4143

Open ckipp01 opened 2 years ago

ckipp01 commented 2 years ago

I don't know if there's something obvious that I'm missing. I can NOT autoformat .sbt files, but sometimes there are sbt files with .scala extension, which makes things a bit complicated.

Ahh, this is a good catch. I can reproduce this with just triggering a format in a Scala 3 project when there is a Scala file in the project dir and I have a setting like:

rewrite.scala3.removeOptionalBraces = true

Which indeed will format the project/*.scala file in a way that will cause the re-import to fail. This is actually something we'll need to fix in Metals itself. I'll create an issue with this discussion over there.

Originally posted by @ckipp01 in https://github.com/scalameta/metals/discussions/4142#discussioncomment-3131759

ckipp01 commented 2 years ago

For context you can reproduce this with a minamal hello-world project in Scala 3. Create a project/Example.scala file with the following:

object Example {
  val hello = "there"
}

And have the following .scalafmt.conf file:

version = 3.5.6
runner.dialect = scala3
preset = default
rewrite.scala3.removeOptionalBraces = true

After an import trigger a format in project/Example.scala, notices it removes the braces, and now you broke you build it the request to re-import will fail.

dos65 commented 2 years ago

I don't think that it's a Metals issue. The same will happen if you run scalafmt directly. We can do a workaround only in Metals but I don't think that it will be a good solution. Honestly, I have no idea how it might be fixed in general.

keynmol commented 2 years ago

Theoretically Metals can fix this.

version = "3.4.0"
runner.dialect = scala3
rewrite.scala3.insertEndMarkerMinLines = 10
rewrite.scala3.removeOptionalBraces = true
rewrite.scala3.convertToNewSyntax = true

fileOverride {
  "glob:**.sbt" {
    runner.dialect = scala212source3
  }

  "glob:**/project/**.*" {
    runner.dialect = scala212source3
  }
}

I've been riding these 2 overrides in most of my Scala 3 projects.

keynmol commented 2 years ago

To clarify - if Metals already takes it upon itself to update scalafmt config when it detects different build targets having different dialects, it can sure recommend to add overrides for SBT files and the meta build.

ckipp01 commented 2 years ago

I don't think that it's a Metals issue.

Sure, but as a user when you use a Metals feature and it breaks your build, they see it as a Metals issue. Thanks for the workaround @keynmol, that's good to have listed. I still think we might want to try and address this either in scalafmt or here.

dos65 commented 2 years ago

@keynmol oh, good idea! That would be a nice improvement for this Metals feature

tgodzik commented 2 years ago

Also, we currently set the highest dialect possible, maybe we should rather set the correct dialect for all the build targets? We could add file overrides for those target versions that there are less of.