feat(scala3): WIP working on scala 3 cross compilation. #390. #461

brbrown25 · 2021-11-19T15:51:46Z

scala 3 rewrite compiler specific code

ahjohannessen · 2021-12-24T11:40:06Z

@brbrown25 What is the status on this? :)

brbrown25 · 2021-12-24T16:46:04Z

@brbrown25 What is the status on this? :)

I had something come up at work and didn't get to make progress, but next week I have time carved out and will push some updates :)

ahjohannessen · 2021-12-24T16:52:53Z

👍

brbrown25 · 2021-12-30T17:09:25Z

@ahjohannessen I've gotten about as far as I can with this. My understanding of macros is limited so I tried to stumble through. I will say that the only thing remaining is to fix up compiler/src/main/scala-3/play/twirl/compiler/TwirlCompiler.scala otherwise I've validated that the others cross compile fine. The only thing we can't make cross compiled is the plugin as it looks like that stops support at 2.12. Sorry I couldn't get it any further along.

ahjohannessen · 2021-12-30T17:38:35Z

@brbrown25 sbt build definitions are on Scala 2.12, so that is expected. I suppose what is missing is some kind soul that knows Scala 3 macros to step up :)

Edit: I meant sbt plugins are only 2.12.

brbrown25 · 2021-12-30T17:42:40Z

@brbrown25 sbt build definitions are on Scala 2.12, so that is expected. I suppose what is missing is some kind soul that knows Scala 3 macros to step up :)

Yeah I got it about as far as I could understand, it's definitely new and exciting territory!

ahjohannessen · 2021-12-30T20:53:17Z

@brbrown25 Which macros need porting?

brbrown25 · 2021-12-30T20:58:44Z

@brbrown25 Which macros need porting?

the following are the things of note. The big one is figuring out the correct imports/way of handling ValDef and Tree. But the things requiring fixing should be isolated to that file and the TemplateAsFunctionCompiler object

twirl/compiler/src/main/scala-3/play/twirl/compiler/TwirlCompiler.scala

Line 511 in 8b5fe47

