Copilot commented on code in PR #1812:
URL: https://github.com/apache/commons-lang/pull/1812#discussion_r4229641007
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -1718,40 +1744,67 @@ public static Type unrollVariables(Map<TypeVariable<?>,
Type> typeArguments, fin
if (typeArguments == null) {
typeArguments = Collections.emptyMap();
}
- return unrollVariables(typeArguments, type, new HashSet<>());
+ return unrollVariables(typeArguments, type, new HashSet<>(),
newIdentitySet());
}
- private static Type unrollVariables(final Map<TypeVariable<?>, Type>
typeArguments, final Type type, final Set<TypeVariable<?>> visited) {
- if (containsTypeVariables(type)) {
- if (type instanceof TypeVariable<?>) {
- final TypeVariable<?> var = (TypeVariable<?>) type;
- if (!visited.add(var)) {
- return var;
- }
- return unrollVariables(typeArguments, typeArguments.get(type),
visited);
+ /**
+ * Tests whether following the owner types of the given type leads back to
a type that was already seen. Only a custom {@link ParameterizedType} can do
that.
+ *
+ * @param type The type whose owner chain to check.
+ * @return Whether the owner chain of {@code type} is cyclic.
+ */
+ private static boolean hasCyclicOwnerChain(final ParameterizedType type) {
+ final Set<Type> seen = newIdentitySet();
+ seen.add(type);
+ for (Type owner = type.getOwnerType(); owner instanceof
ParameterizedType; owner = ((ParameterizedType) owner).getOwnerType()) {
+ if (!seen.add(owner)) {
+ return true;
}
- if (type instanceof ParameterizedType) {
- final ParameterizedType p = (ParameterizedType) type;
- final Map<TypeVariable<?>, Type> parameterizedTypeArguments;
- if (p.getOwnerType() == null) {
- parameterizedTypeArguments = typeArguments;
- } else {
- parameterizedTypeArguments = new HashMap<>(typeArguments);
- parameterizedTypeArguments.putAll(getTypeArguments(p));
- }
+ }
+ return false;
+ }
+
+ private static Type unrollVariables(final Map<TypeVariable<?>, Type>
typeArguments, final Type type, final Set<TypeVariable<?>> visited, final
Set<Type> unrolling) {
+ if (type instanceof TypeVariable<?>) {
+ final TypeVariable<?> var = (TypeVariable<?>) type;
+ if (!visited.add(var)) {
+ return var;
+ }
+ final Type unrolled = unrollVariables(typeArguments,
typeArguments.get(type), visited, unrolling);
+ // Only guard against cycles: the same variable may legitimately
occur more than once in a type.
+ visited.remove(var);
+ return unrolled;
+ }
+ if (type instanceof ParameterizedType && containsTypeVariables(type)) {
+ final ParameterizedType p = (ParameterizedType) type;
+ if (hasCyclicOwnerChain(p) || !unrolling.add(p)) {
+ return p;
+ }
+ try {
final Type[] args = p.getActualTypeArguments().clone();
for (int i = 0; i < args.length; i++) {
- final Type unrolled =
unrollVariables(parameterizedTypeArguments, args[i], visited);
+ final Type unrolled = unrollVariables(typeArguments,
args[i], visited, unrolling);
if (unrolled != null) {
args[i] = unrolled;
}
}
- return parameterizeWithOwner(p.getOwnerType(), (Class<?>)
p.getRawType(), args);
+ final Type owner = p.getOwnerType();
+ final Type unrolledOwner = owner != null && !(owner instanceof
Class<?>) ? unrollVariables(typeArguments, owner, visited, unrolling) : owner;
+ return parameterizeWithOwner(unrolledOwner, (Class<?>)
p.getRawType(), args);
+ } finally {
+ unrolling.remove(p);
}
- if (type instanceof WildcardType) {
- final WildcardType wild = (WildcardType) type;
- return
wildcardType().withUpperBounds(unrollBounds(typeArguments,
wild.getUpperBounds()))
- .withLowerBounds(unrollBounds(typeArguments,
wild.getLowerBounds())).build();
+ }
+ if (type instanceof WildcardType && containsTypeVariables(type)) {
+ final WildcardType wild = (WildcardType) type;
+ if (!unrolling.add(wild)) {
+ return wild;
+ }
+ try {
+ return
wildcardType().withUpperBounds(unrollBounds(typeArguments,
wild.getUpperBounds(), visited, unrolling))
+ .withLowerBounds(unrollBounds(typeArguments,
wild.getLowerBounds(), visited, unrolling)).build();
+ } finally {
+ unrolling.remove(wild);
}
}
Review Comment:
`containsTypeVariables(type)` performs a full recursive walk and is invoked
inside `unrollVariables` for each `ParameterizedType`/`WildcardType`
encountered, which can re-traverse the same subgraphs repeatedly for deeply
nested generic signatures. Consider removing these pre-checks and instead
perform unrolling opportunistically (tracking whether any argument/owner/bound
changed), returning the original `Type` when nothing changes; this avoids extra
passes while keeping behavior identical.
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -1718,40 +1744,67 @@ public static Type unrollVariables(Map<TypeVariable<?>,
Type> typeArguments, fin
if (typeArguments == null) {
typeArguments = Collections.emptyMap();
}
- return unrollVariables(typeArguments, type, new HashSet<>());
+ return unrollVariables(typeArguments, type, new HashSet<>(),
newIdentitySet());
}
- private static Type unrollVariables(final Map<TypeVariable<?>, Type>
typeArguments, final Type type, final Set<TypeVariable<?>> visited) {
- if (containsTypeVariables(type)) {
- if (type instanceof TypeVariable<?>) {
- final TypeVariable<?> var = (TypeVariable<?>) type;
- if (!visited.add(var)) {
- return var;
- }
- return unrollVariables(typeArguments, typeArguments.get(type),
visited);
+ /**
+ * Tests whether following the owner types of the given type leads back to
a type that was already seen. Only a custom {@link ParameterizedType} can do
that.
+ *
+ * @param type The type whose owner chain to check.
+ * @return Whether the owner chain of {@code type} is cyclic.
+ */
+ private static boolean hasCyclicOwnerChain(final ParameterizedType type) {
+ final Set<Type> seen = newIdentitySet();
+ seen.add(type);
+ for (Type owner = type.getOwnerType(); owner instanceof
ParameterizedType; owner = ((ParameterizedType) owner).getOwnerType()) {
+ if (!seen.add(owner)) {
+ return true;
}
- if (type instanceof ParameterizedType) {
- final ParameterizedType p = (ParameterizedType) type;
- final Map<TypeVariable<?>, Type> parameterizedTypeArguments;
- if (p.getOwnerType() == null) {
- parameterizedTypeArguments = typeArguments;
- } else {
- parameterizedTypeArguments = new HashMap<>(typeArguments);
- parameterizedTypeArguments.putAll(getTypeArguments(p));
- }
+ }
+ return false;
+ }
+
+ private static Type unrollVariables(final Map<TypeVariable<?>, Type>
typeArguments, final Type type, final Set<TypeVariable<?>> visited, final
Set<Type> unrolling) {
+ if (type instanceof TypeVariable<?>) {
+ final TypeVariable<?> var = (TypeVariable<?>) type;
+ if (!visited.add(var)) {
+ return var;
+ }
+ final Type unrolled = unrollVariables(typeArguments,
typeArguments.get(type), visited, unrolling);
+ // Only guard against cycles: the same variable may legitimately
occur more than once in a type.
+ visited.remove(var);
+ return unrolled;
Review Comment:
`visited.remove(var)` should be in a `finally` block to ensure the
cycle-detection state is restored even if recursive unrolling throws (e.g.,
`IllegalArgumentException` from deeper validation). This makes the helper
resilient if callers ever catch and continue after an exception in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
