Copilot commented on code in PR #1812:
URL: https://github.com/apache/commons-lang/pull/1812#discussion_r4212139709
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -1728,30 +1743,29 @@ private static Type unrollVariables(final
Map<TypeVariable<?>, Type> typeArgumen
if (!visited.add(var)) {
return var;
}
- return unrollVariables(typeArguments, typeArguments.get(type),
visited);
+ final Type unrolled = unrollVariables(typeArguments,
typeArguments.get(type), visited);
+ // 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) {
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));
- }
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);
if (unrolled != null) {
args[i] = unrolled;
}
}
- return parameterizeWithOwner(p.getOwnerType(), (Class<?>)
p.getRawType(), args);
+ // A raw Class owner (for example Map.class for Map.Entry<T,
Integer>) has nothing to unroll.
+ final Type owner = p.getOwnerType();
+ final Type unrolledOwner = owner != null && !(owner instanceof
Class<?>) ? unrollVariables(typeArguments, owner, visited) : owner;
Review Comment:
The `visited` set tracks only type variables, so it cannot stop cycles
between owner objects. For a custom parameterized type A with no arguments
whose owner is B<T>, where B's owner is A, `unrollVariables({T -> String}, A)`
repeatedly follows A → B → A until `StackOverflowError`. Previously, A was
returned unchanged. A self-owned type with a variable argument has the same
failure. Track active composite `Type` objects by identity throughout
unrolling, including wildcard bounds, and add regression tests for owner cycles
containing variables.
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -377,36 +377,50 @@ private static <T> String classToString(final Class<T>
cls) {
* @since 3.2
*/
public static boolean containsTypeVariables(final Type type) {
+ return containsTypeVariables(type, new HashSet<>());
Review Comment:
The new `HashSet` calls the owner's `hashCode()` when checking for cycles. A
self-owned custom `ParameterizedType` with no arguments and a structural hash
code that hashes its owner still throws `StackOverflowError` before the guard
can detect the cycle. The new regression test misses this because its type
inherits `Object.hashCode()`. Use an identity-backed set, matching `VISITING`
in this file, and add coverage for an owner with a recursive structural hash
code.
--
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]
