prevent mapper generation from type with generic super bound to type with generic extends bound by hduelme · Pull Request #3994 · mapstruct/mapstruct

hduelme · 2026-02-11T21:48:19Z

Currently, MapStruct generates code that attempts to implicitly cast a Collection<? super Type> to Type inside the for loop. This is not valid, since the elements of the collection are not guaranteed to be instances of Type, which results in a compilation error.
The same issue applies to Stream.

I changed the behavior so that mappings from Collection<? super Type> to Collection<? extends Type> and from Stream<? super Type> to Stream<? extends Type> now result in PROPERTYMAPPING_MAPPING_NOT_FOUND.

Additionally, I added a test to verify that mapping from Collection<? extends Type> to Collection<? super Type> works correctly. I did not add a corresponding test for Stream, since it currently generates invalid code (it tries to directly assign Collection<? extends Type> to Collection<Type>).

I am unsure what the correct behavior should be in this case, Currently, MapStruct generates code that attempts to implicitly cast a Collection<? super Type> to Type inside the for loop. This is not valid, since the elements of the collection are not guaranteed to be instances of Type, which results in a compilation error.
The same issue applies to Stream.

I changed the behavior so that mappings from Collection<? super Type> to Collection<? extends Type> and from Stream<? super Type> to Stream<? extends Type> now result in PROPERTYMAPPING_MAPPING_NOT_FOUND.

Additionally, I added a test to verify that mapping from Collection<? extends Type> to Collection<? super Type> works correctly. I did not add a corresponding test for Stream, since it currently generates invalid code (it tries to directly assign Collection<? extends Type> to Collection<Type>).

I am unsure what the correct behavior should be in this case, as MapStruct currently doesn't fully supports type bonds:

Should a dedicated mapping method be generated?
Should a cast be used?
Or should this kind of mapping be rejected entirely?

…with generic extends bound

Copilot

Pull request overview

Adjusts MapStruct’s container (Iterable/Stream) forging to avoid generating uncompilable code when mapping from ? super T sources to ? extends T targets, and adds regression tests around wildcard-bound assignments.

Changes:

Add Type.containsSuperBound() / Type.containsExtendsBound() helpers to detect wildcard bounds in type parameters.
Short-circuit iterable/stream element-mapping forging for source: ? super → target: ? extends to trigger PROPERTYMAPPING_MAPPING_NOT_FOUND instead of generating invalid code.
Add new wildcard-focused tests covering the rejected super -> extends scenarios (Collection + Stream) and the allowed extends -> super assignment (Collection).

Reviewed changes

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

Show a summary per file

File	Description
processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java	Adds helpers for detecting super/extends wildcard bounds in type parameters.
processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java	Prevents forging iterable/stream element mappings for `super`-bounded sources to `extends`-bounded targets by returning `null`.
processor/src/test/java/org/mapstruct/ap/test/generics/wildcard/DistinguishBetweenSuperAndExtendTest.java	Adds compilation-failure assertions for `super -> extends` and a runtime assertion for `extends -> super` (Collection).
processor/src/test/java/org/mapstruct/ap/test/generics/wildcard/ErroneousCollectionSuperToExtendMapper.java	Mapper used to validate compilation error for Collection `super -> extends`.
processor/src/test/java/org/mapstruct/ap/test/generics/wildcard/ErroneousStreamSuperToExtendMapper.java	Mapper used to validate compilation error for Stream `super -> extends`.
processor/src/test/java/org/mapstruct/ap/test/generics/wildcard/CollectionExtendToSuperMapper.java	Mapper used to validate `extends -> super` works for Collection properties.
processor/src/test/java/org/mapstruct/ap/test/generics/wildcard/CollectionExtendTypes.java	Test bean with `Collection<? extends SimpleObject>` property.
processor/src/test/java/org/mapstruct/ap/test/generics/wildcard/CollectionSuperTypes.java	Test bean with `Collection<? super SimpleObject>` property.
processor/src/test/java/org/mapstruct/ap/test/generics/wildcard/SimpleObject.java	Minimal element type for wildcard-bound container tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java

processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java

.../test/java/org/mapstruct/ap/test/generics/wildcard/DistinguishBetweenSuperAndExtendTest.java

filiphr

Thanks for the fix @hduelme. As you can see from the previous review from Copilot, I've been exploring the agents a bit for helping with the reviews 😄.

I did a pass through Claude Code and I think that it found some interesting things that we should address:

Should fix

These are a bit style related and what Claude Code suggests based on our code base.

1. Typo in test method names + naming convention

unabaleToMapSuperToExtendforCollection and unabaleToMapSuperToExtendforStream have a typo ("unabale" → "unable") and don't follow the project's should* naming convention used in neighboring test classes (e.g. WildCardTest). Suggested names:

shouldFailOnSuperToExtendMappingForCollection
shouldFailOnSuperToExtendMappingForStream
allowAssignmentOfExtendBoundToSuperBound → shouldMapExtendBoundToSuperBound

2. Import ordering in DistinguishBetweenSuperAndExtendTest.java

java.util.ArrayList and java.util.List appear after the org.mapstruct.* imports. Project convention places java.* imports first, then javax.*, then org.*, then static imports.

3. Missing line and type in @Diagnostic annotations

The overwhelming convention in this project is to include line and type in @Diagnostic (293 occurrences across 75 files include type). Adding them makes the tests more precise — they verify the error is reported on the correct mapper and line, preventing false passes. See WildCardTest.java in the same test area for examples.

Worth discussing

These are quite interesting, as I didn't even think about them. However, I think that they are correct and we should address them. What do you think?

4. forgeMapMapping is not guarded

