Support Java 17 `sealed` in Java sources by som-snytt · Pull Request #10348 · scala/scala

som-snytt · 2023-03-15T21:02:48Z

Support sealed in Java sources. The related PR supports separate compilation, where PermittedSubclasses attribute specifies sealedness; there seems to be no ticket for that, alas. This PR is Java interop gravy.

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

SethTisue · 2023-03-29T07:40:07Z

@lrytz tentatively milestoned for 2.13.11 since it address a concern you raised on #10105

src/compiler/scala/tools/nsc/typechecker/Namers.scala

som-snytt · 2023-04-01T09:27:55Z

The updated commit just "registers" Java classes and matches for completion after typer, where (per lrytz) either there is a permits or we look at classes from the same source file.

The simple impl could be improved or complexified further. Or simplified.

lrytz · 2023-04-03T10:09:10Z

Thanks, great to see that we can make it work!

Why do you think we should delay until after typer? Forcing is what the typer does, I don't see a particular risk of running into cycles here, we don't need to infer anything to complete these class symbols.

For finding symbols defined in a particular compilation unit, I also think we need new global state, there doesn't seem to be anything we can reuse. A question is where to put it, I guess it doesn't matter too much. Here's a draft, what do you think (last commit)? 2.13.x...lrytz:scala:pr10348

som-snytt · 2023-04-03T10:25:26Z

Yes, that is what I was thinking of (a perRunCaches Map). I thought keeping the logic in one place was simpler, "children we know about" and "other offspring". Oh I see what you mean, we've already seen all the classes for this unit, nothing is gained by waiting.

The other piece I started looking at was emitting the permittedSubclasses attribute for Scala sealed. I'll try to add a commit for that, I was about to imitate some structure for innerClasses.

A final clean-up is also to review the tests for duplication or completeness.

Also, this is a patmat feature, so I like that it looks that way by putting the unwieldy sources map there.

som-snytt · 2023-04-03T10:41:35Z

I added your commits and will push after cleaning up my detritus.

Also I haven't touched the java parser to look more like your earlier edit, which I remember preferring.

som-snytt · 2023-04-03T11:02:41Z

src/compiler/scala/tools/nsc/typechecker/Namers.scala

      val pluginsTp = pluginsTypeSig(res, typer, cdef, WildcardType)
+      cdef.getAndRemoveAttachment[PermittedSubclasses].foreach { permitted =>
+        clazz.updateAttachment[PermittedSubclassSymbols] {
+          PermittedSubclassSymbols(permitted.permits.map(typer.typed(_, Mode.NOmode).symbol))


what happens to errors on bad permits clause here?

src/compiler/scala/tools/nsc/typechecker/Typers.scala

src/partest/scala/tools/partest/nest/AbstractRunner.scala

lrytz

We forgot about inner classes...

package p;
public sealed interface X {
  public default int x() { return 1; }
}
final class Y implements X { }
final class O {
  final static class Z implements X { }
}

package p
class T {
  def n(a: X) = a match { case _: Y => 0 }
}

Should warn but doesn't in mixed compilation. Namer eagerly greates symbols for top-level classes. But symbols for inner classes are only created when the outer class completes.

So if there's a sealed Java type without a permits clause, we probably have to recursively force all classes defined in the same compilation unit.

X.java:9: error: anonymous classes must not extend sealed classes
    X bar = new X() { };
                    ^

😅 OK, at least that

@retronym brought runtime reflection to the table:

scala> import scala.reflect.runtime.{universe => ru}
import scala.reflect.runtime.{universe=>ru}

scala> ru.typeTag[p.X].tpe.typeSymbol.asClass.knownDirectSubclasses
val res0: Set[reflect.runtime.universe.Symbol] = Set()

Also macros / compiler plugins might want to see children, not only the exhaustivity checker. But I think it's fine to defer that.

src/compiler/scala/tools/nsc/transform/patmat/PatternMatching.scala

som-snytt · 2023-04-26T20:33:59Z

@lrytz this will be green after squashing. I guess "zucchini", as a green squash.

I'll take another look at the tweak to populating the sealed class hierarchy. It needs tests with non-sealed descendants.

Required for checking permitted subclasses and for exhaustiveness of patterns. Co-authored-by: Lukas Rytz <lukas.rytz@gmail.com>

lrytz

🥒 ed

LGTM, thank you!

### 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.12 milestone Mar 15, 2023

som-snytt mentioned this pull request Mar 15, 2023

Support JDK 17 PermittedSubclasses in classfiles #10105

Merged

SethTisue modified the milestones: 2.13.12, 2.13.11 Mar 29, 2023

SethTisue added the prio:hi high priority (used only by core team, only near release time) label Mar 29, 2023

SethTisue requested a review from lrytz March 29, 2023 07:39

som-snytt marked this pull request as ready for review March 29, 2023 07:40

lrytz force-pushed the followup/12159-mixed-java-sealed branch from f2a167c to 8751aeb Compare March 29, 2023 07:54

lrytz reviewed Mar 29, 2023

View reviewed changes

src/compiler/scala/tools/nsc/typechecker/Namers.scala Outdated Show resolved Hide resolved

som-snytt force-pushed the followup/12159-mixed-java-sealed branch from 8751aeb to eeac512 Compare April 1, 2023 09:20

som-snytt changed the title ~~Improve exhaustiveness check for mixed java sealed~~ Improve exhaustiveness check for mixed java sealed [ci: last-only] Apr 1, 2023

som-snytt marked this pull request as draft April 1, 2023 10:01

som-snytt force-pushed the followup/12159-mixed-java-sealed branch from eeac512 to 35c0384 Compare April 2, 2023 04:53

som-snytt force-pushed the followup/12159-mixed-java-sealed branch from 35c0384 to a646610 Compare April 3, 2023 10:52

som-snytt commented Apr 3, 2023

View reviewed changes

src/compiler/scala/tools/nsc/typechecker/Typers.scala Show resolved Hide resolved

som-snytt commented Apr 3, 2023

View reviewed changes

src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved

som-snytt commented Apr 3, 2023

View reviewed changes

src/partest/scala/tools/partest/nest/AbstractRunner.scala Show resolved Hide resolved

som-snytt marked this pull request as ready for review April 3, 2023 11:57

lrytz reviewed Apr 4, 2023

View reviewed changes

src/compiler/scala/tools/nsc/transform/patmat/PatternMatching.scala Show resolved Hide resolved

som-snytt changed the title ~~Improve exhaustiveness check for mixed java sealed [ci: last-only]~~ Support Java 17 sealed in Java sources [ci: last-only] Apr 24, 2023

som-snytt force-pushed the followup/12159-mixed-java-sealed branch 2 times, most recently from 2bf8131 to 62c5acf Compare April 25, 2023 00:48

som-snytt changed the title ~~Support Java 17 sealed in Java sources [ci: last-only]~~ Support Java 17 sealed in Java sources Apr 25, 2023

Populate children of Java sealed hierarchy

a0f546d

Required for checking permitted subclasses and for exhaustiveness of patterns. Co-authored-by: Lukas Rytz <lukas.rytz@gmail.com>

lrytz force-pushed the followup/12159-mixed-java-sealed branch from 9c3e87d to a0f546d Compare May 1, 2023 09:20

lrytz approved these changes May 1, 2023

View reviewed changes

lrytz merged commit bd9b10a into scala:2.13.x May 1, 2023

lrytz mentioned this pull request May 1, 2023

Restore test fixed by recent Java-sealed PR #10386

Merged

som-snytt deleted the followup/12159-mixed-java-sealed branch May 1, 2023 14:32

SethTisue added the release-notes worth highlighting in next release notes label May 1, 2023

michelou mentioned this pull request May 3, 2023

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

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 mentioned this pull request Dec 16, 2025

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Java 17 `sealed` in Java sources#10348

Support Java 17 `sealed` in Java sources#10348
lrytz merged 1 commit intoscala:2.13.xfrom
som-snytt:followup/12159-mixed-java-sealed

som-snytt commented Mar 15, 2023 •

edited

Loading

Uh oh!

SethTisue commented Mar 29, 2023

Uh oh!

Uh oh!

som-snytt commented Apr 1, 2023 •

edited

Loading

Uh oh!

lrytz commented Apr 3, 2023

Uh oh!

som-snytt commented Apr 3, 2023 •

edited

Loading

Uh oh!

som-snytt commented Apr 3, 2023 •

edited

Loading

Uh oh!

som-snytt Apr 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrytz left a comment

Uh oh!

Uh oh!

som-snytt commented Apr 26, 2023

Uh oh!

lrytz left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

som-snytt commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Mar 29, 2023

Uh oh!

Uh oh!

som-snytt commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Apr 3, 2023

Uh oh!

som-snytt commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

som-snytt commented Apr 26, 2023

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

som-snytt commented Mar 15, 2023 •

edited

Loading

som-snytt commented Apr 1, 2023 •

edited

Loading

som-snytt commented Apr 3, 2023 •

edited

Loading

som-snytt commented Apr 3, 2023 •

edited

Loading