Fix #4030 remove double getters in null checks by stefanos-kalantzis · Pull Request #4064 · mapstruct/mapstruct

stefanos-kalantzis · 2026-06-05T14:49:38Z

Note: There is an edge-case considered out of scope for this PR, where source is used multiple times for different mappings. Then the generated code has multiple blocks with a different extracted variable, with incremental names. For example in DomainDtoWithNcvsAlwaysMapperImpl:

List<String> strings = source.getStrings();
if ( target.getStrings() != null ) {
    if ( source.hasStrings() ) {
        target.getStrings().clear();
        target.getStrings().addAll( strings );
    }
}
else {
    if ( source.hasStrings() ) {
        List<String> list = strings;
        target.setStrings( new LinkedHashSet<>( list ) );
    }
}
List<String> strings1 = source.getStrings();
if ( target.getLongs() != null ) {
    if ( source.hasStrings() ) {
        target.getLongs().clear();
        target.getLongs().addAll( stringListToLongSet( strings1 ) );
    }
}
else {
    if ( source.hasStrings() ) {
        target.setLongs( stringListToLongSet( strings1 ) );
    }
}

…able

stefanos-kalantzis · 2026-06-09T11:49:19Z

hi @filiphr, I went through the changes again and I think it should be ok to merge.

filiphr

Thanks for tackling #4030, @stefanos-kalantzis.

I have one concern and one test gap (details in the inline comments):

Behavioral change for custom presence-check methods (hasX() / isSetX()). For a SuffixPresenceCheck (e.g. source.hasStrings()), the value getter source.getStrings() is now hoisted into an unconditional local variable, so it runs even when the presence check returns false. See the regenerated _913/DomainDtoWithPresenceCheckMapperImpl:

// before: getStrings() only ran inside the guard
if ( source.hasStrings() ) {
    target.getStrings().addAll( source.getStrings() );
}
// after: getStrings() runs unconditionally
List<String> strings = source.getStrings();
if ( source.hasStrings() ) {
    target.getStrings().addAll( strings );
}

This is the JAXB isSetFoo() / getFoo() idiom - calling getFoo() unconditionally lazily instantiates the source collection, defeating the purpose of the presence check. It's the "expensive / side-effecting getter" case #4030 is meant to protect, and it yields no benefit here since the presence-check path never had a double call to begin with. Root cause: presenceCheckAllowsLocalVar only excludes MethodReferencePresenceCheck; SuffixPresenceCheck falls through to return true. Could we extend the guard to skip hoisting when the presence check doesn't read the source RHS value?

No behavioral test. The PR relies entirely on regenerated golden fixtures, which is not bad per se. However, I think it would be good if we can add some behaviour tests. e.g. for the scenario I mentioned before with the use of the presence checker perhaps?

filiphr · 2026-06-07T10:02:11Z

-        if ( source.getZonedDateTime().isPresent() ) {
+        Optional<ZonedDateTime> zonedDateTime = source.getZonedDateTime();
+        if ( zonedDateTime.isPresent() ) {
            target.setZonedDateTime( dateTimeFormatter_dd_MM_yyyy_HH_mm_z_01894582668.format( source.getZonedDateTime().get() ) );
        }


We are still using source.getZonedDateTime().get() in the optional conversion. I believe this should be zonedDateTime.get() instead of source.getZonedDateTime()

filiphr · 2026-06-08T21:09:29Z

            }
        }

+        private boolean presenceCheckAllowsLocalVar(PresenceCheck presenceCheck) {


🔴 Root cause of the unconditional-getter regression. This guard only excludes MethodReferencePresenceCheck; every other presence-check type (notably SuffixPresenceCheck, i.e. the hasX()/isSetX() idiom) falls through to return true, so a local var is created and the getter is hoisted above the if (source.hasX()) guard — executing it even when the check is false.

The principle here is right ("only hoist when the presence check consumes the source RHS value"), but it's enforced incompletely. Suggest returning false for presence checks that don't read the RHS — SuffixPresenceCheck, and please review JavaExpressionPresenceCheck plus Negate/All/Any wrappers around non-RHS checks. Local-var hoisting is safe/beneficial only for NullPresenceCheck, value-based OptionalPresenceCheck, and MethodReferencePresenceCheck bound to the source RHS.

Minor: this method encodes the subtlest decision in the PR — worth a short comment explaining the intent.

filiphr · 2026-06-08T21:09:30Z

                    sourceReference,
                    sourceRHS
                ) );
+                if ( presenceCheckAllowsLocalVar( sourceRHS.getSourcePresenceCheckerReference() ) ) {


This is where the local var gets set for simple (non-nested) properties. Because presenceCheckAllowsLocalVar returns true for SuffixPresenceCheck (see comment on the method below), a hasX()-guarded property ends up with an unconditionally-evaluated local var. The _913 fixtures show the effect.

filiphr · 2026-06-08T21:09:31Z

                    ctx.getTypeFactory(),
                    targetAccessorType.isFieldAssignment()
                );
