-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
TASTy Reader: support Scala 3.4 [ci: last-only] #10670
Conversation
c3f5fa6
to
7c3a242
Compare
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 |
@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. |
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. |
7c3a242
to
1117731
Compare
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) |
2f211fb
to
3934cdf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/compiler/scala/tools/nsc/classpath/AggregateClassPath.scala
Outdated
Show resolved
Hide resolved
src/compiler/scala/tools/nsc/classpath/AggregateClassPath.scala
Outdated
Show resolved
Hide resolved
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 |
26b007e
to
0ce1156
Compare
I'll take a look next week (was sick today) |
@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 |
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) |
when you see a .class file, check that there is not a sibling .tasty file Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com> Co-authored-by: Lukas Rytz <lukas.rytz@gmail.com>
0cbcaa0
to
bfed4df
Compare
@SethTisue and @lrytz addressed your comments and updated the PR description |
@bishabosha this is failing on Windows, can you have a look? https://github.com/scala/scala/actions/runs/7875223173/job/21486648145 |
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 |
Ok so it turns out we will have to disable these tests ( created scala/scala3#19681 |
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.) |
@bishabosha do I remember/understand correctly that no additional followup PR will be needed for 3.4.0 final? |
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? |
@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? |
Well it's not ready now, however I plan to still release it as a patch version e.g 3.4.1 |
I see. Well, in the blog post you'll surely write about it 😉 , you can highlight that support already landed in 2.13.13. |
just observed a probable bug (not tested) in the classpath loading: - standalone objects e.g. 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 |
👍 we can still include a fix for that |
Additions:
fixes scala/bug#12932