From 64137982799853bcbed2f74ee8c50dab0ff2c4d0 Mon Sep 17 00:00:00 2001 From: Ladislav Thon Date: Fri, 12 Jan 2024 14:37:49 +0100 Subject: [PATCH] Fix class ordering when propagating type annotations, take two To propagate type annotations across nested classes, we sort the entire set of indexed classes so that outer classes come before their inner classes. We don't really care about ordering between classes that are not members of the same nest (using the JEP 181 terminology). That is, we only need a partial order on classes. However, to perform that sort, we use the sorting routine in the JDK, which uses the order defined by a `Comparator`, and that order must be total. To define a total order, this commit takes a completely opposite approach. We compute a nesting level for each class (where a top-level class has a nesting level of 0, a class nested in a top-level class has a nesting level of 1, etc.) and sort by the nesting level. For equal nesting levels, we sort by the class name. This means that all top-level classes come first, all classes nested in top-level classes come second, etc. To verify that a given `Comparator` establishes a total order for a given set of values, this commit includes a simple utility class `TotalOrderChecker`, which was used to reproduce (and understand) the problem of previous implementation of this ordering. --- .../main/java/org/jboss/jandex/Indexer.java | 67 +++++----- ...henPropagatingTypeParameterBoundsTest.java | 67 ++++++++++ .../jandex/test/util/TotalOrderChecker.java | 116 ++++++++++++++++++ 3 files changed, 213 insertions(+), 37 deletions(-) create mode 100644 core/src/test/java/org/jboss/jandex/test/ClassOrderWhenPropagatingTypeParameterBoundsTest.java create mode 100644 core/src/test/java/org/jboss/jandex/test/util/TotalOrderChecker.java 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); + } + } + } + } +}