+                if ( result.getSourceType().isPrimitive() ) {


In the mutable-collection update path (ExistingInstanceSetterWrapperForCollectionsAndMaps), the local var is only reset for primitive source types. So a List<String> fed by a hasX() presence check keeps the hoisted local var and is read unconditionally (this is the exact branch the _913 update mappers hit). If the fix in presenceCheckAllowsLocalVar lands, this primitive-only reset may be redundant — but as it stands today it doesn't cover the regression.

filiphr · 2026-06-08T21:09:31Z

            return;
        }

+        List<String> strings = source.getStrings();


Concrete evidence of the regression: source.getStrings() is now invoked here unconditionally, before the if ( source.hasStrings() ) guards below. Previously it only ran inside those guards. For JAXB-style isSetX()/lazy/expensive getters this is an observable behavior change (and a counting-getter test would fail here).

filiphr · 2026-06-08T21:09:32Z


            Assignment result = rightHandSide;
+            if ( sourceReference == null || !sourceReference.isNested() ) {
+                // For non-nested properties, reset the local var name: the adder wrapper handles


Minor: the comment says "For non-nested properties", but the guard also covers sourceReference == null (expression/constant mappings). Consider "For non-nested (or absent) source references". Optionally note why nested is excluded — nested references keep their local var to avoid re-invoking the forged accessor method.

hduelme

@stefanos-kalantzis Thanks for your contribution. The generated code looks much cleaner now.

I left a few comments regarding the generated code style. It looks like too many local variables are being generated now.

Additionally, I'm not fully convinced about setting setSourceLocalVarName to null later on. It feels a bit like we're using mutable state to undo a previous change, which makes the flow harder to follow. Could we avoid setting it in the first place, or model this differently?

hduelme · 2026-06-09T14:19:03Z

+                List<String> list = strings;
                target.setStrings( new LinkedHashSet<>( list ) );


The assignment is now redundant and should be inlined.

hduelme · 2026-06-09T14:34:33Z

+        List<Wheel> wheels = car.getWheels();
        if ( mappingTarget.getWheels() != null ) {
-            List<WheelDto> list = wheelListToWheelDtoList( car.getWheels() );
+            List<WheelDto> list = wheelListToWheelDtoList( wheels );
            if ( list != null ) {
                mappingTarget.getWheels().clear();
                mappingTarget.getWheels().addAll( list );
            }
        }
        else {
-            List<WheelDto> list = wheelListToWheelDtoList( car.getWheels() );
+            List<WheelDto> list = wheelListToWheelDtoList( wheels );


I don't think that the variable is needed, as wheels is only used in two separated branches.

hduelme · 2026-06-09T14:37:32Z

+        Optional<String> title = song.getTitle();
        if ( song.hasTitle() ) {
-            targetAggregate.setSongTitle( song.getTitle().get() );
+            targetAggregate.setSongTitle( title.get() );


The variable is not needed as song as an check method (hasTitle()).

hduelme · 2026-06-09T14:49:23Z

+                if ( result.getSourceType().isPrimitive() ) {
+                    result.setSourceLocalVarName( null );
+                }


Why are primitives excluded?

hduelme · 2026-06-09T16:03:31Z

+        private boolean presenceCheckAllowsLocalVar(PresenceCheck presenceCheck) {
+            if ( presenceCheck instanceof MethodReferencePresenceCheck ) {
+                return ((MethodReferencePresenceCheck) presenceCheck).getMethodReference().getParameterBindings()
+                    .stream().anyMatch( ParameterBinding::isForSourceRhs );
+            }
+            return true;
+        }
+


What is checked here?

hduelme · 2026-06-09T16:16:55Z

Oops, it looks like @filiphr and I started reviewing at the same time. I didn't check for existing review comments before submitting mine, so there may be some overlap.

stefanos-kalantzis force-pushed the fix-4030-remove-double-getters-in-null-checks branch from a50b711 to 3aa1ca6 Compare June 5, 2026 14:52

remove multiple calls of getters where possible by using a local vari…

d727a50

…able

stefanos-kalantzis force-pushed the fix-4030-remove-double-getters-in-null-checks branch from 3aa1ca6 to d727a50 Compare June 5, 2026 15:11

filiphr reviewed Jun 9, 2026

View reviewed changes

hduelme requested changes Jun 9, 2026

View reviewed changes

		List<String> list = strings;
		target.setStrings( new LinkedHashSet<>( list ) );

Uh oh!

Conversation

stefanos-kalantzis commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefanos-kalantzis commented Jun 9, 2026

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hduelme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hduelme commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stefanos-kalantzis commented Jun 5, 2026 •

edited

Loading