Avoid brittle flags encoding for Java enums #10424

som-snytt · 2023-06-07T19:02:01Z

In the current scheme, an enum should be sealed abstract and have only its elements as "children".

If an enum element has a body, then permittedSubclasses includes the element subclass.

The two tweaks are: don't add those permittedSubclasses as children; set sealed abstract unconditionally (on parsing an enum field, where the enum is already marked sealed).

The symptoms were that patmat would warn about MyEnum$1 (where the anon subclass is a child) or MyEnum forSome not in X,Y,Z (if the enum is not marked abstract).

Second commit expunges the kludge of making enum always abstract just for exhaustivity. Instead, patmat analysis checks for an enum class (isSealedEnum). Class file parser assumes only present flags, except ACC_ENUM is translated to include SEALED for classes. Java parser checks enum members for a deferred member, and sets ABSTRACT accordingly.

SethTisue · 2023-06-10T01:18:11Z

Second commit expunges the kludge of making enum always abstract just for exhaustivity

Is that kludge something we've had since forever, or something that was added recently?

I guess I'm asking partly out of curiosity and partly because I'm wondering what the risk level is of making a code improvement that isn't strictly necessary to fix bug (right?).

In other words, my "shouldn't this be two PRs" finger is twitching slightly, but my "Seth don't be pedantic" finger is twitching in the direction of the first finger, if the metaphor isn't getting too tortured.

SethTisue · 2023-06-10T01:23:01Z

To overthink a little further, I guess whether I care about one-PR-or-two depends on whether 2.13.12 ends up being a longer-incubating, optimistically-merge-things type release (as 2.13.11 was), or whether it ends up being an "oh shit we need to roll another one pronto" type release (as 2.13.10 was). And we don't know that yet.

som-snytt · 2023-06-10T01:25:10Z

@SethTisue I expect @lrytz to weigh in on the second commit, hopefully with a ringing endorsement. Otherwise it can be moved to a separate PR as an internal improvement. (It's an old state of affairs, but I think it turns out to be low-hanging fruit. If I'd been at the recent spree, I would have worn my "low-hanging fruit" t-shirt.)

lrytz

LGTM, both commits, just the isSealedEnum extension method seems a bit of an overkill.

src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala

lrytz · 2023-06-19T14:12:14Z

src/reflect/scala/reflect/internal/ClassfileConstants.scala

