Skip to content

Commit

Permalink
Fix class ordering when propagating type annotations, take two
Browse files Browse the repository at this point in the history
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
Ladicek committed Feb 12, 2024
1 parent d682025 commit 7d06c03
Show file tree
Hide file tree
Showing 2 changed files with 146 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
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 7d06c03

Please sign in to comment.