8289196: Pattern domination not working properly for record patterns #84

lahodaj · 2022-06-28T13:11:25Z

When checking that a pattern in a switch case is not dominated by a preceding pattern, handling for record pattern is mostly missing. This patch is attempting to fix that, and as it requires a bit more code, it moves the check into Check.

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-8289196: Pattern domination not working properly for record patterns

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to 220fcc12

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 84

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/84.diff

bridgekeeper · 2022-06-28T13:12:24Z

👋 Welcome back jlahoda! 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 · 2022-06-28T13:14:49Z

@lahodaj 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 · 2022-06-28T13:19:15Z

Webrevs

vicente-romero-oracle · 2022-07-01T16:28:34Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java

+                return false;
+            }
+            if (existingPatternType.isPrimitive()) {
+                if (!types.isSameType(existingPatternType, currentPatternType)) {


note: shouldn't we return true if both have the same type? I mean this method seems to returns the right thing in that case but it is doing some extra analysis that doesn't seem to be necessary IMO

We could, but I am not sure if that's not "too clever". Returning true when the type is same is based on the fact that only binding patterns (type test patterns) can be of primitive types, so the tests below will always return true. I can do that if strongly preferred, but I somewhat like the separation of concerns, where we verify the types here, and then the structure is checked below, regardless of the pattern's type.

vicente-romero-oracle · 2022-07-01T16:43:29Z

nit: we could probably add this test case for completeness:

class Test {
    int test() {
        record R(int a) {}
        Object o = null;
        switch (o) {
            case R(int a) when true: return -1;
            case R(int a): return 1;
        }
    }
}

lahodaj · 2022-07-01T18:07:45Z

nit: we could probably add this test case for completeness:

class Test {
    int test() {
        record R(int a) {}
        Object o = null;
        switch (o) {
            case R(int a) when true: return -1;
            case R(int a): return 1;
        }
    }
}

Thanks, done.

vicente-romero-oracle

lgtm

openjdk · 2022-07-07T07:42:53Z

@lahodaj 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:

8289196: Pattern domination not working properly for record patterns

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 34 new commits pushed to the master branch:

8f24d25: 6509045: {@inheritdoc} only copies one instance of the specified exception
9a0fa82: 8288949: serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java failing
55fa19b: 8289857: ProblemList jdk/jfr/event/runtime/TestActiveSettingEvent.java
32b650c: 8289840: ProblemList vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java when run with vthread wrapper
ef3f2ed: 8289841: ProblemList vmTestbase/gc/gctests/MemoryEaterMT/MemoryEaterMT.java with ZGC on windows
0526402: 8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc
b3a0e48: 8289439: Clarify relationship between ThreadStart/ThreadEnd and can_support_virtual_threads capability
0b6fd48: 8288128: S390X: Fix crashes after JDK-8284161 (Virtual Threads)
30e134e: 8289091: move oop safety check from SharedRuntime::get_java_tid() to JavaThread::threadObj()
29ea642: 8287847: Fatal Error when suspending virtual thread after it has terminated
... and 24 more: https://git.openjdk.org/jdk19/compare/adbd200dc59c31ad582b51468ec39af5a64db664...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.

lahodaj · 2022-07-07T07:53:24Z

/integrate

openjdk · 2022-07-07T07:54:27Z

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

8f24d25: 6509045: {@inheritdoc} only copies one instance of the specified exception
9a0fa82: 8288949: serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java failing
55fa19b: 8289857: ProblemList jdk/jfr/event/runtime/TestActiveSettingEvent.java
32b650c: 8289840: ProblemList vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java when run with vthread wrapper
ef3f2ed: 8289841: ProblemList vmTestbase/gc/gctests/MemoryEaterMT/MemoryEaterMT.java with ZGC on windows
0526402: 8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc
b3a0e48: 8289439: Clarify relationship between ThreadStart/ThreadEnd and can_support_virtual_threads capability
0b6fd48: 8288128: S390X: Fix crashes after JDK-8284161 (Virtual Threads)
30e134e: 8289091: move oop safety check from SharedRuntime::get_java_tid() to JavaThread::threadObj()
29ea642: 8287847: Fatal Error when suspending virtual thread after it has terminated
... and 24 more: https://git.openjdk.org/jdk19/compare/adbd200dc59c31ad582b51468ec39af5a64db664...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-07T07:54:45Z

@lahodaj Pushed as commit 8dd94a2.

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

8289196: Patttern domination not working property for record patterns

9d22eaf

openjdk bot added the rfr Pull request is ready for review label Jun 28, 2022

openjdk bot added the compiler compiler-dev@openjdk.org label Jun 28, 2022

vicente-romero-oracle reviewed Jul 1, 2022

View reviewed changes

Adding test, as suggested.

220fcc1

vicente-romero-oracle approved these changes Jul 1, 2022

View reviewed changes

Simplifying primitive pattern handling as suggested

bf740ed

lahodaj changed the title ~~8289196: Patttern domination not working property for record patterns~~ 8289196: Pattern domination not working properly for record patterns Jul 7, 2022

openjdk bot added the ready Pull request is ready to be integrated label Jul 7, 2022

openjdk bot added the integrated Pull request has been integrated label Jul 7, 2022

openjdk bot closed this Jul 7, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8289196: Pattern domination not working properly for record patterns #84

8289196: Pattern domination not working properly for record patterns #84

lahodaj commented Jun 28, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jun 28, 2022

openjdk bot commented Jun 28, 2022

mlbridge bot commented Jun 28, 2022 •

edited

vicente-romero-oracle Jul 1, 2022 •

edited

lahodaj Jul 1, 2022

vicente-romero-oracle commented Jul 1, 2022 •

edited

lahodaj commented Jul 1, 2022

vicente-romero-oracle left a comment

openjdk bot commented Jul 7, 2022

lahodaj commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

8289196: Pattern domination not working properly for record patterns #84

8289196: Pattern domination not working properly for record patterns #84

Conversation

lahodaj commented Jun 28, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 28, 2022

openjdk bot commented Jun 28, 2022

mlbridge bot commented Jun 28, 2022 • edited

Webrevs

vicente-romero-oracle Jul 1, 2022 • edited

Choose a reason for hiding this comment

lahodaj Jul 1, 2022

Choose a reason for hiding this comment

vicente-romero-oracle commented Jul 1, 2022 • edited

lahodaj commented Jul 1, 2022

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 7, 2022

lahodaj commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

lahodaj commented Jun 28, 2022 •

edited by openjdk bot

mlbridge bot commented Jun 28, 2022 •

edited

vicente-romero-oracle Jul 1, 2022 •

edited

vicente-romero-oracle commented Jul 1, 2022 •

edited