@@ -353,7 +353,7 @@ object ClassfileConstants {
      case JAVA_ACC_STATIC     => STATIC
      case JAVA_ACC_ABSTRACT   => if (isClass) ABSTRACT else DEFERRED
      case JAVA_ACC_INTERFACE  => TRAIT | INTERFACE | ABSTRACT
-      case JAVA_ACC_ENUM       => JAVA_ENUM
+      case JAVA_ACC_ENUM       => if (isClass) JAVA_ENUM | SEALED else JAVA_ENUM


Here I don't follow, when is isClass true, when is it false?

The enum member fields get the flag. (I believe I did not imagine that.)

som-snytt · 2023-06-19T15:26:55Z

Updated with two suggestions. (Edit: and rebased)

In the beginning, I expected to be calling isBlahBlah all over the place, but I see it wound up as just a tweak to two expressions.

That is not in proportion to the degree of suffering required.

Also adjusted indentation on the flag translation, so that it fits in my default terminal.

Reluctantly renamed variable abbie to hasAbstractFlag (change not explicitly requested).

I did not squash the two commits, but I did squash the two amendments. Dangerous to do before finishing first coffee! After deleting the package object, it tried to leak back into the second commit. ("How does git actually work again?")

lrytz

Reluctantly renamed variable abbie to hasAbstractFlag (change not explicitly requested).

I like sentimental codebases

### What changes were proposed in this pull request? This pr aims to upgrade Scala from 2.13.11 to 2.13.12. Additionally, this pr adds ``-Wconf:msg=legacy-binding:s`` to suppress similar compiler warnings as below: ``` [ERROR] /Users/yangjie01/SourceCode/git/spark-mine-13/core/src/main/scala/org/apache/spark/deploy/client/StandaloneAppClient.scala:171: reference to stop is ambiguous; it is both defined in the enclosing class StandaloneAppClient and inherited in the enclosing class ClientEndpoint as method stop (defined in trait RpcEndpoint, inherited through parent trait ThreadSafeRpcEndpoint) In Scala 2, symbols inherited from a superclass shadow symbols defined in an outer scope. Such references are ambiguous in Scala 3. To continue using the inherited symbol, write `this.stop`. Or use `-Wconf:msg=legacy-binding:s` to silence this warning. [quickfixable] Applicable -Wconf / nowarn filters for this fatal warning: msg=<part of the message>, cat=other, site=org.apache.spark.deploy.client.StandaloneAppClient.ClientEndpoint.receive ``` ### Why are the changes needed? The new version bring some regression fixes: - scala/scala#10422 - scala/scala#10424 And a new feature: Quickfixes ``` For some errors and warnings, the compiler now suggests an edit that could fix the issue. Tooling such as IDEs can then offer the edits, or the compiler itself will make the change if run again with -quickfix. ``` - scala/scala#10406 - scala/scala#10482 - scala/scala#10484 The release notes as follows: - https://github.com/scala/scala/releases/tag/v2.13.12 ### Does this PR introduce _any_ user-facing change? Yes, Scala version changed. ### How was this patch tested? Pass Github ### Was this patch authored or co-authored using generative AI tooling? NO Closes #43185 from LuciferYang/SPARK-45331. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

This pr aims to upgrade Scala from 2.13.11 to 2.13.12. Additionally, this pr adds ``-Wconf:msg=legacy-binding:s`` to suppress similar compiler warnings as below: ``` [ERROR] /Users/yangjie01/SourceCode/git/spark-mine-13/core/src/main/scala/org/apache/spark/deploy/client/StandaloneAppClient.scala:171: reference to stop is ambiguous; it is both defined in the enclosing class StandaloneAppClient and inherited in the enclosing class ClientEndpoint as method stop (defined in trait RpcEndpoint, inherited through parent trait ThreadSafeRpcEndpoint) In Scala 2, symbols inherited from a superclass shadow symbols defined in an outer scope. Such references are ambiguous in Scala 3. To continue using the inherited symbol, write `this.stop`. Or use `-Wconf:msg=legacy-binding:s` to silence this warning. [quickfixable] Applicable -Wconf / nowarn filters for this fatal warning: msg=<part of the message>, cat=other, site=org.apache.spark.deploy.client.StandaloneAppClient.ClientEndpoint.receive ``` The new version bring some regression fixes: - scala/scala#10422 - scala/scala#10424 And a new feature: Quickfixes ``` For some errors and warnings, the compiler now suggests an edit that could fix the issue. Tooling such as IDEs can then offer the edits, or the compiler itself will make the change if run again with -quickfix. ``` - scala/scala#10406 - scala/scala#10482 - scala/scala#10484 The release notes as follows: - https://github.com/scala/scala/releases/tag/v2.13.12 Yes, Scala version changed. Pass Github NO Closes apache#43185 from LuciferYang/SPARK-45331. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

scala-jenkins added this to the 2.13.12 milestone Jun 7, 2023

som-snytt force-pushed the issue/12800-enum-exhaustivity branch from 3a608df to 88d252f Compare June 7, 2023 23:39

som-snytt changed the title ~~Set enum as abstract always~~ Avoid brittle flags encoding for Java enums Jun 7, 2023

som-snytt marked this pull request as ready for review June 8, 2023 00:27

som-snytt force-pushed the issue/12800-enum-exhaustivity branch from 88d252f to 51ce3e8 Compare June 9, 2023 16:21

SethTisue requested a review from lrytz June 10, 2023 01:08

SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Jun 10, 2023

lrytz reviewed Jun 19, 2023

View reviewed changes

som-snytt force-pushed the issue/12800-enum-exhaustivity branch from 51ce3e8 to 9294fba Compare June 19, 2023 15:19

som-snytt added 2 commits June 19, 2023 08:20

Set enum as abstract always

8d5cb07

Unwind abstract enum kludge

11db53c

som-snytt force-pushed the issue/12800-enum-exhaustivity branch from 9294fba to 11db53c Compare June 19, 2023 15:20

lrytz approved these changes Jun 20, 2023

View reviewed changes

lrytz merged commit 0842f23 into scala:2.13.x Jun 20, 2023

som-snytt deleted the issue/12800-enum-exhaustivity branch June 20, 2023 08:48

SethTisue added release-notes worth highlighting in next release notes and removed prio:blocker release blocker (used only by core team, only near release time) labels Jun 20, 2023

LuciferYang mentioned this pull request Sep 30, 2023

[SPARK-45331][BUILD] Upgrade Scala to 2.13.12 apache/spark#43185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid brittle flags encoding for Java enums #10424

Avoid brittle flags encoding for Java enums #10424

som-snytt commented Jun 7, 2023 •

edited

Loading

SethTisue commented Jun 10, 2023

SethTisue commented Jun 10, 2023 •

edited

Loading

som-snytt commented Jun 10, 2023 •

edited

Loading

lrytz left a comment

lrytz Jun 19, 2023

som-snytt Jun 19, 2023

som-snytt commented Jun 19, 2023 •

edited

Loading

lrytz left a comment

Avoid brittle flags encoding for Java enums #10424

Avoid brittle flags encoding for Java enums #10424

Conversation

som-snytt commented Jun 7, 2023 • edited Loading

SethTisue commented Jun 10, 2023

SethTisue commented Jun 10, 2023 • edited Loading

som-snytt commented Jun 10, 2023 • edited Loading

lrytz left a comment

Choose a reason for hiding this comment

lrytz Jun 19, 2023

Choose a reason for hiding this comment

som-snytt Jun 19, 2023

Choose a reason for hiding this comment

som-snytt commented Jun 19, 2023 • edited Loading

lrytz left a comment

Choose a reason for hiding this comment

som-snytt commented Jun 7, 2023 •

edited

Loading

SethTisue commented Jun 10, 2023 •

edited

Loading

som-snytt commented Jun 10, 2023 •

edited

Loading

som-snytt commented Jun 19, 2023 •

edited

Loading