Skip to content

Commit

Permalink
Merge pull request #352 from Ladicek/index-postprocessing-sort
Browse files Browse the repository at this point in the history
Fix class ordering when propagating type annotations, take two
  • Loading branch information
Ladicek authored Feb 12, 2024
2 parents d682025 + 6413798 commit a50c850
Show file tree
Hide file tree
Showing 3 changed files with 213 additions and 37 deletions.
67 changes: 30 additions & 37 deletions core/src/main/java/org/jboss/jandex/Indexer.java
Original file line number Diff line number Diff line change
Expand Up @@ -2572,49 +2572,42 @@ public Index complete() {
}

private void propagateTypeParameterBounds() {
List<ClassInfo> classes = new ArrayList<>(this.classes.values());
classes.sort(new Comparator<ClassInfo>() {
private boolean isEnclosedIn(ClassInfo c1, ClassInfo c2) {
if (c1.enclosingClass() != null) {
if (c2.name().equals(c1.enclosingClass())) {
return true;
}

ClassInfo c1EnclosingClass = Indexer.this.classes.get(c1.enclosingClass());
if (c1EnclosingClass != null) {
return isEnclosedIn(c1EnclosingClass, c2);
}
}

if (c1.enclosingMethod() != null) {
if (c2.name().equals(c1.enclosingMethod().enclosingClass())) {
return true;
}

ClassInfo enclosingMethodClass = Indexer.this.classes.get(c1.enclosingMethod().enclosingClass());
if (enclosingMethodClass != null) {
return isEnclosedIn(enclosingMethodClass, c2);
}
// we need to process indexed classes such that class A is processed before class B
// when B is enclosed in A (potentially indirectly)
//
// we construct a two-level total order which provides this guarantee:
// 1. for each class, compute its nesting level (top-level classes have nesting level 0,
// classes nested in top-level classes have nesting level 1, etc.) and sort the classes
// in ascending nesting level order
// 2. for equal nesting levels, sort by class name
// (see also `TotalOrderChecker`)
Map<DotName, Integer> nestingLevels = new HashMap<>();
for (ClassInfo clazz : classes.values()) {
DotName name = clazz.name();
int nestingLevel = 0;
while (clazz != null) {
if (clazz.enclosingClass() != null) {
clazz = classes.get(clazz.enclosingClass());
nestingLevel++;
} else if (clazz.enclosingMethod() != null) {
clazz = classes.get(clazz.enclosingMethod().enclosingClass());
nestingLevel++;
} else {
clazz = null;
}

return false;
}
nestingLevels.put(name, nestingLevel);
}

List<ClassInfo> classes = new ArrayList<>(this.classes.values());
classes.sort(new Comparator<ClassInfo>() {
@Override
public int compare(ClassInfo c1, ClassInfo c2) {
if (c1.name().equals(c2.name())) {
return 0;
} else if (isEnclosedIn(c1, c2)) {
// c1 is enclosed in c2, so c2 must be processed sooner
return 1;
} else if (isEnclosedIn(c2, c1)) {
// c2 is enclosed in c1, so c1 must be processed sooner
return -1;
} else {
// we really only need partial order here,
// but `Comparator` must establish total order
return c1.name().compareTo(c2.name());
int diff = Integer.compare(nestingLevels.get(c1.name()), nestingLevels.get(c2.name()));
if (diff != 0) {
return diff;
}
return c1.name().compareTo(c2.name());
}
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package org.jboss.jandex.test;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.lang.reflect.Modifier;

import org.jboss.jandex.Indexer;
import org.junit.jupiter.api.Test;

import net.bytebuddy.ByteBuddy;
import net.bytebuddy.asm.AsmVisitorWrapper;
import net.bytebuddy.description.field.FieldDescription;
import net.bytebuddy.description.field.FieldList;
import net.bytebuddy.description.method.MethodList;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.implementation.Implementation;
import net.bytebuddy.jar.asm.ClassVisitor;
import net.bytebuddy.pool.TypePool;
import net.bytebuddy.utility.OpenedClassReader;

public class ClassOrderWhenPropagatingTypeParameterBoundsTest {
// a Java compiler will never generate inner classes like this (an inner class's name
// always has its outer class's name as a prefix), but it's legal and some bytecode
// obfuscators do this

@Test
public void test() throws IOException {
byte[] a = new ByteBuddy().subclass(Object.class)
.name("org.jboss.jandex.test.A")
.visit(new AsmVisitorWrapper.AbstractBase() {
@Override
public ClassVisitor wrap(TypeDescription instrumentedType, ClassVisitor classVisitor,
Implementation.Context implementationContext, TypePool typePool,
FieldList<FieldDescription.InDefinedShape> fields, MethodList<?> methods, int writerFlags,
int readerFlags) {
return new ClassVisitor(OpenedClassReader.ASM_API, classVisitor) {
@Override
public void visitEnd() {
super.visitInnerClass("org/jboss/jandex/test/A", "org/jboss/jandex/test/C", "A",
Modifier.STATIC);
}
};
}
})
.make()
.getBytes();
byte[] b = new ByteBuddy().subclass(Object.class)
.name("org.jboss.jandex.test.B")
.make()
.getBytes();
byte[] c = new ByteBuddy().subclass(Object.class)
.name("org.jboss.jandex.test.C")
.make()
.getBytes();

Indexer indexer = new Indexer();
indexer.index(new ByteArrayInputStream(a));
indexer.index(new ByteArrayInputStream(b));
indexer.index(new ByteArrayInputStream(c));

// this is not guaranteed to fail when the `Comparator` used in `Indexer.propagateTypeParameterBounds()`
// is incorrect (because the sorting algorithm doesn't have to fail when its `Comparator` is incorrect,
// especially with such a small list of classes to sort), but inserting a call to `TotalOrderChecker.check()`
// there is enough to catch problems
indexer.complete();
}
}
116 changes: 116 additions & 0 deletions core/src/test/java/org/jboss/jandex/test/util/TotalOrderChecker.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package org.jboss.jandex.test.util;

import java.util.Collection;
import java.util.Comparator;

/**
* Verifies that a given comparator establishes a total order on the given collection of values.
* <p>
* From {@link Comparator}:
* <p>
* The <i>relation</i> that defines the <i>imposed ordering</i> that a given comparator {@code c} imposes
* on a given set of objects {@code S} is:
*
* <pre>
* {(x, y) such that c.compare(x, y) &lt;= 0}.
* </pre>
*
* The <i>quotient</i> for this total order is:
*
* <pre>
* {(x, y) such that c.compare(x, y) == 0}.
* </pre>
* <p>
* From <a href="https://en.wikipedia.org/wiki/Total_order">https://en.wikipedia.org/wiki/Total_order</a>:
* <p>
* A total order is a binary relation &le; on some set {@code X}, which satisfies the following for all {@code a},
* {@code b} and {@code c} in {@code X}:
* <ol>
* <li>a &le; a (reflexive)</li>
* <li>if a &le; b and b &le; c then a &le; c (transitive)</li>
* <li>if a &le; b and b &le; a then a = b (antisymmetric)</li>
* <li>a &le; b or b &le; a (strongly connected)</li>
* </ol>
*
* @param <T> the type of values
*/
public class TotalOrderChecker<T> {
private final Collection<T> values;
private final Comparator<T> comparator;
private final boolean throwOnFailure;

public TotalOrderChecker(Collection<T> values, Comparator<T> comparator, boolean throwOnFailure) {
this.values = values;
this.comparator = comparator;
this.throwOnFailure = throwOnFailure;
}

public void check() {
checkReflexive();
checkTransitive();
checkAntisymmetric();
checkStronglyConnected();
}

// ---

private boolean isEqual(T a, T b) {
return comparator.compare(a, b) == 0;
}

private boolean isInRelation(T a, T b) {
return comparator.compare(a, b) <= 0;
}

private void fail(String message) {
if (throwOnFailure) {
throw new AssertionError(message);
} else {
System.out.println(message);
}
}

private void checkReflexive() {
for (T a : values) {
if (!isInRelation(a, a)) {
fail("not reflexive due to " + a);
}
}
}

private void checkTransitive() {
for (T a : values) {
for (T b : values) {
for (T c : values) {
if (isInRelation(a, b) && isInRelation(b, c)) {
if (!isInRelation(a, c)) {
fail("not transitive due to " + a + ", " + b + " and " + c);
}
}
}
}
}
}

private void checkAntisymmetric() {
for (T a : values) {
for (T b : values) {
if (isInRelation(a, b) && isInRelation(b, a)) {
if (!isEqual(a, b)) {
fail("not antisymmetric due to " + a + " and " + b);
}
}
}
}
}

private void checkStronglyConnected() {
for (T a : values) {
for (T b : values) {
if (!isInRelation(a, b) && !isInRelation(b, a)) {
fail("not strongly connected due to " + a + " and " + b);
}
}
}
}
}

0 comments on commit a50c850

Please sign in to comment.