8322477: order of subclasses in the permits clause can differ between compilations #17284

vicente-romero-oracle · 2024-01-05T17:42:33Z

This is a very interesting issue. Given code like:

sealed interface Sealed {
    record R1() implements Sealed {}
    record R2() implements Sealed {}
}

As we know javac will infer the permits clause of sealed interface Sealed logically the order should correspond to the order in which the permitted subclasses appear in the source code. Well it has been consistently observed by the reported of this bug, that some tools like Gradle while doing incremental compilation can make javac infer either R1, R2 or R2, R1 as permitted subclasses. The reason is not clear still under investigation on their side but the fact is that javac is generating inconsistent output for some classes with this shape. The proposed solution is to store the position of the permitted subclasses being discovered by javac so that the order of the permitted subclasses corresponds to the original order in the source file. Efforts to reduce the project where the issue was discovered to a small reproductor have been unsuccessful but the proposed patch have fixed the issue observed by the reporter.

TIA

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-8322477: order of subclasses in the permits clause can differ between compilations (Bug - P4)

Reviewers

Jan Lahoda (@lahodaj - Reviewer) ⚠️ Review applies to 20ff11bc

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17284

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

Using diff file

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

Webrev

Link to Webrev Comment

… compilations

bridgekeeper · 2024-01-05T17:43:58Z

👋 Welcome back vromero! 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 · 2024-01-05T17:45:27Z

@vicente-romero-oracle 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 · 2024-01-05T17:48:45Z

Webrevs

vicente-romero-oracle · 2024-01-05T17:54:25Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java

