Skip to content

Commit

Permalink
8322477: order of subclasses in the permits clause can differ between…
Browse files Browse the repository at this point in the history 
… compilations

Reviewed-by: jlahoda
  • Loading branch information
Vicente Romero committed Jan 10, 2024
1 parent c96cbe4 commit 5ba69e1
Show file tree
Hide file tree
Showing 9 changed files with 106 additions and 39 deletions.
40 changes: 37 additions & 3 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import java.lang.annotation.Annotation;
import java.lang.annotation.Inherited;
import java.util.ArrayList;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
Expand Down Expand Up @@ -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;

public boolean isPermittedExplicit = false;

private record PermittedClassWithPos(Symbol permittedClass, int pos) {}

public ClassSymbol(long flags, Name name, Type type, Symbol owner) {
super(TYP, flags, name, type, owner);
this.members_field = null;
Expand All @@ -1315,7 +1318,7 @@ public ClassSymbol(long flags, Name name, Type type, Symbol owner) {
this.sourcefile = null;
this.classfile = null;
this.annotationTypeMetadata = AnnotationTypeMetadata.notAnAnnotationType();
this.permitted = List.nil();
this.permitted = new ArrayList<>();
}

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

public void addPermittedSubclass(ClassSymbol csym, int pos) {
Assert.check(!isPermittedExplicit);
// we need to insert at the right pos
PermittedClassWithPos element = new PermittedClassWithPos(csym, pos);
int index = Collections.binarySearch(permitted, element, java.util.Comparator.comparing(PermittedClassWithPos::pos));
if (index < 0) {
index = -index - 1;
}
permitted.add(index, element);
}

public boolean isPermittedSubclass(Symbol csym) {
for (PermittedClassWithPos permittedClassWithPos : permitted) {
if (permittedClassWithPos.permittedClass.equals(csym)) {
return true;
}
}
return false;
}

public void clearPermittedSubclasses() {
permitted.clear();
}

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

/** The Java source which this symbol represents.
*/
public String toString() {
Expand Down Expand Up @@ -1643,7 +1677,7 @@ public boolean isRecord() {

@DefinedBy(Api.LANGUAGE_MODEL)
public List<Type> getPermittedSubclasses() {
return permitted.map(s -> s.type);
return permitted.stream().map(s -> s.permittedClass().type).collect(List.collector());
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1701,7 +1701,7 @@ private boolean areDisjoint(ClassSymbol ts, ClassSymbol ss) {
// permitted subtypes have to be disjoint with the other symbol
ClassSymbol sealedOne = ts.isSealed() ? ts : ss;
ClassSymbol other = sealedOne == ts ? ss : ts;
return sealedOne.permitted.stream().allMatch(sym -> areDisjoint((ClassSymbol)sym, other));
return sealedOne.getPermittedSubclasses().stream().allMatch(type -> areDisjoint((ClassSymbol)type.tsym, other));
}
return false;
}
Expand Down
46 changes: 23 additions & 23 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5383,58 +5383,58 @@ void attribClass(ClassSymbol c) throws CompletionFailure {
if (c.isSealed() &&
!c.isEnum() &&
!c.isPermittedExplicit &&
c.permitted.isEmpty()) {
c.getPermittedSubclasses().isEmpty()) {
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()) {
boolean isTypeVar = false;
if (subTypeSym.type.getTag() == TYPEVAR) {
if (subType.getTag() == TYPEVAR) {
isTypeVar = true; //error recovery
log.error(TreeInfo.diagnosticPositionFor(subTypeSym, env.tree),
Errors.InvalidPermitsClause(Fragments.IsATypeVariable(subTypeSym.type)));
log.error(TreeInfo.diagnosticPositionFor(subType.tsym, env.tree),
Errors.InvalidPermitsClause(Fragments.IsATypeVariable(subType)));
}
if (subTypeSym.isAnonymous() && !c.isEnum()) {
log.error(TreeInfo.diagnosticPositionFor(subTypeSym, env.tree), Errors.LocalClassesCantExtendSealed(Fragments.Anonymous));
if (subType.tsym.isAnonymous() && !c.isEnum()) {
log.error(TreeInfo.diagnosticPositionFor(subType.tsym, env.tree), Errors.LocalClassesCantExtendSealed(Fragments.Anonymous));
}
if (permittedTypes.contains(subTypeSym)) {
if (permittedTypes.contains(subType.tsym)) {
DiagnosticPosition pos =
env.enclClass.permitting.stream()
.filter(permittedExpr -> TreeInfo.diagnosticPositionFor(subTypeSym, permittedExpr, true) != null)
.filter(permittedExpr -> TreeInfo.diagnosticPositionFor(subType.tsym, permittedExpr, true) != null)
.limit(2).collect(List.collector()).get(1);
log.error(pos, Errors.InvalidPermitsClause(Fragments.IsDuplicated(subTypeSym.type)));
log.error(pos, Errors.InvalidPermitsClause(Fragments.IsDuplicated(subType)));
} else {
permittedTypes.add(subTypeSym);
permittedTypes.add(subType.tsym);
}
if (sealedInUnnamed) {
if (subTypeSym.packge() != c.packge()) {
log.error(TreeInfo.diagnosticPositionFor(subTypeSym, env.tree),
if (subType.tsym.packge() != c.packge()) {
log.error(TreeInfo.diagnosticPositionFor(subType.tsym, env.tree),
Errors.ClassInUnnamedModuleCantExtendSealedInDiffPackage(c)
);
}
} else if (subTypeSym.packge().modle != c.packge().modle) {
log.error(TreeInfo.diagnosticPositionFor(subTypeSym, env.tree),
} else if (subType.tsym.packge().modle != c.packge().modle) {
log.error(TreeInfo.diagnosticPositionFor(subType.tsym, env.tree),
Errors.ClassInModuleCantExtendSealedInDiffModule(c, c.packge().modle)
);
}
if (subTypeSym == c.type.tsym || types.isSuperType(subTypeSym.type, c.type)) {
log.error(TreeInfo.diagnosticPositionFor(subTypeSym, ((JCClassDecl)env.tree).permitting),
if (subType.tsym == c.type.tsym || types.isSuperType(subType, c.type)) {
log.error(TreeInfo.diagnosticPositionFor(subType.tsym, ((JCClassDecl)env.tree).permitting),
Errors.InvalidPermitsClause(
subTypeSym == c.type.tsym ?
subType.tsym == c.type.tsym ?
Fragments.MustNotBeSameClass :
Fragments.MustNotBeSupertype(subTypeSym.type)
Fragments.MustNotBeSupertype(subType)
)
);
} else if (!isTypeVar) {
boolean thisIsASuper = types.directSupertypes(subTypeSym.type)
boolean thisIsASuper = types.directSupertypes(subType)
.stream()
.anyMatch(d -> d.tsym == c);
if (!thisIsASuper) {
log.error(TreeInfo.diagnosticPositionFor(subTypeSym, env.tree),
Errors.InvalidPermitsClause(Fragments.DoesntExtendSealed(subTypeSym.type)));
log.error(TreeInfo.diagnosticPositionFor(subType.tsym, env.tree),
Errors.InvalidPermitsClause(Fragments.DoesntExtendSealed(subType)));
}
}
}
Expand Down Expand Up @@ -5469,7 +5469,7 @@ void attribClass(ClassSymbol c) throws CompletionFailure {

if (!c.type.isCompound()) {
for (ClassSymbol supertypeSym : sealedSupers) {
if (!supertypeSym.permitted.contains(c.type.tsym)) {
if (!supertypeSym.isPermittedSubclass(c.type.tsym)) {
log.error(TreeInfo.diagnosticPositionFor(c.type.tsym, env.tree), Errors.CantInheritFromSealed(supertypeSym));
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -945,8 +945,8 @@ private Set<Symbol> allPermittedSubTypes(ClassSymbol root, Predicate<ClassSymbol
current.complete();

if (current.isSealed() && current.isAbstract()) {
for (Symbol sym : current.permitted) {
ClassSymbol csym = (ClassSymbol) sym;
for (Type t : current.getPermittedSubclasses()) {
ClassSymbol csym = (ClassSymbol) t.tsym;

if (accept.test(csym)) {
permittedSubtypesClosure = permittedSubtypesClosure.prepend(csym);
Expand Down
4 changes: 2 additions & 2 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ private void fillPermits(JCClassDecl tree, Env<AttrContext> baseEnv) {
!supClass.isPermittedExplicit &&
supClassEnv != null &&
supClassEnv.toplevel == baseEnv.toplevel) {
supClass.permitted = supClass.permitted.append(sym);
supClass.addPermittedSubclass(sym, tree.pos);
}
}
}
Expand All @@ -932,7 +932,7 @@ private void fillPermits(JCClassDecl tree, Env<AttrContext> baseEnv) {
Type pt = attr.attribBase(permitted, baseEnv, false, false, false);
permittedSubtypeSymbols.append(pt.tsym);
}
sym.permitted = permittedSubtypeSymbols.toList();
sym.setPermittedSubclasses(permittedSubtypeSymbols.toList());
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1298,7 +1298,7 @@ protected void read(Symbol sym, int attrLen) {
for (int i = 0; i < numberOfPermittedSubtypes; i++) {
subtypes.add(poolReader.getClass(nextChar()));
}
((ClassSymbol)sym).permitted = subtypes.toList();
((ClassSymbol)sym).setPermittedSubclasses(subtypes.toList());
}
}
},
Expand Down Expand Up @@ -2930,7 +2930,7 @@ void readClass(ClassSymbol c) {
for (int i = 0; i < methodCount; i++) skipMember();
readClassAttrs(c);

if (c.permitted != null && !c.permitted.isEmpty()) {
if (!c.getPermittedSubclasses().isEmpty()) {
c.flags_field |= SEALED;
}

Expand Down
8 changes: 4 additions & 4 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -922,11 +922,11 @@ private void listNested(Symbol sym, ListBuffer<ClassSymbol> seen) {
/** Write "PermittedSubclasses" attribute.
*/
int writePermittedSubclassesIfNeeded(ClassSymbol csym) {
if (csym.permitted.nonEmpty()) {
if (csym.getPermittedSubclasses().nonEmpty()) {
int alenIdx = writeAttr(names.PermittedSubclasses);
databuf.appendChar(csym.permitted.size());
for (Symbol c : csym.permitted) {
databuf.appendChar(poolWriter.putClass((ClassSymbol) c));
databuf.appendChar(csym.getPermittedSubclasses().size());
for (Type t : csym.getPermittedSubclasses()) {
databuf.appendChar(poolWriter.putClass((ClassSymbol) t.tsym));
}
endAttr(alenIdx);
return 1;
Expand Down
2 changes: 1 addition & 1 deletion ...jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1643,7 +1643,7 @@ public void visitClassDef(JCClassDecl node) {
originalAnnos.forEach(a -> visitAnnotation(a));
}
// we should empty the list of permitted subclasses for next round
node.sym.permitted = List.nil();
node.sym.clearPermittedSubclasses();
}
node.sym = null;
}
Expand Down
35 changes: 34 additions & 1 deletion test/langtools/tools/javac/sealed/SealedDiffConfigurationsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ private void checkSealedClassFile(Path out, String cfName, List<String> expected
} catch (ConstantPoolException ex) {
}
});
subtypeNames.sort((s1, s2) -> s1.compareTo(s2));
for (int i = 0; i < expectedSubTypeNames.size(); i++) {
Assert.check(expectedSubTypeNames.get(0).equals(subtypeNames.get(0)));
}
Expand Down Expand Up @@ -695,4 +694,38 @@ public void testSupertypePermitsLoop(Path base) throws Exception {
.run()
.writeAll();
}

@Test
public void testClientSwapsPermittedSubclassesOrder(Path base) throws Exception {
Path src = base.resolve("src");
Path foo = src.resolve("Foo.java");
Path fooUser = src.resolve("FooUser.java");

tb.writeFile(foo,
"""
public sealed interface Foo {
record R1() implements Foo {}
record R2() implements Foo {}
}
""");

tb.writeFile(fooUser,
"""
public class FooUser {
// see that the order of arguments differ from the order of subclasses of Foo in the source above
// we need to check that the order of permitted subclasses of Foo in the class file corresponds to the
// original order in the source code
public void blah(Foo.R2 a, Foo.R1 b) {}
}
""");

Path out = base.resolve("out");
Files.createDirectories(out);

new JavacTask(tb)
.outdir(out)
.files(fooUser, foo)
.run();
checkSealedClassFile(out, "Foo.class", List.of("Foo$R1", "Foo$R2"));
}
}

1 comment on commit 5ba69e1

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.