diff --git a/core/src/main/java/org/jboss/jandex/Indexer.java b/core/src/main/java/org/jboss/jandex/Indexer.java index 29df11eb..93d9e625 100644 --- a/core/src/main/java/org/jboss/jandex/Indexer.java +++ b/core/src/main/java/org/jboss/jandex/Indexer.java @@ -2572,49 +2572,42 @@ public Index complete() { } private void propagateTypeParameterBounds() { - List classes = new ArrayList<>(this.classes.values()); - classes.sort(new Comparator() { - 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 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 classes = new ArrayList<>(this.classes.values()); + classes.sort(new Comparator() { @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()); } }); diff --git a/core/src/test/java/org/jboss/jandex/test/ClassOrderWhenPropagatingTypeParameterBoundsTest.java b/core/src/test/java/org/jboss/jandex/test/ClassOrderWhenPropagatingTypeParameterBoundsTest.java new file mode 100644 index 00000000..49e812b3 --- /dev/null +++ b/core/src/test/java/org/jboss/jandex/test/ClassOrderWhenPropagatingTypeParameterBoundsTest.java @@ -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 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(); + } +} diff --git a/core/src/test/java/org/jboss/jandex/test/util/TotalOrderChecker.java b/core/src/test/java/org/jboss/jandex/test/util/TotalOrderChecker.java new file mode 100644 index 00000000..a1847697 --- /dev/null +++ b/core/src/test/java/org/jboss/jandex/test/util/TotalOrderChecker.java @@ -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. + *

+ * From {@link Comparator}: + *

+ * The relation that defines the imposed ordering that a given comparator {@code c} imposes + * on a given set of objects {@code S} is: + * + * 

+ *        {(x, y) such that c.compare(x, y) <= 0}.
+ *
+ * + * The quotient for this total order is: + * + * 
+ *        {(x, y) such that c.compare(x, y) == 0}.
+ *
+ *

+ * From https://en.wikipedia.org/wiki/Total_order: + *

+ * A total order is a binary relation ≤ on some set {@code X}, which satisfies the following for all {@code a}, + * {@code b} and {@code c} in {@code X}: + *

    + *
  1. a ≤ a (reflexive)
    2. + *
  2. if a ≤ b and b ≤ c then a ≤ c (transitive)
    3. + *
  3. if a ≤ b and b ≤ a then a = b (antisymmetric)
    4. + *
  4. a ≤ b or b ≤ a (strongly connected)
    5. + *
+ * + * @param the type of values + */ +public class TotalOrderChecker { + private final Collection values; + private final Comparator comparator; + private final boolean throwOnFailure; + + public TotalOrderChecker(Collection values, Comparator 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); + } + } + } + } +}