@@ -1303,10 +1304,12 @@ public static class ClassSymbol extends TypeSymbol implements TypeElement {
        // sealed classes related fields
        /** The classes, or interfaces, permitted to extend this class, or interface
         */
-        public List<Symbol> permitted;
+        private java.util.List<PermittedClassWithPos> permitted;


some of the changes come only because of the fact that this field is not private. We can keep it public and the code will be simpler but there could be erroneous uses of the field

vicente-romero-oracle · 2024-01-05T17:58:09Z

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

                    log.error(TreeInfo.diagnosticPositionFor(c, env.tree), Errors.SealedClassMustHaveSubclasses);
                }

                if (c.isSealed()) {
                    Set<Symbol> permittedTypes = new HashSet<>();
                    boolean sealedInUnnamed = c.packge().modle == syms.unnamedModule || c.packge().modle == syms.noModule;
-                    for (Symbol subTypeSym : c.permitted) {
+                    for (Type subType : c.getPermittedSubclasses()) {


most changes here are due to the use of getPermittedSubclasses, which is a previously existing API, that returns a list of types.

vicente-romero-oracle · 2024-01-09T22:07:12Z

at the end the swap in the permits clause order can be boiled down to symbol completions that were triggered "before" expected by a call to Symbol::flags but, even if this was the right solution it is not future-proofed as a future change can include a call to Symbol::flags that can alter the expected order of the permits clause. This is why I think that relying on the original positions in the source code as the key for sorting the elements in the permits clause is a most. For example the patch below fixes the issue with current master, but as mentioned above this is not a good solution looking forward:

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
index 342ccb26798..9a939ef44db 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
@@ -4441,7 +4441,7 @@ public void visitSelect(JCFieldAccess tree) {
         if (isType(sitesym)) {
             if (sym.name != names._this && sym.name != names._super) {
                 // Check if type-qualified fields or methods are static (JLS)
-                if ((sym.flags() & STATIC) == 0 &&
+                if ((sym.flags_field & STATIC) == 0 &&
                     sym.name != names._super &&
                     (sym.kind == VAR || sym.kind == MTH)) {
                     rs.accessBase(rs.new StaticError(sym),
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
index f9b228203d4..76a71ed97dc 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
@@ -3814,7 +3814,7 @@ void checkDeprecated(Supplier<DiagnosticPosition> pos, final Symbol other, final
     }
 
     void checkSunAPI(final DiagnosticPosition pos, final Symbol s) {
-        if ((s.flags() & PROPRIETARY) != 0) {
+        if ((s.flags_field & PROPRIETARY) != 0) {
             deferredLintHandler.report(() -> {
                 log.mandatoryWarning(pos, Warnings.SunProprietary(s));
             });
@@ -3828,8 +3828,8 @@ void checkProfile(final DiagnosticPosition pos, final Symbol s) {
     }
 
     void checkPreview(DiagnosticPosition pos, Symbol other, Symbol s) {
-        if ((s.flags() & PREVIEW_API) != 0 && !preview.participatesInPreview(syms, other, s) && !disablePreviewCheck) {
-            if ((s.flags() & PREVIEW_REFLECTIVE) == 0) {
+        if ((s.flags_field & PREVIEW_API) != 0 && !preview.participatesInPreview(syms, other, s) && !disablePreviewCheck) {
+            if ((s.flags_field & PREVIEW_REFLECTIVE) == 0) {
                 if (!preview.isEnabled()) {
                     log.error(pos, Errors.IsPreview(s));
                 } else {
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
index 3980f03713d..f8b2614306b 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
@@ -415,7 +415,7 @@ public boolean isAccessible(Env<AttrContext> env, Type site, Symbol sym, boolean
             return true;
         }
 
-        switch ((short)(sym.flags() & AccessFlags)) {
+        switch ((short)(sym.flags_field & AccessFlags)) {
         case PRIVATE:
             return
                 (env.enclClass.sym == sym.owner // fast special case

lahodaj

While it might be nicer to have the order implied, I can see that's not really easy to achieve in case of arbitrary order of completion. So, this looks OK to me. One suggestion for consideration slightly simplify the code. No re-review needed if the suggestion is accepted.

lahodaj · 2024-01-10T16:32:10Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java

@@ -1327,6 +1330,41 @@ public ClassSymbol(long flags, Name name, Symbol owner) {
            this.type.tsym = this;
        }

+        public void addPermittedSubclass(ClassSymbol csym, int pos) {
+            if (isPermittedExplicit) {


If I understand it properly, the codepaths to add implicitly and explicitly declared permitted subclasses are completely different. And when the permitted subclasses are explicit, the addPermittedSubclass won't be called.

For consideration: just using Assert.check(!isPermittedExplicit), drop the if.

sure will do, thanks

openjdk · 2024-01-10T16:35:48Z

@vicente-romero-oracle 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:

8322477: order of subclasses in the permits clause can differ between compilations

Reviewed-by: jlahoda

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

a7db4fe: 8323428: Shenandoah: Unused memory in regions compacted during a full GC should be mangled
b86c3b7: 8309218: java/util/concurrent/locks/Lock/OOMEInAQS.java still times out with ZGC, Generational ZGC, and SerialGC
475306b: 7057369: (fs spec) FileStore getUsableSpace and getUnallocatedSpace could be clearer
f016934: 8323518: Parallel: Remove unused methods in psParallelCompact.hpp
2174f66: 8323005: Parallel: Refactor PSPromotionManager::claim_or_forward_depth
1617067: 8323331: fix typo hpage_pdm_size
2806ade: 8321685: Missing ResourceMark in code called from JvmtiEnvBase::get_vthread_jvf
ec38505: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
d2d58dd: 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process
b2a39c5: 8316241: Test jdk/jdk/jfr/jvm/TestChunkIntegrity.java failed
... and 79 more: https://git.openjdk.org/jdk/compare/ade40741cab0b5e4d8519a55ebcd51e386999f5d...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.

liach · 2024-01-10T17:32:17Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java

+        public void setPermittedSubclasses(List<Symbol> permittedSubs) {
+            permitted.clear();
+            for (Symbol csym : permittedSubs) {
+                permitted.add(new PermittedClassWithPos(csym, 0));


If we have multiple 0 positions in the list, a future addPermittedSubclass call may not produce a deterministic list if it's called with pos 0, as that's how Arrays.binarySearch behaves.

right but as Jan mentioned we will be calling either addPermittedSubclass or setPermittedSubclasses not both. We need to set a default position value if none is provided, 0 is a good option or -1

vicente-romero-oracle · 2024-01-10T17:51:34Z

/integrate

openjdk · 2024-01-10T17:52:52Z

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

c96cbe4: 8313083: Print 'rss' and 'cache' as part of the container information
a7db4fe: 8323428: Shenandoah: Unused memory in regions compacted during a full GC should be mangled
b86c3b7: 8309218: java/util/concurrent/locks/Lock/OOMEInAQS.java still times out with ZGC, Generational ZGC, and SerialGC
475306b: 7057369: (fs spec) FileStore getUsableSpace and getUnallocatedSpace could be clearer
f016934: 8323518: Parallel: Remove unused methods in psParallelCompact.hpp
2174f66: 8323005: Parallel: Refactor PSPromotionManager::claim_or_forward_depth
1617067: 8323331: fix typo hpage_pdm_size
2806ade: 8321685: Missing ResourceMark in code called from JvmtiEnvBase::get_vthread_jvf
ec38505: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
d2d58dd: 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process
... and 80 more: https://git.openjdk.org/jdk/compare/ade40741cab0b5e4d8519a55ebcd51e386999f5d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-10T17:52:58Z

@vicente-romero-oracle Pushed as commit 5ba69e1.

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

8322477: order of subclasses in the permits clause can differ between…

2970277

… compilations

openjdk bot added the rfr Pull request is ready for review label Jan 5, 2024

openjdk bot added the compiler compiler-dev@openjdk.org label Jan 5, 2024

vicente-romero-oracle commented Jan 5, 2024

View reviewed changes

vicente-romero-oracle added 2 commits January 9, 2024 22:30

adding regression test

637f424

updating regression test

20ff11b

lahodaj approved these changes Jan 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 10, 2024

addressing review comments

b9703e8

liach reviewed Jan 10, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 10, 2024

openjdk bot closed this Jan 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 10, 2024

vicente-romero-oracle deleted the JDK-8322477 branch January 10, 2024 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8322477: order of subclasses in the permits clause can differ between compilations #17284

8322477: order of subclasses in the permits clause can differ between compilations #17284

vicente-romero-oracle commented Jan 5, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 5, 2024

openjdk bot commented Jan 5, 2024

mlbridge bot commented Jan 5, 2024 •

edited

Loading

vicente-romero-oracle Jan 5, 2024

vicente-romero-oracle Jan 5, 2024

vicente-romero-oracle commented Jan 9, 2024 •

edited

Loading

lahodaj left a comment

lahodaj Jan 10, 2024

vicente-romero-oracle Jan 10, 2024

openjdk bot commented Jan 10, 2024 •

edited

Loading

liach Jan 10, 2024

vicente-romero-oracle Jan 10, 2024 •

edited

Loading

vicente-romero-oracle commented Jan 10, 2024

openjdk bot commented Jan 10, 2024

openjdk bot commented Jan 10, 2024

8322477: order of subclasses in the permits clause can differ between compilations #17284

8322477: order of subclasses in the permits clause can differ between compilations #17284

Conversation

vicente-romero-oracle commented Jan 5, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 5, 2024

openjdk bot commented Jan 5, 2024

mlbridge bot commented Jan 5, 2024 • edited Loading

Webrevs

vicente-romero-oracle Jan 5, 2024

Choose a reason for hiding this comment

vicente-romero-oracle Jan 5, 2024

Choose a reason for hiding this comment

vicente-romero-oracle commented Jan 9, 2024 • edited Loading

lahodaj left a comment

Choose a reason for hiding this comment

lahodaj Jan 10, 2024

Choose a reason for hiding this comment

vicente-romero-oracle Jan 10, 2024

Choose a reason for hiding this comment

openjdk bot commented Jan 10, 2024 • edited Loading

liach Jan 10, 2024

Choose a reason for hiding this comment

vicente-romero-oracle Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

vicente-romero-oracle commented Jan 10, 2024

openjdk bot commented Jan 10, 2024

openjdk bot commented Jan 10, 2024

vicente-romero-oracle commented Jan 5, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 5, 2024 •

edited

Loading

vicente-romero-oracle commented Jan 9, 2024 •

edited

Loading

openjdk bot commented Jan 10, 2024 •

edited

Loading

vicente-romero-oracle Jan 10, 2024 •

edited

Loading