Support JDK 17 `PermittedSubclasses` in classfiles #10105

som-snytt · 2022-08-06T21:00:49Z

Fixes scala/bug#12171
Fixes scala/bug#12159

Also adds test coverage for scala/bug#12474

ClassfileParser hint from #9228

Implementation: a quick hack takes non-sealed in Java sources as three tokens.

SethTisue · 2022-08-10T12:30:20Z

Does Scala's sealed set the classfile flag (under -target:17 or higher)? Should it?

lrytz

I had some WIP branch for this, sorry I didn't share... LGTM already.

In my branch I included Flags.SEALED in the modifiers of the ClassDef, which triggers population of children in the namer (IIRC) and enables exhaustivity checking. However, that's not enough, because Java symbols are only completed lazily, so only completed symbols are going to be known children. We can defer that to later I guess.

src/reflect/scala/reflect/internal/StdNames.scala

src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala

src/compiler/scala/tools/nsc/javac/JavaParsers.scala

SethTisue · 2022-10-26T11:49:50Z

@som-snytt care to respond to the review feedback? this one would be really nice to get into 2.13.11

som-snytt · 2022-10-26T12:40:33Z

That is not sarcastic 👍 . I don't know if we need to stipulate that every time now?

SethTisue · 2022-10-26T15:11:36Z

Is that a reference to https://www.walesonline.co.uk/news/uk-news/generation-z-ers-giving-passive-25249315 ?

In my world, thumbs-up are always assumed to be sincere 👍

som-snytt · 2022-10-26T15:13:00Z

I'm aware that 👎 is always sincere.

som-snytt · 2022-10-31T09:27:42Z

Children added per PermittedSubclasses. Drafting to support permits clause.

SethTisue · 2023-01-31T02:22:19Z

@som-snytt this still seems like a high-value PR to me; let us know if you could use any input from the team in helping it get across the finish line

som-snytt · 2023-01-31T06:10:36Z

@SethTisue thanks, I just checked out this branch today as part of my periodic attempt to "groom" and "reduce" my open PRs. Please let me know if 2.13.11 is imminent.

SethTisue · 2023-02-17T03:10:02Z

Please let me know if 2.13.11 is imminent.

Well, it won't come out before 3.3.0. I started https://contributors.scala-lang.org/t/scala-2-13-11-release-planning/6088 just now.

SethTisue · 2023-02-17T03:13:54Z

With any luck, a rebase on top of #10310 will fix Travis-CI. :knockonwood:

SethTisue · 2023-03-29T07:41:03Z

[Von Neumann] was consulted by a group who was building a rocket ship to send into outer space. When he saw the incomplete structure, he asked, "Where did you get the plans for this ship?" He was told, "We have our own staff of engineers." He disdainfully replied: "Engineers! Why I have completely sewn up the whole mathematical theory of rocketry. See my paper of 1952." Well, the group consulted the 1952 paper, completely scrapped their 10 million dollar structure, and rebuilt the rocket exactly according to Von Neumann's plans. The minute they launched it, the entire structure blew up. They angrily called Von Neumann back and said: "We followed your instructions to the letter. Yet when we started it, it blew up! Why?" Von Neumann replied, "Ah yes; that is technically known as the blow-up problem - I treated that in my paper of 1954."

