- Notifications
You must be signed in to change notification settings - Fork3.1k
ClassfileParser: allow missing param names (for JDK 21)#10397
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
SethTisue commentedMay 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
my procedure for testing this locally:
this gives us, afaics, sufficient confidence to merge the PR. after it's merged, the next steps will be:
|
SethTisue commentedMay 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
on 2.13.x, we'll need to do very similar rigmarole. on that branch, we don't have the circularity between sbt and Scala to contend with, but we will still need to re-STARR in order to be able to bootstrap on JDK 21 we'll merge the fix forward from 2.12.x, also merge forward the |
lrytz left a comment
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.
LGTM
this is a forward port ofscala/scala#10397 (fixingscala/bug#12783,from which Scala 3 also suffers) — the same fix we'll be shipping in2.12.18 and 2.13.11I haven't included a unit test, because* a similar change has already been well validated in a Scala 2 context* without this change, the compiler can't compile anything at all on JDK21* we need the reference compiler to include this change before we canbootstrap on JDK 21 anywaybut I did manually test it locally, by bootstrapping on JDK 11,publishing the resulting compiler locally, and then launching the REPLand evaluating `2 + 2`on 3.3.0-RC6, that fails with:> error while loading AccessFlag,> class file /modules/java.base/java/lang/reflect/AccessFlag.class isbroken, reading aborted with class java.lang.RuntimeException> bad constant pool index: 0 at pos: 5189> error while loading ElementType,> class file /modules/java.base/java/lang/annotation/ElementType.classis broken, reading aborted with class java.lang.RuntimeException> bad constant pool index: 0 at pos: 1220> 2 errors foundwhereas with this change, it succeeds.
### What changes were proposed in this pull request?This PR aims to upgrade Scala to 2.12.18-https://www.scala-lang.org/news/2.12.18### Why are the changes needed?This release adds support for JDK 20 and 21:-scala/scala#10185-scala/scala#10362-scala/scala#10397-scala/scala#10400The full release notes as follows:-https://github.com/scala/scala/releases/tag/v2.12.18### Does this PR introduce _any_ user-facing change?Yes, this is a Scala version change.### How was this patch tested?Existing TestCloses#41627 from LuciferYang/SPARK-43832.Authored-by: yangjie01 <yangjie01@baidu.com>Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request?This PR aims to upgrade Scala to 2.12.18-https://www.scala-lang.org/news/2.12.18### Why are the changes needed?This release adds support for JDK 20 and 21:-scala/scala#10185-scala/scala#10362-scala/scala#10397-scala/scala#10400The full release notes as follows:-https://github.com/scala/scala/releases/tag/v2.12.18### Does this PR introduce _any_ user-facing change?Yes, this is a Scala version change.### How was this patch tested?Existing TestClosesapache#41627 from LuciferYang/SPARK-43832.Authored-by: yangjie01 <yangjie01@baidu.com>Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request?This PR aims to upgrade Scala to 2.13.11-https://www.scala-lang.org/news/2.13.11Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced byscala/scala#10083, we must fix it when we upgrading to use Scala 3,### Why are the changes needed?This release improves collections, adds support for JDK 20 and 21, adds support for JDK 17 `sealed`:-scala/scala#10363-scala/scala#10184-scala/scala#10397-scala/scala#10348-scala/scala#10105There are 2 known issues in this version:-scala/bug#12800-scala/bug#12799For the first one, there is no compilation warning messages related to `match may not be exhaustive` in Spark compile log, and for the second one, there is no case of `method.isAnnotationPresent(Deprecated.class)` in Spark code, there is justhttps://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130in Spark Code, and I checked `javax.annotation.Nonnull` no this issue.So I think These two issues will not affect Spark itself, but this doesn't mean it won't affect the code written by end users themselvesThe full release notes as follows:-https://github.com/scala/scala/releases/tag/v2.13.11### Does this PR introduce _any_ user-facing change?Yes, this is a Scala version change.### How was this patch tested?- Existing Test- Checked Java 8/17 + Scala 2.13.11 using GA, all test passedJava 8 + Scala 2.13.11:https://github.com/LuciferYang/spark/runs/14337279564Java 17 + Scala 2.13.11:https://github.com/LuciferYang/spark/runs/14343012195Closes#41626 from LuciferYang/SPARK-40497.Authored-by: yangjie01 <yangjie01@baidu.com>Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request?This PR aims to upgrade Scala to 2.13.11-https://www.scala-lang.org/news/2.13.11Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced byscala/scala#10083, we must fix it when we upgrading to use Scala 3,### Why are the changes needed?This release improves collections, adds support for JDK 20 and 21, adds support for JDK 17 `sealed`:-scala/scala#10363-scala/scala#10184-scala/scala#10397-scala/scala#10348-scala/scala#10105There are 2 known issues in this version:-scala/bug#12800-scala/bug#12799For the first one, there is no compilation warning messages related to `match may not be exhaustive` in Spark compile log, and for the second one, there is no case of `method.isAnnotationPresent(Deprecated.class)` in Spark code, there is justhttps://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130in Spark Code, and I checked `javax.annotation.Nonnull` no this issue.So I think These two issues will not affect Spark itself, but this doesn't mean it won't affect the code written by end users themselvesThe full release notes as follows:-https://github.com/scala/scala/releases/tag/v2.13.11### Does this PR introduce _any_ user-facing change?Yes, this is a Scala version change.### How was this patch tested?- Existing Test- Checked Java 8/17 + Scala 2.13.11 using GA, all test passedJava 8 + Scala 2.13.11:https://github.com/LuciferYang/spark/runs/14337279564Java 17 + Scala 2.13.11:https://github.com/LuciferYang/spark/runs/14343012195Closesapache#41626 from LuciferYang/SPARK-40497.Authored-by: yangjie01 <yangjie01@baidu.com>Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
this is a forward port ofscala/scala#10397 (fixingscala/bug#12783,from which Scala 3 also suffers)
### What changes were proposed in this pull request?This PR aims to re-upgrade Scala to 2.13.11, afterSPARK-45144 was merged, the build issues mentioned in#41943 should no longer exist.-https://www.scala-lang.org/news/2.13.11Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced byscala/scala#10083, we must fix it when we upgrading to use Scala 3### Why are the changes needed?This release improves collections, adds support for JDK 20 and 21, adds support for JDK 17 `sealed`:-scala/scala#10363-scala/scala#10184-scala/scala#10397-scala/scala#10348-scala/scala#10105There are 2 known issues in this version:-scala/bug#12800-scala/bug#12799For the first one, there is no compilation warning messages related to `match may not be exhaustive` in Spark compile log, and for the second one, there is no case of `method.isAnnotationPresent(Deprecated.class)` in Spark code, there is justhttps://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130in Spark Code, and I checked `javax.annotation.Nonnull` no this issue.So I think These two issues will not affect Spark itself, but this doesn't mean it won't affect the code written by end users themselvesThe full release notes as follows:-https://github.com/scala/scala/releases/tag/v2.13.11### Does this PR introduce _any_ user-facing change?Yes, this is a Scala version change.### How was this patch tested?- Existing Test### Was this patch authored or co-authored using generative AI tooling?NoCloses#42918 from LuciferYang/SPARK-40497-2.Authored-by: yangjie01 <yangjie01@baidu.com>Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
fixesscala/bug#12783