Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
ckipp01 opened this issue May 10, 2023 Discussed in #17453 · 3 comments · May be fixed by #17522
Open

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

ckipp01 opened this issue May 10, 2023 Discussed in #17453 · 3 comments · May be fixed by #17522
Assignees
@adpi2
Labels
area:rewriting tool itype:bug

Comments

@ckipp01
Copy link
Member

ckipp01 commented May 10, 2023

Discussed in #17453

Originally posted by PeuTit May 10, 2023
Creating a new Play application with g8 and the play-scala-seed.g8 template.

sbt new playframework/play-scala-seed.g8 --branch 2.9.x

I was testing the rewrite capabilities of the compiler with the following options:

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]   <h1>Welcome to Play!</h1>
[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] </html>
[error]        ^
[error] /Users/titouan/Documents/work/perso/scala/test-tmp-play-scala3/app/views/index.scala.html:4:15: Not found: _display_:
[error]   <h1>Welcome to Play!</h1>
[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]     <head>
[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

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
Copy link
Member Author

ckipp01 commented May 10, 2023 •
edited
Loading

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
Copy link
Member Author

ckipp01 commented May 11, 2023

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 adpi2 self-assigned this May 12, 2023
@adpi2
Copy link
Member

adpi2 commented May 12, 2023 •
edited
Loading

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 {
}

@adpi2 adpi2 linked a pull request May 16, 2023 that will close this issue
Open
@Kordyjan Kordyjan modified the milestone: 3.5.0 May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adpi2 adpi2

Labels
area:rewriting tool itype:bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Patch indentation when removing braces (and other bug fixes in -indent -rewrite) adpi2/scala3
3 participants
@Kordyjan @adpi2 @ckipp01