Support JDK 17 `PermittedSubclasses` in classfiles by som-snytt · Pull Request #10105 · scala/scala

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:

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>

references scala#10105

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 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 most infamous flaky test scala/scala-dev#840

Open

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

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

iuliand-db pushed a commit to databricks/scala that referenced this pull request Dec 16, 2025

adjust tests on JDK 17+ for PermittedSubclasses change

2ef0961

references scala#10105

iuliand-db mentioned this pull request Dec 16, 2025

[PLAT-149873] Backport Java sealed support to 2.12.20 databricks/scala#4

Merged

iuliand-db pushed a commit to databricks/scala that referenced this pull request Dec 30, 2025

adjust tests on JDK 17+ for PermittedSubclasses change

b2e9888

references scala#10105

Conversation

som-snytt commented Aug 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz left a comment • edited by SethTisue Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SethTisue commented Oct 26, 2022

Uh oh!

som-snytt commented Oct 26, 2022

Uh oh!

SethTisue commented Oct 26, 2022

Uh oh!

som-snytt commented Oct 26, 2022

Uh oh!

som-snytt commented Oct 31, 2022

Uh oh!

SethTisue commented Jan 31, 2023

Uh oh!

som-snytt commented Jan 31, 2023

Uh oh!

SethTisue commented Feb 17, 2023

Uh oh!

SethTisue commented Feb 17, 2023

Uh oh!

preveen-stack commented Apr 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Apr 24, 2023

Uh oh!

SethTisue commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Apr 24, 2023

Uh oh!

som-snytt commented Apr 24, 2023

Uh oh!

SethTisue commented Apr 27, 2023

Uh oh!

som-snytt commented Apr 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

som-snytt commented Aug 6, 2022 •

edited

Loading

SethTisue commented Aug 10, 2022 •

edited

Loading

lrytz left a comment •

edited by SethTisue

Loading

preveen-stack commented Apr 22, 2023 •

edited

Loading

SethTisue commented Apr 24, 2023 •

edited

Loading