scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.88k stars 1.06k forks source link

Rewrite compiler flag on some generated code can break it #17456

Open ckipp01 opened 1 year ago

ckipp01 commented 1 year ago

Discussed in https://github.com/lampepfl/dotty/discussions/17453

Originally posted by **PeuTit** May 10, 2023 Creating a new Play application with g8 and the [play-scala-seed.g8 template](https://github.com/playframework/play-scala-seed.g8). ```sh sbt new playframework/play-scala-seed.g8 --branch 2.9.x ``` I was testing the rewrite capabilities of the compiler with the following options: ```scala scalacOptions ++= Seq( "-rewrite", "-indent", ) ``` Launching a sbt shell & compiling the project, I'm facing the following errors: ``` [info] compiling 7 Scala sources and 1 Java source to /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/target/scala-3.3.0-RC5/classes ... [error] /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/app/views/index.scala.html:4:28: Illegal start of toplevel definition [error]

Welcome to Play!

[error] ^ [error] /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/app/views/main.scala.html:12:11: Illegal start of toplevel definition [error] @* Here's where we render the page title `String`. *@ [error] ^ [error] /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/app/views/main.scala.html:25:8: render is already defined as method render in /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/target/scala-3.3.0-RC5/twirl/main/views/html/index.template.scala [error] [error] Note that overloaded methods must all be defined in the same group of toplevel definitions [error] [error] ^ [error] /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/app/views/index.scala.html:4:15: Not found: _display_: [error]

Welcome to Play!

[error] ^ [error] /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/app/views/index.scala.html:5:2: Not found: apply [error] } [error] ^ [error] /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/app/views/main.scala.html:11:9: Not found: _display_: [error] [error] ^ [error] 6 errors found [error] (Compile / compileIncremental) Compilation failed [error] Total time: 1 s, completed May 10, 2023, 10:40:04 AM ``` Does anyone have ever faced this issue previously? [minimal example](https://github.com/PeuTit/play-scala3-rewrite)

Actually, I was able to reproduce this. Interesting one. So what happens is that on the first compile it will take the Scala code that gets generated from the index.scala.html and attempt to rewrite it. It does this, but outputs invalid code. For example here is what your index.scala.html really looks like when passed to the compiler:

package views.html

import _root_.play.twirl.api.TwirlFeatureImports._
import _root_.play.twirl.api.TwirlHelperImports._
import _root_.play.twirl.api.Html
import _root_.play.twirl.api.JavaScript
import _root_.play.twirl.api.Txt
import _root_.play.twirl.api.Xml
import models._
import controllers._
import play.api.i18n._
import views.html._
import play.api.templates.PlayMagic._
import play.api.mvc._
import play.api.data._

object index extends _root_.play.twirl.api.BaseScalaTemplate[play.twirl.api.HtmlFormat.Appendable,_root_.play.twirl.api.Format[play.twirl.api.HtmlFormat.Appendable]](play.twirl.api.HtmlFormat) with _root_.play.twirl.api.Template0[play.twirl.api.HtmlFormat.Appendable] {

  /**/
  def apply/*1.2*/():play.twirl.api.HtmlFormat.Appendable = {
    _display_ {
      {

Seq[Any](format.raw/*2.1*/("""
"""),_display_(/*3.2*/main("Welcome to Play")/*3.25*/ {_display_(Seq[Any](format.raw/*3.27*/("""
  """),format.raw/*4.3*/("""<h1>Welcome to Play!</h1>
""")))}),format.raw/*5.2*/("""
"""))
      }
    }
  }

  def render(): play.twirl.api.HtmlFormat.Appendable = apply()

  def f:(() => play.twirl.api.HtmlFormat.Appendable) = () => apply()

  def ref: this.type = this

}

              /*
                  -- GENERATED --
                  SOURCE: app/views/index.scala.html
                  HASH: f7d71d8a9f655317469a39627564796bcf2a545b
                  MATRIX: 722->1|818->4|845->6|876->29|915->31|944->34|1000->61
                  LINES: 21->1|26->2|27->3|27->3|27->3|28->4|29->5
                  -- GENERATED --
              */

This then gets rewritten to

package views.html

import _root_.play.twirl.api.TwirlFeatureImports._
import _root_.play.twirl.api.TwirlHelperImports._
import _root_.play.twirl.api.Html
import _root_.play.twirl.api.JavaScript
import _root_.play.twirl.api.Txt
import _root_.play.twirl.api.Xml
import models._
import controllers._
import play.api.i18n._
import views.html._
import play.api.templates.PlayMagic._
import play.api.mvc._
import play.api.data._

object index extends _root_.play.twirl.api.BaseScalaTemplate[play.twirl.api.HtmlFormat.Appendable,_root_.play.twirl.api.Format[play.twirl.api.HtmlFormat.Appendable]](play.twirl.api.HtmlFormat) with _root_.play.twirl.api.Template0[play.twirl.api.HtmlFormat.Appendable]:

  /**/
  def apply/*1.2*/():play.twirl.api.HtmlFormat.Appendable =
    _display_:

  Seq[Any](format.raw/*2.1*/("""
  """),_display_(/*3.2*/main("Welcome to Play")/*3.25*/ {_display_(Seq[Any](format.raw/*3.27*/("""
    """),format.raw/*4.3*/("""<h1>Welcome to Play!</h1>
  """)))}),format.raw/*5.2*/("""
  """))

    def render(): play.twirl.api.HtmlFormat.Appendable = apply()

    def f:(() => play.twirl.api.HtmlFormat.Appendable) = () => apply()

    def ref: this.type = this

                /*
                    -- GENERATED --
                    SOURCE: app/views/index.scala.html
                    HASH: f7d71d8a9f655317469a39627564796bcf2a545b
                    MATRIX: 722->1|818->4|845->6|876->29|915->31|944->34|1000->61
                    LINES: 21->1|26->2|27->3|27->3|27->3|28->4|29->5
                    -- GENERATED --
                */     

Which has a whole handful of problems. The thing is, is that I don't even think we should be trying to rewrite these files, but I don't know if there is a good way to filter them out since Dotty I believe is just going to try and rewrite everything, whether it's generated code or not.

ckipp01 commented 1 year ago

Alright so I ended up just moving this from the discussion into an issue since in reality it does produce broken code and this could bit uses if they try this out. In general I just don't think we should try to rewrite this, but I'm also not 100% sure how to avoid this. I'm not sure if there is some tag on a source when it's passed to the compiler indicating that it was generated or something, but if so we could detect that and not try to rewrite it I guess. I'm open for any idea/suggestions others might have here.

There's also the idea that in reality, this is valid code, so in reality rewriting should be able to work here.

ckipp01 commented 1 year ago

Alright, you can minimize this down to:

//> using scala 3.3.1-RC1-bin-20230510-d6c643c-NIGHTLY
//> using options -rewrite -indent

object example {
  def foo(a: Any) = ???

  def appply() = foo {
Seq(1, 2, 3)
  }
}

Running scala-cli compile . on this will result in:

//> using scala 3.3.1-RC1-bin-20230510-d6c643c-NIGHTLY
//> using options -rewrite -indent

object example:
  def foo(a: Any) = ???

  def appply() = foo:
Seq(1, 2, 3)

Which has broken indentation for the Seq(1, 2, 3).

adpi2 commented 1 year ago

In general it seems the compiler does not try to fix the indentation at all.

def f = {
val x = ""
x
}

is rewritten into:

def f =
val x = ""
x

I also found a few related/unrelated bugs/features:

  1. The compiler does not rewrite the body if there is no '\n' after it:
class A {
  def foo = ""
}

class B {
  def bar = ""
} // no \n here

is rewritten to

class A:
  def foo = ""

class B {
  def bar = ""
}
  1. It does not remove the braces of empty bodies class A {} or
    class B {
    }