Copilot commented on code in PR #1812:
URL: https://github.com/apache/commons-lang/pull/1812#discussion_r4218925469
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -371,42 +371,68 @@ private static <T> String classToString(final Class<T>
cls) {
/**
* Tests, recursively, whether any of the type parameters associated with
{@code type} are bound to variables.
+ * <p>
+ * The owner type of a parameterized type, for example {@code Outer<T>} of
{@code Outer<T>.Inner}, is checked as well.
+ * </p>
*
* @param type The type to check for type variables.
* @return Whether any of the type parameters associated with {@code type}
are bound to variables.
* @since 3.2
*/
public static boolean containsTypeVariables(final Type type) {
+ return containsTypeVariables(type, newIdentitySet());
+ }
+
+ /**
+ * Creates a set that compares by identity, so that cycle detection never
calls {@code hashCode()} or {@code equals()} of a custom {@link Type}.
+ *
+ * @return A new identity-based set.
+ */
+ private static Set<Type> newIdentitySet() {
+ return Collections.newSetFromMap(new IdentityHashMap<>());
+ }
+
+ private static boolean containsTypeVariables(final Type type, final
Set<Type> visited) {
Review Comment:
Cycle detection in `containsTypeVariables` only guards traversal via
`getOwnerType()`. A custom `ParameterizedType` can form a cycle through
`getActualTypeArguments()` (e.g., an argument referencing the enclosing
`ParameterizedType`), which would cause unbounded
recursion/`StackOverflowError` here. Consider extending the identity-based
`visited` guard to cover type arguments too (e.g., add/remove each `arg` around
the recursive call, or add/remove `type` itself at method entry), so cycles via
arguments are also cut without invoking `equals()`/`hashCode()`.
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -371,42 +371,68 @@ private static <T> String classToString(final Class<T>
cls) {
/**
* Tests, recursively, whether any of the type parameters associated with
{@code type} are bound to variables.
+ * <p>
+ * The owner type of a parameterized type, for example {@code Outer<T>} of
{@code Outer<T>.Inner}, is checked as well.
+ * </p>
*
* @param type The type to check for type variables.
* @return Whether any of the type parameters associated with {@code type}
are bound to variables.
* @since 3.2
*/
public static boolean containsTypeVariables(final Type type) {
+ return containsTypeVariables(type, newIdentitySet());
+ }
+
+ /**
+ * Creates a set that compares by identity, so that cycle detection never
calls {@code hashCode()} or {@code equals()} of a custom {@link Type}.
+ *
+ * @return A new identity-based set.
+ */
+ private static Set<Type> newIdentitySet() {
+ return Collections.newSetFromMap(new IdentityHashMap<>());
+ }
+
+ private static boolean containsTypeVariables(final Type type, final
Set<Type> visited) {
if (type instanceof TypeVariable<?>) {
return true;
}
if (type instanceof Class<?>) {
return ((Class<?>) type).getTypeParameters().length > 0;
}
if (type instanceof ParameterizedType) {
- for (final Type arg : ((ParameterizedType)
type).getActualTypeArguments()) {
- if (containsTypeVariables(arg)) {
+ final ParameterizedType parameterizedType = (ParameterizedType)
type;
+ for (final Type arg : parameterizedType.getActualTypeArguments()) {
+ if (containsTypeVariables(arg, visited)) {
return true;
}
}
Review Comment:
Cycle detection in `containsTypeVariables` only guards traversal via
`getOwnerType()`. A custom `ParameterizedType` can form a cycle through
`getActualTypeArguments()` (e.g., an argument referencing the enclosing
`ParameterizedType`), which would cause unbounded
recursion/`StackOverflowError` here. Consider extending the identity-based
`visited` guard to cover type arguments too (e.g., add/remove each `arg` around
the recursive call, or add/remove `type` itself at method entry), so cycles via
arguments are also cut without invoking `equals()`/`hashCode()`.
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -1667,17 +1693,18 @@ private static String typeVariableToString(final
TypeVariable<?> typeVariable) {
}
/**
- * Unrolls variables in a type bounds array.
+ * Unrolls variables in a type bounds array, preserving cycle state.
*
* @param typeArguments assignments {@link Map}.
* @param bounds in which to expand variables.
+ * @param visited set of visited type variables for cycle detection.
Review Comment:
The updated `unrollBounds(...)` signature adds the `unrolling` parameter,
but the Javadoc does not document it. Add an `@param unrolling` entry
describing that it tracks (identity-based) types currently being unrolled to
prevent recursion cycles.
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -1718,40 +1745,69 @@ 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) {
+ /**
+ * 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;
+ }
+ }
+ return false;
+ }
+
+ private static Type unrollVariables(final Map<TypeVariable<?>, Type>
typeArguments, final Type type, final Set<TypeVariable<?>> visited, final
Set<Type> unrolling) {
if (containsTypeVariables(type)) {
Review Comment:
`unrollVariables(...)` calls the public `containsTypeVariables(type)`, which
now allocates a fresh identity-backed set on every invocation. Since
`unrollVariables` is recursive, this can introduce substantial allocation and
repeated traversal overhead. A tangible improvement is to avoid calling the
public method from the recursive path: either (a) add an internal
`containsTypeVariables(type, identityVisited)` variant that reuses a single
identity set for the duration of one `unrollVariables` call, or (b) restructure
`unrollVariables` to dispatch on `instanceof` and only recurse where necessary,
removing the pre-check.
--
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]
