Copilot commented on code in PR #1782:
URL: https://github.com/apache/commons-lang/pull/1782#discussion_r3949577500
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -1229,27 +1229,35 @@ private static boolean isAssignable(final Type type,
final WildcardType toWildca
// if there are assignments for unresolved type variables,
// now's the time to substitute them.
toBound = substituteTypeVariables(toBound, typeVarAssigns);
- // each upper bound of the subject type has to be assignable to
- // each
- // upper bound of the target type
+ // at least one upper bound of the subject type has to be
assignable to
+ // each upper bound of the target type
+ boolean satisfied = false;
for (final Type bound : upperBounds) {
- if (!isAssignable(bound, toBound, typeVarAssigns)) {
- return false;
+ if (isAssignable(bound, toBound, typeVarAssigns)) {
+ satisfied = true;
+ break;
}
}
+ if (!satisfied) {
+ return false;
+ }
}
for (Type toBound : toLowerBounds) {
// if there are assignments for unresolved type variables,
// now's the time to substitute them.
toBound = substituteTypeVariables(toBound, typeVarAssigns);
// each lower bound of the target type has to be assignable to
- // each
- // lower bound of the subject type
+ // at least one lower bound of the subject type
Review Comment:
The updated comment is incomplete/awkwardly phrased (it reads like a
continuation of the previous wording but doesn’t clearly state the full rule).
Please rewrite it to clearly describe the intended lower-bound relationship
(e.g., ‘for each target lower bound, at least one subject lower bound must
satisfy …’), or restore the original comment if you revert the logic.
##########
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java:
##########
@@ -1229,27 +1229,35 @@ private static boolean isAssignable(final Type type,
final WildcardType toWildca
// if there are assignments for unresolved type variables,
// now's the time to substitute them.
toBound = substituteTypeVariables(toBound, typeVarAssigns);
- // each upper bound of the subject type has to be assignable to
- // each
- // upper bound of the target type
+ // at least one upper bound of the subject type has to be
assignable to
+ // each upper bound of the target type
+ boolean satisfied = false;
for (final Type bound : upperBounds) {
- if (!isAssignable(bound, toBound, typeVarAssigns)) {
- return false;
+ if (isAssignable(bound, toBound, typeVarAssigns)) {
+ satisfied = true;
+ break;
}
}
+ if (!satisfied) {
+ return false;
+ }
}
for (Type toBound : toLowerBounds) {
// if there are assignments for unresolved type variables,
// now's the time to substitute them.
toBound = substituteTypeVariables(toBound, typeVarAssigns);
// each lower bound of the target type has to be assignable to
- // each
- // lower bound of the subject type
+ // at least one lower bound of the subject type
+ boolean satisfied = false;
for (final Type bound : lowerBounds) {
- if (!isAssignable(toBound, bound, typeVarAssigns)) {
- return false;
+ if (isAssignable(toBound, bound, typeVarAssigns)) {
+ satisfied = true;
+ break;
}
}
+ if (!satisfied) {
+ return false;
+ }
}
Review Comment:
This PR is described as fixing wildcard assignability for multiple *upper*
bounds, but it also changes the *lower bound* semantics from ‘all bounds must
match’ to ‘any bound may match’. That’s a behavioral change beyond the stated
scope and could loosen assignability results. Either (a) justify this change in
the PR description and add focused tests that demonstrate the intended
lower-bound behavior, or (b) revert the lower-bound loop to the prior stricter
logic and keep the fix limited to upper bounds.
--
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]
