Conversation
mrdziuban
left a comment
mrdziuban
left a comment
There was a problem hiding this comment.
There are still some test failures I'm not sure how to resolve, primarily mentioning issues like this:
tastyquery.Exceptions$MemberNotFoundException: Member ImpureFunction1/T not found in PackageRef(scala)
There are also some binary compatibility problems:
tasty-query: Failed binary compatibility check against ch.epfl.scala:tasty-query_3:1.6.1! Found 4 potential problems
* static method <clinit>()Unit in object tastyquery.Names#Name does not have a correspondent in current version
filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("tastyquery.Names#Name.<clinit>")
* static method <clinit>()Unit in object tastyquery.Signatures#Signature does not have a correspondent in current version
filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("tastyquery.Signatures#Signature.<clinit>")
* static method <clinit>()Unit in object tastyquery.jdk.ClasspathLoaders#FileKind does not have a correspondent in current version
filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("tastyquery.jdk.ClasspathLoaders#FileKind.<clinit>")
* static method <clinit>()Unit in object tastyquery.reader.classfiles.ClassfileReader#ConstantInfo does not have a correspondent in current version
filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("tastyquery.reader.classfiles.ClassfileReader#ConstantInfo.<clinit>")
Please let me know how you'd like to resolve these.
|
|
||
| def readResourceCodeFile(relPath: String): String = | ||
| val path = getEnvVar(ResourceCodeEnvVar).nn + "/" + relPath | ||
| val path = getEnvVar(ResourceCodeEnvVar) + "/" + relPath |
There was a problem hiding this comment.
All the .nn removals are from spots where the compiler reported that it was unnecessary because the qualifier is already not null
|
|
||
| lazy val scala3ClasspathIndex: Int = | ||
| classpathEntries.indexWhere(_.contains("scala3-library_3").nn) | ||
| classpathEntries.indexWhere(_.contains("scala-library-3")) |
There was a problem hiding this comment.
Without this change, the tests in ClasspathEntrySuite fail with an error:
tastyquery.Exceptions$UnknownClasspathEntry: Unknown classpath entry: /Users/matt/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala3-library_3/3.8.0/scala3-library_3-3.8.0.jar, it is probably from another Classpath.
at tastyquery.Contexts$.tastyquery$Contexts$Context$$_$findSymbolsByClasspathEntry$$anonfun$1(Contexts.scala:91)
at scala.Option.getOrElse(Option.scala:203)
at tastyquery.Contexts$Context.findSymbolsByClasspathEntry(Contexts.scala:92)
at tastyquery.ClasspathEntrySuite.lookupSyms(ClasspathEntrySuite.scala:15)
at tastyquery.ClasspathEntrySuite.$init$$$anonfun$1(ClasspathEntrySuite.scala:20)
at tastyquery.UnrestrictedUnpicklingSuite.testWithContext$$anonfun$1(UnrestrictedUnpicklingSuite.scala:14)
at tastyquery.UnrestrictedUnpicklingSuite.testWithContext$$anonfun$2$$anonfun$1(UnrestrictedUnpicklingSuite.scala:25)
at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:503)
at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(ForkJoinTask.java:1750)
at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(ForkJoinTask.java:1742)
at java.util.concurrent.ForkJoinTask$InterruptibleTask.exec(ForkJoinTask.java:1659)
at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:511)
at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1450)
at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2019)
at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:187)
|
|
||
| val syms = lookupSyms(scala3ClasspathEntry) | ||
|
|
||
| assert(syms.size < 300, s"scala 3 library has unexpected root symbols, found ${syms.size}") |
There was a problem hiding this comment.
My understanding is that the Scala 3 library now does contain root symbols, so this assertion is no longer relevant
There was a problem hiding this comment.
I don't know the full context of the test, but scala-library.jar jar now has all Scala Standard Library definitions, scala3-library_3.jar is now an empty (POM-only) JAR file kept for binary compatibility.
There was a problem hiding this comment.
Thanks @WojciechMazur. That fact, combined with the change to look for scala-library-3 instead of scala3-library_3, makes it make sense why this assertion started failing on 3.8
| assert( | ||
| clue(isSym.declaredType) | ||
| .isIntersectionOf(_.isRef(typeXSym), _.isApplied(_.isRef(ListClass), List(_.isRef(tTypeCaptureSym)))) | ||
| .isIntersectionOf(_.isApplied(_.isRef(ListClass), List(_.isRef(tTypeCaptureSym))), _.isRef(typeXSym)) |
There was a problem hiding this comment.
The order of these is just flipped
| _.isType { | ||
| case lambda: TypeLambda => lambda.resultType.isApplied(_.isRef(ListClass), List(_ => true)) | ||
| case _ => false | ||
| }, |
There was a problem hiding this comment.
This second type arg is the one that needed to change. I'm not 100% sure why, but _.isRef(ListClass) is no longer true. Instead it's now a TypeLambda
| assert(clue(deprecatedAnnot.argIfConstant(0)).isDefined) | ||
| assert(clue(deprecatedAnnot.argIfConstant(1)) == Some(Constant("2.13.0"))) | ||
|
|
||
| intercept[UnsupportedOperationException](deprecatedAnnot.annotConstructor) |
There was a problem hiding this comment.
I think this is no longer an unsupported operation because the annotation isn't a Scala 2 annotation anymore
| _.isType { | ||
| case lambda: TypeLambda => lambda.resultType.isApplied(_.isRef(IterableClass), List(_ => true)) | ||
| case _ => false | ||
| }, |
There was a problem hiding this comment.
Similar to the _.isRef(ListClass) change above, this is now a TypeLambda
| assert( | ||
| clue(mt.resultType) | ||
| .isApplied(_.isRef(AmpersandType), List(_ eq typeParamRef, _.isRef(EfficientSplitClass))) | ||
| ) |
There was a problem hiding this comment.
I'm not sure if this is a good change. Should an AppliedType(scala.&, List(...)) be considered an AndType? It is not currently
There was a problem hiding this comment.
AppliedType(scala.&, List(...)) is indeed not an AndType. scala.& is a polymorphic type alias to a magical right-hand-side that is an actual, "internal" & type. See https://www.scala-lang.org/files/archive/spec/3.4/12-the-scala-standard-library.html#fundamental-type-aliases
| Developer("bishabosha", "Jamie Thompson", "bishbashboshjt@gmail.com", url("https://github.com/bishabosha")), | ||
| ), | ||
| versionPolicyIntention := Compatibility.BinaryAndSourceCompatible, | ||
| versionPolicyIntention := Compatibility.BinaryCompatible, |
There was a problem hiding this comment.
This needs to be relaxed temporarily to allow the Scala and Scala.js upgrades
| "-Werror", | ||
| "-Wsafe-init", | ||
| "-source:future", | ||
| // "-source:future", |
There was a problem hiding this comment.
Keeping this enabled would require 100+ syntactical changes, e.g. from if (pred) ... to if pred then ... and while (pred) ... to while pred do ....
I've commented it out for now to reduce noise in the diff. I'd be happy to follow up with the syntactical changes once this is merged.
|
Amazing, thank you! Would you like me to follow up with the syntax changes necessary to re-enable |
|
If you want, though I probably won't prioritize reviewing that in the coming weeks. I have other more pressing duties. |
No problem, I just opened #463 for whenever you have a chance |
3 participants
Upgrades to Scala 3.8.0 (which is currently being released: scala/scala3#24947) and 2.13.18 for Scala 2 tests. Also upgrades to Scala.js 1.20.1 to match the compiler upgrade in scala/scala3#23884.
There are still a handful of issues to address, see comments below.