(this PR = 1952 paper, #10348 = 1954 paper)

som-snytt · 2023-03-30T17:21:24Z

This PR gets you to ISS, if you want to dock with Tiangong then you need the other PR.

som-snytt · 2023-04-20T18:08:14Z

@SethTisue rebased in honor of today's Starship test launch. Can we merge this? Then I can rebase the follow-up for mixed compilation.

preveen-stack · 2023-04-22T08:51:06Z

Just to provide some reference blog post on java sealed classes

lrytz · 2023-04-24T08:47:45Z

If we split up the support for Java sealed in two merges, I think this first one should not add the incomplete support for sealed children. IIUC, what works is

support for the new syntax
support for children in the ClassfileParser

So I suggest removing the SEALED flag from symbols of sealed interfaces that come from source files. WDYT?

SethTisue · 2023-04-24T18:23:35Z

Since we're so close to release, having two separate PRs makes me uncomfortable as well.

If there are two PRs, we can only merge the first one if it would be acceptable (if not ideal) to release even if the second PR has to be pushed to 2.13.12.

But surely it makes the most sense to either include both, or neither? I think it's confusing to users to ship partial support. (Som, I understand that your intention is not to ship partial support, but regardless, I think being this close to release time forces us to be more careful than we might otherwise be.)

som-snytt · 2023-04-24T19:17:13Z

Reverted to Aug 10, when flag was not set.

som-snytt · 2023-04-24T20:47:23Z

The flaky test

but also

Accept (non-)sealed in Java sources. Defer setting sealed from source to follow-up PR, which will populate child classes lazily.

references scala#10105

SethTisue · 2023-04-27T02:25:55Z

a bit of followup at #10383

som-snytt · 2023-04-27T03:51:22Z

@SethTisue from now on, I'm only going to use the word "support" in the sense of "moral support".

### 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.11 Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced by scala/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#10105 There are 2 known issues in this version: - scala/bug#12800 - scala/bug#12799 For 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 just https://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130 in 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 themselves The 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 passed Java 8 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14337279564 Java 17 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14343012195 Closes #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.11 Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced by scala/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#10105 There are 2 known issues in this version: - scala/bug#12800 - scala/bug#12799 For 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 just https://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130 in 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 themselves The 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 passed Java 8 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14337279564 Java 17 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14343012195 Closes apache#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 re-upgrade Scala to 2.13.11, after SPARK-45144 was merged, the build issues mentioned in #41943 should no longer exist. - https://www.scala-lang.org/news/2.13.11 Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced by scala/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#10105 There are 2 known issues in this version: - scala/bug#12800 - scala/bug#12799 For 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 just https://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130 in 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 themselves The 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? No Closes #42918 from LuciferYang/SPARK-40497-2. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

scala-jenkins added this to the 2.13.10 milestone Aug 6, 2022

som-snytt force-pushed the issue/12159-java-sealed branch from 0cf154b to da5ed4f Compare August 6, 2022 21:07

som-snytt marked this pull request as ready for review August 6, 2022 23:10

SethTisue modified the milestones: 2.13.10, 2.13.9 Aug 10, 2022

SethTisue added the release-notes worth highlighting in next release notes label Aug 10, 2022

SethTisue changed the title ~~Accept (non-)sealed in Java~~ Support JDK 17 Aug 10, 2022

SethTisue changed the title ~~Support JDK 17~~ Support JDK 17's sealed feature in Java sources and Java-generated classfiles Aug 10, 2022

lrytz reviewed Aug 10, 2022

View reviewed changes

src/reflect/scala/reflect/internal/StdNames.scala Show resolved Hide resolved

src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala Outdated Show resolved Hide resolved

src/compiler/scala/tools/nsc/javac/JavaParsers.scala Show resolved Hide resolved

SethTisue modified the milestones: 2.13.9, 2.13.10 Sep 1, 2022

lrytz modified the milestones: 2.13.10, 2.13.11 Sep 26, 2022

som-snytt force-pushed the issue/12159-java-sealed branch from 0710c7b to 20e7535 Compare October 31, 2022 02:43

som-snytt marked this pull request as draft October 31, 2022 09:18

som-snytt force-pushed the issue/12159-java-sealed branch from 20e7535 to 62288fd Compare February 11, 2023 21:00

som-snytt marked this pull request as ready for review February 11, 2023 21:12

som-snytt force-pushed the issue/12159-java-sealed branch from 62288fd to 7bba454 Compare February 15, 2023 00:09

SethTisue force-pushed the issue/12159-java-sealed branch from 7bba454 to a4ae8ef Compare February 20, 2023 20:28

SethTisue self-assigned this Feb 20, 2023

som-snytt force-pushed the issue/12159-java-sealed branch from 42bf9bb to 3afe1a7 Compare April 20, 2023 18:02

som-snytt force-pushed the issue/12159-java-sealed branch from 3afe1a7 to 80ee36a Compare April 24, 2023 19:12

som-snytt changed the title ~~Support JDK 17's sealed feature in Java sources and Java-generated classfiles~~ Support JDK 17's sealed feature in Java-generated classfiles Apr 24, 2023

som-snytt changed the title ~~Support JDK 17's sealed feature in Java-generated classfiles~~ Support JDK 17 PermittedSubclasses in classfiles Apr 24, 2023

Honor PermittedSubclasses

5849a58

Accept (non-)sealed in Java sources. Defer setting sealed from source to follow-up PR, which will populate child classes lazily.

som-snytt force-pushed the issue/12159-java-sealed branch from 80ee36a to 5849a58 Compare April 24, 2023 21:21

som-snytt mentioned this pull request Apr 24, 2023

The less famous flaky test scala/scala-dev#840

Open

som-snytt merged commit ab8feac into scala:2.13.x Apr 25, 2023
3 checks passed

som-snytt deleted the issue/12159-java-sealed branch April 25, 2023 00:37

SethTisue removed the prio:hi high priority (used only by core team, only near release time) label Apr 27, 2023

SethTisue mentioned this pull request Apr 27, 2023

adjust tests on JDK 17+ for PermittedSubclasses change #10383

Merged

SethTisue added a commit to SethTisue/scala that referenced this pull request Apr 27, 2023

adjust tests on JDK 17+ for PermittedSubclasses change

727be50

references scala#10105

michelou mentioned this pull request May 3, 2023

Scala 2.13.11 draft release notes scala/scala-dev#843

Closed

SethTisue mentioned this pull request Jun 6, 2023

Exhaustivity checker emits spurious warning when matching on Java enum type (2.13.11 regression) scala/bug#12800

Closed

LuciferYang mentioned this pull request Jun 19, 2023

[SPARK-40497][BUILD] Upgrade Scala to 2.13.11 apache/spark#41626

Closed

LuciferYang mentioned this pull request Sep 14, 2023

[SPARK-40497][BUILD] Re-upgrade Scala to 2.13.11 apache/spark#42918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support JDK 17 `PermittedSubclasses` in classfiles #10105

Support JDK 17 `PermittedSubclasses` in classfiles #10105

som-snytt commented Aug 6, 2022 •

edited

SethTisue commented Aug 10, 2022 •

edited

lrytz left a comment •

edited by SethTisue

SethTisue commented Oct 26, 2022

som-snytt commented Oct 26, 2022

SethTisue commented Oct 26, 2022

som-snytt commented Oct 26, 2022

som-snytt commented Oct 31, 2022

SethTisue commented Jan 31, 2023

som-snytt commented Jan 31, 2023

SethTisue commented Feb 17, 2023

SethTisue commented Feb 17, 2023

SethTisue commented Mar 29, 2023 •

edited

som-snytt commented Mar 30, 2023

som-snytt commented Apr 20, 2023

preveen-stack commented Apr 22, 2023 •

edited

lrytz commented Apr 24, 2023

SethTisue commented Apr 24, 2023 •

edited

som-snytt commented Apr 24, 2023

som-snytt commented Apr 24, 2023

SethTisue commented Apr 27, 2023

som-snytt commented Apr 27, 2023

Support JDK 17 PermittedSubclasses in classfiles #10105

Support JDK 17 PermittedSubclasses in classfiles #10105

Conversation

som-snytt commented Aug 6, 2022 • edited

SethTisue commented Aug 10, 2022 • edited

lrytz left a comment • edited by SethTisue

Choose a reason for hiding this comment

SethTisue commented Oct 26, 2022

som-snytt commented Oct 26, 2022

SethTisue commented Oct 26, 2022

som-snytt commented Oct 26, 2022

som-snytt commented Oct 31, 2022

SethTisue commented Jan 31, 2023

som-snytt commented Jan 31, 2023

SethTisue commented Feb 17, 2023

SethTisue commented Feb 17, 2023

SethTisue commented Mar 29, 2023 • edited

som-snytt commented Mar 30, 2023

som-snytt commented Apr 20, 2023

preveen-stack commented Apr 22, 2023 • edited

lrytz commented Apr 24, 2023

SethTisue commented Apr 24, 2023 • edited

som-snytt commented Apr 24, 2023

som-snytt commented Apr 24, 2023

SethTisue commented Apr 27, 2023

som-snytt commented Apr 27, 2023

Support JDK 17 `PermittedSubclasses` in classfiles #10105

Support JDK 17 `PermittedSubclasses` in classfiles #10105

som-snytt commented Aug 6, 2022 •

edited

SethTisue commented Aug 10, 2022 •

edited

lrytz left a comment •

edited by SethTisue

SethTisue commented Mar 29, 2023 •

edited

preveen-stack commented Apr 22, 2023 •

edited

SethTisue commented Apr 24, 2023 •

edited