object TemplateAsFunctionCompiler {

twirl/compiler/src/main/scala-3/play/twirl/compiler/TwirlCompiler.scala

Line 629 in 8b5fe47

class CompilerInstance {

twirl/compiler/src/main/scala-3/play/twirl/compiler/TwirlCompiler.scala

Line 657 in 8b5fe47

// val origBootclasspath =

(the updating the bootclasspath)

twirl/compiler/src/main/scala-3/play/twirl/compiler/TwirlCompiler.scala

Line 681 in 8b5fe47

def treeFrom(file: SourceFile): Tree = {

twirl/compiler/src/main/scala-3/play/twirl/compiler/TwirlCompiler.scala

Line 701 in 8b5fe47

global.close(???)

ahjohannessen · 2021-12-30T20:58:48Z

@brbrown25 Briefly
Scanned through the code, seems there is no
macro usage as far as I can tell. Is it more about interacting with the new compiler API that is the hurdle?

brbrown25 · 2021-12-30T21:00:49Z

@brbrown25 Briefly Scanned through the code, seems there is no macro usage as far as I can tell. Is it more about interacting with the new compiler API that is the hurdle?

Yeah the compiler Api is the hurdle, although tts actually possibly that macros could be used in place of the compiler Api, for this case, but I'm not 100% on that.

build.sbt

ahjohannessen · 2022-01-25T22:09:34Z

@brbrown25 This seems interesting tototoshi@6bcb772

brbrown25 · 2022-01-25T22:13:39Z

@brbrown25 This seems interesting tototoshi@6bcb772

@ahjohannessen I can pull in those changes and update this probably late this evening for first thing tomorrow!

tototoshi · 2022-01-26T03:13:53Z

My branch is here. https://github.com/tototoshi/twirl/tree/scalameta

I think it might be easier to share the source code between scala2 and scala3 using scalameta than using presentation compiler. (I don't know if twirl can depend on scalameta 🤔 ).

The main code already can be built in scala3, but the test code not yet. It has tests that confirm the generated codes can be really compiled, so still requires dotty.

brbrown25 · 2022-01-26T03:36:58Z

My branch is here. https://github.com/tototoshi/twirl/tree/scalameta

I think it might be easier to share the source code between scala2 and scala3 using scalameta than using presentation compiler. (I don't know if twirl can depend on scalameta 🤔 ).

I'm gonna play around with that and I agree if we came remove the presentation compiler piece it should make life a lot easier.

brbrown25 · 2022-01-26T14:12:40Z

@tototoshi I merged your work into mine and then tweaked mine to accommodate your scalameta changes, indeed it gets compiler to run the compile stage happily enough. The only piece that remains is updating the CompilerSpec so that the tests pass under all versions. I'll try and look at that today, but feel free to hack around with what's in this, I think we're getting pretty close!

tototoshi · 2022-01-30T04:06:48Z

I have implemented a dotty-based test helper in my working branch to support Scala3. So now it can be built with Scala3, including the tests.
https://github.com/tototoshi/twirl/tree/scalameta

And I found another problem.

In Scala2, an if expression without else returns Any or AnyVal, but in Scala3, it seems to return Unit.
It makes Scala3 display nothing when using twirl @if without else.

// Scala 2.13
scala> val a = if (true) 1
val a: AnyVal = 1

scala> val a = if (true) "a"
val a: Any = a

// Scala 3
scala> val a = if (true) 1
1 warning found
-- [E129] Potential Issue Warning: ---------------------------------------------
1 |val a = if (true) 1
  |                  ^
  |A pure expression does nothing in statement position; you may be omitting necessary parentheses

longer explanation available when compiling with `-explain`

scala> println(a)
()

I modified the TwirlParser to interpret the if without else as Twirl syntax instead of Scala code.
I think the implementation needs some refactoring, but It passes tests at least.

mkurz

Thanks for you work!
Some minor things need to be addressed.
Also I think you actually didn't run the test on Travis with the correct Scala version... So let's see how that turns out after setting the correct version.

Also, the changes in TwirlParser look ok to me, I know this needed change because of the if problem you described, however I hope this is not breaking apps.

Please update the pull request, I may review a second time and after merging, we can release 1.6.0-M2 so people can start testing.
Thanks again!

build.sbt

compiler/src/test/scala-3/play/twirl/compiler/test/Helper.scala

.travis.yml

…ries

…layframework#390.

mkurz · 2022-03-11T00:21:41Z

I did some local testing:

$ sbt
sbt:twirl> ++3.1.1
sbt:twirl> test
...
-- [E006] Not Found Error: /home/mkurz/work/github-play-repos/twirl/compiler/target/test/twirl-parser/generated-templates/html/error.template.scala:19:63 
19 |Seq[Any](format.raw/*2.1*/("""<h1>Hello """),_display_(/*2.12*/world),format.raw/*2.17*/("""</h1>
   |                                                               ^^^^^
   |                                                        Not found: world

...

here is a screenshot so you see this is highlighted in red:

This from

twirl/compiler/src/test/scala/play/twirl/compiler/test/CompilerSpec.scala

Lines 174 to 180 in 2c064e7

    
           "fail compilation for error.scala.html" in { 
        
             val helper = newCompilerHelper 
        
             the[CompilationError] thrownBy helper.compile[(() => Html)]("error.scala.html", "html.error") must have( 
        
               Symbol("line")(2), 
        
               Symbol("column")(12) 
        
             ) 
        
           }

However, it is not a real error, the test passes and the CompilationError object will correctly be thrown. I guess this is just the output of the Scala 3 compiler twirl executes... However the problem I have with that is that these log entries may confuse people... In Scala 2.12/2.13 these entries will not be displayed (because the Scala 2 compiler is used which obviously acts different).
Is it possible to suppress this log?

brbrown25 · 2022-03-11T04:55:11Z

I'm away from computer for a few days for family vacation but can look into n it when I return.

tototoshi · 2022-03-11T15:05:40Z

I added a commit to suppress compile errors log.
@brbrown25 Have a good vacation 😄

mkurz · 2022-03-11T15:10:29Z

@tototoshi I looked at your last commit, however you now suppress logging of compile errors only for tests? Or am I wrong? My idea was to always suppress that errors, not just for the CI tests... It would be also confusing in real apps to see that errors. WDYT?

brbrown25 · 2022-03-11T15:11:04Z

@tototoshi thank you!

tototoshi · 2022-03-11T15:28:05Z

@mkurz
My change is only for tests. In real apps, the compilation of the scala files generated by twirl is the task of sbt-twirl.
And sbt-twirl does not support cross-build against scala3 yet (because it is sbt-plugin running only on scala 2.12).
I think the problem you are thinking about will appear in the future when sbt is running on scala 3. But as for now, I think the current state is enough.

tototoshi · 2022-03-11T15:31:40Z

Umm... My understanding may be wrong. sbt-plugin runs on scala2.12 but the compilation of templates should be run on scala3.
OK. I will check it later.

tototoshi · 2022-03-12T06:42:37Z

Now I understand the problem.
It seems that the sourcePositionMappers setting provided by sbt-twirl doesn't work with Scala3. I didn't add changes to sbt-plugin, and the sources generated by twirl has no difference between Scala2 and Scala3.
I doubt this problem comes with sbt and dotty. I will investigate them next.

tototoshi · 2022-03-14T05:21:25Z

I found the cause.

https://github.com/lampepfl/dotty/blob/d2ebd75a2e63e3dd885e7bad94472931d0bae296/sbt-bridge/src/dotty/tools/xsbt/DelegatingReporter.java#L47

When Scala3 is compiled using sbt, compilation errors are reported by this DelegatingReporter. DelegatingReporter delegates the log output process to sbt's ManagedLoggedReporter. The problem seems to be that the error information passed to ManagedLoggedReporter includes messages generated by Dotty as rendered parameter of Problem.

If the rendered parameter is included in the error information, sbt will display the contents of the rendered parameter, and information other than the rendered parameter is ignored. So the error information in the Twirl template translated by sourcePositionMappers in LoggedReporter will not be displayed.

https://github.com/sbt/sbt/blob/fedf6af60d86ed39ae9c94161f13a79ffc4494e8/internal/util-interface/src/main/java/xsbti/Problem.java#L21-L28
https://github.com/sbt/zinc/blob/f6d36ff81aab6063a8ed5dd06c51ea6b9fef16f8/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/ProblemStringFormats.scala#L27-L28

If I modify the DelegatingReporter so that the rendered parameter is not passed to sbt, it is possible to display the same error message as when compiling with Scala2. However, the message stored in the rendered parameter is from the Dotty compiler and contains more detailed error messages, so I feel a bit uncomfortable making it completely invisible.

I think I have nothing that I can do In this Twirl repository, and ideally, either or both sbt and Dotty's sbt integration need to be improved.

tototoshi · 2022-03-15T13:36:11Z

scala/scala3#14691

…travis config. playframework#390.

brbrown25 · 2022-03-16T15:42:13Z

@tototoshi, @mkurz I just realized I messed up and didn't update the travis.yml so I've pushed that fix and we should get a clean travis pass on 3.1.1 now.

mkurz · 2022-03-21T12:25:04Z

Travis not working, closeing/reopening to trigger it...

brbrown25 · 2022-03-27T23:15:35Z

@mkurz since we got green builds in Travis do you think we might be ready to merge this soon?

mkurz

OK lets merge. Like said I did some local testing, also tests on CI pass so it looks promising. In case problems occur when can always fix them in upcoming pr/commits.
@tototoshi I saw sbt/zinc#1074 (comment), if you come up with a fix let us know.

I am too tired now, but I can release 1.6.0-M2 this week so people can play around with it, also to upgrade in other projects like Play to see if we trigger some other bugs.

mkurz · 2022-03-28T22:26:21Z

Opened #498 to keep track of the Scala 3/sourcePositionMapper problem

mkurz · 2023-11-13T08:17:59Z

This causes

A whitespace after if(){}else{} is not showing #711

If someone has time to look into that, I would appreciate it, thanks!

brbrown25 marked this pull request as draft November 19, 2021 15:51

brbrown25 force-pushed the ISSUE-390 branch 2 times, most recently from d6b6adb to 8b5fe47 Compare December 30, 2021 17:05

mkurz reviewed Jan 4, 2022

View reviewed changes

build.sbt Show resolved Hide resolved

brbrown25 force-pushed the ISSUE-390 branch 3 times, most recently from 64097c4 to 7030de6 Compare January 4, 2022 13:32

tototoshi added 3 commits January 25, 2022 21:50

Add Scala 3 to cross build

88688a8

Add a testcase for call-by-name parameters

8f82bc2

Use scalameta in TemplateAsFunctionCompiler

6bcb772

tototoshi added 4 commits January 29, 2022 23:32

Make CompilerSpec compatible with Scala3

88eb6da

Make @if expression work in Scala3

47259b7

Fix error reporting

f10f3da

Merge remote-tracking branch 'playframework/main' into scalameta

a10afc4

mkurz requested changes Mar 10, 2022

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

build.sbt Outdated Show resolved Hide resolved

compiler/src/test/scala-3/play/twirl/compiler/test/Helper.scala Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

tototoshi and others added 4 commits March 10, 2022 22:48

Removed onChangeBuildSource setting to align with other Play reposito…

d432c66

…ries

Remove unused code

909f11e

Replace TRAVIS_SCALA_VERSION with SCALA_VERSION

63be771

chore(build.sbt): removing the scripted change as it is not needed. p…

4174fe4

…layframework#390.

brbrown25 requested a review from mkurz March 10, 2022 14:20

Suppress compile errors log during running CompilerSpec

e892f83

chore(travis.yml): updating the scala version from 3.1.0 to 3.1.1 in …

2a3898f

…travis config. playframework#390.

mkurz closed this Mar 21, 2022

mkurz reopened this Mar 21, 2022

mkurz approved these changes Mar 28, 2022

View reviewed changes

mkurz merged commit addebd0 into playframework:main Mar 28, 2022

This was referenced Mar 28, 2022

Scala 3 support plans? #390

Closed

Fix problems regarding Scala 3 and sourcePositionMapper #498

Open

ihostage added this to the 1.6.0 milestone Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scala3): WIP working on scala 3 cross compilation. #390. #461

feat(scala3): WIP working on scala 3 cross compilation. #390. #461

brbrown25 commented Nov 19, 2021 •

edited

Loading

ahjohannessen commented Dec 24, 2021

brbrown25 commented Dec 24, 2021

ahjohannessen commented Dec 24, 2021

brbrown25 commented Dec 30, 2021

ahjohannessen commented Dec 30, 2021 •

edited

Loading

brbrown25 commented Dec 30, 2021

ahjohannessen commented Dec 30, 2021

brbrown25 commented Dec 30, 2021

ahjohannessen commented Dec 30, 2021

brbrown25 commented Dec 30, 2021

ahjohannessen commented Jan 25, 2022

brbrown25 commented Jan 25, 2022

tototoshi commented Jan 26, 2022

brbrown25 commented Jan 26, 2022

brbrown25 commented Jan 26, 2022

tototoshi commented Jan 30, 2022

mkurz left a comment

mkurz commented Mar 11, 2022

brbrown25 commented Mar 11, 2022

tototoshi commented Mar 11, 2022

mkurz commented Mar 11, 2022

brbrown25 commented Mar 11, 2022

tototoshi commented Mar 11, 2022

tototoshi commented Mar 11, 2022

tototoshi commented Mar 12, 2022

tototoshi commented Mar 14, 2022

tototoshi commented Mar 15, 2022

brbrown25 commented Mar 16, 2022

mkurz commented Mar 21, 2022

brbrown25 commented Mar 27, 2022

mkurz left a comment

mkurz commented Mar 28, 2022

mkurz commented Nov 13, 2023

feat(scala3): WIP working on scala 3 cross compilation. #390. #461

feat(scala3): WIP working on scala 3 cross compilation. #390. #461

Conversation

brbrown25 commented Nov 19, 2021 • edited Loading

ahjohannessen commented Dec 24, 2021

brbrown25 commented Dec 24, 2021

ahjohannessen commented Dec 24, 2021

brbrown25 commented Dec 30, 2021

ahjohannessen commented Dec 30, 2021 • edited Loading

brbrown25 commented Dec 30, 2021

ahjohannessen commented Dec 30, 2021

brbrown25 commented Dec 30, 2021

ahjohannessen commented Dec 30, 2021

brbrown25 commented Dec 30, 2021

ahjohannessen commented Jan 25, 2022

brbrown25 commented Jan 25, 2022

tototoshi commented Jan 26, 2022

brbrown25 commented Jan 26, 2022

brbrown25 commented Jan 26, 2022

tototoshi commented Jan 30, 2022

mkurz left a comment

Choose a reason for hiding this comment

mkurz commented Mar 11, 2022

brbrown25 commented Mar 11, 2022

tototoshi commented Mar 11, 2022

mkurz commented Mar 11, 2022

brbrown25 commented Mar 11, 2022

tototoshi commented Mar 11, 2022

tototoshi commented Mar 11, 2022

tototoshi commented Mar 12, 2022

tototoshi commented Mar 14, 2022

tototoshi commented Mar 15, 2022

brbrown25 commented Mar 16, 2022

mkurz commented Mar 21, 2022

brbrown25 commented Mar 27, 2022

mkurz left a comment

Choose a reason for hiding this comment

mkurz commented Mar 28, 2022

mkurz commented Nov 13, 2023

brbrown25 commented Nov 19, 2021 •

edited

Loading

ahjohannessen commented Dec 30, 2021 •

edited

Loading