-
Notifications
You must be signed in to change notification settings - Fork 93
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.
- Loading branch information
Showing
3 changed files
with
213 additions
and
37 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
67 changes: 67 additions & 0 deletions
67
...src/test/java/org/jboss/jandex/test/ClassOrderWhenPropagatingTypeParameterBoundsTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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
116
core/src/test/java/org/jboss/jandex/test/util/TotalOrderChecker.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) <= 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 ≤ on some set {@code X}, which satisfies the following for all {@code a}, | ||
| * {@code b} and {@code c} in {@code X}: | ||
| * <ol> | ||
| * <li>a ≤ a (reflexive)</li> | ||
| * <li>if a ≤ b and b ≤ c then a ≤ c (transitive)</li> | ||
| * <li>if a ≤ b and b ≤ a then a = b (antisymmetric)</li> | ||
| * <li>a ≤ b or b ≤ 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); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |