8332463: Byte conditional pattern case element dominates short constant case element

[biboudis]

It seems that the compiler introduced a rule that does not exist in the spec. The fix is simple, and it will fix the behaviour of JDK 23 according to the spec. For example the following is accepted by JDK 22 and needs to continue to be accepted by JDK 23:

public static int test() {
    Byte i = (byte) 42;
    return switch (i) {
        case Byte ib   -> 1;
        case (short) 0 -> 2; // OK - not dominated
    };
}

Similarly for primitive type patterns:

public static int test() {
    Byte i = (byte) 42;
    return switch (i) {
        case Byte ib  -> 1;
        case short s  -> 2; // Also not dominated since there is no unconditionally exact conversion from short to Byte
    };
}

public static int test() {
    int i = 42;
    return switch (i) {
        case Integer ib -> 1;
        case byte ip    -> 2; // Also not dominated since there is no unconditionally exact conversion from byte to Integer
    };
}

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8332463: Byte conditional pattern case element dominates short constant case element (Bug - P3)

Reviewers

• Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19301/head:pull/19301
$ git checkout pull/19301

Update a local copy of the PR:
$ git checkout pull/19301
$ git pull https://git.openjdk.org/jdk.git pull/19301/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19301

View PR using the GUI difftool:
$ git pr show -t 19301

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19301.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back abimpoudis! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@biboudis This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8332463: Byte conditional pattern case element dominates short constant case element

Reviewed-by: vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 88 new commits pushed to the master branch:

• d59c12f: 8329718: Incorrect @since tags in elements in jdk.compiler and java.compiler
• b4d1454: 8332740: [BACKOUT] JDK-8331081 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• 37c4778: 8332096: hotspot-ide-project fails with this-escape
• 2170e99: 8331081: 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• a0c5714: 8332071: Convert package.html files in java.management.rmi to package-info.java
• afed7d0: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic
• 9ca90cc: 8332610: Remove unused nWakeups in ObjectMonitor
• 92d3350: 8331920: ubsan: g1CardSetContainers.inline.hpp:266:5: runtime error: index 2 out of bounds for type 'G1CardSetHowl::ContainerPtr [2]' reported
• 4f1a10f: 8332360: JVM hangs at exit when running on a uniprocessor
• c3bc23f: 8326306: RISC-V: Re-structure MASM calls and jumps
• ... and 78 more: https://git.openjdk.org/jdk/compare/2f10a316ff0c5a4c124b94f6fabb38fb119d2c82...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@biboudis The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (8576b3d7)

[mlbridge]

Mailing list message from David Alayachew on compiler-dev:

I understand the first 2 cases, but not the third. How is byte -> Integer
not unconditionally exact? What possible scenario could occur where one
would lose data?

On Mon, May 20, 2024, 4:31?AM Aggelos Biboudis <abimpoudis at openjdk.org>
wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20240520/ab63dd3d/attachment.htm>

[mlbridge]

Mailing list message from Angelos Bimpoudis on compiler-dev:

The reason is that according to Casting Contexts/Section 5.5 (or Testing Contexts in JDK 23/Section 5.7 which is very similar to casting contexts) not only it is not unconditionally exact but there is no conversion at all that can support byte to Integer. If there were one, we would see a bullet that says: a widening primitive conversion followed by boxing.

The permitted conversions are the ones listed in Chapter 5.5 or 5.7 in the accompanying spec diff ? https://cr.openjdk.org/~abimpoudis/instanceof/jep455-20240424/specs/instanceof-jls.html#jls-5.7.

This is also evident by trying to do the cast in JShell:

jshell> byte b = (byte) 42
b ==> 42

jshell> (Integer) b
| Error:
| incompatible types: byte cannot be converted to java.lang.Integer
| (Integer) b
| ^

Note, that while dominance checks the relation between two case labels, those two case labels can be independently applicable to the selector type.
________________________________
From: David Alayachew <davidalayachew at gmail.com>
Sent: 21 May 2024 00:12
To: Aggelos Biboudis <abimpoudis at openjdk.org>
Cc: compiler-dev <compiler-dev at openjdk.org>
Subject: Re: RFR: 8332463: Byte conditional pattern case element dominates short constant case element

I understand the first 2 cases, but not the third. How is byte -> Integer not unconditionally exact? What possible scenario could occur where one would lose data?

On Mon, May 20, 2024, 4:31?AM Aggelos Biboudis <abimpoudis at openjdk.org<mailto:abimpoudis at openjdk.org>> wrote:
It seems that the compiler introduced a rule that does not exist in the spec. The fix is simple, and it will fix the behaviour of JDK 23 according to the spec. For example the following is accepted by JDK 22 and needs to continue to be accepted by JDK 23:

public static int test() {
Byte i = (byte) 42;
return switch (i) {
case Byte ib -> 1;
case (short) 0 -> 2; // OK - not dominated
};
}

Similarly for primitive type patterns:

public static int test() {
Byte i = (byte) 42;
return switch (i) {
case Byte ib -> 1;
case short s -> 2; // Also not dominated since there is no unconditionally exact conversion from short to Byte
};
}

public static int test() {
int i = 42;
return switch (i) {
case Integer ib -> 1;
case byte ip -> 2; // Also not dominated since there is no unconditionally exact conversion from byte to Integer
};
}

-------------

Commit messages:
- 8332463: Byte conditional pattern case element dominates short constant case element

Changes: https://git.openjdk.org/jdk/pull/19301/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19301&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8332463
Stats: 104 lines in 5 files changed: 94 ins; 7 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/19301.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19301/head:pull/19301

PR: https://git.openjdk.org/jdk/pull/19301
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20240521/27fe1139/attachment-0001.htm>

[mlbridge]

Mailing list message from David Alayachew on compiler-dev:

Then maybe I am not following along here.

The link you sent says this.

a boxing conversion followed by a widening reference conversion (5.1.5

<https://docs.oracle.com/javase/specs/jls/se22/html/jls-5.html#jls-5.1.5>)

Isn't what I described just an abbreviated version of this casting context?

Namely, byte --> Byte --> Integer

And even if not (I would like to know why not), my other question wasn't
answered -- why not allow this?

Specifically, I cannot imagine a single value of byte that could not be
representable as Integer.

I guess, what does this lack of ability grant us that we would lose if we
made it possible? To go straight from byte to Integer, I mean.

On Tue, May 21, 2024, 5:10?AM Angelos Bimpoudis <
angelos.bimpoudis at oracle.com> wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20240521/3d852729/attachment-0001.htm>

[biboudis]

While byte --> Byte is boxing indeed, Byte --> Integer is not a widening reference conversion. From the spec:

A widening reference conversion exists from any reference type S to any reference type T, provided S is a subtype of T (§4.10).

and

A class or interface is disjoint from another class or interface if it can be determined statically that they have no instances in common (other than the null value)

The two reference types in the second conversion are disjoint. Evident that you cannot even ask if a Byte is instanceof Integer.

jshell> Byte b = 42
b ==> 42

jshell> b instanceof Integer
|  Error:
|  incompatible types: java.lang.Byte cannot be converted to java.lang.Integer
|  b instanceof Integer
|  ^

What you are really asking is whether or not Byte can be converted to Integer since we know that both can be null and also all possible reference values of the first also belong to the domain of the second. So actually maybe null is the only problematic value (because it witnesses that both types are reference types)? Today we manage conversions by those tables in Chapter 5. With the arrival of valhalla we will need to think what place the conversion of Byte! to Integer! has. (a Byte will be a null-widened Byte!.

[vicente-romero-oracle]

lgtm

[biboudis]

/integrate

[openjdk]

Going to push as commit c4557a7.
Since your change was applied there have been 88 commits pushed to the master branch:

• d59c12f: 8329718: Incorrect @since tags in elements in jdk.compiler and java.compiler
• b4d1454: 8332740: [BACKOUT] JDK-8331081 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• 37c4778: 8332096: hotspot-ide-project fails with this-escape
• 2170e99: 8331081: 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• a0c5714: 8332071: Convert package.html files in java.management.rmi to package-info.java
• afed7d0: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic
• 9ca90cc: 8332610: Remove unused nWakeups in ObjectMonitor
• 92d3350: 8331920: ubsan: g1CardSetContainers.inline.hpp:266:5: runtime error: index 2 out of bounds for type 'G1CardSetHowl::ContainerPtr [2]' reported
• 4f1a10f: 8332360: JVM hangs at exit when running on a uniprocessor
• c3bc23f: 8326306: RISC-V: Re-structure MASM calls and jumps
• ... and 78 more: https://git.openjdk.org/jdk/compare/2f10a316ff0c5a4c124b94f6fabb38fb119d2c82...master

Your commit was automatically rebased without conflicts.

[openjdk]

@biboudis Pushed as commit c4557a7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from David Alayachew on compiler-dev:

Got it, ty vm.

I had assumed that Byte to Integer was already implemented logic. I had
heard that discussion in Valhalla many times.

Ok, makes perfect sense now. So, when Valhalla reevaluates the relationship
between the various wrapper classes, this might change?

On Wed, May 22, 2024 at 5:41?AM Aggelos Biboudis <abimpoudis at openjdk.org>
wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20240522/784916d7/attachment.htm>