TASTy Reader: support Scala 3.4 [ci: last-only]

[bishabosha]

Additions:

• support for reading TASTy 28.4 (stable) which will release in Scala 3.4.0
• a new error message format for reading a TASTy file with an unexpected version. The new message format is meant to be more readable and actionable.
• support for reading TASTy-only classpaths. Previous 2.13.x versions required both a pair of Foo.class and Foo.tasty to read any top-level Scala 3 class Foo, now there only needs to be the TASTy file.
• support for reading Java definitions from TASTy files. Java definitions can appear in TASTy files, for example to support pipelined compilation.

fixes scala/bug#12932

[SethTisue]

I don't have permission to push to this PR, but it looks like you've run afoul of fatal warnings only being enabled by default in CI. You need to:

diff --git src/compiler/scala/tools/tasty/Attributes.scala src/compiler/scala/tools/tasty/Attributes.scala
index 5d886ea998..44ed185777 100644
--- src/compiler/scala/tools/tasty/Attributes.scala
+++ src/compiler/scala/tools/tasty/Attributes.scala
@@ -15,6 +15,6 @@ package scala.tools.tasty
 class Attributes(val isJava: Boolean)
 
 object Attributes {
-  val empty = new Attributes(false)
-  val java = new Attributes(true)
+  val empty = new Attributes(isJava = false)
+  val java = new Attributes(isJava = true)
 }

if you want @som-snytt to let you out of named-arguments jail

[SethTisue]

@lrytz and/or @som-snytt will probably want to eyeball the pipelining changes. it seems okay to me but I don't think my review is worth too much here.

[som-snytt]

Our goal here with named boolean args is to be more opinionated than dotty.

I'll make an effort to overcome my ignorance about all things tasty. One can only be intrigued by a pr from a branch at scalacenter, that organization cloaked in mystery.

[bishabosha]

let it be noted that I also need to test more java language features before this PR is ready (enums already are causing an issue)

[lrytz]

The change in AggregateClassPath looks good, but I think it's not enough.

When a directory or jar contains both a/A.class and a/A.tasty, it seems to me the classes method in DirectoryClassPath or ZipArchiveClassPath will have a ClassFileEntry for both.

I haven't checked in detail where the classes method is used, but probably that should be deterministic too.

Usually we have an aggregate classpath of course, but I think in principle it could be a single entry.

Let me know if you prefer for me to take a look or pair on it.

[bishabosha]

When a directory or jar contains both a/A.class and a/A.tasty, it seems to me the classes method in DirectoryClassPath or ZipArchiveClassPath will have a ClassFileEntry for both.

right, I wasn't sure if this method was supposed to only return a single entry per class, so I was relying on the aggregate classpath to merge them.

I think I would prefer some guidance here, I guess each "list" method should do its own merging at the end to reduce quadratic complexity

[lrytz]

I'll take a look next week (was sick today)

[lrytz]

@bishabosha what do you think about this approach?

https://github.com/scalacenter/scala/compare/tasty/support-scala-3.4...lrytz:scala:pr10670?expand=1

[bishabosha]

@bishabosha what do you think about this approach?

https://github.com/scalacenter/scala/compare/tasty/support-scala-3.4...lrytz:scala:pr10670?expand=1

This was what I thought you might want to avoid because you are doing a directory lookup on every class file, rather than a single extra pass to merge that replaces class by tasty if it is encountered

[lrytz]

Yeah, but the alternatives are pretty hairy, need to implement the filtering in a couple of places. I wonder if it's that bad? Directories are probably not performance relevant, the ordinary use case is Jars. There the lookup is a hash map lookup.

[bishabosha]

Yeah, but the alternatives are pretty hairy, need to implement the filtering in a couple of places. I wonder if it's that bad? Directories are probably not performance relevant, the ordinary use case is Jars. There the lookup is a hash map lookup.

I'm happy to go with this as dotty does the same, does the CI have benchmarks for if this is a significant problem for standard scala 2 projects with no tasty? (I'd assume its not an issue)

[bishabosha]

@SethTisue and @lrytz addressed your comments and updated the PR description

[SethTisue]

@bishabosha this is failing on Windows, can you have a look? https://github.com/scala/scala/actions/runs/7875223173/job/21486648145

[bishabosha]

first look is that only the pipelining tests fail, and then only the java definitions can't be found, so my initial thoughts are that either the path to the java tasty jar isn't correct, the classpath isn't formatted correctly, or perhaps the jar isn't even written on windows? have to check this

[bishabosha]

Ok so it turns out we will have to disable these tests (run-pipelined and neg-pipelined on windows) - Dotty was writing a backslash for jar entries in windows (only for the -Yjava-tasty-output jar)

created scala/scala3#19681

[SethTisue]

it turns out we will have to disable these tests (run-pipelined and neg-pipelined on windows

Okay, sounds good. I see the PR (#10688).

In the meantime I can go ahead and announce the 2.13.13 release candidate today, then. (Community build is green.)

[SethTisue]

@bishabosha do I remember/understand correctly that no additional followup PR will be needed for 3.4.0 final?

[bishabosha]

Nothing is necessary, a good to have is removing the toolOveride to support the RC (which also enables 3.3.x nightlies)

[SethTisue]

Nothing is necessary, a good to have is removing the toolOveride to support the RC (which also enables 3.3.x nightlies)

Mind pre-submitting that as a draft, so I can just hit "merge" when the time comes?

[SethTisue]

@bishabosha in the draft release notes I mention 3.4 support but don't mention pipelining. it's too soon to highlight this for users, I assume?

[bishabosha]

it's too soon to highlight this for users, I assume?

Well it's not ready now, however I plan to still release it as a patch version e.g 3.4.1

[SethTisue]

I see. Well, in the blog post you'll surely write about it 😉 , you can highlight that support already landed in 2.13.13.

[bishabosha]

just observed a probable bug (not tested) in the classpath loading: - standalone objects e.g. object foo will have foo$.class and be associated with foo.tasty, i.e. dropping $ as well as .class

here is the dotty implementation of resolving a tasty name from a class file:

private def classNameToTasty(fileName: String): String =
    val classOrModuleName = fileName.stripSuffix(".class")
    val className =
      if classOrModuleName.endsWith("$")
        && classOrModuleName != "Null$" // scala.runtime.Null$
        && classOrModuleName != "Nothing$" // scala.runtime.Nothing$
        // Special case for `object $` in Amonite.
        // This is an ad-hoc workaround for Amonite `object $`. See issue #19702
        // This definition is not valid Scala.
        && classOrModuleName != "$"
      then classOrModuleName.stripSuffix("$")
      else classOrModuleName
    className + SUFFIX_TASTY

[lrytz]

👍 we can still include a fix for that

[SethTisue]

some followup work: #10689, #10694