Fix class ordering when propagating type annotations, take two

core/src/main/java/org/jboss/jandex/Indexer.java

@@ -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());
             }
         });
 

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<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();
+    }
+}

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.
+ * <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);
+                }
+            }
+        }
+    }
+}