The fix guards forgeWithElementMapping() (called by forgeIterableMapping and forgeStreamMapping), but forgeMapMapping() is a completely separate code path that does not go through forgeWithElementMapping. Would a Map<? super K, ? super V> → Map<? extends K, ? extends V> mapping hit the same class of invalid code generation? If so, the same guard should be added there. If not (or if it's out of scope), it might be worth adding a comment or a follow-up issue.

5. containsSuperBound() / containsExtendsBound() semantics with multi-type-parameter types

These methods use anyMatch across all type parameters. For single-type-parameter containers (Collection, Stream) this works perfectly. But for a type like Map<? super K, ? extends V>, both methods would return true, which could incorrectly block a valid mapping if the guard were ever applied to Map types. This is fine today, but worth keeping in mind — a Javadoc note clarifying the intended usage scope would help prevent misuse.

Minor / optional

Pre-existing Javadoc bug: hasExtendsBound() at Type.java:477 — the @return says "wild card super bound" but means "extends bound" (copy-paste from hasSuperBound()). Not introduced by this PR, but could be fixed while you're in the area.
Structural inconsistency: ErroneousStreamSuperToExtendMapper uses inner classes for source/target types while ErroneousCollectionSuperToExtendMapper uses separate top-level classes. Using the same pattern for both would improve consistency.
Stream extends→super test gap: The PR description explains this is omitted due to a separate existing bug. A @Disabled test or code comment would help track this so it doesn't get forgotten.

What's good

The fix is surgical and minimal — 3 lines in the right place
The null return correctly propagates through the existing error reporting path (verified by tracing callers)
The positive test verifies runtime behavior, not just compilation
Error messages in @ExpectedCompilationOutcome are specific and fully qualified

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

hduelme · 2026-03-15T23:53:22Z

@filiphr I addressed points 1 to 3 from your review, as well as the old Javadoc bug.

Causes other errors first

If MapStruct tries to map a List<? super SimpleObject> to another Collection the code generated is still invalide: collectionSuperTypes.setSimpleObjectsCollection( new ArrayList<SimpleObject>( collection ) );. The type argument used for ArrayList is incorrect.
If MapStruct tries to deep clone a collection, it still generates an invalid loop.
To properly fix these issues, MapStruct would need consistent support for extends and super bounds across the entire code path, as the bounds are currently mostly ignored (e.g. through targetType.withoutBounds()).

In theory true
However, as mentioned in point 4, MapStruct currently doesn't generate valid code at all for these cases.

I discarded Copilot’s suggestion to simply ignore all super targets, although at the moment that would at least prevent the invalid code generation.

The tests added with this PR are valid and can serve as guidance for future improvements. My solution tries to block some of the invalid code generation, but there are still many additional cases.

The proper long-term solution would be full generic bound support, since currently the bounds are largely ignored.

filiphr · 2026-03-26T22:12:49Z

Thanks for addressing all the comments @hduelme.

Causes other errors first

I'm not sure that I am following your explanation here. You are talking about List<? super SimpleObject>, and I agree with you about that. However, that's under "5. containsSuperBound() / containsExtendsBound() semantics with multi-type-parameter types", right?

This is for "forgeMapMapping is not guarded", i.e. mapping Map<? super K, ? super V> from Map<? extends K, ? extends V>. Am I not seeing something?

I discarded Copilot’s suggestion to simply ignore all super targets, although at the moment that would at least prevent the invalid code generation.

That's completely fine. I've only wanted to test it out, to see what it would generate.

The tests added with this PR are valid and can serve as guidance for future improvements. My solution tries to block some of the invalid code generation, but there are still many additional cases.

That's great and extremely appreciated.

The proper long-term solution would be full generic bound support, since currently the bounds are largely ignored.

I wouldn't say that they are largely ignored, we do have a lot of quirks around generics. Especially around method reusing etc.

hduelme · 2026-03-27T21:19:35Z

@filiphr Sorry my explanation was not really clear.

I'm not sure that I am following your explanation here. You are talking about List<? super SimpleObject>, and I agree with you about that. However, that's under "5. containsSuperBound() / containsExtendsBound() semantics with multi-type-parameter types", right?

This is for "forgeMapMapping is not guarded", i.e. mapping Map<? super K, ? super V> from Map<? extends K, ? extends V>. Am I not seeing something?

First of all the point highlighted by Claude Code are correct. In 4. the behavior how mapstruct handles Map<? super K, ? super V> and Map<? extends K, ? extends V> was discussed. While checking I found:

Map<? super K, ? super V> from Map<? extends K, ? extends V> currently generated valide code with a helper method:

 protected Map<SimpleObject, SimpleObject> simpleObjectSimpleObjectMapToSimpleObjectSimpleObjectMap(Map<? extends SimpleObject, ? extends SimpleObject> map) {
        if ( map == null ) {
            return null;
        }

        Map<SimpleObject, SimpleObject> map1 = LinkedHashMap.newLinkedHashMap( map.size() );

        for ( java.util.Map.Entry<? extends SimpleObject, ? extends SimpleObject> entry : map.entrySet() ) {
            SimpleObject key = entry.getKey();
            SimpleObject value = entry.getValue();
            map1.put( key, value );
        }

        return map1;
    }

Map<? extends K, ? extends V> from Map<? super K, ? super V> currently generated invalide code with a helper method:

protected Map<SimpleObject, SimpleObject> simpleObjectSimpleObjectMapToSimpleObjectSimpleObjectMap1(Map<? super SimpleObject, ? super SimpleObject> map) {
       if ( map == null ) {
           return null;
       }

       Map<SimpleObject, SimpleObject> map1 = LinkedHashMap.newLinkedHashMap( map.size() );

       for ( java.util.Map.Entry<? super SimpleObject, ? super SimpleObject> entry : map.entrySet() ) {
           SimpleObject key = entry.getKey();
           SimpleObject value = entry.getValue();
           map1.put( key, value );
       }

       return map1;
   }

when mapping Map<? extends K, ? extends V> from Map<? extends K, ? extends V> a valide copy constructor is generated:

        Map<? super SimpleObject, ? super SimpleObject> map = dd.getSimpleObjectsCollection();
        if ( map != null ) {
            collectionSuperTypes.setSimpleObjectsCollection( new LinkedHashMap<SimpleObject, SimpleObject>( map ) );
        }

5.when mapping Map<? super K, ? super V> from Map<? super K, ? super V> a invalide copy constructor is generated:

        Map<? extends SimpleObject, ? extends SimpleObject> map = dd.getSimpleObjectsCollection();
        if ( map != null ) {
            collectionSuperTypes2.setSimpleObjectsCollection( new LinkedHashMap<SimpleObject, SimpleObject>( map ) );
        }

So yes, the same problem plus some new problems are in forgeMapMapping.

The 5. point is although correct, in my implementation the position is ignored, that could cause problems in the future.

I wouldn't say that they are largely ignored, we do have a lot of quirks around generics. Especially around method reusing etc.

Totally right, Many things are already working, I was referring to the current dropping of bounds by calling withoutBounds() in forgeWithElementMapping and forgeMapMapping.

While investigating, I found, that my implementation is although missing a valide mapping, A super bounded type can be mapped to a java.lang,Object. For example:

List<? super SimpleObject> superTypeList = new ArrayList<>();

List<Object> objectList = new ArrayList<>( superTypeList );
List<? extends Object> extendsList = new ArrayList<>( superTypeList );

I think the better solution for this issue would be replacing the sourceType ? super T with java.lang,Object if source is not assignable to targetType.

filiphr

Thanks for the clarification @hduelme. Everything makes sense. I didn't realize hat we actually already handle the map properly.

I think the better solution for this issue would be replacing the sourceType ? super T with java.lang,Object if source is not assignable to targetType.

That's indeed a good one. Everything looks good now.

prevent mapper generation from type with generic super bound to type …

0be36c7

…with generic extends bound

hduelme mentioned this pull request Feb 11, 2026

fix self check in equals of Type #3995

Merged

filiphr requested a review from Copilot March 15, 2026 06:43

Copilot started reviewing on behalf of filiphr March 15, 2026 06:43 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

filiphr requested changes Mar 15, 2026

View reviewed changes

hduelme and others added 6 commits March 15, 2026 23:39

typo

ee946d5

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

improve description for containsSuperBound and containsExtendsBound

541fb12

reorder imports

d3000e6

rename tests according to code base

b971335

add line and type in @diagnostic

896e26c

fix Javadoc

ebceb09

hduelme mentioned this pull request Mar 16, 2026

Enforce spaces inside parentheses for control flow statements via checkstyle #4016

Merged

hduelme added 2 commits March 27, 2026 22:26

replace ? super by java.lang,Object if necessary

26f3f91

remove contains bounds methods

b1587a8

filiphr approved these changes Mar 27, 2026

View reviewed changes

filiphr added this to the 1.7.0.Beta2 milestone Mar 27, 2026

filiphr added the enhancement label Mar 27, 2026

filiphr merged commit 36c58b6 into mapstruct:main Mar 27, 2026
5 checks passed

hduelme mentioned this pull request Mar 27, 2026

fix import order for checkstyle #4024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

prevent mapper generation from type with generic super bound to type with generic extends bound#3994

prevent mapper generation from type with generic super bound to type with generic extends bound#3994
filiphr merged 9 commits intomapstruct:mainfrom
hduelme:prevent-mapper-generation-from-super-to-extends-bound

hduelme commented Feb 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filiphr left a comment

Uh oh!

hduelme commented Mar 15, 2026

Uh oh!

filiphr commented Mar 26, 2026

Uh oh!

hduelme commented Mar 27, 2026

Uh oh!

filiphr left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

hduelme commented Feb 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Should fix

Worth discussing

Minor / optional

What's good

Uh oh!

hduelme commented Mar 15, 2026

Uh oh!

filiphr commented Mar 26, 2026

Uh oh!

hduelme commented Mar 27, 2026

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants