Skip to content

Fix TypeUtils.isAssignable() for wildcards with multiple upper bounds - #1782

Draft
Alwaysgaurav1 wants to merge 1 commit into
apache:masterfrom
Alwaysgaurav1:fix/typeutils-wildcard-multiple-bounds
Draft

Fix TypeUtils.isAssignable() for wildcards with multiple upper bounds#1782
Alwaysgaurav1 wants to merge 1 commit into
apache:masterfrom
Alwaysgaurav1:fix/typeutils-wildcard-multiple-bounds

Conversation

@Alwaysgaurav1

Copy link
Copy Markdown
Contributor

Description

Fixes an issue in TypeUtils.isAssignable(Type, WildcardType, Map) where checking assignability of a WildcardType with multiple upper bounds (intersection types such as ? extends Serializable & Cloneable) to another WildcardType (such as ? extends Serializable) incorrectly returned false.

Root Cause

When the subject type is a WildcardType, the upper bounds loop previously enforced that every upper bound in the subject wildcard had to be assignable to each target upper bound toBound (forall bound in upperBounds: isAssignable(bound, toBound)):

for (Type toBound : toUpperBounds) {
    toBound = substituteTypeVariables(toBound, typeVarAssigns);
    for (final Type bound : upperBounds) {
        if (!isAssignable(bound, toBound, typeVarAssigns)) {
            return false;
        }
    }
}

@Alwaysgaurav1

Copy link
Copy Markdown
Contributor Author

@garydgregory, please review it quickly.

@garydgregory

Copy link
Copy Markdown
Member

@Alwaysgaurav1
Excuse me, quickly? Why? You're not the only PR author on the planet you know.

@Alwaysgaurav1

Copy link
Copy Markdown
Contributor Author

@garydgregory,

Okay, whenever you want . I am okay.

@garydgregory
garydgregory requested a lite review from Copilot September 7, 2026 11:52
@garydgregory

garydgregory commented Sep 7, 2026

Copy link
Copy Markdown
Member

@Alwaysgaurav1
Until copilot comes back with its review, here are a couple of findings to address:

  • Lower-bound behavior changes without a regression test.

At updated TypeUtils lines 1251–1259, the patch changes “every source lower bound” to “at least one source lower bound.” This is outside the upper-bound problem described by the PR.

See:

TypeUtils.isAssignable(
    TypeUtils.wildcardType()
        .withLowerBounds(Number.class, CharSequence.class).build(),
    TypeUtils.wildcardType()
        .withLowerBounds(Integer.class).build())

The result changes from false to true. This is not itself evidence of incorrect behavior, but it needs an explicit rationale and positive/negative tests. Either include those or leave the lower-bound change for a separate PR.

  • The new test does not protect the “every target bound” requirement.

All four assertions use a target with one upper bound. Add a target with multiple bounds where only one is satisfied, and another where all are satisfied. Also test the reverse direction: ? extends Serializable must not become assignable to the synthetic Serializable & Cloneable wildcard.
These cases protect the distinction between “for every target bound, some source bound matches” and the incorrect “any matching pair is enough.”

@garydgregory
garydgregory marked this pull request as draft September 7, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Fixes TypeUtils.isAssignable(Type, WildcardType, Map) so wildcard assignability works correctly when the source wildcard has multiple upper bounds (intersection types).

Changes:

  • Update wildcard upper-bound checking to require any source upper bound to satisfy each target upper bound.
  • Apply similar “any bound” logic to lower-bound checking.
  • Add a regression test for wildcard assignability with multiple upper bounds.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java Adjusts wildcard upper/lower bound matching logic in isAssignable(...).
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java Adds a regression test covering intersection upper bounds assignability.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 1245 to 1261
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;
}
}
Comment on lines 1249 to +1250
// 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants