Skip to content
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.

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

Scala 3 support #136

Merged
merged 12 commits into from
Sep 23, 2024
Merged

Scala 3 support #136

merged 12 commits into from
Sep 23, 2024

Conversation

mrdziuban
Copy link
Contributor

Closes #76

Adds support for Scala 3.3.{0,1,2,3}, Scala 3.4.{0,1,2,3}, and Scala 3.5.0.

I aimed for as close as a 1-to-1 port as possible, especially in PluginPhase.scala and DependencyExtraction.scala where the meat of the logic lives. I'm happy to clarify/elaborate on any specific code

@@ -53,12 +52,12 @@ object CycleTests extends TestSuite {
test("innercycle") - make("success/pkg/innercycle")
}
}
test("self") - make("../../src", extraIncludes = Nil)
test("self") - make(s"../../$srcDirName", extraIncludes = Nil)
test("force") - {
test("warn") - {
test("fail") - {
make("force/simple", force = true, warn = true).exists {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with utest, but should this be wrapped in assert to prove that it's true?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends what make does. If make performs the assert internally, then no need to wrap in an assert. If make returns a Boolean, then you need to wrap in an assert

Copy link
Contributor Author

@mrdziuban mrdziuban Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make returns a Seq[(String, String)] (and throws an exception if compilation fails), but doesn't call assert. The call to .exists here then returns a Boolean, so it looks like we do need assert. Updated to add it and to also make sure that we're matching against the uppercase string "WARNING" since the severity levels use uppercase strings

Comment on lines 87 to 89
case ERROR => "error"
case INFO => "info"
case WARNING => "warning"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scala 3 diagnostics don't have a severity, instead they have a level: Int that maps to one of these 3

)
}

def findAcyclics() = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how much of the logic in this file can be shared between the Scala 2 and Scala 3 implementations? Like clearly the logic for working with symbols and types and trees is all bespoke, but it seems there's a lot of stuff working with Values and DepNodes that can be shared if properly separated out from the compiler-specific logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I was able to share pretty much all of it by abstracting over CompilationUnit, Tree, and Symbol

@lihaoyi
Copy link
Member

lihaoyi commented Sep 22, 2024

@mrdziuban sorry for the delay, just had a chance to try this out on a small test project. The cycle detection seems find, though the reporting doesn't seem to work. The Scala 2 version reports a cycle error location at each point in the cycle with More dependencies at lines... to show additional edges in a file, whereas the Scala 3 version reports all the edges

Given these files

// Foo.scala
package foo
object Foo {
  val value = Foo2.value
  def main(args: Array[String]): Unit = {}
}
// Foo2.scala
package foo
object Foo2 {
  val value = "hello"
  val value2 = Foo.value
  def main(args: Array[String]): Unit = Foo.main(args)
}

The Scala 2 version of this reports the cycle as follows:

#05 [error] Unwanted cyclic dependency
#05 [info] /Users/lihaoyi/test/foo/src/Foo2.scala:5:16:
#05 [info]   val value2 = Foo.value
#05 [info]                ^
#05 [info] symbol: object Foo
#05 [info] More dependencies at lines 6 6
#05 [info] /Users/lihaoyi/test/foo/src/Foo.scala:3:15:
#05 [info]   val value = Foo2.value
#05 [info]               ^
#05 [info] symbol: object Foo2
#05 [error] one error found

Whereas the Scala 3 version reports it as follows

#02 [error] Unwanted cyclic dependency
#02 [info] -- Info: /Users/lihaoyi/test/foo/src/Foo.scala:3:19 ----------------------------
#02 [info] 3 |  val value = Foo2.value
#02 [info]   |              ^^^^^^^^^^
#02 [info] -- Info: /Users/lihaoyi/test/foo/src/Foo2.scala:5:19 ---------------------------
#02 [info] 5 |  val value2 = Foo.value
#02 [info]   |               ^^^^^^^^^
#02 [error] Unwanted cyclic dependency
#02 [info] -- Info: /Users/lihaoyi/test/foo/src/Foo.scala:3:19 ----------------------------
#02 [info] 3 |  val value = Foo2.value
#02 [info]   |              ^^^^^^^^^^
#02 [info] -- Info: /Users/lihaoyi/test/foo/src/Foo2.scala:5:19 ---------------------------
#02 [info] 5 |  val value2 = Foo.value
#02 [info]   |               ^^^^^^^^^
#02 [error] two errors found

The Scala 3 version is missing the More dependencies at lines... indicator, and seems to duplicate the cycle being reported between val value2 = Foo.value and value value = Foo2.value

@mrdziuban
Copy link
Contributor Author

No problem! I was able to resolve both of these issues.

the reporting doesn't seem to work

It turned out this was because the "More dependencies at..." reporting was using report.inform, which only appears if the -verbose flag is set -- https://github.com/scala/scala3/blob/3.5.0/compiler/src/dotty/tools/dotc/report.scala#L16-L17

seems to duplicate the cycle being reported

I'm pretty sure this is a nuance of how the plugin is run between Scala 2 and 3. In Scala 2 it seems like Phase#run is only called once, whereas in Scala 3 PluginPhase#run is called once per CompilationUnit. So in your case the output was duplicated, but with three files it would have been triplicated, etc.

I fixed this by adding a var alreadyRun at the Plugin level and only running the phase if it's false.

@lihaoyi lihaoyi merged commit 014f9ab into com-lihaoyi:main Sep 23, 2024
2 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Sep 23, 2024

@mrdziuban looks great! Thanks for your help. I'll wire you the bounty using the details you sent me earlier

@lefou lefou added this to the after 0.3.13 milestone Sep 23, 2024
@mrdziuban mrdziuban deleted the scala-3 branch September 23, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scala 3 support (1000USD Bounty)
3 participants