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

[Vicente Romero]

On Fri, 5 Jan 2024 17:42:33 GMT, Vicente Romero <vromero at openjdk.org> wrote:

> 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

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

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

PR Comment: https://git.openjdk.org/jdk/pull/17284#issuecomment-1883876588