GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — prevent mapper generation from type with generic super bound to type with generic extends bound by hduelme · Pull Request #3994 · mapstruct/mapstruct

Conversation

hduelme commented Feb 11, 2026

Copy link

Copy Markdown

Contributor

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 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

filiphr requested changes

Mar 15, 2026

View reviewed changes

filiphr left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hduelme and others added 6 commits

March 15, 2026 23:39


          typo

ee946d5

Co-authored-by: Copilot Autofix powered by AI <[email protected]>


          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 commented Mar 15, 2026

Copy link

Copy Markdown

Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hduelme mentioned this pull request

Mar 16, 2026

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

Merged

filiphr commented Mar 26, 2026

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hduelme commented Mar 27, 2026

Copy link

Copy Markdown

Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hduelme reacted with thumbs up emoji

All reactions

👍 1 reaction

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

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Mar 27, 2026

fix import order for checkstyle #4024

Merged

use diamond operator for collections and maps on SetterWrapperForCollectionsAndMapsWithNullCheck #4026

Closed

filiphr added a commit to filiphr/mapstruct that referenced this pull request

May 1, 2026


          mapstruct#1018 Use diamond operator for new expressions in generate…

79aca2e

…d code

Adds a `NewInstanceCreation` ModelElement that renders `new T<>` and
contributes only the raw class to imports, so type parameters used solely
in the new-expression are no longer orphaned (Issue mapstruct#955). Also fixes
mapstruct#3994's follow-up wildcard cases where explicit type arguments produced
uncompilable Java.

filiphr mentioned this pull request

May 1, 2026

#1018 Use diamond operator for new expressions in generated code #4045

Merged

filiphr added a commit that referenced this pull request

May 4, 2026


          #1018 Use diamond operator for new expressions in generated code (#…

01bdde3

…4045)

Adds a `NewInstanceCreation` ModelElement that renders `new T<>` and
contributes only the raw class to imports, so type parameters used solely
in the new-expression are no longer orphaned (Issue #955). Also fixes
#3994's follow-up wildcard cases where explicit type arguments produced
uncompilable Java.

Reviewers

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

filiphr

filiphr approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

1.7.0.Beta2

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.